[PATCH v3 00/16]: hw/i386/vmport: Bug fixes and improvements

[PATCH v3 00/16]: hw/i386/vmport: Bug fixes and improvements

[Liran Alon]

Hi,

This series aims to fix several bugs in VMPort and improve it by supporting
more VMPort commands and make command results more configurable to
user via QEMU command-line.

This functionality was proven to be useful to run various VMware VMs
when attempting to run them as-is on top of QEMU/KVM.

For more details, see commit messages.

Regards,
-Liran

v1->v2:
* Fix coding convention [Patchew Bot & MST].
* Create new header file for vmport.h [MST].
* Move KVM_APIC_BUS_FREQUENCY from linux-headers/asm-x86/kvm.h
  auto-generated header [MST]
* Elaborate more that vmx-version refers to the VMware userspace
  VMM in commit message. [MST]
* Use le32_to_cpu() on BIOS_UUID vmport command. [MST]
* Introduce VMPort compatability version property to maintain migration
  compatibility. [MST]

v2->v3:
- Repalce VMPort compatability version property with multiple boolean
  compatability properties. [MST]
- Prefix "vmx-*" properties with "vmware-vmx-*" to avoid confusion with
  Intel VT-x short name. Prefix suggested by MST. [MST]
- Remove VMX_Type enum and instead hard-code default vmware-vmx-type
  value and only reference open-vm-tools for rest of values. [MST]
- Add reference (link) to VMware open-vm-tools code. [MST]

[PATCH v3 01/16] hw/i386/vmport: Add reference to VMware open-vm-tools

[Liran Alon]

This official VMware open-source project can be used as reference to
understand how guest code interacts with VMPort virtual device. Thus,
providing understanding on how device is expected to behave.

Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/i386/vmport.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c
index 1f31e27c8aa4..b4c5a57bb0e9 100644
--- a/hw/i386/vmport.c
+++ b/hw/i386/vmport.c
@@ -21,6 +21,13 @@
  * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
  * THE SOFTWARE.
  */
+
+/*
+ * Guest code that interacts with this virtual device can be found
+ * in VMware open-vm-tools open-source project:
+ * https://github.com/vmware/open-vm-tools
+ */
+
 #include "qemu/osdep.h"
 #include "hw/isa/isa.h"
 #include "hw/i386/pc.h"
-- 
2.20.1

[PATCH v3 02/16] hw/i386/vmport: Add device properties

[Liran Alon]

No functional change.

This is done as a preparation for the following patches that will
introduce several device properties.

Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/i386/vmport.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c
index b4c5a57bb0e9..6ed110ef71a6 100644
--- a/hw/i386/vmport.c
+++ b/hw/i386/vmport.c
@@ -32,6 +32,7 @@
 #include "hw/isa/isa.h"
 #include "hw/i386/pc.h"
 #include "hw/input/i8042.h"
+#include "hw/qdev-properties.h"
 #include "sysemu/hw_accel.h"
 #include "qemu/log.h"
 #include "trace.h"
@@ -161,6 +162,10 @@ static void vmport_realizefn(DeviceState *dev, Error **errp)
     vmport_register(VMPORT_CMD_GETRAMSIZE, vmport_cmd_ram_size, NULL);
 }
 
+static Property vmport_properties[] = {
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void vmport_class_initfn(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -168,6 +173,7 @@ static void vmport_class_initfn(ObjectClass *klass, void *data)
     dc->realize = vmport_realizefn;
     /* Reason: realize sets global port_state */
     dc->user_creatable = false;
+    device_class_set_props(dc, vmport_properties);
 }
 
 static const TypeInfo vmport_info = {
-- 
2.20.1

Re: [PATCH v3 02/16] hw/i386/vmport: Add device properties

[Philippe Mathieu-Daudé]

On 3/12/20 5:54 PM, Liran Alon wrote:
> No functional change.
> 
> This is done as a preparation for the following patches that will
> introduce several device properties.
> 
> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> ---
>   hw/i386/vmport.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c
> index b4c5a57bb0e9..6ed110ef71a6 100644
> --- a/hw/i386/vmport.c
> +++ b/hw/i386/vmport.c
> @@ -32,6 +32,7 @@
>   #include "hw/isa/isa.h"
>   #include "hw/i386/pc.h"
>   #include "hw/input/i8042.h"
> +#include "hw/qdev-properties.h"
>   #include "sysemu/hw_accel.h"
>   #include "qemu/log.h"
>   #include "trace.h"
> @@ -161,6 +162,10 @@ static void vmport_realizefn(DeviceState *dev, Error **errp)
>       vmport_register(VMPORT_CMD_GETRAMSIZE, vmport_cmd_ram_size, NULL);
>   }
>   
> +static Property vmport_properties[] = {
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>   static void vmport_class_initfn(ObjectClass *klass, void *data)
>   {
>       DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -168,6 +173,7 @@ static void vmport_class_initfn(ObjectClass *klass, void *data)
>       dc->realize = vmport_realizefn;
>       /* Reason: realize sets global port_state */
>       dc->user_creatable = false;
> +    device_class_set_props(dc, vmport_properties);
>   }
>   
>   static const TypeInfo vmport_info = {
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

[PATCH v3 03/16] hw/i386/vmport: Propagate IOPort read to vCPU EAX register

[Liran Alon]

vmport_ioport_read() returns the value that should propagate to vCPU EAX
register when guest reads VMPort IOPort (i.e. By x86 IN instruction).

However, because vmport_ioport_read() calls cpu_synchronize_state(), the
returned value gets overridden by the value in QEMU vCPU EAX register.
i.e. cpu->env.regs[R_EAX].

To fix this issue, change vmport_ioport_read() to explicitly override
cpu->env.regs[R_EAX] with the value it wish to propagate to vCPU EAX
register.

Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/core/machine.c |  1 +
 hw/i386/vmport.c  | 32 +++++++++++++++++++++++++++++---
 2 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 9e8c06036faf..0496112b741c 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -39,6 +39,7 @@ GlobalProperty hw_compat_4_2[] = {
     { "usb-redir", "suppress-remote-wake", "off" },
     { "qxl", "revision", "4" },
     { "qxl-vga", "revision", "4" },
+    { "vmport", "x-read-set-eax", "off" },
 };
 const size_t hw_compat_4_2_len = G_N_ELEMENTS(hw_compat_4_2);
 
diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c
index 6ed110ef71a6..72f09ef5cba3 100644
--- a/hw/i386/vmport.c
+++ b/hw/i386/vmport.c
@@ -43,6 +43,11 @@
 #define VMPORT_ENTRIES 0x2c
 #define VMPORT_MAGIC   0x564D5868
 
+/* Compatibility flags for migration */
+#define VMPORT_COMPAT_READ_SET_EAX_BIT      0
+#define VMPORT_COMPAT_READ_SET_EAX          \
+    (1 << VMPORT_COMPAT_READ_SET_EAX_BIT)
+
 #define VMPORT(obj) OBJECT_CHECK(VMPortState, (obj), TYPE_VMPORT)
 
 typedef struct VMPortState {
@@ -51,6 +56,8 @@ typedef struct VMPortState {
     MemoryRegion io;
     VMPortReadFunc *func[VMPORT_ENTRIES];
     void *opaque[VMPORT_ENTRIES];
+
+    uint32_t compat_flags;
 } VMPortState;
 
 static VMPortState *port_state;
@@ -80,17 +87,33 @@ static uint64_t vmport_ioport_read(void *opaque, hwaddr addr,
 
     eax = env->regs[R_EAX];
     if (eax != VMPORT_MAGIC) {
-        return eax;
+        goto out;
     }
 
     command = env->regs[R_ECX];
     trace_vmport_command(command);
     if (command >= VMPORT_ENTRIES || !s->func[command]) {
         qemu_log_mask(LOG_UNIMP, "vmport: unknown command %x\n", command);
-        return eax;
+        goto out;
     }
 
-    return s->func[command](s->opaque[command], addr);
+    eax = s->func[command](s->opaque[command], addr);
+
+out:
+    /*
+     * The call above to cpu_synchronize_state() gets vCPU registers values
+     * to QEMU but also cause QEMU to write QEMU vCPU registers values to
+     * vCPU implementation (e.g. Accelerator such as KVM) just before
+     * resuming guest.
+     *
+     * Therefore, in order to make IOPort return value propagate to
+     * guest EAX, we need to explicitly update QEMU EAX register value.
+     */
+    if (s->compat_flags & VMPORT_COMPAT_READ_SET_EAX) {
+        cpu->env.regs[R_EAX] = eax;
+    }
+
+    return eax;
 }
 
 static void vmport_ioport_write(void *opaque, hwaddr addr,
@@ -163,6 +186,9 @@ static void vmport_realizefn(DeviceState *dev, Error **errp)
 }
 
 static Property vmport_properties[] = {
+    /* Used to enforce compatibility for migration */
+    DEFINE_PROP_BIT("x-read-set-eax", VMPortState, compat_flags,
+                    VMPORT_COMPAT_READ_SET_EAX_BIT, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.20.1

[PATCH v3 04/16] hw/i386/vmport: Set EAX to -1 on failed and unsupported commands

[Liran Alon]

This is used as a signal for VMware Tools to know if a command it
attempted to invoke, failed or is unsupported. As a result, VMware Tools
will either report failure to user or fallback to another backdoor command
in attempt to perform some operation.

A few examples:
* open-vm-tools TimeSyncReadHost() function fallbacks to
CMD_GETTIMEFULL command when CMD_GETTIMEFULL_WITH_LAG
fails/unsupported.
* open-vm-tools Hostinfo_NestingSupported() function verifies
EAX != -1 to check for success.
* open-vm-tools Hostinfo_VCPUInfoBackdoor() functions checks
if reserved-bit is set to indicate command is unimplemented.

Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/core/machine.c |  1 +
 hw/i386/vmport.c  | 19 +++++++++++++++----
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 0496112b741c..2595a13e5650 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -40,6 +40,7 @@ GlobalProperty hw_compat_4_2[] = {
     { "qxl", "revision", "4" },
     { "qxl-vga", "revision", "4" },
     { "vmport", "x-read-set-eax", "off" },
+    { "vmport", "x-signal-unsupported-cmd", "off" },
 };
 const size_t hw_compat_4_2_len = G_N_ELEMENTS(hw_compat_4_2);
 
diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c
index 72f09ef5cba3..e67c7bb2afea 100644
--- a/hw/i386/vmport.c
+++ b/hw/i386/vmport.c
@@ -44,9 +44,12 @@
 #define VMPORT_MAGIC   0x564D5868
 
 /* Compatibility flags for migration */
-#define VMPORT_COMPAT_READ_SET_EAX_BIT      0
-#define VMPORT_COMPAT_READ_SET_EAX          \
+#define VMPORT_COMPAT_READ_SET_EAX_BIT              0
+#define VMPORT_COMPAT_SIGNAL_UNSUPPORTED_CMD_BIT    1
+#define VMPORT_COMPAT_READ_SET_EAX              \
     (1 << VMPORT_COMPAT_READ_SET_EAX_BIT)
+#define VMPORT_COMPAT_SIGNAL_UNSUPPORTED_CMD    \
+    (1 << VMPORT_COMPAT_SIGNAL_UNSUPPORTED_CMD_BIT)
 
 #define VMPORT(obj) OBJECT_CHECK(VMPortState, (obj), TYPE_VMPORT)
 
@@ -87,17 +90,23 @@ static uint64_t vmport_ioport_read(void *opaque, hwaddr addr,
 
     eax = env->regs[R_EAX];
     if (eax != VMPORT_MAGIC) {
-        goto out;
+        goto err;
     }
 
     command = env->regs[R_ECX];
     trace_vmport_command(command);
     if (command >= VMPORT_ENTRIES || !s->func[command]) {
         qemu_log_mask(LOG_UNIMP, "vmport: unknown command %x\n", command);
-        goto out;
+        goto err;
     }
 
     eax = s->func[command](s->opaque[command], addr);
+    goto out;
+
+err:
+    if (s->compat_flags & VMPORT_COMPAT_SIGNAL_UNSUPPORTED_CMD) {
+        eax = UINT32_MAX;
+    }
 
 out:
     /*
@@ -189,6 +198,8 @@ static Property vmport_properties[] = {
     /* Used to enforce compatibility for migration */
     DEFINE_PROP_BIT("x-read-set-eax", VMPortState, compat_flags,
                     VMPORT_COMPAT_READ_SET_EAX_BIT, true),
+    DEFINE_PROP_BIT("x-signal-unsupported-cmd", VMPortState, compat_flags,
+                    VMPORT_COMPAT_SIGNAL_UNSUPPORTED_CMD_BIT, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.20.1

[PATCH v3 05/16] hw/i386/vmport: Introduce vmware-vmx-version property

[Liran Alon]

vmware-vmx-version is a number returned from CMD_GETVERSION which specifies
to guest VMware Tools the the host VMX version. If the host reports a number
that is different than what the guest VMware Tools expects, it may force
guest to upgrade VMware Tools. (See comment above VERSION_MAGIC and
VmCheck_IsVirtualWorld() function in open-vm-tools open-source code).

For better readability and allow maintaining compatability for guests
which may expect different vmware-vmx-version, make vmware-vmx-version a
VMPort object property. This would allow user to control it's value via
"-global vmport.vmware-vmx-version=X".

Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/i386/vmport.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c
index e67c7bb2afea..8e662303d5d3 100644
--- a/hw/i386/vmport.c
+++ b/hw/i386/vmport.c
@@ -60,6 +60,8 @@ typedef struct VMPortState {
     VMPortReadFunc *func[VMPORT_ENTRIES];
     void *opaque[VMPORT_ENTRIES];
 
+    uint32_t vmware_vmx_version;
+
     uint32_t compat_flags;
 } VMPortState;
 
@@ -138,7 +140,7 @@ static uint32_t vmport_cmd_get_version(void *opaque, uint32_t addr)
     X86CPU *cpu = X86_CPU(current_cpu);
 
     cpu->env.regs[R_EBX] = VMPORT_MAGIC;
-    return 6;
+    return port_state->vmware_vmx_version;
 }
 
 static uint32_t vmport_cmd_ram_size(void *opaque, uint32_t addr)
@@ -200,6 +202,11 @@ static Property vmport_properties[] = {
                     VMPORT_COMPAT_READ_SET_EAX_BIT, true),
     DEFINE_PROP_BIT("x-signal-unsupported-cmd", VMPortState, compat_flags,
                     VMPORT_COMPAT_SIGNAL_UNSUPPORTED_CMD_BIT, true),
+
+    /* Default value taken from open-vm-tools code VERSION_MAGIC definition */
+    DEFINE_PROP_UINT32("vmware-vmx-version", VMPortState,
+                       vmware_vmx_version, 6),
+
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.20.1

Re: [PATCH v3 05/16] hw/i386/vmport: Introduce vmware-vmx-version property

[Philippe Mathieu-Daudé]

On 3/12/20 5:54 PM, Liran Alon wrote:
> vmware-vmx-version is a number returned from CMD_GETVERSION which specifies
> to guest VMware Tools the the host VMX version. If the host reports a number
> that is different than what the guest VMware Tools expects, it may force
> guest to upgrade VMware Tools. (See comment above VERSION_MAGIC and
> VmCheck_IsVirtualWorld() function in open-vm-tools open-source code).
> 
> For better readability and allow maintaining compatability for guests
> which may expect different vmware-vmx-version, make vmware-vmx-version a
> VMPort object property. This would allow user to control it's value via
> "-global vmport.vmware-vmx-version=X".
> 
> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> ---
>   hw/i386/vmport.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c
> index e67c7bb2afea..8e662303d5d3 100644
> --- a/hw/i386/vmport.c
> +++ b/hw/i386/vmport.c
> @@ -60,6 +60,8 @@ typedef struct VMPortState {
>       VMPortReadFunc *func[VMPORT_ENTRIES];
>       void *opaque[VMPORT_ENTRIES];
>   
> +    uint32_t vmware_vmx_version;
> +
>       uint32_t compat_flags;
>   } VMPortState;
>   
> @@ -138,7 +140,7 @@ static uint32_t vmport_cmd_get_version(void *opaque, uint32_t addr)
>       X86CPU *cpu = X86_CPU(current_cpu);
>   
>       cpu->env.regs[R_EBX] = VMPORT_MAGIC;
> -    return 6;
> +    return port_state->vmware_vmx_version;
>   }
>   
>   static uint32_t vmport_cmd_ram_size(void *opaque, uint32_t addr)
> @@ -200,6 +202,11 @@ static Property vmport_properties[] = {
>                       VMPORT_COMPAT_READ_SET_EAX_BIT, true),
>       DEFINE_PROP_BIT("x-signal-unsupported-cmd", VMPortState, compat_flags,
>                       VMPORT_COMPAT_SIGNAL_UNSUPPORTED_CMD_BIT, true),
> +
> +    /* Default value taken from open-vm-tools code VERSION_MAGIC definition */
> +    DEFINE_PROP_UINT32("vmware-vmx-version", VMPortState,
> +                       vmware_vmx_version, 6),
> +
>       DEFINE_PROP_END_OF_LIST(),
>   };
>   
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

[PATCH v3 06/16] hw/i386/vmport: Report vmware-vmx-type in CMD_GETVERSION

[Liran Alon]

As can be seen from VmCheck_GetVersion() in open-vm-tools code,
CMD_GETVERSION should return vmware-vmx-type in ECX register.

Default is to fake host as VMware ESX server. But user can control
this value by "-global vmport.vmware-vmx-type=X".

Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/core/machine.c |  1 +
 hw/i386/vmport.c  | 27 +++++++++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 2595a13e5650..0e2f37420360 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -41,6 +41,7 @@ GlobalProperty hw_compat_4_2[] = {
     { "qxl-vga", "revision", "4" },
     { "vmport", "x-read-set-eax", "off" },
     { "vmport", "x-signal-unsupported-cmd", "off" },
+    { "vmport", "x-report-vmx-type", "off" },
 };
 const size_t hw_compat_4_2_len = G_N_ELEMENTS(hw_compat_4_2);
 
diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c
index 8e662303d5d3..ead2f2d5326f 100644
--- a/hw/i386/vmport.c
+++ b/hw/i386/vmport.c
@@ -46,10 +46,13 @@
 /* Compatibility flags for migration */
 #define VMPORT_COMPAT_READ_SET_EAX_BIT              0
 #define VMPORT_COMPAT_SIGNAL_UNSUPPORTED_CMD_BIT    1
+#define VMPORT_COMPAT_REPORT_VMX_TYPE_BIT           2
 #define VMPORT_COMPAT_READ_SET_EAX              \
     (1 << VMPORT_COMPAT_READ_SET_EAX_BIT)
 #define VMPORT_COMPAT_SIGNAL_UNSUPPORTED_CMD    \
     (1 << VMPORT_COMPAT_SIGNAL_UNSUPPORTED_CMD_BIT)
+#define VMPORT_COMPAT_REPORT_VMX_TYPE           \
+    (1 << VMPORT_COMPAT_REPORT_VMX_TYPE_BIT)
 
 #define VMPORT(obj) OBJECT_CHECK(VMPortState, (obj), TYPE_VMPORT)
 
@@ -61,6 +64,7 @@ typedef struct VMPortState {
     void *opaque[VMPORT_ENTRIES];
 
     uint32_t vmware_vmx_version;
+    uint8_t vmware_vmx_type;
 
     uint32_t compat_flags;
 } VMPortState;
@@ -140,6 +144,9 @@ static uint32_t vmport_cmd_get_version(void *opaque, uint32_t addr)
     X86CPU *cpu = X86_CPU(current_cpu);
 
     cpu->env.regs[R_EBX] = VMPORT_MAGIC;
+    if (port_state->compat_flags & VMPORT_COMPAT_REPORT_VMX_TYPE) {
+        cpu->env.regs[R_ECX] = port_state->vmware_vmx_type;
+    }
     return port_state->vmware_vmx_version;
 }
 
@@ -202,10 +209,30 @@ static Property vmport_properties[] = {
                     VMPORT_COMPAT_READ_SET_EAX_BIT, true),
     DEFINE_PROP_BIT("x-signal-unsupported-cmd", VMPortState, compat_flags,
                     VMPORT_COMPAT_SIGNAL_UNSUPPORTED_CMD_BIT, true),
+    DEFINE_PROP_BIT("x-report-vmx-type", VMPortState, compat_flags,
+                    VMPORT_COMPAT_REPORT_VMX_TYPE_BIT, true),
 
     /* Default value taken from open-vm-tools code VERSION_MAGIC definition */
     DEFINE_PROP_UINT32("vmware-vmx-version", VMPortState,
                        vmware_vmx_version, 6),
+    /*
+     * Value determines which VMware product type host report itself to guest.
+     *
+     * Most guests are fine with exposing host as VMware ESX server.
+     * Some legacy/proprietary guests hard-code a given type.
+     *
+     * For a complete list of values, refer to enum VMXType at open-vm-tools
+     * project (Defined at lib/include/vm_vmx_type.h).
+     *
+     * Reasonable options:
+     * 0 - Unset
+     * 1 - VMware Express (deprecated)
+     * 2 - VMware ESX Server
+     * 3 - VMware Server (Deprecated)
+     * 4 - VMware Workstation
+     * 5 - ACE 1.x (Deprecated)
+     */
+    DEFINE_PROP_UINT8("vmware-vmx-type", VMPortState, vmware_vmx_type, 2),
 
     DEFINE_PROP_END_OF_LIST(),
 };
-- 
2.20.1

[PATCH v3 07/16] hw/i386/vmport: Introduce vmport.h

[Liran Alon]

No functional change. This is mere refactoring.

Suggested-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/i386/pc.c             |  1 +
 hw/i386/vmmouse.c        |  1 +
 hw/i386/vmport.c         |  1 +
 include/hw/i386/pc.h     | 13 -------------
 include/hw/i386/vmport.h | 16 ++++++++++++++++
 5 files changed, 19 insertions(+), 13 deletions(-)
 create mode 100644 include/hw/i386/vmport.h

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 6ab4acb0c62e..6ac71e1af32b 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -31,6 +31,7 @@
 #include "hw/i386/apic.h"
 #include "hw/i386/topology.h"
 #include "hw/i386/fw_cfg.h"
+#include "hw/i386/vmport.h"
 #include "sysemu/cpus.h"
 #include "hw/block/fdc.h"
 #include "hw/ide.h"
diff --git a/hw/i386/vmmouse.c b/hw/i386/vmmouse.c
index e8e62bd96b8c..49a546fd3bb6 100644
--- a/hw/i386/vmmouse.c
+++ b/hw/i386/vmmouse.c
@@ -26,6 +26,7 @@
 #include "qapi/error.h"
 #include "ui/console.h"
 #include "hw/i386/pc.h"
+#include "hw/i386/vmport.h"
 #include "hw/input/i8042.h"
 #include "hw/qdev-properties.h"
 #include "migration/vmstate.h"
diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c
index ead2f2d5326f..e9ea5fe7f765 100644
--- a/hw/i386/vmport.c
+++ b/hw/i386/vmport.c
@@ -31,6 +31,7 @@
 #include "qemu/osdep.h"
 #include "hw/isa/isa.h"
 #include "hw/i386/pc.h"
+#include "hw/i386/vmport.h"
 #include "hw/input/i8042.h"
 #include "hw/qdev-properties.h"
 #include "sysemu/hw_accel.h"
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index d5ac76d54e1f..60c988c4a5aa 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -134,19 +134,6 @@ typedef struct PCMachineClass {
 
 GSIState *pc_gsi_create(qemu_irq **irqs, bool pci_enabled);
 
-/* vmport.c */
-#define TYPE_VMPORT "vmport"
-typedef uint32_t (VMPortReadFunc)(void *opaque, uint32_t address);
-
-static inline void vmport_init(ISABus *bus)
-{
-    isa_create_simple(bus, TYPE_VMPORT);
-}
-
-void vmport_register(unsigned char command, VMPortReadFunc *func, void *opaque);
-void vmmouse_get_data(uint32_t *data);
-void vmmouse_set_data(const uint32_t *data);
-
 /* pc.c */
 extern int fd_bootchk;
 
diff --git a/include/hw/i386/vmport.h b/include/hw/i386/vmport.h
new file mode 100644
index 000000000000..f0c1e985ca08
--- /dev/null
+++ b/include/hw/i386/vmport.h
@@ -0,0 +1,16 @@
+#ifndef HW_VMPORT_H
+#define HW_VMPORT_H
+
+#define TYPE_VMPORT "vmport"
+typedef uint32_t (VMPortReadFunc)(void *opaque, uint32_t address);
+
+static inline void vmport_init(ISABus *bus)
+{
+    isa_create_simple(bus, TYPE_VMPORT);
+}
+
+void vmport_register(unsigned char command, VMPortReadFunc *func, void *opaque);
+void vmmouse_get_data(uint32_t *data);
+void vmmouse_set_data(const uint32_t *data);
+
+#endif
-- 
2.20.1

Re: [PATCH v3 07/16] hw/i386/vmport: Introduce vmport.h

[Philippe Mathieu-Daudé]

On 3/12/20 5:54 PM, Liran Alon wrote:
> No functional change. This is mere refactoring.
> 
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> ---
>   hw/i386/pc.c             |  1 +
>   hw/i386/vmmouse.c        |  1 +
>   hw/i386/vmport.c         |  1 +
>   include/hw/i386/pc.h     | 13 -------------
>   include/hw/i386/vmport.h | 16 ++++++++++++++++

What about moving it to hw/i386/vmport.h (no under include/)?

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>   5 files changed, 19 insertions(+), 13 deletions(-)
>   create mode 100644 include/hw/i386/vmport.h
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 6ab4acb0c62e..6ac71e1af32b 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -31,6 +31,7 @@
>   #include "hw/i386/apic.h"
>   #include "hw/i386/topology.h"
>   #include "hw/i386/fw_cfg.h"
> +#include "hw/i386/vmport.h"
>   #include "sysemu/cpus.h"
>   #include "hw/block/fdc.h"
>   #include "hw/ide.h"
> diff --git a/hw/i386/vmmouse.c b/hw/i386/vmmouse.c
> index e8e62bd96b8c..49a546fd3bb6 100644
> --- a/hw/i386/vmmouse.c
> +++ b/hw/i386/vmmouse.c
> @@ -26,6 +26,7 @@
>   #include "qapi/error.h"
>   #include "ui/console.h"
>   #include "hw/i386/pc.h"
> +#include "hw/i386/vmport.h"
>   #include "hw/input/i8042.h"
>   #include "hw/qdev-properties.h"
>   #include "migration/vmstate.h"
> diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c
> index ead2f2d5326f..e9ea5fe7f765 100644
> --- a/hw/i386/vmport.c
> +++ b/hw/i386/vmport.c
> @@ -31,6 +31,7 @@
>   #include "qemu/osdep.h"
>   #include "hw/isa/isa.h"
>   #include "hw/i386/pc.h"
> +#include "hw/i386/vmport.h"
>   #include "hw/input/i8042.h"
>   #include "hw/qdev-properties.h"
>   #include "sysemu/hw_accel.h"
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index d5ac76d54e1f..60c988c4a5aa 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -134,19 +134,6 @@ typedef struct PCMachineClass {
>   
>   GSIState *pc_gsi_create(qemu_irq **irqs, bool pci_enabled);
>   
> -/* vmport.c */
> -#define TYPE_VMPORT "vmport"
> -typedef uint32_t (VMPortReadFunc)(void *opaque, uint32_t address);
> -
> -static inline void vmport_init(ISABus *bus)
> -{
> -    isa_create_simple(bus, TYPE_VMPORT);
> -}
> -
> -void vmport_register(unsigned char command, VMPortReadFunc *func, void *opaque);
> -void vmmouse_get_data(uint32_t *data);
> -void vmmouse_set_data(const uint32_t *data);
> -
>   /* pc.c */
>   extern int fd_bootchk;
>   
> diff --git a/include/hw/i386/vmport.h b/include/hw/i386/vmport.h
> new file mode 100644
> index 000000000000..f0c1e985ca08
> --- /dev/null
> +++ b/include/hw/i386/vmport.h
> @@ -0,0 +1,16 @@
> +#ifndef HW_VMPORT_H
> +#define HW_VMPORT_H
> +
> +#define TYPE_VMPORT "vmport"
> +typedef uint32_t (VMPortReadFunc)(void *opaque, uint32_t address);
> +
> +static inline void vmport_init(ISABus *bus)
> +{
> +    isa_create_simple(bus, TYPE_VMPORT);
> +}
> +
> +void vmport_register(unsigned char command, VMPortReadFunc *func, void *opaque);
> +void vmmouse_get_data(uint32_t *data);
> +void vmmouse_set_data(const uint32_t *data);
> +
> +#endif
> 

Re: [PATCH v3 07/16] hw/i386/vmport: Introduce vmport.h

[Liran Alon]

On 13/03/2020 21:57, Philippe Mathieu-Daudé wrote:
> On 3/12/20 5:54 PM, Liran Alon wrote:
>> No functional change. This is mere refactoring.
>>
>> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Liran Alon <liran.alon@oracle.com>
>> ---
>>   hw/i386/pc.c             |  1 +
>>   hw/i386/vmmouse.c        |  1 +
>>   hw/i386/vmport.c         |  1 +
>>   include/hw/i386/pc.h     | 13 -------------
>>   include/hw/i386/vmport.h | 16 ++++++++++++++++
>
> What about moving it to hw/i386/vmport.h (no under include/)?
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>
>
Can you explain the logic that separates between hw/i386/*.h to 
include/hw/i386/*.h?
If it makes sense, sure I will move it. I just don't know what is the 
convention here.

-Liran

Re: [PATCH v3 07/16] hw/i386/vmport: Introduce vmport.h

[Philippe Mathieu-Daudé]

On 3/13/20 11:38 PM, Liran Alon wrote:
> On 13/03/2020 21:57, Philippe Mathieu-Daudé wrote:
>> On 3/12/20 5:54 PM, Liran Alon wrote:
>>> No functional change. This is mere refactoring.
>>>
>>> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
>>> Signed-off-by: Liran Alon <liran.alon@oracle.com>
>>> ---
>>>   hw/i386/pc.c             |  1 +
>>>   hw/i386/vmmouse.c        |  1 +
>>>   hw/i386/vmport.c         |  1 +
>>>   include/hw/i386/pc.h     | 13 -------------
>>>   include/hw/i386/vmport.h | 16 ++++++++++++++++
>>
>> What about moving it to hw/i386/vmport.h (no under include/)?
>>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>
>>
> Can you explain the logic that separates between hw/i386/*.h to 
> include/hw/i386/*.h?

Headers in the include/hw/ namespace can be consumed by all machine targets.
If this is a target-specific device, having it local to the target 
(hw/i386/) protect generic code (and other targets) of using it. This 
helps detecting wrong dependencies between components.

> If it makes sense, sure I will move it. I just don't know what is the 
> convention here.

Michael/Paolo/Eduardo what do you recommend?

Re: [PATCH v3 07/16] hw/i386/vmport: Introduce vmport.h

[Liran Alon]

On 14/03/2020 10:31, Philippe Mathieu-Daudé wrote:
> On 3/13/20 11:38 PM, Liran Alon wrote:
>> On 13/03/2020 21:57, Philippe Mathieu-Daudé wrote:
>>> On 3/12/20 5:54 PM, Liran Alon wrote:
>>>> No functional change. This is mere refactoring.
>>>>
>>>> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
>>>> Signed-off-by: Liran Alon <liran.alon@oracle.com>
>>>> ---
>>>>   hw/i386/pc.c             |  1 +
>>>>   hw/i386/vmmouse.c        |  1 +
>>>>   hw/i386/vmport.c         |  1 +
>>>>   include/hw/i386/pc.h     | 13 -------------
>>>>   include/hw/i386/vmport.h | 16 ++++++++++++++++
>>>
>>> What about moving it to hw/i386/vmport.h (no under include/)?
>>>
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>
>>>
>> Can you explain the logic that separates between hw/i386/*.h to 
>> include/hw/i386/*.h?
>
> Headers in the include/hw/ namespace can be consumed by all machine 
> targets.
But this doesn't seem true for headers in include/hw/i386/*.h...
It contains things that are target-specific. E.g. ioapic.h, x86-iommu.h, 
intel_iommu.h and etc.
I still don't quite understand the separation between these directories. 
It seems both are i386-specific and one of them shouldn't exists.
> If this is a target-specific device, having it local to the target 
> (hw/i386/) protect generic code (and other targets) of using it. This 
> helps detecting wrong dependencies between components.
>
>> If it makes sense, sure I will move it. I just don't know what is the 
>> convention here.
>
> Michael/Paolo/Eduardo what do you recommend?
>

Re: [PATCH v3 07/16] hw/i386/vmport: Introduce vmport.h

[Michael S. Tsirkin]

On Sat, Mar 14, 2020 at 09:31:31AM +0100, Philippe Mathieu-Daudé wrote:
> On 3/13/20 11:38 PM, Liran Alon wrote:
> > On 13/03/2020 21:57, Philippe Mathieu-Daudé wrote:
> > > On 3/12/20 5:54 PM, Liran Alon wrote:
> > > > No functional change. This is mere refactoring.
> > > > 
> > > > Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> > > > Signed-off-by: Liran Alon <liran.alon@oracle.com>
> > > > ---
> > > >   hw/i386/pc.c             |  1 +
> > > >   hw/i386/vmmouse.c        |  1 +
> > > >   hw/i386/vmport.c         |  1 +
> > > >   include/hw/i386/pc.h     | 13 -------------
> > > >   include/hw/i386/vmport.h | 16 ++++++++++++++++
> > > 
> > > What about moving it to hw/i386/vmport.h (no under include/)?
> > > 
> > > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > > 
> > > 
> > Can you explain the logic that separates between hw/i386/*.h to
> > include/hw/i386/*.h?
> 
> Headers in the include/hw/ namespace can be consumed by all machine targets.
> If this is a target-specific device, having it local to the target
> (hw/i386/) protect generic code (and other targets) of using it. This helps
> detecting wrong dependencies between components.

I think it's true. However when headers were moved to include we
weren't always able to do this correctly. So some i386
specific headers are under include.


> > If it makes sense, sure I will move it. I just don't know what is the
> > convention here.
> 
> Michael/Paolo/Eduardo what do you recommend?

Re: [PATCH v3 07/16] hw/i386/vmport: Introduce vmport.h

[Liran Alon]

On 14/03/2020 20:25, Michael S. Tsirkin wrote:
> On Sat, Mar 14, 2020 at 09:31:31AM +0100, Philippe Mathieu-Daudé wrote:
>> On 3/13/20 11:38 PM, Liran Alon wrote:
>>> On 13/03/2020 21:57, Philippe Mathieu-Daudé wrote:
>>>> On 3/12/20 5:54 PM, Liran Alon wrote:
>>>>> No functional change. This is mere refactoring.
>>>>>
>>>>> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
>>>>> Signed-off-by: Liran Alon <liran.alon@oracle.com>
>>>>> ---
>>>>>    hw/i386/pc.c             |  1 +
>>>>>    hw/i386/vmmouse.c        |  1 +
>>>>>    hw/i386/vmport.c         |  1 +
>>>>>    include/hw/i386/pc.h     | 13 -------------
>>>>>    include/hw/i386/vmport.h | 16 ++++++++++++++++
>>>> What about moving it to hw/i386/vmport.h (no under include/)?
>>>>
>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>>
>>>>
>>> Can you explain the logic that separates between hw/i386/*.h to
>>> include/hw/i386/*.h?
>> Headers in the include/hw/ namespace can be consumed by all machine targets.
>> If this is a target-specific device, having it local to the target
>> (hw/i386/) protect generic code (and other targets) of using it. This helps
>> detecting wrong dependencies between components.
> I think it's true. However when headers were moved to include we
> weren't always able to do this correctly. So some i386
> specific headers are under include.
>
OK. So if I understand correctly, you also support moving this header to 
hw/i386/ instead of include/hw/i386/.
So I will do so in v4.

Thanks,
-Liran

[PATCH v3 08/16] hw/i386/vmport: Define enum for all commands

[Liran Alon]

No functional change.

Defining an enum for all VMPort commands have the following advantages:
* It gets rid of the error-prone requirement to update VMPORT_ENTRIES
when new VMPort commands are added to QEMU.
* It makes it clear to know by looking at one place at the source, what
are all the VMPort commands supported by QEMU.

Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/i386/vmmouse.c        | 18 ++++++------------
 hw/i386/vmport.c         | 11 ++---------
 include/hw/i386/vmport.h | 11 ++++++++++-
 3 files changed, 18 insertions(+), 22 deletions(-)

diff --git a/hw/i386/vmmouse.c b/hw/i386/vmmouse.c
index 49a546fd3bb6..afb8e9e09a30 100644
--- a/hw/i386/vmmouse.c
+++ b/hw/i386/vmmouse.c
@@ -34,12 +34,6 @@
 /* debug only vmmouse */
 //#define DEBUG_VMMOUSE
 
-/* VMMouse Commands */
-#define VMMOUSE_GETVERSION	10
-#define VMMOUSE_DATA		39
-#define VMMOUSE_STATUS		40
-#define VMMOUSE_COMMAND		41
-
 #define VMMOUSE_READ_ID			0x45414552
 #define VMMOUSE_DISABLE			0x000000f5
 #define VMMOUSE_REQUEST_RELATIVE	0x4c455252
@@ -197,10 +191,10 @@ static uint32_t vmmouse_ioport_read(void *opaque, uint32_t addr)
     command = data[2] & 0xFFFF;
 
     switch (command) {
-    case VMMOUSE_STATUS:
+    case VMPORT_CMD_VMMOUSE_STATUS:
         data[0] = vmmouse_get_status(s);
         break;
-    case VMMOUSE_COMMAND:
+    case VMPORT_CMD_VMMOUSE_COMMAND:
         switch (data[1]) {
         case VMMOUSE_DISABLE:
             vmmouse_disable(s);
@@ -219,7 +213,7 @@ static uint32_t vmmouse_ioport_read(void *opaque, uint32_t addr)
             break;
         }
         break;
-    case VMMOUSE_DATA:
+    case VMPORT_CMD_VMMOUSE_DATA:
         vmmouse_data(s, data, data[1]);
         break;
     default:
@@ -276,9 +270,9 @@ static void vmmouse_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    vmport_register(VMMOUSE_STATUS, vmmouse_ioport_read, s);
-    vmport_register(VMMOUSE_COMMAND, vmmouse_ioport_read, s);
-    vmport_register(VMMOUSE_DATA, vmmouse_ioport_read, s);
+    vmport_register(VMPORT_CMD_VMMOUSE_STATUS, vmmouse_ioport_read, s);
+    vmport_register(VMPORT_CMD_VMMOUSE_COMMAND, vmmouse_ioport_read, s);
+    vmport_register(VMPORT_CMD_VMMOUSE_DATA, vmmouse_ioport_read, s);
 }
 
 static Property vmmouse_properties[] = {
diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c
index e9ea5fe7f765..6ab094311d6c 100644
--- a/hw/i386/vmport.c
+++ b/hw/i386/vmport.c
@@ -38,10 +38,6 @@
 #include "qemu/log.h"
 #include "trace.h"
 
-#define VMPORT_CMD_GETVERSION 0x0a
-#define VMPORT_CMD_GETRAMSIZE 0x14
-
-#define VMPORT_ENTRIES 0x2c
 #define VMPORT_MAGIC   0x564D5868
 
 /* Compatibility flags for migration */
@@ -72,12 +68,9 @@ typedef struct VMPortState {
 
 static VMPortState *port_state;
 
-void vmport_register(unsigned char command, VMPortReadFunc *func, void *opaque)
+void vmport_register(VMPortCommand command, VMPortReadFunc *func, void *opaque)
 {
-    if (command >= VMPORT_ENTRIES) {
-        return;
-    }
-
+    assert(command < VMPORT_ENTRIES);
     trace_vmport_register(command, func, opaque);
     port_state->func[command] = func;
     port_state->opaque[command] = opaque;
diff --git a/include/hw/i386/vmport.h b/include/hw/i386/vmport.h
index f0c1e985ca08..0501ccac6ddf 100644
--- a/include/hw/i386/vmport.h
+++ b/include/hw/i386/vmport.h
@@ -4,12 +4,21 @@
 #define TYPE_VMPORT "vmport"
 typedef uint32_t (VMPortReadFunc)(void *opaque, uint32_t address);
 
+typedef enum {
+    VMPORT_CMD_GETVERSION       = 10,
+    VMPORT_CMD_GETRAMSIZE       = 20,
+    VMPORT_CMD_VMMOUSE_DATA     = 39,
+    VMPORT_CMD_VMMOUSE_STATUS   = 40,
+    VMPORT_CMD_VMMOUSE_COMMAND  = 41,
+    VMPORT_ENTRIES
+} VMPortCommand;
+
 static inline void vmport_init(ISABus *bus)
 {
     isa_create_simple(bus, TYPE_VMPORT);
 }
 
-void vmport_register(unsigned char command, VMPortReadFunc *func, void *opaque);
+void vmport_register(VMPortCommand command, VMPortReadFunc *func, void *opaque);
 void vmmouse_get_data(uint32_t *data);
 void vmmouse_set_data(const uint32_t *data);
 
-- 
2.20.1

Re: [PATCH v3 08/16] hw/i386/vmport: Define enum for all commands

[Philippe Mathieu-Daudé]

On 3/12/20 5:54 PM, Liran Alon wrote:
> No functional change.
> 
> Defining an enum for all VMPort commands have the following advantages:
> * It gets rid of the error-prone requirement to update VMPORT_ENTRIES
> when new VMPort commands are added to QEMU.
> * It makes it clear to know by looking at one place at the source, what
> are all the VMPort commands supported by QEMU.
> 
> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> ---
>   hw/i386/vmmouse.c        | 18 ++++++------------
>   hw/i386/vmport.c         | 11 ++---------
>   include/hw/i386/vmport.h | 11 ++++++++++-

Please setup scripts/git.orderfile to ease reviews.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>   3 files changed, 18 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/i386/vmmouse.c b/hw/i386/vmmouse.c
> index 49a546fd3bb6..afb8e9e09a30 100644
> --- a/hw/i386/vmmouse.c
> +++ b/hw/i386/vmmouse.c
> @@ -34,12 +34,6 @@
>   /* debug only vmmouse */
>   //#define DEBUG_VMMOUSE
>   
> -/* VMMouse Commands */
> -#define VMMOUSE_GETVERSION	10
> -#define VMMOUSE_DATA		39
> -#define VMMOUSE_STATUS		40
> -#define VMMOUSE_COMMAND		41
> -
>   #define VMMOUSE_READ_ID			0x45414552
>   #define VMMOUSE_DISABLE			0x000000f5
>   #define VMMOUSE_REQUEST_RELATIVE	0x4c455252
> @@ -197,10 +191,10 @@ static uint32_t vmmouse_ioport_read(void *opaque, uint32_t addr)
>       command = data[2] & 0xFFFF;
>   
>       switch (command) {
> -    case VMMOUSE_STATUS:
> +    case VMPORT_CMD_VMMOUSE_STATUS:
>           data[0] = vmmouse_get_status(s);
>           break;
> -    case VMMOUSE_COMMAND:
> +    case VMPORT_CMD_VMMOUSE_COMMAND:
>           switch (data[1]) {
>           case VMMOUSE_DISABLE:
>               vmmouse_disable(s);
> @@ -219,7 +213,7 @@ static uint32_t vmmouse_ioport_read(void *opaque, uint32_t addr)
>               break;
>           }
>           break;
> -    case VMMOUSE_DATA:
> +    case VMPORT_CMD_VMMOUSE_DATA:
>           vmmouse_data(s, data, data[1]);
>           break;
>       default:
> @@ -276,9 +270,9 @@ static void vmmouse_realizefn(DeviceState *dev, Error **errp)
>           return;
>       }
>   
> -    vmport_register(VMMOUSE_STATUS, vmmouse_ioport_read, s);
> -    vmport_register(VMMOUSE_COMMAND, vmmouse_ioport_read, s);
> -    vmport_register(VMMOUSE_DATA, vmmouse_ioport_read, s);
> +    vmport_register(VMPORT_CMD_VMMOUSE_STATUS, vmmouse_ioport_read, s);
> +    vmport_register(VMPORT_CMD_VMMOUSE_COMMAND, vmmouse_ioport_read, s);
> +    vmport_register(VMPORT_CMD_VMMOUSE_DATA, vmmouse_ioport_read, s);
>   }
>   
>   static Property vmmouse_properties[] = {
> diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c
> index e9ea5fe7f765..6ab094311d6c 100644
> --- a/hw/i386/vmport.c
> +++ b/hw/i386/vmport.c
> @@ -38,10 +38,6 @@
>   #include "qemu/log.h"
>   #include "trace.h"
>   
> -#define VMPORT_CMD_GETVERSION 0x0a
> -#define VMPORT_CMD_GETRAMSIZE 0x14
> -
> -#define VMPORT_ENTRIES 0x2c
>   #define VMPORT_MAGIC   0x564D5868
>   
>   /* Compatibility flags for migration */
> @@ -72,12 +68,9 @@ typedef struct VMPortState {
>   
>   static VMPortState *port_state;
>   
> -void vmport_register(unsigned char command, VMPortReadFunc *func, void *opaque)
> +void vmport_register(VMPortCommand command, VMPortReadFunc *func, void *opaque)
>   {
> -    if (command >= VMPORT_ENTRIES) {
> -        return;
> -    }
> -
> +    assert(command < VMPORT_ENTRIES);
>       trace_vmport_register(command, func, opaque);
>       port_state->func[command] = func;
>       port_state->opaque[command] = opaque;
> diff --git a/include/hw/i386/vmport.h b/include/hw/i386/vmport.h
> index f0c1e985ca08..0501ccac6ddf 100644
> --- a/include/hw/i386/vmport.h
> +++ b/include/hw/i386/vmport.h
> @@ -4,12 +4,21 @@
>   #define TYPE_VMPORT "vmport"
>   typedef uint32_t (VMPortReadFunc)(void *opaque, uint32_t address);
>   
> +typedef enum {
> +    VMPORT_CMD_GETVERSION       = 10,
> +    VMPORT_CMD_GETRAMSIZE       = 20,
> +    VMPORT_CMD_VMMOUSE_DATA     = 39,
> +    VMPORT_CMD_VMMOUSE_STATUS   = 40,
> +    VMPORT_CMD_VMMOUSE_COMMAND  = 41,
> +    VMPORT_ENTRIES
> +} VMPortCommand;
> +
>   static inline void vmport_init(ISABus *bus)
>   {
>       isa_create_simple(bus, TYPE_VMPORT);
>   }
>   
> -void vmport_register(unsigned char command, VMPortReadFunc *func, void *opaque);
> +void vmport_register(VMPortCommand command, VMPortReadFunc *func, void *opaque);
>   void vmmouse_get_data(uint32_t *data);
>   void vmmouse_set_data(const uint32_t *data);
>   
> 

Re: [PATCH v3 08/16] hw/i386/vmport: Define enum for all commands

[Philippe Mathieu-Daudé]

On 3/13/20 8:59 PM, Philippe Mathieu-Daudé wrote:
> On 3/12/20 5:54 PM, Liran Alon wrote:
>> No functional change.
>>
>> Defining an enum for all VMPort commands have the following advantages:
>> * It gets rid of the error-prone requirement to update VMPORT_ENTRIES
>> when new VMPort commands are added to QEMU.
>> * It makes it clear to know by looking at one place at the source, what
>> are all the VMPort commands supported by QEMU.
>>
>> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
>> Signed-off-by: Liran Alon <liran.alon@oracle.com>
>> ---
>>   hw/i386/vmmouse.c        | 18 ++++++------------
>>   hw/i386/vmport.c         | 11 ++---------
>>   include/hw/i386/vmport.h | 11 ++++++++++-
> 
> Please setup scripts/git.orderfile to ease reviews.
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
>>   3 files changed, 18 insertions(+), 22 deletions(-)
>>
>> diff --git a/hw/i386/vmmouse.c b/hw/i386/vmmouse.c
>> index 49a546fd3bb6..afb8e9e09a30 100644
>> --- a/hw/i386/vmmouse.c
>> +++ b/hw/i386/vmmouse.c
>> @@ -34,12 +34,6 @@
>>   /* debug only vmmouse */
>>   //#define DEBUG_VMMOUSE
>> -/* VMMouse Commands */
>> -#define VMMOUSE_GETVERSION    10
>> -#define VMMOUSE_DATA        39
>> -#define VMMOUSE_STATUS        40
>> -#define VMMOUSE_COMMAND        41
>> -
>>   #define VMMOUSE_READ_ID            0x45414552
>>   #define VMMOUSE_DISABLE            0x000000f5
>>   #define VMMOUSE_REQUEST_RELATIVE    0x4c455252
>> @@ -197,10 +191,10 @@ static uint32_t vmmouse_ioport_read(void 
>> *opaque, uint32_t addr)
>>       command = data[2] & 0xFFFF;
>>       switch (command) {
>> -    case VMMOUSE_STATUS:
>> +    case VMPORT_CMD_VMMOUSE_STATUS:
>>           data[0] = vmmouse_get_status(s);
>>           break;
>> -    case VMMOUSE_COMMAND:
>> +    case VMPORT_CMD_VMMOUSE_COMMAND:
>>           switch (data[1]) {
>>           case VMMOUSE_DISABLE:
>>               vmmouse_disable(s);
>> @@ -219,7 +213,7 @@ static uint32_t vmmouse_ioport_read(void *opaque, 
>> uint32_t addr)
>>               break;
>>           }
>>           break;
>> -    case VMMOUSE_DATA:
>> +    case VMPORT_CMD_VMMOUSE_DATA:
>>           vmmouse_data(s, data, data[1]);
>>           break;
>>       default:
>> @@ -276,9 +270,9 @@ static void vmmouse_realizefn(DeviceState *dev, 
>> Error **errp)
>>           return;
>>       }
>> -    vmport_register(VMMOUSE_STATUS, vmmouse_ioport_read, s);
>> -    vmport_register(VMMOUSE_COMMAND, vmmouse_ioport_read, s);
>> -    vmport_register(VMMOUSE_DATA, vmmouse_ioport_read, s);
>> +    vmport_register(VMPORT_CMD_VMMOUSE_STATUS, vmmouse_ioport_read, s);
>> +    vmport_register(VMPORT_CMD_VMMOUSE_COMMAND, vmmouse_ioport_read, s);
>> +    vmport_register(VMPORT_CMD_VMMOUSE_DATA, vmmouse_ioport_read, s);
>>   }
>>   static Property vmmouse_properties[] = {
>> diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c
>> index e9ea5fe7f765..6ab094311d6c 100644
>> --- a/hw/i386/vmport.c
>> +++ b/hw/i386/vmport.c
>> @@ -38,10 +38,6 @@
>>   #include "qemu/log.h"
>>   #include "trace.h"
>> -#define VMPORT_CMD_GETVERSION 0x0a
>> -#define VMPORT_CMD_GETRAMSIZE 0x14
>> -
>> -#define VMPORT_ENTRIES 0x2c
>>   #define VMPORT_MAGIC   0x564D5868
>>   /* Compatibility flags for migration */
>> @@ -72,12 +68,9 @@ typedef struct VMPortState {
>>   static VMPortState *port_state;
>> -void vmport_register(unsigned char command, VMPortReadFunc *func, 
>> void *opaque)
>> +void vmport_register(VMPortCommand command, VMPortReadFunc *func, 
>> void *opaque)
>>   {
>> -    if (command >= VMPORT_ENTRIES) {
>> -        return;
>> -    }
>> -
>> +    assert(command < VMPORT_ENTRIES);
>>       trace_vmport_register(command, func, opaque);
>>       port_state->func[command] = func;
>>       port_state->opaque[command] = opaque;
>> diff --git a/include/hw/i386/vmport.h b/include/hw/i386/vmport.h
>> index f0c1e985ca08..0501ccac6ddf 100644
>> --- a/include/hw/i386/vmport.h
>> +++ b/include/hw/i386/vmport.h
>> @@ -4,12 +4,21 @@
>>   #define TYPE_VMPORT "vmport"
>>   typedef uint32_t (VMPortReadFunc)(void *opaque, uint32_t address);
>> +typedef enum {
>> +    VMPORT_CMD_GETVERSION       = 10,

Can we rename as VMPORT_CMD_GET_VERSION which is easier to read?

>> +    VMPORT_CMD_GETRAMSIZE       = 20,

Ditto _GET_RAM_SIZE?

>> +    VMPORT_CMD_VMMOUSE_DATA     = 39,
>> +    VMPORT_CMD_VMMOUSE_STATUS   = 40,
>> +    VMPORT_CMD_VMMOUSE_COMMAND  = 41,
>> +    VMPORT_ENTRIES
>> +} VMPortCommand;
>> +
>>   static inline void vmport_init(ISABus *bus)
>>   {
>>       isa_create_simple(bus, TYPE_VMPORT);
>>   }
>> -void vmport_register(unsigned char command, VMPortReadFunc *func, 
>> void *opaque);
>> +void vmport_register(VMPortCommand command, VMPortReadFunc *func, 
>> void *opaque);
>>   void vmmouse_get_data(uint32_t *data);
>>   void vmmouse_set_data(const uint32_t *data);
>>

Re: [PATCH v3 08/16] hw/i386/vmport: Define enum for all commands

[Liran Alon]

On 13/03/2020 22:05, Philippe Mathieu-Daudé wrote:
> On 3/13/20 8:59 PM, Philippe Mathieu-Daudé wrote:
>> On 3/12/20 5:54 PM, Liran Alon wrote:
>>> --- a/include/hw/i386/vmport.h
>>> +++ b/include/hw/i386/vmport.h
>>> @@ -4,12 +4,21 @@
>>>   #define TYPE_VMPORT "vmport"
>>>   typedef uint32_t (VMPortReadFunc)(void *opaque, uint32_t address);
>>> +typedef enum {
>>> +    VMPORT_CMD_GETVERSION       = 10,
>
> Can we rename as VMPORT_CMD_GET_VERSION which is easier to read?
Sure. Will do on v4.
>
>>> +    VMPORT_CMD_GETRAMSIZE       = 20,
>
> Ditto _GET_RAM_SIZE?
Sure. Will do in v4.

-Liran

Re: [PATCH v3 08/16] hw/i386/vmport: Define enum for all commands

[Liran Alon]

On 13/03/2020 21:59, Philippe Mathieu-Daudé wrote:
> On 3/12/20 5:54 PM, Liran Alon wrote:
>> No functional change.
>>
>> Defining an enum for all VMPort commands have the following advantages:
>> * It gets rid of the error-prone requirement to update VMPORT_ENTRIES
>> when new VMPort commands are added to QEMU.
>> * It makes it clear to know by looking at one place at the source, what
>> are all the VMPort commands supported by QEMU.
>>
>> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
>> Signed-off-by: Liran Alon <liran.alon@oracle.com>
>> ---
>>   hw/i386/vmmouse.c        | 18 ++++++------------
>>   hw/i386/vmport.c         | 11 ++---------
>>   include/hw/i386/vmport.h | 11 ++++++++++-
>
> Please setup scripts/git.orderfile to ease reviews.
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>
Oh ok. Will do for next submissions.

Thanks,
-Liran

[PATCH v3 09/16] hw/i386/vmport: Add support for CMD_GETBIOSUUID

[Liran Alon]

This is VMware documented functionallity that some guests rely on.
Returns the BIOS UUID of the current virtual machine.

Note that we also introduce a new compatability flag "x-cmds-v2" to
make sure to expose new VMPort commands only to new machine-types.
This flag will also be used by the following patches that will introduce
additional VMPort commands.

Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/core/machine.c        |  1 +
 hw/i386/vmport.c         | 22 ++++++++++++++++++++++
 include/hw/i386/vmport.h |  1 +
 3 files changed, 24 insertions(+)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 0e2f37420360..523fa56991dc 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -42,6 +42,7 @@ GlobalProperty hw_compat_4_2[] = {
     { "vmport", "x-read-set-eax", "off" },
     { "vmport", "x-signal-unsupported-cmd", "off" },
     { "vmport", "x-report-vmx-type", "off" },
+    { "vmport", "x-cmds-v2", "off" },
 };
 const size_t hw_compat_4_2_len = G_N_ELEMENTS(hw_compat_4_2);
 
diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c
index 6ab094311d6c..3fb8a8bd458a 100644
--- a/hw/i386/vmport.c
+++ b/hw/i386/vmport.c
@@ -34,6 +34,7 @@
 #include "hw/i386/vmport.h"
 #include "hw/input/i8042.h"
 #include "hw/qdev-properties.h"
+#include "sysemu/sysemu.h"
 #include "sysemu/hw_accel.h"
 #include "qemu/log.h"
 #include "trace.h"
@@ -44,12 +45,15 @@
 #define VMPORT_COMPAT_READ_SET_EAX_BIT              0
 #define VMPORT_COMPAT_SIGNAL_UNSUPPORTED_CMD_BIT    1
 #define VMPORT_COMPAT_REPORT_VMX_TYPE_BIT           2
+#define VMPORT_COMPAT_CMDS_V2_BIT                   3
 #define VMPORT_COMPAT_READ_SET_EAX              \
     (1 << VMPORT_COMPAT_READ_SET_EAX_BIT)
 #define VMPORT_COMPAT_SIGNAL_UNSUPPORTED_CMD    \
     (1 << VMPORT_COMPAT_SIGNAL_UNSUPPORTED_CMD_BIT)
 #define VMPORT_COMPAT_REPORT_VMX_TYPE           \
     (1 << VMPORT_COMPAT_REPORT_VMX_TYPE_BIT)
+#define VMPORT_COMPAT_CMDS_V2                   \
+    (1 << VMPORT_COMPAT_CMDS_V2_BIT)
 
 #define VMPORT(obj) OBJECT_CHECK(VMPortState, (obj), TYPE_VMPORT)
 
@@ -144,6 +148,18 @@ static uint32_t vmport_cmd_get_version(void *opaque, uint32_t addr)
     return port_state->vmware_vmx_version;
 }
 
+static uint32_t vmport_cmd_get_bios_uuid(void *opaque, uint32_t addr)
+{
+    X86CPU *cpu = X86_CPU(current_cpu);
+    uint32_t *uuid_parts = (uint32_t *)(qemu_uuid.data);
+
+    cpu->env.regs[R_EAX] = le32_to_cpu(uuid_parts[0]);
+    cpu->env.regs[R_EBX] = le32_to_cpu(uuid_parts[1]);
+    cpu->env.regs[R_ECX] = le32_to_cpu(uuid_parts[2]);
+    cpu->env.regs[R_EDX] = le32_to_cpu(uuid_parts[3]);
+    return cpu->env.regs[R_EAX];
+}
+
 static uint32_t vmport_cmd_ram_size(void *opaque, uint32_t addr)
 {
     X86CPU *cpu = X86_CPU(current_cpu);
@@ -192,9 +208,13 @@ static void vmport_realizefn(DeviceState *dev, Error **errp)
     isa_register_ioport(isadev, &s->io, 0x5658);
 
     port_state = s;
+
     /* Register some generic port commands */
     vmport_register(VMPORT_CMD_GETVERSION, vmport_cmd_get_version, NULL);
     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);
+    }
 }
 
 static Property vmport_properties[] = {
@@ -205,6 +225,8 @@ static Property vmport_properties[] = {
                     VMPORT_COMPAT_SIGNAL_UNSUPPORTED_CMD_BIT, true),
     DEFINE_PROP_BIT("x-report-vmx-type", VMPortState, compat_flags,
                     VMPORT_COMPAT_REPORT_VMX_TYPE_BIT, true),
+    DEFINE_PROP_BIT("x-cmds-v2", VMPortState, compat_flags,
+                    VMPORT_COMPAT_CMDS_V2_BIT, true),
 
     /* Default value taken from open-vm-tools code VERSION_MAGIC definition */
     DEFINE_PROP_UINT32("vmware-vmx-version", VMPortState,
diff --git a/include/hw/i386/vmport.h b/include/hw/i386/vmport.h
index 0501ccac6ddf..7f33512ca6f0 100644
--- a/include/hw/i386/vmport.h
+++ b/include/hw/i386/vmport.h
@@ -6,6 +6,7 @@ typedef uint32_t (VMPortReadFunc)(void *opaque, uint32_t address);
 
 typedef enum {
     VMPORT_CMD_GETVERSION       = 10,
+    VMPORT_CMD_GETBIOSUUID      = 19,
     VMPORT_CMD_GETRAMSIZE       = 20,
     VMPORT_CMD_VMMOUSE_DATA     = 39,
     VMPORT_CMD_VMMOUSE_STATUS   = 40,
-- 
2.20.1

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

[Liran Alon]

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)
 {
@@ -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.
+     * Value taken from VMware Workstation 5.5.
+     **/
+    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

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

[Michael S. Tsirkin]

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

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

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

[Michael S. Tsirkin]

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.



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

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


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


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

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

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

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
> 

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

[Liran Alon]

On 14/03/2020 20:18, Michael S. Tsirkin wrote:
> 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.
>
So guest doesn't assume that this time only moves forward...

Can you clarify then which change do you suggest making to this patch in 
this regard? It seems correct to me.
i.e. The CMD_GETTIME implementation seems correct to me and it doesn't 
need to do anything special to handle host time moving backwards.

-Liran

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

[Michael S. Tsirkin]

On Sat, Mar 14, 2020 at 09:04:30PM +0200, Liran Alon wrote:
> 
> On 14/03/2020 20:18, Michael S. Tsirkin wrote:
> > 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.
> > 
> So guest doesn't assume that this time only moves forward...
> 
> Can you clarify then which change do you suggest making to this patch in
> this regard? It seems correct to me.
> i.e. The CMD_GETTIME implementation seems correct to me and it doesn't need
> to do anything special to handle host time moving backwards.
> 
> -Liran
> 

I think something needs to be done to make sure host time as reported by
this command does not move backwards significantly. Just forwarding
gettimeofday won't cut it IMHO. Look at kvm clock for inspiration of
things to do.


-- 
MST

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

[Liran Alon]

On 14/03/2020 21:14, Michael S. Tsirkin wrote:
> On Sat, Mar 14, 2020 at 09:04:30PM +0200, Liran Alon wrote:
>> On 14/03/2020 20:18, Michael S. Tsirkin wrote:
>>> 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.
>>>
>> So guest doesn't assume that this time only moves forward...
>>
>> Can you clarify then which change do you suggest making to this patch in
>> this regard? It seems correct to me.
>> i.e. The CMD_GETTIME implementation seems correct to me and it doesn't need
>> to do anything special to handle host time moving backwards.
>>
>> -Liran
>>
> I think something needs to be done to make sure host time as reported by
> this command does not move backwards significantly. Just forwarding
> gettimeofday won't cut it IMHO. Look at kvm clock for inspiration of
> things to do.
>
It isn't required by the command interface definition.
It's explicitly specified in timeSync.c that guest code handles the case 
host time goes backwards.
We are not inventing the interface, we just implement it according to 
it's definition.

-Liran

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

[Michael S. Tsirkin]

On Sat, Mar 14, 2020 at 09:17:30PM +0200, Liran Alon wrote:
> 
> On 14/03/2020 21:14, Michael S. Tsirkin wrote:
> > On Sat, Mar 14, 2020 at 09:04:30PM +0200, Liran Alon wrote:
> > > On 14/03/2020 20:18, Michael S. Tsirkin wrote:
> > > > 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.
> > > > 
> > > So guest doesn't assume that this time only moves forward...
> > > 
> > > Can you clarify then which change do you suggest making to this patch in
> > > this regard? It seems correct to me.
> > > i.e. The CMD_GETTIME implementation seems correct to me and it doesn't need
> > > to do anything special to handle host time moving backwards.
> > > 
> > > -Liran
> > > 
> > I think something needs to be done to make sure host time as reported by
> > this command does not move backwards significantly. Just forwarding
> > gettimeofday won't cut it IMHO. Look at kvm clock for inspiration of
> > things to do.
> > 
> It isn't required by the command interface definition.
> It's explicitly specified in timeSync.c that guest code handles the case
> host time goes backwards.

According to what you wrote it does not crash but also does not do
updates. That will work well only if the time difference isn't large.
Then with time as guest gets interrupted by host, the time difference
shrinks and eventually you are resyncing again.  And I'm guessing there
are hypervisor management interfaces in place to make sure that is so.
Linux is not guaranteed to have such interfaces so you have to come
up with QEMU specific ones.


> We are not inventing the interface, we just implement it according to it's
> definition.
> 
> -Liran

Host time is a vague enough terminology. I suspect this implementation
is too simplistic to cover all cases.

-- 
MST

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

[Nikita Leshenko]

> On 14 Mar 2020, at 21:26, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> On Sat, Mar 14, 2020 at 09:17:30PM +0200, Liran Alon wrote:
>> 
>> On 14/03/2020 21:14, Michael S. Tsirkin wrote:
>>> On Sat, Mar 14, 2020 at 09:04:30PM +0200, Liran Alon wrote:
>>>> On 14/03/2020 20:18, Michael S. Tsirkin wrote:
>>>>> 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.
>>>>> 
>>>> So guest doesn't assume that this time only moves forward...
>>>> 
>>>> Can you clarify then which change do you suggest making to this patch in
>>>> this regard? It seems correct to me.
>>>> i.e. The CMD_GETTIME implementation seems correct to me and it doesn't need
>>>> to do anything special to handle host time moving backwards.
>>>> 
>>>> -Liran
>>>> 
>>> I think something needs to be done to make sure host time as reported by
>>> this command does not move backwards significantly. Just forwarding
>>> gettimeofday won't cut it IMHO. Look at kvm clock for inspiration of
>>> things to do.
>>> 
>> It isn't required by the command interface definition.
>> It's explicitly specified in timeSync.c that guest code handles the case
>> host time goes backwards.
> 
> According to what you wrote it does not crash but also does not do
> updates. That will work well only if the time difference isn't large.
> Then with time as guest gets interrupted by host, the time difference
> shrinks and eventually you are resyncing again.  And I'm guessing there
> are hypervisor management interfaces in place to make sure that is so.
> Linux is not guaranteed to have such interfaces so you have to come
> up with QEMU specific ones.
> 
> 
>> We are not inventing the interface, we just implement it according to it's
>> definition.
>> 
>> -Liran
> 
> Host time is a vague enough terminology. I suspect this implementation
> is too simplistic to cover all cases.
> 
> -- 
> MST

I saw this discussion from the side and I just wanted to point out that as far
as I understand this interface is needed to sync *wallclock time* between the
guest and the host, mostly for user experience. There is no guarantee that it's
monotonic. Unlike TSC, we don't need to take special care with regard to live
migration or drift.

Just as NTP may report inconsistent time, it's OK if this interface is somewhat
inconsistent.

I think that the reason that open-vm-tools doesn't move time backwards is to
help applications that treat wallclock time as if it's monotonic time and break
if the date is moved backwards (which may happen more frequently in virtual
environment so it's handled specifically). But this doesn't change the fact that
this PV interface is for reporting wall time. So I couldn't understand what
source other than gettimeofday() you were suggesting for Liran to use to report
wallclock time.

Nikita

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

[Liran Alon]

On 14/03/2020 21:58, Nikita Leshenko wrote:
>
>> On 14 Mar 2020, at 21:26, Michael S. Tsirkin <mst@redhat.com> wrote:
>>
>> On Sat, Mar 14, 2020 at 09:17:30PM +0200, Liran Alon wrote:
>>> On 14/03/2020 21:14, Michael S. Tsirkin wrote:
>>>> On Sat, Mar 14, 2020 at 09:04:30PM +0200, Liran Alon wrote:
>>>>> On 14/03/2020 20:18, Michael S. Tsirkin wrote:
>>>>>> 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.
>>>>>>
>>>>> So guest doesn't assume that this time only moves forward...
>>>>>
>>>>> Can you clarify then which change do you suggest making to this patch in
>>>>> this regard? It seems correct to me.
>>>>> i.e. The CMD_GETTIME implementation seems correct to me and it doesn't need
>>>>> to do anything special to handle host time moving backwards.
>>>>>
>>>>> -Liran
>>>>>
>>>> I think something needs to be done to make sure host time as reported by
>>>> this command does not move backwards significantly. Just forwarding
>>>> gettimeofday won't cut it IMHO. Look at kvm clock for inspiration of
>>>> things to do.
>>>>
>>> It isn't required by the command interface definition.
>>> It's explicitly specified in timeSync.c that guest code handles the case
>>> host time goes backwards.
>> According to what you wrote it does not crash but also does not do
>> updates. That will work well only if the time difference isn't large.
>> Then with time as guest gets interrupted by host, the time difference
>> shrinks and eventually you are resyncing again.  And I'm guessing there
>> are hypervisor management interfaces in place to make sure that is so.
>> Linux is not guaranteed to have such interfaces so you have to come
>> up with QEMU specific ones.
>>
>>
>>> We are not inventing the interface, we just implement it according to it's
>>> definition.
>>>
>>> -Liran
>> Host time is a vague enough terminology. I suspect this implementation
>> is too simplistic to cover all cases.
>>
>> -- 
>> MST
> I saw this discussion from the side and I just wanted to point out that as far
> as I understand this interface is needed to sync *wallclock time* between the
> guest and the host, mostly for user experience. There is no guarantee that it's
> monotonic. Unlike TSC, we don't need to take special care with regard to live
> migration or drift.
>
> Just as NTP may report inconsistent time, it's OK if this interface is somewhat
> inconsistent.
>
> I think that the reason that open-vm-tools doesn't move time backwards is to
> help applications that treat wallclock time as if it's monotonic time and break
> if the date is moved backwards (which may happen more frequently in virtual
> environment so it's handled specifically). But this doesn't change the fact that
> this PV interface is for reporting wall time. So I couldn't understand what
> source other than gettimeofday() you were suggesting for Liran to use to report
> wallclock time.
>
> Nikita
>
Michael, you can also refer to this VMware time-keeping whitepaper: 
https://www.vmware.com/pdf/vmware_timekeeping.pdf.
According to section "Initializing and Correcting Wall-Clock Time":
"""
VMware Tools can also optionally be used to correct long‐term drift and 
errors by periodically
resynchronizing the virtual machine’s clock to the host’s clock, but the 
current version at this writing is limited.
In particular, in guest operating systems other than NetWare, it does 
not correct errors in which the guest clock
is ahead of real time, only those in which the guest clock is behind.

"""

If I understand correctly, this seems to validate my assumption that 
current implementation for CMD_GETTIME is sufficient.

-Liran

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

[Michael S. Tsirkin]

On Sat, Mar 14, 2020 at 10:05:20PM +0200, Liran Alon wrote:
> Michael, you can also refer to this VMware time-keeping whitepaper:
> https://www.vmware.com/pdf/vmware_timekeeping.pdf.
> According to section "Initializing and Correcting Wall-Clock Time":
> """
> VMware Tools can also optionally be used to correct long‐term drift and
> errors by periodically
> resynchronizing the virtual machine’s clock to the host’s clock, but the
> current version at this writing is limited.
> In particular, in guest operating systems other than NetWare, it does not
> correct errors in which the guest clock
> is ahead of real time, only those in which the guest clock is behind.
> 
> """

This talks about guest time.
What this does not mention is whether hosts need to employ any mechanisms
to synchronise wall clock between hosts.

> If I understand correctly, this seems to validate my assumption that current
> implementation for CMD_GETTIME is sufficient.

So I am concerned this does not interact well with other time sources
in QEMU. For example, it's very useful to set guest time with -rtc base
flag.

Can you use qemu_get_timedate?



-- 
MST

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

[Liran Alon]

On 14/03/2020 22:56, Michael S. Tsirkin wrote:
> On Sat, Mar 14, 2020 at 10:05:20PM +0200, Liran Alon wrote:
>> Michael, you can also refer to this VMware time-keeping whitepaper:
>> https://urldefense.com/v3/__https://www.vmware.com/pdf/vmware_timekeeping.pdf__;!!GqivPVa7Brio!K8sfnfvVgKwrQ4SMwX-K6-S5yR4ln9_qZ6o4GzIpQkohfWtinlplNhXzFlyUgks$ .
>> According to section "Initializing and Correcting Wall-Clock Time":
>> """
>> VMware Tools can also optionally be used to correct long‐term drift and
>> errors by periodically
>> resynchronizing the virtual machine’s clock to the host’s clock, but the
>> current version at this writing is limited.
>> In particular, in guest operating systems other than NetWare, it does not
>> correct errors in which the guest clock
>> is ahead of real time, only those in which the guest clock is behind.
>>
>> """
> This talks about guest time.
> What this does not mention is whether hosts need to employ any mechanisms
> to synchronise wall clock between hosts.
The above mentioned whitepaper also discuss how VMware maintains the 
wallclock time across migrations (vMotion).
See section "Using VMware Tools Clock Synchronization" in whitepaper.

Specifically, there is an option in .vmx file named 
"time.synchronize.resume.disk" which:
"""
If set to TRUE, the clock syncs after resuming from suspend and after 
migrating to a new host using the VMware VMotion feature.
"""

The matching functionality in open-vm-tools can can be seen in 
services/plugins/timeSync/timeSync.c where ToolsOnLoad()
registers the "Time_Synchronize" RpcCallback, which is 
TimeSyncTcloHandler(), that is possibly allowed to sync time backwards 
(Note the "backwardSync" var).

The current patch-series I have submitted doesn't implement this 
RpcCallback functionality.
That work can be delayed to a future patch-series that will add this 
extra functionality as-well.

>> If I understand correctly, this seems to validate my assumption that current
>> implementation for CMD_GETTIME is sufficient.
> So I am concerned this does not interact well with other time sources
> in QEMU. For example, it's very useful to set guest time with -rtc base
> flag.
>
> Can you use qemu_get_timedate?
>
This is a very good point.
VMware also have the ability that allows user to explicitly set guest 
time with .vmx "rtc.startTime" option.
(The time-zone can also be set by specifying an offset from UTC with 
"rtc.diffFromUTC" option)

However, if you will read section "Using VMware Tools Clock 
Synchronization -> Disabling All Synchronization" in above mentioned 
whitepaper,
you will notice that in VMware's design, VMPort CMD_GETTIME command is 
intentionally not synced with virtual CMOS TOD. i.e. The section explicitly
documents that if a user wants to set guest time to fictitious time, 
user must disable VMware Tools time sync functionality by manipulating
"tools.syncTime" and "time.synchronize.*" configuration options as desired.

Therefore, I think current patch VMPort CMD_GETTIME command 
implementation is correct.
What do you think?

-Liran

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

[Liran Alon]

On 15/03/2020 13:56, Liran Alon wrote:
>
> On 14/03/2020 22:56, Michael S. Tsirkin wrote:
>> On Sat, Mar 14, 2020 at 10:05:20PM +0200, Liran Alon wrote:
>>> Michael, you can also refer to this VMware time-keeping whitepaper:
>>> https://urldefense.com/v3/__https://www.vmware.com/pdf/vmware_timekeeping.pdf__;!!GqivPVa7Brio!K8sfnfvVgKwrQ4SMwX-K6-S5yR4ln9_qZ6o4GzIpQkohfWtinlplNhXzFlyUgks$ 
>>> .
>>> According to section "Initializing and Correcting Wall-Clock Time":
>>> """
>>> VMware Tools can also optionally be used to correct long‐term drift and
>>> errors by periodically
>>> resynchronizing the virtual machine’s clock to the host’s clock, but 
>>> the
>>> current version at this writing is limited.
>>> In particular, in guest operating systems other than NetWare, it 
>>> does not
>>> correct errors in which the guest clock
>>> is ahead of real time, only those in which the guest clock is behind.
>>>
>>> """
>> This talks about guest time.
>> What this does not mention is whether hosts need to employ any 
>> mechanisms
>> to synchronise wall clock between hosts.
> The above mentioned whitepaper also discuss how VMware maintains the 
> wallclock time across migrations (vMotion).
> See section "Using VMware Tools Clock Synchronization" in whitepaper.
>
> Specifically, there is an option in .vmx file named 
> "time.synchronize.resume.disk" which:
> """
> If set to TRUE, the clock syncs after resuming from suspend and after 
> migrating to a new host using the VMware VMotion feature.
> """
>
> The matching functionality in open-vm-tools can can be seen in 
> services/plugins/timeSync/timeSync.c where ToolsOnLoad()
> registers the "Time_Synchronize" RpcCallback, which is 
> TimeSyncTcloHandler(), that is possibly allowed to sync time backwards 
> (Note the "backwardSync" var).
>
> The current patch-series I have submitted doesn't implement this 
> RpcCallback functionality.
> That work can be delayed to a future patch-series that will add this 
> extra functionality as-well.
>
>>> If I understand correctly, this seems to validate my assumption that 
>>> current
>>> implementation for CMD_GETTIME is sufficient.
>> So I am concerned this does not interact well with other time sources
>> in QEMU. For example, it's very useful to set guest time with -rtc base
>> flag.
>>
>> Can you use qemu_get_timedate?
>>
> This is a very good point.
> VMware also have the ability that allows user to explicitly set guest 
> time with .vmx "rtc.startTime" option.
> (The time-zone can also be set by specifying an offset from UTC with 
> "rtc.diffFromUTC" option)
>
> However, if you will read section "Using VMware Tools Clock 
> Synchronization -> Disabling All Synchronization" in above mentioned 
> whitepaper,
> you will notice that in VMware's design, VMPort CMD_GETTIME command is 
> intentionally not synced with virtual CMOS TOD. i.e. The section 
> explicitly
> documents that if a user wants to set guest time to fictitious time, 
> user must disable VMware Tools time sync functionality by manipulating
> "tools.syncTime" and "time.synchronize.*" configuration options as 
> desired.
>
> Therefore, I think current patch VMPort CMD_GETTIME command 
> implementation is correct.
> What do you think?
>
> -Liran
>
Gentle ping.

I would like to send the next version of the patch-series.
But before I do, I would like to know what we have agreed upon regarding 
this patch.

Thanks,
-Liran

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

[Liran Alon]

On 22/03/2020 13:22, Liran Alon wrote:
>
> On 15/03/2020 13:56, Liran Alon wrote:
>>
>> On 14/03/2020 22:56, Michael S. Tsirkin wrote:
>>> On Sat, Mar 14, 2020 at 10:05:20PM +0200, Liran Alon wrote:
>>>> Michael, you can also refer to this VMware time-keeping whitepaper:
>>>> https://urldefense.com/v3/__https://www.vmware.com/pdf/vmware_timekeeping.pdf__;!!GqivPVa7Brio!K8sfnfvVgKwrQ4SMwX-K6-S5yR4ln9_qZ6o4GzIpQkohfWtinlplNhXzFlyUgks$ 
>>>> .
>>>> According to section "Initializing and Correcting Wall-Clock Time":
>>>> """
>>>> VMware Tools can also optionally be used to correct long‐term drift 
>>>> and
>>>> errors by periodically
>>>> resynchronizing the virtual machine’s clock to the host’s clock, 
>>>> but the
>>>> current version at this writing is limited.
>>>> In particular, in guest operating systems other than NetWare, it 
>>>> does not
>>>> correct errors in which the guest clock
>>>> is ahead of real time, only those in which the guest clock is behind.
>>>>
>>>> """
>>> This talks about guest time.
>>> What this does not mention is whether hosts need to employ any 
>>> mechanisms
>>> to synchronise wall clock between hosts.
>> The above mentioned whitepaper also discuss how VMware maintains the 
>> wallclock time across migrations (vMotion).
>> See section "Using VMware Tools Clock Synchronization" in whitepaper.
>>
>> Specifically, there is an option in .vmx file named 
>> "time.synchronize.resume.disk" which:
>> """
>> If set to TRUE, the clock syncs after resuming from suspend and after 
>> migrating to a new host using the VMware VMotion feature.
>> """
>>
>> The matching functionality in open-vm-tools can can be seen in 
>> services/plugins/timeSync/timeSync.c where ToolsOnLoad()
>> registers the "Time_Synchronize" RpcCallback, which is 
>> TimeSyncTcloHandler(), that is possibly allowed to sync time 
>> backwards (Note the "backwardSync" var).
>>
>> The current patch-series I have submitted doesn't implement this 
>> RpcCallback functionality.
>> That work can be delayed to a future patch-series that will add this 
>> extra functionality as-well.
>>
>>>> If I understand correctly, this seems to validate my assumption 
>>>> that current
>>>> implementation for CMD_GETTIME is sufficient.
>>> So I am concerned this does not interact well with other time sources
>>> in QEMU. For example, it's very useful to set guest time with -rtc base
>>> flag.
>>>
>>> Can you use qemu_get_timedate?
>>>
>> This is a very good point.
>> VMware also have the ability that allows user to explicitly set guest 
>> time with .vmx "rtc.startTime" option.
>> (The time-zone can also be set by specifying an offset from UTC with 
>> "rtc.diffFromUTC" option)
>>
>> However, if you will read section "Using VMware Tools Clock 
>> Synchronization -> Disabling All Synchronization" in above mentioned 
>> whitepaper,
>> you will notice that in VMware's design, VMPort CMD_GETTIME command 
>> is intentionally not synced with virtual CMOS TOD. i.e. The section 
>> explicitly
>> documents that if a user wants to set guest time to fictitious time, 
>> user must disable VMware Tools time sync functionality by manipulating
>> "tools.syncTime" and "time.synchronize.*" configuration options as 
>> desired.
>>
>> Therefore, I think current patch VMPort CMD_GETTIME command 
>> implementation is correct.
>> What do you think?
>>
>> -Liran
>>
> Gentle ping.
>
> I would like to send the next version of the patch-series.
> But before I do, I would like to know what we have agreed upon 
> regarding this patch.
>
> Thanks,
> -Liran
>
Another gentle ping before I send v4 of patch-series.

Thanks,
-Liran

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

[Michael S. Tsirkin]

On Sat, Mar 14, 2020 at 09:58:23PM +0200, Nikita Leshenko wrote:
> I think that the reason that open-vm-tools doesn't move time backwards is to
> help applications that treat wallclock time as if it's monotonic time and break
> if the date is moved backwards (which may happen more frequently in virtual
> environment so it's handled specifically). But this doesn't change the fact that
> this PV interface is for reporting wall time.
> So I couldn't understand what
> source other than gettimeofday() you were suggesting for Liran to use to report
> wallclock time.

Some kind of offset to wallclock time I'm guessing. For example,
we could save wall clock on vm save, and if it goes
backwards on vm load, add an offset.

-- 
MST

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

[Liran Alon]

On 14/03/2020 20:18, Michael S. Tsirkin wrote:
> 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.
>
So guest doesn't assume that this time only moves forward...

Can you clarify then which change do you suggest making to this patch in 
this regard? It seems correct to me.
i.e. The CMD_GETTIME implementation seems correct to me and it doesn't 
need to do anything special to handle host time moving backwards.

-Liran

[PATCH v3 11/16] hw/i386/vmport: Add support for CMD_GETTIMEFULL

[Liran Alon]

Similar to CMD_GETTIME but lacks the 136-year overflow issue,
by returning full 64-bit of host uSeconds.

Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/i386/vmport.c         | 17 +++++++++++++++++
 include/hw/i386/vmport.h |  1 +
 2 files changed, 18 insertions(+)

diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c
index c5b659c59343..7e57eda4b526 100644
--- a/hw/i386/vmport.c
+++ b/hw/i386/vmport.c
@@ -183,6 +183,22 @@ static uint32_t vmport_cmd_time(void *opaque, uint32_t addr)
     return (uint32_t)tv.tv_sec;
 }
 
+static uint32_t vmport_cmd_time_full(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_ESI] = (uint32_t)((uint64_t)tv.tv_sec >> 32);
+    cpu->env.regs[R_EDX] = (uint32_t)tv.tv_sec;
+    cpu->env.regs[R_EBX] = (uint32_t)tv.tv_usec;
+    cpu->env.regs[R_ECX] = port_state->max_time_lag_us;
+    return VMPORT_MAGIC;
+}
+
 /* vmmouse helpers */
 void vmmouse_get_data(uint32_t *data)
 {
@@ -230,6 +246,7 @@ static void vmport_realizefn(DeviceState *dev, Error **errp)
     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);
+        vmport_register(VMPORT_CMD_GETTIMEFULL, vmport_cmd_time_full, NULL);
     }
 }
 
diff --git a/include/hw/i386/vmport.h b/include/hw/i386/vmport.h
index 50416c8c8f3e..5d19963ed417 100644
--- a/include/hw/i386/vmport.h
+++ b/include/hw/i386/vmport.h
@@ -12,6 +12,7 @@ typedef enum {
     VMPORT_CMD_VMMOUSE_DATA     = 39,
     VMPORT_CMD_VMMOUSE_STATUS   = 40,
     VMPORT_CMD_VMMOUSE_COMMAND  = 41,
+    VMPORT_CMD_GETTIMEFULL      = 46,
     VMPORT_ENTRIES
 } VMPortCommand;
 
-- 
2.20.1

Re: [PATCH v3 11/16] hw/i386/vmport: Add support for CMD_GETTIMEFULL

[Michael S. Tsirkin]

On Thu, Mar 12, 2020 at 06:54:26PM +0200, Liran Alon wrote:
> Similar to CMD_GETTIME but lacks the 136-year overflow issue,
> by returning full 64-bit of host uSeconds.
> 
> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> ---
>  hw/i386/vmport.c         | 17 +++++++++++++++++
>  include/hw/i386/vmport.h |  1 +
>  2 files changed, 18 insertions(+)
> 
> diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c
> index c5b659c59343..7e57eda4b526 100644
> --- a/hw/i386/vmport.c
> +++ b/hw/i386/vmport.c
> @@ -183,6 +183,22 @@ static uint32_t vmport_cmd_time(void *opaque, uint32_t addr)
>      return (uint32_t)tv.tv_sec;
>  }
>  
> +static uint32_t vmport_cmd_time_full(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_ESI] = (uint32_t)((uint64_t)tv.tv_sec >> 32);
> +    cpu->env.regs[R_EDX] = (uint32_t)tv.tv_sec;
> +    cpu->env.regs[R_EBX] = (uint32_t)tv.tv_usec;
> +    cpu->env.regs[R_ECX] = port_state->max_time_lag_us;
> +    return VMPORT_MAGIC;
> +}
> +
>  /* vmmouse helpers */
>  void vmmouse_get_data(uint32_t *data)
>  {

And with usec precision, same comments apply in an even stronger way.


> @@ -230,6 +246,7 @@ static void vmport_realizefn(DeviceState *dev, Error **errp)
>      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);
> +        vmport_register(VMPORT_CMD_GETTIMEFULL, vmport_cmd_time_full, NULL);
>      }
>  }
>  
> diff --git a/include/hw/i386/vmport.h b/include/hw/i386/vmport.h
> index 50416c8c8f3e..5d19963ed417 100644
> --- a/include/hw/i386/vmport.h
> +++ b/include/hw/i386/vmport.h
> @@ -12,6 +12,7 @@ typedef enum {
>      VMPORT_CMD_VMMOUSE_DATA     = 39,
>      VMPORT_CMD_VMMOUSE_STATUS   = 40,
>      VMPORT_CMD_VMMOUSE_COMMAND  = 41,
> +    VMPORT_CMD_GETTIMEFULL      = 46,
>      VMPORT_ENTRIES
>  } VMPortCommand;
>  
> -- 
> 2.20.1

Re: [PATCH v3 11/16] hw/i386/vmport: Add support for CMD_GETTIMEFULL

[Liran Alon]

On 13/03/2020 2:06, Michael S. Tsirkin wrote:
> On Thu, Mar 12, 2020 at 06:54:26PM +0200, Liran Alon wrote:
>> Similar to CMD_GETTIME but lacks the 136-year overflow issue,
>> by returning full 64-bit of host uSeconds.
>>
>> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
>> Signed-off-by: Liran Alon <liran.alon@oracle.com>
>> ---
>>   hw/i386/vmport.c         | 17 +++++++++++++++++
>>   include/hw/i386/vmport.h |  1 +
>>   2 files changed, 18 insertions(+)
>>
>> diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c
>> index c5b659c59343..7e57eda4b526 100644
>> --- a/hw/i386/vmport.c
>> +++ b/hw/i386/vmport.c
>> @@ -183,6 +183,22 @@ static uint32_t vmport_cmd_time(void *opaque, uint32_t addr)
>>       return (uint32_t)tv.tv_sec;
>>   }
>>   
>> +static uint32_t vmport_cmd_time_full(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_ESI] = (uint32_t)((uint64_t)tv.tv_sec >> 32);
>> +    cpu->env.regs[R_EDX] = (uint32_t)tv.tv_sec;
>> +    cpu->env.regs[R_EBX] = (uint32_t)tv.tv_usec;
>> +    cpu->env.regs[R_ECX] = port_state->max_time_lag_us;
>> +    return VMPORT_MAGIC;
>> +}
>> +
>>   /* vmmouse helpers */
>>   void vmmouse_get_data(uint32_t *data)
>>   {
> And with usec precision, same comments apply in an even stronger way.

Please tell me if you still have specific question or comment on this 
after ready my reply to the previous patch.
i.e. Something I should handle regarding this patch on v4.

Thanks,
-Liran

[PATCH v3 12/16] hw/i386/vmport: Add support for CMD_GET_VCPU_INFO

[Liran Alon]

Command currently returns that it is unimplemented by setting
the reserved-bit in it's return value.

Following patches will return various useful vCPU information
to guest.

Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/i386/vmport.c         | 14 ++++++++++++++
 include/hw/i386/vmport.h |  1 +
 2 files changed, 15 insertions(+)

diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c
index 7e57eda4b526..2ce78aaf7b4c 100644
--- a/hw/i386/vmport.c
+++ b/hw/i386/vmport.c
@@ -55,6 +55,13 @@
 #define VMPORT_COMPAT_CMDS_V2                   \
     (1 << VMPORT_COMPAT_CMDS_V2_BIT)
 
+/* vCPU features reported by CMD_GET_VCPU_INFO */
+#define VCPU_INFO_SLC64_BIT             0
+#define VCPU_INFO_SYNC_VTSCS_BIT        1
+#define VCPU_INFO_HV_REPLAY_OK_BIT      2
+#define VCPU_INFO_LEGACY_X2APIC_BIT     3
+#define VCPU_INFO_RESERVED_BIT          31
+
 #define VMPORT(obj) OBJECT_CHECK(VMPortState, (obj), TYPE_VMPORT)
 
 typedef struct VMPortState {
@@ -199,6 +206,11 @@ static uint32_t vmport_cmd_time_full(void *opaque, uint32_t addr)
     return VMPORT_MAGIC;
 }
 
+static uint32_t vmport_cmd_get_vcpu_info(void *opaque, uint32_t addr)
+{
+    return 1 << VCPU_INFO_RESERVED_BIT;
+}
+
 /* vmmouse helpers */
 void vmmouse_get_data(uint32_t *data)
 {
@@ -247,6 +259,8 @@ static void vmport_realizefn(DeviceState *dev, Error **errp)
         vmport_register(VMPORT_CMD_GETBIOSUUID, vmport_cmd_get_bios_uuid, NULL);
         vmport_register(VMPORT_CMD_GETTIME, vmport_cmd_time, NULL);
         vmport_register(VMPORT_CMD_GETTIMEFULL, vmport_cmd_time_full, NULL);
+        vmport_register(VMPORT_CMD_GET_VCPU_INFO, vmport_cmd_get_vcpu_info,
+                        NULL);
     }
 }
 
diff --git a/include/hw/i386/vmport.h b/include/hw/i386/vmport.h
index 5d19963ed417..34cc050b1ffa 100644
--- a/include/hw/i386/vmport.h
+++ b/include/hw/i386/vmport.h
@@ -13,6 +13,7 @@ typedef enum {
     VMPORT_CMD_VMMOUSE_STATUS   = 40,
     VMPORT_CMD_VMMOUSE_COMMAND  = 41,
     VMPORT_CMD_GETTIMEFULL      = 46,
+    VMPORT_CMD_GET_VCPU_INFO    = 68,
     VMPORT_ENTRIES
 } VMPortCommand;
 
-- 
2.20.1

Re: [PATCH v3 12/16] hw/i386/vmport: Add support for CMD_GET_VCPU_INFO

[Michael S. Tsirkin]

On Thu, Mar 12, 2020 at 06:54:27PM +0200, Liran Alon wrote:
> Command currently returns that it is unimplemented by setting
> the reserved-bit in it's return value.
> 
> Following patches will return various useful vCPU information
> to guest.
> 
> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> ---
>  hw/i386/vmport.c         | 14 ++++++++++++++
>  include/hw/i386/vmport.h |  1 +
>  2 files changed, 15 insertions(+)
> 
> diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c
> index 7e57eda4b526..2ce78aaf7b4c 100644
> --- a/hw/i386/vmport.c
> +++ b/hw/i386/vmport.c
> @@ -55,6 +55,13 @@
>  #define VMPORT_COMPAT_CMDS_V2                   \
>      (1 << VMPORT_COMPAT_CMDS_V2_BIT)
>  
> +/* vCPU features reported by CMD_GET_VCPU_INFO */
> +#define VCPU_INFO_SLC64_BIT             0
> +#define VCPU_INFO_SYNC_VTSCS_BIT        1
> +#define VCPU_INFO_HV_REPLAY_OK_BIT      2
> +#define VCPU_INFO_LEGACY_X2APIC_BIT     3
> +#define VCPU_INFO_RESERVED_BIT          31
> +
>  #define VMPORT(obj) OBJECT_CHECK(VMPortState, (obj), TYPE_VMPORT)
>  
>  typedef struct VMPortState {


Prefix with VMPORT_ please, and add comments.


> @@ -199,6 +206,11 @@ static uint32_t vmport_cmd_time_full(void *opaque, uint32_t addr)
>      return VMPORT_MAGIC;
>  }
>  
> +static uint32_t vmport_cmd_get_vcpu_info(void *opaque, uint32_t addr)
> +{
> +    return 1 << VCPU_INFO_RESERVED_BIT;
> +}
> +
>  /* vmmouse helpers */
>  void vmmouse_get_data(uint32_t *data)
>  {
> @@ -247,6 +259,8 @@ static void vmport_realizefn(DeviceState *dev, Error **errp)
>          vmport_register(VMPORT_CMD_GETBIOSUUID, vmport_cmd_get_bios_uuid, NULL);
>          vmport_register(VMPORT_CMD_GETTIME, vmport_cmd_time, NULL);
>          vmport_register(VMPORT_CMD_GETTIMEFULL, vmport_cmd_time_full, NULL);
> +        vmport_register(VMPORT_CMD_GET_VCPU_INFO, vmport_cmd_get_vcpu_info,
> +                        NULL);
>      }
>  }
>  
> diff --git a/include/hw/i386/vmport.h b/include/hw/i386/vmport.h
> index 5d19963ed417..34cc050b1ffa 100644
> --- a/include/hw/i386/vmport.h
> +++ b/include/hw/i386/vmport.h
> @@ -13,6 +13,7 @@ typedef enum {
>      VMPORT_CMD_VMMOUSE_STATUS   = 40,
>      VMPORT_CMD_VMMOUSE_COMMAND  = 41,
>      VMPORT_CMD_GETTIMEFULL      = 46,
> +    VMPORT_CMD_GET_VCPU_INFO    = 68,
>      VMPORT_ENTRIES
>  } VMPortCommand;
>  
> -- 
> 2.20.1

Re: [PATCH v3 12/16] hw/i386/vmport: Add support for CMD_GET_VCPU_INFO

[Liran Alon]

On 13/03/2020 2:09, Michael S. Tsirkin wrote:
> On Thu, Mar 12, 2020 at 06:54:27PM +0200, Liran Alon wrote:
>> Command currently returns that it is unimplemented by setting
>> the reserved-bit in it's return value.
>>
>> Following patches will return various useful vCPU information
>> to guest.
>>
>> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
>> Signed-off-by: Liran Alon <liran.alon@oracle.com>
>> ---
>>   hw/i386/vmport.c         | 14 ++++++++++++++
>>   include/hw/i386/vmport.h |  1 +
>>   2 files changed, 15 insertions(+)
>>
>> diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c
>> index 7e57eda4b526..2ce78aaf7b4c 100644
>> --- a/hw/i386/vmport.c
>> +++ b/hw/i386/vmport.c
>> @@ -55,6 +55,13 @@
>>   #define VMPORT_COMPAT_CMDS_V2                   \
>>       (1 << VMPORT_COMPAT_CMDS_V2_BIT)
>>   
>> +/* vCPU features reported by CMD_GET_VCPU_INFO */
>> +#define VCPU_INFO_SLC64_BIT             0
>> +#define VCPU_INFO_SYNC_VTSCS_BIT        1
>> +#define VCPU_INFO_HV_REPLAY_OK_BIT      2
>> +#define VCPU_INFO_LEGACY_X2APIC_BIT     3
>> +#define VCPU_INFO_RESERVED_BIT          31
>> +
>>   #define VMPORT(obj) OBJECT_CHECK(VMPortState, (obj), TYPE_VMPORT)
>>   
>>   typedef struct VMPortState {
>
> Prefix with VMPORT_ please, and add comments.
Ok regarding prefix.
Which comments do you expect? What every flag means? Sure.

-Liran

[PATCH v3 13/16] hw/i386/vmport: Allow x2apic without IR

[Liran Alon]

Signal to guest that hypervisor supports x2apic without VT-d/IOMMU
Interrupt-Remapping support. This allows guest to use x2apic in
case all APIC IDs fits in 8-bit (i.e. Max APIC ID < 255).

See Linux kernel commit 4cca6ea04d31 ("x86/apic: Allow x2apic
without IR on VMware platform") and Linux try_to_enable_x2apic()
function.

Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/i386/vmport.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c
index 2ce78aaf7b4c..1664a6b97332 100644
--- a/hw/i386/vmport.c
+++ b/hw/i386/vmport.c
@@ -208,7 +208,14 @@ static uint32_t vmport_cmd_time_full(void *opaque, uint32_t addr)
 
 static uint32_t vmport_cmd_get_vcpu_info(void *opaque, uint32_t addr)
 {
-    return 1 << VCPU_INFO_RESERVED_BIT;
+    X86CPU *cpu = X86_CPU(current_cpu);
+    uint32_t ret = 0;
+
+    if (cpu->env.features[FEAT_1_ECX] & CPUID_EXT_X2APIC) {
+        ret |= 1 << VCPU_INFO_LEGACY_X2APIC_BIT;
+    }
+
+    return ret;
 }
 
 /* vmmouse helpers */
-- 
2.20.1

[PATCH v3 14/16] i386/cpu: Store LAPIC bus frequency in CPU structure

[Liran Alon]

No functional change.
This information will be used by following patches.

Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 target/i386/cpu.h |  1 +
 target/i386/kvm.c | 10 +++++++---
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 576f309bbfc8..9c7cd7cde107 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1580,6 +1580,7 @@ typedef struct CPUX86State {
     bool tsc_valid;
     int64_t tsc_khz;
     int64_t user_tsc_khz; /* for sanity check only */
+    uint64_t apic_bus_freq;
 #if defined(CONFIG_KVM) || defined(CONFIG_HVF)
     void *xsave_buf;
 #endif
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index 69eb43d796e6..8534ca9760d7 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -60,6 +60,10 @@
     do { } while (0)
 #endif
 
+/* From arch/x86/kvm/lapic.h */
+#define KVM_APIC_BUS_CYCLE_NS       1
+#define KVM_APIC_BUS_FREQUENCY      (1000000000ULL / KVM_APIC_BUS_CYCLE_NS)
+
 #define MSR_KVM_WALL_CLOCK  0x11
 #define MSR_KVM_SYSTEM_TIME 0x12
 
@@ -1496,6 +1500,8 @@ int kvm_arch_init_vcpu(CPUState *cs)
         }
     }
 
+    env->apic_bus_freq = KVM_APIC_BUS_FREQUENCY;
+
     /* Paravirtualization CPUIDs */
     r = hyperv_handle_properties(cs, cpuid_data.entries);
     if (r < 0) {
@@ -1800,9 +1806,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
         c = &cpuid_data.entries[cpuid_i++];
         c->function = KVM_CPUID_SIGNATURE | 0x10;
         c->eax = env->tsc_khz;
-        /* LAPIC resolution of 1ns (freq: 1GHz) is hardcoded in KVM's
-         * APIC_BUS_CYCLE_NS */
-        c->ebx = 1000000;
+        c->ebx = env->apic_bus_freq / 1000; /* Hz to KHz */
         c->ecx = c->edx = 0;
 
         c = cpuid_find_entry(&cpuid_data.cpuid, kvm_base, 0);
-- 
2.20.1

[PATCH v3 15/16] hw/i386/vmport: Add support for CMD_GETHZ

[Liran Alon]

This command returns to guest information on LAPIC bus frequency and TSC
frequency.

One can see how this interface is used by Linux vmware_platform_setup()
introduced in Linux commit 88b094fb8d4f ("x86: Hypervisor detection and
get tsc_freq from hypervisor").

Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/i386/vmport.c         | 19 +++++++++++++++++++
 include/hw/i386/vmport.h |  1 +
 2 files changed, 20 insertions(+)

diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c
index 1664a6b97332..9d3921cf418d 100644
--- a/hw/i386/vmport.c
+++ b/hw/i386/vmport.c
@@ -176,6 +176,24 @@ static uint32_t vmport_cmd_ram_size(void *opaque, uint32_t addr)
     return ram_size;
 }
 
+static uint32_t vmport_cmd_get_hz(void *opaque, uint32_t addr)
+{
+    X86CPU *cpu = X86_CPU(current_cpu);
+
+    if (cpu->env.tsc_khz && cpu->env.apic_bus_freq) {
+        uint64_t tsc_freq = (uint64_t)cpu->env.tsc_khz * 1000;
+
+        cpu->env.regs[R_ECX] = cpu->env.apic_bus_freq;
+        cpu->env.regs[R_EBX] = (uint32_t)(tsc_freq >> 32);
+        cpu->env.regs[R_EAX] = (uint32_t)tsc_freq;
+    } else {
+        /* Signal cmd as not supported */
+        cpu->env.regs[R_EBX] = UINT32_MAX;
+    }
+
+    return cpu->env.regs[R_EAX];
+}
+
 static uint32_t vmport_cmd_time(void *opaque, uint32_t addr)
 {
     X86CPU *cpu = X86_CPU(current_cpu);
@@ -265,6 +283,7 @@ static void vmport_realizefn(DeviceState *dev, Error **errp)
     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);
+        vmport_register(VMPORT_CMD_GETHZ, vmport_cmd_get_hz, NULL);
         vmport_register(VMPORT_CMD_GETTIMEFULL, vmport_cmd_time_full, NULL);
         vmport_register(VMPORT_CMD_GET_VCPU_INFO, vmport_cmd_get_vcpu_info,
                         NULL);
diff --git a/include/hw/i386/vmport.h b/include/hw/i386/vmport.h
index 34cc050b1ffa..aee809521aa0 100644
--- a/include/hw/i386/vmport.h
+++ b/include/hw/i386/vmport.h
@@ -12,6 +12,7 @@ typedef enum {
     VMPORT_CMD_VMMOUSE_DATA     = 39,
     VMPORT_CMD_VMMOUSE_STATUS   = 40,
     VMPORT_CMD_VMMOUSE_COMMAND  = 41,
+    VMPORT_CMD_GETHZ            = 45,
     VMPORT_CMD_GETTIMEFULL      = 46,
     VMPORT_CMD_GET_VCPU_INFO    = 68,
     VMPORT_ENTRIES
-- 
2.20.1

Re: [PATCH v3 15/16] hw/i386/vmport: Add support for CMD_GETHZ

[Philippe Mathieu-Daudé]

On 3/12/20 5:54 PM, Liran Alon wrote:
> This command returns to guest information on LAPIC bus frequency and TSC
> frequency.
> 
> One can see how this interface is used by Linux vmware_platform_setup()
> introduced in Linux commit 88b094fb8d4f ("x86: Hypervisor detection and
> get tsc_freq from hypervisor").
> 
> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> ---
>   hw/i386/vmport.c         | 19 +++++++++++++++++++
>   include/hw/i386/vmport.h |  1 +
>   2 files changed, 20 insertions(+)
> 
> diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c
> index 1664a6b97332..9d3921cf418d 100644
> --- a/hw/i386/vmport.c
> +++ b/hw/i386/vmport.c
> @@ -176,6 +176,24 @@ static uint32_t vmport_cmd_ram_size(void *opaque, uint32_t addr)
>       return ram_size;
>   }
>   
> +static uint32_t vmport_cmd_get_hz(void *opaque, uint32_t addr)
> +{
> +    X86CPU *cpu = X86_CPU(current_cpu);
> +
> +    if (cpu->env.tsc_khz && cpu->env.apic_bus_freq) {
> +        uint64_t tsc_freq = (uint64_t)cpu->env.tsc_khz * 1000;
> +
> +        cpu->env.regs[R_ECX] = cpu->env.apic_bus_freq;
> +        cpu->env.regs[R_EBX] = (uint32_t)(tsc_freq >> 32);
> +        cpu->env.regs[R_EAX] = (uint32_t)tsc_freq;
> +    } else {
> +        /* Signal cmd as not supported */
> +        cpu->env.regs[R_EBX] = UINT32_MAX;
> +    }
> +
> +    return cpu->env.regs[R_EAX];
> +}
> +
>   static uint32_t vmport_cmd_time(void *opaque, uint32_t addr)
>   {
>       X86CPU *cpu = X86_CPU(current_cpu);
> @@ -265,6 +283,7 @@ static void vmport_realizefn(DeviceState *dev, Error **errp)
>       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);
> +        vmport_register(VMPORT_CMD_GETHZ, vmport_cmd_get_hz, NULL);
>           vmport_register(VMPORT_CMD_GETTIMEFULL, vmport_cmd_time_full, NULL);
>           vmport_register(VMPORT_CMD_GET_VCPU_INFO, vmport_cmd_get_vcpu_info,
>                           NULL);
> diff --git a/include/hw/i386/vmport.h b/include/hw/i386/vmport.h
> index 34cc050b1ffa..aee809521aa0 100644
> --- a/include/hw/i386/vmport.h
> +++ b/include/hw/i386/vmport.h
> @@ -12,6 +12,7 @@ typedef enum {
>       VMPORT_CMD_VMMOUSE_DATA     = 39,
>       VMPORT_CMD_VMMOUSE_STATUS   = 40,
>       VMPORT_CMD_VMMOUSE_COMMAND  = 41,
> +    VMPORT_CMD_GETHZ            = 45,

Can you rename to something easier to read, such _GET_FREQS_HZ or nicer?

>       VMPORT_CMD_GETTIMEFULL      = 46,
>       VMPORT_CMD_GET_VCPU_INFO    = 68,
>       VMPORT_ENTRIES
> 

Re: [PATCH v3 15/16] hw/i386/vmport: Add support for CMD_GETHZ

[Liran Alon]

On 13/03/2020 22:07, Philippe Mathieu-Daudé wrote:
> On 3/12/20 5:54 PM, Liran Alon wrote:
>>
>> diff --git a/include/hw/i386/vmport.h b/include/hw/i386/vmport.h
>> index 34cc050b1ffa..aee809521aa0 100644
>> --- a/include/hw/i386/vmport.h
>> +++ b/include/hw/i386/vmport.h
>> @@ -12,6 +12,7 @@ typedef enum {
>>       VMPORT_CMD_VMMOUSE_DATA     = 39,
>>       VMPORT_CMD_VMMOUSE_STATUS   = 40,
>>       VMPORT_CMD_VMMOUSE_COMMAND  = 41,
>> +    VMPORT_CMD_GETHZ            = 45,
>
> Can you rename to something easier to read, such _GET_FREQS_HZ or nicer?
>
I actually prefer to stick with names similar to open-vm-tools. i.e. 
Similar to the definitions in lib/include/backdoor_def.h.
This helps correlates a command in QEMU code to guest code (in 
open-vm-tools) that interacts with it.
I can rename to just VMPORT_CMD_GET_HZ (Similar to what you suggested 
for previous commands).
But I don't have a strong opinion on this. If you still think 
_GET_FREQ_HZ is preferred, I will rename to that.

-Liran

Re: [PATCH v3 15/16] hw/i386/vmport: Add support for CMD_GETHZ

[Philippe Mathieu-Daudé]

On 3/13/20 11:44 PM, Liran Alon wrote:
> On 13/03/2020 22:07, Philippe Mathieu-Daudé wrote:
>> On 3/12/20 5:54 PM, Liran Alon wrote:
>>>
>>> diff --git a/include/hw/i386/vmport.h b/include/hw/i386/vmport.h
>>> index 34cc050b1ffa..aee809521aa0 100644
>>> --- a/include/hw/i386/vmport.h
>>> +++ b/include/hw/i386/vmport.h
>>> @@ -12,6 +12,7 @@ typedef enum {
>>>       VMPORT_CMD_VMMOUSE_DATA     = 39,
>>>       VMPORT_CMD_VMMOUSE_STATUS   = 40,
>>>       VMPORT_CMD_VMMOUSE_COMMAND  = 41,
>>> +    VMPORT_CMD_GETHZ            = 45,
>>
>> Can you rename to something easier to read, such _GET_FREQS_HZ or nicer?
>>
> I actually prefer to stick with names similar to open-vm-tools. i.e. 
> Similar to the definitions in lib/include/backdoor_def.h.
> This helps correlates a command in QEMU code to guest code (in 
> open-vm-tools) that interacts with it.

Well, we don't have to follow bad examples ;)

> I can rename to just VMPORT_CMD_GET_HZ (Similar to what you suggested 
> for previous commands).

Thanks, this makes code review more easier.

> But I don't have a strong opinion on this. If you still think 
> _GET_FREQ_HZ is preferred, I will rename to that.
> 
> -Liran
> 

Re: [PATCH v3 15/16] hw/i386/vmport: Add support for CMD_GETHZ

[Michael S. Tsirkin]

On Sat, Mar 14, 2020 at 12:44:55AM +0200, Liran Alon wrote:
> 
> On 13/03/2020 22:07, Philippe Mathieu-Daudé wrote:
> > On 3/12/20 5:54 PM, Liran Alon wrote:
> > > 
> > > diff --git a/include/hw/i386/vmport.h b/include/hw/i386/vmport.h
> > > index 34cc050b1ffa..aee809521aa0 100644
> > > --- a/include/hw/i386/vmport.h
> > > +++ b/include/hw/i386/vmport.h
> > > @@ -12,6 +12,7 @@ typedef enum {
> > >       VMPORT_CMD_VMMOUSE_DATA     = 39,
> > >       VMPORT_CMD_VMMOUSE_STATUS   = 40,
> > >       VMPORT_CMD_VMMOUSE_COMMAND  = 41,
> > > +    VMPORT_CMD_GETHZ            = 45,
> > 
> > Can you rename to something easier to read, such _GET_FREQS_HZ or nicer?
> > 
> I actually prefer to stick with names similar to open-vm-tools. i.e. Similar
> to the definitions in lib/include/backdoor_def.h.

Please, do not copy without attribution. It really applies everywhere,
I commented on another enum and you fixed it there, but please
go over your code and try to generally apply the same rules.

> This helps correlates a command in QEMU code to guest code (in
> open-vm-tools) that interacts with it.
> I can rename to just VMPORT_CMD_GET_HZ (Similar to what you suggested for
> previous commands).
> But I don't have a strong opinion on this. If you still think _GET_FREQ_HZ
> is preferred, I will rename to that.
> 
> -Liran


Generally I don't think a hard to read code somewhere is a good reason
to have hard to read code in QEMU, especially since it tends to
proliferate.  It seems unlikely that VMPORT_CMD_GETHZ appears verbatim
anywhere, and applying transformation rules is just too tricky. The best
way to map host code to guest code in light of coding style differences
etc is using comments. You did it in case of the type values, it
applies equally here.


-- 
MST

Re: [PATCH v3 15/16] hw/i386/vmport: Add support for CMD_GETHZ

[Liran Alon]

On 14/03/2020 23:52, Michael S. Tsirkin wrote:
> On Sat, Mar 14, 2020 at 12:44:55AM +0200, Liran Alon wrote:
>> On 13/03/2020 22:07, Philippe Mathieu-Daudé wrote:
>>> On 3/12/20 5:54 PM, Liran Alon wrote:
>>>> diff --git a/include/hw/i386/vmport.h b/include/hw/i386/vmport.h
>>>> index 34cc050b1ffa..aee809521aa0 100644
>>>> --- a/include/hw/i386/vmport.h
>>>> +++ b/include/hw/i386/vmport.h
>>>> @@ -12,6 +12,7 @@ typedef enum {
>>>>        VMPORT_CMD_VMMOUSE_DATA     = 39,
>>>>        VMPORT_CMD_VMMOUSE_STATUS   = 40,
>>>>        VMPORT_CMD_VMMOUSE_COMMAND  = 41,
>>>> +    VMPORT_CMD_GETHZ            = 45,
>>> Can you rename to something easier to read, such _GET_FREQS_HZ or nicer?
>>>
>> I actually prefer to stick with names similar to open-vm-tools. i.e. Similar
>> to the definitions in lib/include/backdoor_def.h.
> Please, do not copy without attribution. It really applies everywhere,
> I commented on another enum and you fixed it there, but please
> go over your code and try to generally apply the same rules.
This is not a copy of the enum as the other case you replied on.
It's just names "inspired" or "similar" to original names. They are not 
even the same.
>
>> This helps correlates a command in QEMU code to guest code (in
>> open-vm-tools) that interacts with it.
>> I can rename to just VMPORT_CMD_GET_HZ (Similar to what you suggested for
>> previous commands).
>> But I don't have a strong opinion on this. If you still think _GET_FREQ_HZ
>> is preferred, I will rename to that.
>>
>> -Liran
>
> Generally I don't think a hard to read code somewhere is a good reason
> to have hard to read code in QEMU, especially since it tends to
> proliferate.  It seems unlikely that VMPORT_CMD_GETHZ appears verbatim
> anywhere, and applying transformation rules is just too tricky. The best
> way to map host code to guest code in light of coding style differences
> etc is using comments. You did it in case of the type values, it
> applies equally here.
>
Honestly, even though I used slightly different names than original 
open-vm-tools code, I think it's quite trivial to coorelate.
Both by similar name (not same), by value and by function. That's why I 
don't have a strong opinion about the name.
I think VMPORT_CMD_GET_HZ is sufficient, but honestly I would name it 
however you want. I really don't care.

I don't think any special comment is necessary here for correlation. But 
I don't mind putting above enum a general comment such as:
/* See open-vm-tools lib/include/backdoor_def.h to match these to guest 
commands */

-Liran

[PATCH v3 16/16] hw/i386/vmport: Assert vmport initialized before registering commands

[Liran Alon]

vmport_register() is also called from other modules such as vmmouse.
Therefore, these modules rely that vmport is realized before those call
sites. If this is violated, vmport_register() will NULL-deref.

To make such issues easier to debug, assert in vmport_register() that
vmport is already realized.

Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/i386/vmport.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c
index 9d3921cf418d..134d793a4c65 100644
--- a/hw/i386/vmport.c
+++ b/hw/i386/vmport.c
@@ -83,6 +83,8 @@ static VMPortState *port_state;
 void vmport_register(VMPortCommand command, VMPortReadFunc *func, void *opaque)
 {
     assert(command < VMPORT_ENTRIES);
+    assert(port_state);
+
     trace_vmport_register(command, func, opaque);
     port_state->func[command] = func;
     port_state->opaque[command] = opaque;
-- 
2.20.1

Re: [PATCH v3 00/16]: hw/i386/vmport: Bug fixes and improvements

[Paolo Bonzini]

On 12/03/20 17:54, Liran Alon wrote:
> Hi,
> 
> This series aims to fix several bugs in VMPort and improve it by supporting
> more VMPort commands and make command results more configurable to
> user via QEMU command-line.
> 
> This functionality was proven to be useful to run various VMware VMs
> when attempting to run them as-is on top of QEMU/KVM.
> 
> For more details, see commit messages.
> 
> Regards,
> -Liran
> 
> v1->v2:
> * Fix coding convention [Patchew Bot & MST].
> * Create new header file for vmport.h [MST].
> * Move KVM_APIC_BUS_FREQUENCY from linux-headers/asm-x86/kvm.h
>   auto-generated header [MST]
> * Elaborate more that vmx-version refers to the VMware userspace
>   VMM in commit message. [MST]
> * Use le32_to_cpu() on BIOS_UUID vmport command. [MST]
> * Introduce VMPort compatability version property to maintain migration
>   compatibility. [MST]
> 
> v2->v3:
> - Repalce VMPort compatability version property with multiple boolean
>   compatability properties. [MST]
> - Prefix "vmx-*" properties with "vmware-vmx-*" to avoid confusion with
>   Intel VT-x short name. Prefix suggested by MST. [MST]
> - Remove VMX_Type enum and instead hard-code default vmware-vmx-type
>   value and only reference open-vm-tools for rest of values. [MST]
> - Add reference (link) to VMware open-vm-tools code. [MST]
> 

I actually agree with Liran on most comments, so I just dropped the
GETTIME and GETTIMEFULL functionality and queued everything else.

Thanks,

Paolo