Re: [PATCH v3 10/16] hw/i386/vmport: Add support for CMD_GETTIME

[Liran Alon]

On 13/03/2020 2:04, Michael S. Tsirkin wrote:
> On Thu, Mar 12, 2020 at 06:54:25PM +0200, Liran Alon wrote:
> > This command is used by guest to gettimeofday() from host.
> > See usage example in open-vm-tools TimeSyncReadHost() function.
> >
> > Reviewed-by: Nikita Leshenko <address@hidden>
> > Signed-off-by: Liran Alon <address@hidden>
> > ---
> >   hw/i386/vmport.c         | 21 +++++++++++++++++++++
> >   include/hw/i386/vmport.h |  1 +
> >   2 files changed, 22 insertions(+)
> >
> > diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c
> > index 3fb8a8bd458a..c5b659c59343 100644
> > --- a/hw/i386/vmport.c
> > +++ b/hw/i386/vmport.c
> > @@ -66,6 +66,7 @@ typedef struct VMPortState {
> >   
> >       uint32_t vmware_vmx_version;
> >       uint8_t vmware_vmx_type;
> > +    uint32_t max_time_lag_us;
> >   
> >       uint32_t compat_flags;
> >   } VMPortState;
> > @@ -168,6 +169,20 @@ static uint32_t vmport_cmd_ram_size(void *opaque, uint32_t 
> > addr)
> >       return ram_size;
> >   }
> >   
> > +static uint32_t vmport_cmd_time(void *opaque, uint32_t addr)
> > +{
> > +    X86CPU *cpu = X86_CPU(current_cpu);
> > +    qemu_timeval tv;
> > +
> > +    if (qemu_gettimeofday(&tv) < 0) {
> > +        return UINT32_MAX;
> > +    }
> > +
> > +    cpu->env.regs[R_EBX] = (uint32_t)tv.tv_usec;
> > +    cpu->env.regs[R_ECX] = port_state->max_time_lag_us;
> > +    return (uint32_t)tv.tv_sec;
> > +}
> > +
> >   /* vmmouse helpers */
> >   void vmmouse_get_data(uint32_t *data)
> >   {
> That's a very weird thing to return to the guest.
> For example it's not monotonic across migrations.
That's the VMware PV interface... I didn't design it. :P
Regarding how it handles the fact time is not monotonic across 
migrations, see big comment at the start of 
services/plugins/timeSync/timeSync.c in open-vm-tools regarding the 
time-sync algorithm used by VMware Tools.
Specifically:
"""
During normal operation this plugin only steps the time forward and only 
if the error is greater than one second.
"""
> And what does max_time_lag_us refer to, anyway?
According to the comment in open-vm-tools TimeSyncReadHost():
"""
maximum time lag allowed (config option), which is a threshold that 
keeps the tools from being over eager about resetting the time when it 
is only a little bit off.
"""

Looking at open-vm-tools timeSync.c code, it defines the threshold of 
how far guest time can be from host time before deciding to do a "step 
correction".
A "step correction" is defined as explicitly setting the time in the 
guest to the time in the host.
> So please add documentation about what this does.
You are right. I agree.
I think it would be best to just point to open-vm-tools 
services/plugins/timeSync/timeSync.c.
Do you agree or should I copy some paragraphs from there?
> If there's no document to refer to then pls write
> code comments or a document under docs/ - this does not
> belong in commit log.
>
>
>
> > @@ -214,6 +229,7 @@ static void vmport_realizefn(DeviceState *dev, Error **errp)
> >       vmport_register(VMPORT_CMD_GETRAMSIZE, vmport_cmd_ram_size, NULL);
> >       if (s->compat_flags & VMPORT_COMPAT_CMDS_V2) {
> >           vmport_register(VMPORT_CMD_GETBIOSUUID, vmport_cmd_get_bios_uuid, 
> > NULL);
> > +        vmport_register(VMPORT_CMD_GETTIME, vmport_cmd_time, NULL);
> >       }
> >   }
> >   
> > @@ -249,6 +265,11 @@ static Property vmport_properties[] = {
> >        * 5 - ACE 1.x (Deprecated)
> >        */
> >       DEFINE_PROP_UINT8("vmware-vmx-type", VMPortState, vmware_vmx_type, 2),
> > +    /*
> > +     * Max amount of time lag that can go uncorrected.
> What does uncorrected mean?
You are right this is a bad phrasing taken from open-vm-tools.
It should mean "How far we allow guest time to go from host time before 
guest VMware Tools will sync it to host time".
How you prefer to phrase this?
> > +     * Value taken from VMware Workstation 5.5.
>
> How do we know this makes sense for KVM? That has significantly
> different runtime characteristics.
This is just a threshold as you can understand from the above reply of 
mine (I should rephrase the comments to make this clearer).
So we just chose a threshold that makes sense for common workloads.
One of the reasons to put this as a property, is to still allow user to 
override it.
> Also, the version returns ESX server, why does it make
> sense to take some values from workstation?
I believe (don't remember) that ESXi was observed to return similar value.
Most of our workloads that runs with this came from ESXi and we never 
examined an issue regarding this in our production environment.
Which makes sense as this is just a thresthold that specifies when guest 
time should be synced to host time.
You prefer I would just remove this comment?

Thanks,
-Liran

> > +     **/
> > +    DEFINE_PROP_UINT32("max-time-lag", VMPortState, max_time_lag_us, 1000000),
> >   
> >       DEFINE_PROP_END_OF_LIST(),
> >   };
> > diff --git a/include/hw/i386/vmport.h b/include/hw/i386/vmport.h
> > index 7f33512ca6f0..50416c8c8f3e 100644
> > --- a/include/hw/i386/vmport.h
> > +++ b/include/hw/i386/vmport.h
> > @@ -8,6 +8,7 @@ typedef enum {
> >       VMPORT_CMD_GETVERSION       = 10,
> >       VMPORT_CMD_GETBIOSUUID      = 19,
> >       VMPORT_CMD_GETRAMSIZE       = 20,
> > +    VMPORT_CMD_GETTIME          = 23,
> >       VMPORT_CMD_VMMOUSE_DATA     = 39,
> >       VMPORT_CMD_VMMOUSE_STATUS   = 40,
> >       VMPORT_CMD_VMMOUSE_COMMAND  = 41,
> > --
> > 2.20.1