[PULL 027/115] hw/i386/vmport: Propagate IOPort read to vCPU EAX register

Paolo Bonzini posted 115 patches 5 years, 8 months ago
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Colin Xu <colin.xu@intel.com>, "Michael S. Tsirkin" <mst@redhat.com>, Wenchao Wang <wenchao.wang@intel.com>, Peter Maydell <peter.maydell@linaro.org>, Alex Williamson <alex.williamson@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Fam Zheng <fam@euphon.net>, Igor Mammedov <imammedo@redhat.com>, Laurent Vivier <lvivier@redhat.com>, Marcelo Tosatti <mtosatti@redhat.com>, Roman Bolshakov <r.bolshakov@yadro.com>, Thomas Huth <thuth@redhat.com>, Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>, Richard Henderson <rth@twiddle.net>, Eduardo Habkost <ehabkost@redhat.com>, Hannes Reinecke <hare@suse.com>
There is a newer version of this series
[PULL 027/115] hw/i386/vmport: Propagate IOPort read to vCPU EAX register
Posted by Paolo Bonzini 5 years, 8 months ago
From: Liran Alon <liran.alon@oracle.com>

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>
Message-Id: <20200312165431.82118-4-liran.alon@oracle.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.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 bb3a7b18b1..83f0fe5c91 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -43,6 +43,7 @@ GlobalProperty hw_compat_4_2[] = {
     { "qxl", "revision", "4" },
     { "qxl-vga", "revision", "4" },
     { "fw_cfg", "acpi-mr-restore", "false" },
+    { "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 5985167dcf..3bb420a245 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;
+    }
+
+    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 s->func[command](s->opaque[command], addr);
+    return eax;
 }
 
 static void vmport_ioport_write(void *opaque, hwaddr addr,
@@ -142,6 +165,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.26.2



Re: [PULL 027/115] hw/i386/vmport: Propagate IOPort read to vCPU EAX register
Posted by Laurent Vivier 5 years, 7 months ago
On 11/06/2020 21:43, Paolo Bonzini wrote:
> From: Liran Alon <liran.alon@oracle.com>
> 
> 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>
> Message-Id: <20200312165431.82118-4-liran.alon@oracle.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.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 bb3a7b18b1..83f0fe5c91 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -43,6 +43,7 @@ GlobalProperty hw_compat_4_2[] = {
>      { "qxl", "revision", "4" },
>      { "qxl-vga", "revision", "4" },
>      { "fw_cfg", "acpi-mr-restore", "false" },
> +    { "vmport", "x-read-set-eax", "off" },

This is modifying the hw_compat_4_2 properties while qemu-5.0 has been
already released. I think all the vmport property updates [1] should go
to hw_compat_5_0.

Liran? Paolo?

Thanks,
Laurent

[1]

b889212973da hw/i386/vmport: Propagate IOPort read to vCPU EAX register
0342ee761ef2 hw/i386/vmport: Set EAX to -1 on failed and unsupported
commands
f8bdc550370f hw/i386/vmport: Report vmware-vmx-type in CMD_GETVERSION
aaacf1c15a22 hw/i386/vmport: Add support for CMD_GETBIOSUUID

    { "vmport", "x-read-set-eax", "off" },
    { "vmport", "x-signal-unsupported-cmd", "off" },
    { "vmport", "x-report-vmx-type", "off" },
    { "vmport", "x-cmds-v2", "off" },


Re: [PULL 027/115] hw/i386/vmport: Propagate IOPort read to vCPU EAX register
Posted by Liran Alon 5 years, 7 months ago
On 23/06/2020 11:46, Laurent Vivier wrote:
> On 11/06/2020 21:43, Paolo Bonzini wrote:
>> From: Liran Alon <liran.alon@oracle.com>
>>
>> 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>
>> Message-Id: <20200312165431.82118-4-liran.alon@oracle.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.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 bb3a7b18b1..83f0fe5c91 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -43,6 +43,7 @@ GlobalProperty hw_compat_4_2[] = {
>>       { "qxl", "revision", "4" },
>>       { "qxl-vga", "revision", "4" },
>>       { "fw_cfg", "acpi-mr-restore", "false" },
>> +    { "vmport", "x-read-set-eax", "off" },
> This is modifying the hw_compat_4_2 properties while qemu-5.0 has been
> already released. I think all the vmport property updates [1] should go
> to hw_compat_5_0.
>
> Liran? Paolo?

When I submitted these patches, QEMU 5.0 wasn't released yet. That's why 
I updated hw_compat_4_2[].

Having said that, I believe the compatibility risk here is very small 
and therefore because QEMU 5.0 was
released for a very short time-span before these patches were merged, 
I'm not sure it's really preferable
to move these flags to hw_compat_5_0[]. But I will leave this for Paolo 
to decide.
(Note that moving these flags will also risk in comparability people 
running with current patches and
specifying explicitly machine-type 5.0...)

-Liran

>
> Thanks,
> Laurent
>
> [1]
>
> b889212973da hw/i386/vmport: Propagate IOPort read to vCPU EAX register
> 0342ee761ef2 hw/i386/vmport: Set EAX to -1 on failed and unsupported
> commands
> f8bdc550370f hw/i386/vmport: Report vmware-vmx-type in CMD_GETVERSION
> aaacf1c15a22 hw/i386/vmport: Add support for CMD_GETBIOSUUID
>
>      { "vmport", "x-read-set-eax", "off" },
>      { "vmport", "x-signal-unsupported-cmd", "off" },
>      { "vmport", "x-report-vmx-type", "off" },
>      { "vmport", "x-cmds-v2", "off" },
>

Re: [PULL 027/115] hw/i386/vmport: Propagate IOPort read to vCPU EAX register
Posted by Paolo Bonzini 5 years, 7 months ago
On 23/06/20 11:34, Liran Alon wrote:
> Having said that, I believe the compatibility risk here is very small
> and therefore because QEMU 5.0 was
> released for a very short time-span before these patches were merged,
> I'm not sure it's really preferable
> to move these flags to hw_compat_5_0[]. But I will leave this for Paolo
> to decide.
> (Note that moving these flags will also risk in comparability people
> running with current patches and
> specifying explicitly machine-type 5.0...)

Since this has never made it into a release, I'll fix them up.

Paolo


Re: [PULL 027/115] hw/i386/vmport: Propagate IOPort read to vCPU EAX register
Posted by Laurent Vivier 5 years, 7 months ago
On 23/06/2020 12:25, Paolo Bonzini wrote:
> On 23/06/20 11:34, Liran Alon wrote:
>> Having said that, I believe the compatibility risk here is very small
>> and therefore because QEMU 5.0 was
>> released for a very short time-span before these patches were merged,
>> I'm not sure it's really preferable
>> to move these flags to hw_compat_5_0[]. But I will leave this for Paolo
>> to decide.
>> (Note that moving these flags will also risk in comparability people
>> running with current patches and
>> specifying explicitly machine-type 5.0...)
> 
> Since this has never made it into a release, I'll fix them up.

I agree, I think this is what to do.

Thanks,
Laurent