[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Liran Alon
Subject: Re: [PATCH v3 10/16] hw/i386/vmport: Add support for CMD_GETTIME
Date: Fri, 13 Mar 2020 18:26:54 +0200
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:68.0) Gecko/20100101 Thunderbird/68.5.0

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 
       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.
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
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
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, 
+        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.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]