| Message ID | 20200611194449.31468-28-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,001/115] docker.py/build: support -t and -f arguments | expand |
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" },
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" }, >
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
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
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(), };