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

[Liran Alon]

On 13/03/2020 17:47, Michael S. Tsirkin wrote:
> On Fri, Mar 13, 2020 at 05:25:20PM +0200, Liran Alon wrote:
> > > > @@ -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.
> Looks like guest assumes this time only moves forward.
> So something needs to be done to avoid it moving
> backward across migrations.
Where do you see this assumption in guest code? I don't think this is true.
Guest code seems to handle this by making sure to only step the time 
forward.
Read carefully services/plugins/timeSync/timeSync.c and point me to what 
I'm missing if you think otherwise (i.e. I missed something).
> > """
> > > 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?
> Neither. Their documentation will be from guest point of view.  Please
> look at that code and write documentation from host point of view.
> Your documentation for the lag parameter is I think a good
> example of how to do it.
Ok. Will try to phrase something for v4.
> > > 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?
> Sounds like a good explanation. Maybe we allow -> can
> since "we" is hypervisor and it's actually under guest control.
Ok. Will add this to v4.
> > > > +     * 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.
> Well close to 100% of users will have no idea what to set it to.
I agree. :) That's why there is a default value.
> > > 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?
> Maybe add " TODO: should this depend on vmare-vmx-type? ".

Ok. Will add to v4.

Thanks,
-Liran