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

["Michael S. Tsirkin" <mst@redhat.com>]

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 <nikita.leshchenko@oracle.com>
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> ---
>  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.
And what does max_time_lag_us refer to, anyway?


So please add documentation about what this does.
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?

> +     * Value taken from VMware Workstation 5.5.


How do we know this makes sense for KVM? That has significantly
different runtime characteristics.


Also, the version returns ESX server, why does it make
sense to take some values from workstation?

> +     **/
> +    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