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

[Michael S. Tsirkin]

On Fri, Mar 13, 2020 at 06:26:54PM +0200, Liran Alon wrote:
> 
> 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.

Exactly. So if host time moved backward e.g. by 100s, then for 100s
time is not correcting. Which possibly vmware has a way to mitigate
against e.g. by synchronising host time using their
management app.

> Read carefully services/plugins/timeSync/timeSync.c and point me to what I'm
> missing if you think otherwise (i.e. I missed something).

I'm just going by what you write in a patch.

> > > """
> > > > 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
>