Re: [Qemu-devel] [BUGFIX][PATCH for 2.2 v3 1/1] -machine vmport=auto: Fix handling of VMWare ioport emulation for xen
On Wed, Nov 19, 2014 at 07:38:10PM -0500, Don Slutz wrote:
c/s 9b23cfb76b3a5e9eb5cc899eaf2f46bc46d33ba4
or
c/s b154537ad07598377ebf98252fb7d2aff127983b
moved the testing of xen_enabled() from pc_init1() to
pc_machine_initfn().
xen_enabled() does not return the correct value in
pc_machine_initfn().
Changed vmport from a bool to an enum. Added the value auto to do
the old way.
Signed-off-by: Don Slutz dsl...@verizon.com
This looks fine to me. A couple of minor comments below.
Also this changes qapi schema file, let's get ack from maintainers -
my understanding is that just adding a definition there won't
affect any users, correct?
---
hw/i386/pc.c | 23 ++-
hw/i386/pc_piix.c| 27 ++-
hw/i386/pc_q35.c | 27 ++-
include/hw/i386/pc.h | 2 +-
qapi-schema.json | 16
qemu-options.hx | 8 +---
vl.c | 2 +-
7 files changed, 89 insertions(+), 16 deletions(-)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 1205db8..2f68e4d 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1711,18 +1711,23 @@ static void pc_machine_set_max_ram_below_4g(Object
*obj, Visitor *v,
pcms-max_ram_below_4g = value;
}
-static bool pc_machine_get_vmport(Object *obj, Error **errp)
+static void pc_machine_get_vmport(Object *obj, Visitor *v, void *opaque,
+ const char *name, Error **errp)
{
PCMachineState *pcms = PC_MACHINE(obj);
+int vmport = pcms-vmport;
-return pcms-vmport;
+visit_type_enum(v, vmport, vmport_lookup, NULL, name, errp);
}
-static void pc_machine_set_vmport(Object *obj, bool value, Error **errp)
+static void pc_machine_set_vmport(Object *obj, Visitor *v, void *opaque,
+ const char *name, Error **errp)
{
PCMachineState *pcms = PC_MACHINE(obj);
+int vmport;
-pcms-vmport = value;
+visit_type_enum(v, vmport, vmport_lookup, NULL, name, errp);
+pcms-vmport = vmport;
}
static void pc_machine_initfn(Object *obj)
@@ -1737,11 +1742,11 @@ static void pc_machine_initfn(Object *obj)
pc_machine_get_max_ram_below_4g,
pc_machine_set_max_ram_below_4g,
NULL, NULL, NULL);
-pcms-vmport = !xen_enabled();
-object_property_add_bool(obj, PC_MACHINE_VMPORT,
- pc_machine_get_vmport,
- pc_machine_set_vmport,
- NULL);
+pcms-vmport = VMPORT_AUTO;
+object_property_add(obj, PC_MACHINE_VMPORT, str,
+pc_machine_get_vmport,
+pc_machine_set_vmport,
+NULL, NULL, NULL);
}
static void pc_machine_class_init(ObjectClass *oc, void *data)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 7bb97a4..81a7b83 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -101,6 +101,7 @@ static void pc_init1(MachineState *machine,
FWCfgState *fw_cfg = NULL;
PcGuestInfo *guest_info;
ram_addr_t lowmem;
+bool no_vmport;
/* Check whether RAM fits below 4G (leaving 1/2 GByte for IO memory).
* If it doesn't, we need to split it in chunks below and above 4G.
@@ -234,9 +235,33 @@ static void pc_init1(MachineState *machine,
pc_vga_init(isa_bus, pci_enabled ? pci_bus : NULL);
Probably should be assert(pc_machine-vmport != VMPORT_MAX) -
we never get any values except on/off/auto.
+if (xen_enabled()) {
+switch (pc_machine-vmport) {
+case VMPORT_MAX:
+case VMPORT_AUTO:
+case VMPORT_OFF:
+no_vmport = true;
+break;
+case VMPORT_ON:
+no_vmport = false;
+break;
+}
+} else {
+switch (pc_machine-vmport) {
+case VMPORT_MAX:
+case VMPORT_OFF:
+no_vmport = true;
+break;
+case VMPORT_AUTO:
+case VMPORT_ON:
+no_vmport = false;
+break;
+}
+}
+
Can't above be moved to a function in pc.c, and be reused between piix
and q35? It's big enough to avoid open-coding, IMHO.
/* init basic PC hardware */
pc_basic_device_init(isa_bus, gsi, rtc_state, floppy,
- !pc_machine-vmport, 0x4);
+ no_vmport, 0x4);
pc_nic_init(isa_bus, pci_bus);
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 598e679..4f932d6 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -88,6 +88,7 @@ static void pc_q35_init(MachineState *machine)
PcGuestInfo *guest_info;
ram_addr_t lowmem;
DriveInfo *hd[MAX_SATA_PORTS];
+bool no_vmport;
/* Check whether RAM fits below 4G (leaving 1/2 GByte for IO memory
* and 256 Mbytes for PCI Express Enhanced
Re: [Qemu-devel] [BUGFIX][PATCH for 2.2 v3 1/1] -machine vmport=auto: Fix handling of VMWare ioport emulation for xen
On Wed, Nov 19, 2014 at 09:11:41PM -0700, Eric Blake wrote:
On 11/19/2014 05:38 PM, Don Slutz wrote:
c/s 9b23cfb76b3a5e9eb5cc899eaf2f46bc46d33ba4
or
c/s b154537ad07598377ebf98252fb7d2aff127983b
moved the testing of xen_enabled() from pc_init1() to
pc_machine_initfn().
xen_enabled() does not return the correct value in
pc_machine_initfn().
Changed vmport from a bool to an enum. Added the value auto to do
the old way.
+++ b/qapi-schema.json
@@ -3513,3 +3513,19 @@
# Since: 2.1
##
{ 'command': 'rtc-reset-reinjection' }
+
+##
+# @vmport
+#
+# An enumeration of the options on enabling of VMWare ioport emulation
+#
+# @auto: system selects the old default
+#
+# @on: provide the vmport device
+#
+# @off: do not provide the vmport device
+#
+# Since: 2.2
+##
+{ 'enum': 'vmport',
All other enums in .json files are named in StudlyCaps. Please name
this starting with a capital letter, and reserve all-lower-case names
for commands and built-in types.
Hi Eric,
The values here are not vmport-specific.
Do you think we should have a generic OnOffAuto type?
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [BUGFIX][PATCH for 2.2 v3 1/1] -machine vmport=auto: Fix handling of VMWare ioport emulation for xen
* Paolo Bonzini (pbonz...@redhat.com) wrote:
On 20/11/2014 01:58, Eduardo Habkost wrote:
if (pc_machine-vmport == VMPORT_AUTO) {
no_vmport = xen_enabled();
} else {
no_vmport = (pc_machine-vmport == VMPORT_ON);
}
I'm still not sure why the configuration should differ for -M pc
depending on whether xen is enabled.
I think this goes back to:
commit 1611977c3d8fdbdac6090cbd1fcee4aed6d9
Author: Anthony PERARD anthony.per...@citrix.com
Date: Tue May 3 17:06:54 2011 +0100
pc, Disable vmport initialisation with Xen.
This is because there is not synchronisation of the vcpu register
between Xen and QEMU, so vmport can't work properly.
This patch introduces no_vmport parameter to pc_basic_device_init.
Signed-off-by: Anthony PERARD anthony.per...@citrix.com
Signed-off-by: Alexander Graf ag...@suse.de
Dave
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] [BUGFIX][PATCH for 2.2 v3 1/1] -machine vmport=auto: Fix handling of VMWare ioport emulation for xen
On 20/11/2014 11:00, Dr. David Alan Gilbert wrote: I'm still not sure why the configuration should differ for -M pc depending on whether xen is enabled. I think this goes back to: commit 1611977c3d8fdbdac6090cbd1fcee4aed6d9 Author: Anthony PERARD anthony.per...@citrix.com Date: Tue May 3 17:06:54 2011 +0100 pc, Disable vmport initialisation with Xen. This is because there is not synchronisation of the vcpu register between Xen and QEMU, so vmport can't work properly. This patch introduces no_vmport parameter to pc_basic_device_init. Signed-off-by: Anthony PERARD anthony.per...@citrix.com Signed-off-by: Alexander Graf ag...@suse.de Yes, but Xen has since implemented vmport (commit 37f9e258). It's fine to have a conservative default for -M xenfv and possibly -M pc-2.1, but -M pc can require the latest hypervisor. Paolo
Re: [Qemu-devel] [BUGFIX][PATCH for 2.2 v3 1/1] -machine vmport=auto: Fix handling of VMWare ioport emulation for xen
On Thu, Nov 20, 2014 at 12:00:19PM +0100, Paolo Bonzini wrote: On 20/11/2014 11:00, Dr. David Alan Gilbert wrote: I'm still not sure why the configuration should differ for -M pc depending on whether xen is enabled. I think this goes back to: commit 1611977c3d8fdbdac6090cbd1fcee4aed6d9 Author: Anthony PERARD anthony.per...@citrix.com Date: Tue May 3 17:06:54 2011 +0100 pc, Disable vmport initialisation with Xen. This is because there is not synchronisation of the vcpu register between Xen and QEMU, so vmport can't work properly. This patch introduces no_vmport parameter to pc_basic_device_init. Signed-off-by: Anthony PERARD anthony.per...@citrix.com Signed-off-by: Alexander Graf ag...@suse.de Yes, but Xen has since implemented vmport (commit 37f9e258). It's fine to have a conservative default for -M xenfv and possibly -M pc-2.1, but -M pc can require the latest hypervisor. Are there any ABI stability expectations when using -machine pc-2.1,accel=xen? I guess there aren't any, and in this case the first patch (the one changing default_machine_opts) would be enough. -- Eduardo
Re: [Qemu-devel] [BUGFIX][PATCH for 2.2 v3 1/1] -machine vmport=auto: Fix handling of VMWare ioport emulation for xen
On 11/19/14 19:58, Eduardo Habkost wrote:
On Wed, Nov 19, 2014 at 07:38:10PM -0500, Don Slutz wrote:
[...]
@@ -234,9 +235,33 @@ static void pc_init1(MachineState *machine,
pc_vga_init(isa_bus, pci_enabled ? pci_bus : NULL);
+if (xen_enabled()) {
+switch (pc_machine-vmport) {
+case VMPORT_MAX:
+case VMPORT_AUTO:
+case VMPORT_OFF:
+no_vmport = true;
+break;
+case VMPORT_ON:
+no_vmport = false;
+break;
+}
+} else {
+switch (pc_machine-vmport) {
+case VMPORT_MAX:
+case VMPORT_OFF:
+no_vmport = true;
+break;
+case VMPORT_AUTO:
+case VMPORT_ON:
+no_vmport = false;
+break;
+}
+}
+
/* init basic PC hardware */
pc_basic_device_init(isa_bus, gsi, rtc_state, floppy,
- !pc_machine-vmport, 0x4);
+ no_vmport, 0x4);
pc_nic_init(isa_bus, pci_bus);
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 598e679..4f932d6 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -88,6 +88,7 @@ static void pc_q35_init(MachineState *machine)
PcGuestInfo *guest_info;
ram_addr_t lowmem;
DriveInfo *hd[MAX_SATA_PORTS];
+bool no_vmport;
/* Check whether RAM fits below 4G (leaving 1/2 GByte for IO memory
* and 256 Mbytes for PCI Express Enhanced Configuration Access Mapping
@@ -242,9 +243,33 @@ static void pc_q35_init(MachineState *machine)
pc_register_ferr_irq(gsi[13]);
+if (xen_enabled()) {
+switch (pc_machine-vmport) {
+case VMPORT_MAX:
+case VMPORT_AUTO:
+case VMPORT_OFF:
+no_vmport = true;
+break;
+case VMPORT_ON:
+no_vmport = false;
+break;
+}
+} else {
+switch (pc_machine-vmport) {
+case VMPORT_MAX:
+case VMPORT_OFF:
+no_vmport = true;
+break;
+case VMPORT_AUTO:
+case VMPORT_ON:
+no_vmport = false;
+break;
+}
+}
What about simplifying those 24 lines to the following 5 lines:
if (pc_machine-vmport == VMPORT_AUTO) {
no_vmport = xen_enabled();
} else {
no_vmport = (pc_machine-vmport == VMPORT_ON);
}
Looks much better. Will switch to this.
diff --git a/qapi-schema.json b/qapi-schema.json
index d0926d9..f7ee3ad 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3513,3 +3513,19 @@
# Since: 2.1
##
{ 'command': 'rtc-reset-reinjection' }
+
+##
+# @vmport
+#
+# An enumeration of the options on enabling of VMWare ioport emulation
+#
+# @auto: system selects the old default
+#
+# @on: provide the vmport device
+#
+# @off: do not provide the vmport device
+#
+# Since: 2.2
+##
+{ 'enum': 'vmport',
Coding style for enum typedefs is CamelCase.
Probably something more descriptive than just VMPort would be better.
This enum does not describe the vmport itself, but just the on/off/auto
setting. Maybe VMPortConfig?
That would be fine with me.
+ 'data': [ 'auto', 'on', 'off' ] }
I believe there may be other properties with exactly the same behavior.
Maybe we could use a generic enum name, like Tristate?
But we have a problem, here: the existing code accepts on, yes and
true for boolean values. Now it is going to accept only on' and
off. This breaks compatibility.
This is new at 2.2, so I think we can adjust either way (just the 3 or add
others).
Maybe my enum suggestion was not a very good one? I would like to hear
opinions from others.
In either case, I am sure your previous bugfix is more appropriate for
2.2. Changing the data type of the vmport property is much more
intrusive than just allowing it to return an incorrect value temporarily
between instance_init and machine-init().
I can post the v2 if that makes sense (as v4?) will wait a little
while. If the v2
is the one in 2.2 then allowing the other options may make sense.
-Don Slutz
Re: [Qemu-devel] [BUGFIX][PATCH for 2.2 v3 1/1] -machine vmport=auto: Fix handling of VMWare ioport emulation for xen
On 11/20/14 01:04, Paolo Bonzini wrote:
On 20/11/2014 01:58, Eduardo Habkost wrote:
if (pc_machine-vmport == VMPORT_AUTO) {
no_vmport = xen_enabled();
} else {
no_vmport = (pc_machine-vmport == VMPORT_ON);
}
I'm still not sure why the configuration should differ for -M pc
depending on whether xen is enabled.
Paolo
The key reason is that with current xen, if vmport is enabled QEMU will
crash:
Forwarded Message
Subject: Re: [Qemu-devel] qemu 2.2 crash on linux hvm domU (full
backtrace included)
Date: Wed, 19 Nov 2014 15:04:58 +0100
From: Fabio Fantoni fabio.fant...@m2r.biz
To: xen-devel xen-de...@lists.xensource.com, qemu-devel@nongnu.org
qemu-devel@nongnu.org, spice-de...@lists.freedesktop.org
CC: anthony PERARD anthony.per...@citrix.com, dsl...@verizon.com,
Stefano Stabellini stefano.stabell...@eu.citrix.com
Il 14/11/2014 12:25, Fabio Fantoni ha scritto:
dom0 xen-unstable from staging git with x86/hvm: Extend HVM cpuid
leaf with vcpu id and x86/hvm: Add per-vcpu evtchn upcalls patches,
and qemu 2.2 from spice git (spice/next commit
e779fa0a715530311e6f59fc8adb0f6eca914a89):
https://github.com/Fantu/Xen/commits/rebase/m2r-staging
I tried with qemu tag v2.2.0-rc2 and crash still happen, here the full
backtrace of latest test:
Program received signal SIGSEGV, Segmentation fault.
0x55689b07 in vmport_ioport_read (opaque=0x564443a0, addr=0,
size=4) at /mnt/vm/xen/Xen/tools/qemu-xen-dir/hw/misc/vmport.c:73
73 eax = env-regs[R_EAX];
(gdb) bt full
#0 0x55689b07 in vmport_ioport_read (opaque=0x564443a0,
addr=0,
size=4) at /mnt/vm/xen/Xen/tools/qemu-xen-dir/hw/misc/vmport.c:73
s = 0x564443a0
cs = 0x0
cpu = 0x0
__func__ = vmport_ioport_read
env = 0x8250
command = 0 '\000'
eax = 0
#1 0x55655fc4 in memory_region_read_accessor (mr=0x5628,
addr=0, value=0x7fffd8d0, size=4, shift=0, mask=4294967295)
at /mnt/vm/xen/Xen/tools/qemu-xen-dir/memory.c:410
tmp = 0
#2 0x556562b7 in access_with_adjusted_size (addr=0,
value=0x7fffd8d0, size=4, access_size_min=4, access_size_max=4,
access=0x55655f62 memory_region_read_accessor,
mr=0x5628)
at /mnt/vm/xen/Xen/tools/qemu-xen-dir/memory.c:480
access_mask = 4294967295
access_size = 4
i = 0
#3 0x556590e9 in memory_region_dispatch_read1
(mr=0x5628,
addr=0, size=4) at /mnt/vm/xen/Xen/tools/qemu-xen-dir/memory.c:1077
data = 0
#4 0x556591b1 in memory_region_dispatch_read (mr=0x5628,
addr=0, pval=0x7fffd9a8, size=4)
---Type return to continue, or q return to quit---
at /mnt/vm/xen/Xen/tools/qemu-xen-dir/memory.c:1099
No locals.
...
and in QEMU 2.1 and older it just xen_enabled().
-Don Slutz
Re: [Qemu-devel] [BUGFIX][PATCH for 2.2 v3 1/1] -machine vmport=auto: Fix handling of VMWare ioport emulation for xen
On 20/11/2014 16:07, Don Slutz wrote: The key reason is that with current xen, if vmport is enabled QEMU will crash: Thanks, that helps understanding the patch. :) Paolo
Re: [Qemu-devel] [BUGFIX][PATCH for 2.2 v3 1/1] -machine vmport=auto: Fix handling of VMWare ioport emulation for xen
On 11/20/14 04:13, Michael S. Tsirkin wrote:
On Wed, Nov 19, 2014 at 09:11:41PM -0700, Eric Blake wrote:
On 11/19/2014 05:38 PM, Don Slutz wrote:
c/s 9b23cfb76b3a5e9eb5cc899eaf2f46bc46d33ba4
or
c/s b154537ad07598377ebf98252fb7d2aff127983b
moved the testing of xen_enabled() from pc_init1() to
pc_machine_initfn().
xen_enabled() does not return the correct value in
pc_machine_initfn().
Changed vmport from a bool to an enum. Added the value auto to do
the old way.
+++ b/qapi-schema.json
@@ -3513,3 +3513,19 @@
# Since: 2.1
##
{ 'command': 'rtc-reset-reinjection' }
+
+##
+# @vmport
+#
+# An enumeration of the options on enabling of VMWare ioport emulation
+#
+# @auto: system selects the old default
+#
+# @on: provide the vmport device
+#
+# @off: do not provide the vmport device
+#
+# Since: 2.2
+##
+{ 'enum': 'vmport',
All other enums in .json files are named in StudlyCaps. Please name
this starting with a capital letter, and reserve all-lower-case names
for commands and built-in types.
Hi Eric,
The values here are not vmport-specific.
Do you think we should have a generic OnOffAuto type?
I am waiting for a a clear direction on how to go.
VMPortConfig, Tristate, and OnOffAuto are the 3 names
I am tracking.
-Don Slutz
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [BUGFIX][PATCH for 2.2 v3 1/1] -machine vmport=auto: Fix handling of VMWare ioport emulation for xen
On Thu, Nov 20, 2014 at 10:16:37AM -0500, Don Slutz wrote:
On 11/20/14 04:13, Michael S. Tsirkin wrote:
On Wed, Nov 19, 2014 at 09:11:41PM -0700, Eric Blake wrote:
[...]
+{ 'enum': 'vmport',
All other enums in .json files are named in StudlyCaps. Please name
this starting with a capital letter, and reserve all-lower-case names
for commands and built-in types.
Hi Eric,
The values here are not vmport-specific.
Do you think we should have a generic OnOffAuto type?
I am waiting for a a clear direction on how to go.
VMPortConfig, Tristate, and OnOffAuto are the 3 names
I am tracking.
I prefer a non-vmport-specific name. OnOffAuto is more descriptive than
Tristate, and is good enough to me.
--
Eduardo
Re: [Qemu-devel] [BUGFIX][PATCH for 2.2 v3 1/1] -machine vmport=auto: Fix handling of VMWare ioport emulation for xen
On 11/20/14 06:00, Paolo Bonzini wrote: On 20/11/2014 11:00, Dr. David Alan Gilbert wrote: I'm still not sure why the configuration should differ for -M pc depending on whether xen is enabled. I think this goes back to: commit 1611977c3d8fdbdac6090cbd1fcee4aed6d9 Author: Anthony PERARD anthony.per...@citrix.com Date: Tue May 3 17:06:54 2011 +0100 pc, Disable vmport initialisation with Xen. This is because there is not synchronisation of the vcpu register between Xen and QEMU, so vmport can't work properly. This patch introduces no_vmport parameter to pc_basic_device_init. Signed-off-by: Anthony PERARD anthony.per...@citrix.com Signed-off-by: Alexander Graf ag...@suse.de Yes, but Xen has since implemented vmport (commit 37f9e258). It's fine to have a conservative default for -M xenfv and possibly -M pc-2.1, but -M pc can require the latest hypervisor. The QEMU part of xen using vmport was done. The code in xen will not be part of xen 4.5 (expected to be released next month), it is currently scheduled for 4.6 (some time next year) and is planning to use vmport=on when it is enabled in xen. You are right that -M pc can require a newer (yet to exist) hypervisor, but I feel it will cause less confusion and allow QEMU 2.2 to be used unchanged with older xen versions; if the default is kept unchanged. -Don Slutz Paolo
Re: [Qemu-devel] [BUGFIX][PATCH for 2.2 v3 1/1] -machine vmport=auto: Fix handling of VMWare ioport emulation for xen
On 11/20/2014 01:44 AM, Michael S. Tsirkin wrote: On Wed, Nov 19, 2014 at 07:38:10PM -0500, Don Slutz wrote: c/s 9b23cfb76b3a5e9eb5cc899eaf2f46bc46d33ba4 or c/s b154537ad07598377ebf98252fb7d2aff127983b moved the testing of xen_enabled() from pc_init1() to pc_machine_initfn(). xen_enabled() does not return the correct value in pc_machine_initfn(). Changed vmport from a bool to an enum. Added the value auto to do the old way. Signed-off-by: Don Slutz dsl...@verizon.com This looks fine to me. A couple of minor comments below. Also this changes qapi schema file, let's get ack from maintainers - my understanding is that just adding a definition there won't affect any users, correct? Correct; adding a definition won't break existing users, whether or not the new type is currently used by current commands. However, it's still worth designing the new enum type to be potentially reusable; the idea of naming it OnOffAuto makes sense to me. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
[Qemu-devel] [BUGFIX][PATCH for 2.2 v3 1/1] -machine vmport=auto: Fix handling of VMWare ioport emulation for xen
c/s 9b23cfb76b3a5e9eb5cc899eaf2f46bc46d33ba4
or
c/s b154537ad07598377ebf98252fb7d2aff127983b
moved the testing of xen_enabled() from pc_init1() to
pc_machine_initfn().
xen_enabled() does not return the correct value in
pc_machine_initfn().
Changed vmport from a bool to an enum. Added the value auto to do
the old way.
Signed-off-by: Don Slutz dsl...@verizon.com
---
hw/i386/pc.c | 23 ++-
hw/i386/pc_piix.c| 27 ++-
hw/i386/pc_q35.c | 27 ++-
include/hw/i386/pc.h | 2 +-
qapi-schema.json | 16
qemu-options.hx | 8 +---
vl.c | 2 +-
7 files changed, 89 insertions(+), 16 deletions(-)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 1205db8..2f68e4d 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1711,18 +1711,23 @@ static void pc_machine_set_max_ram_below_4g(Object
*obj, Visitor *v,
pcms-max_ram_below_4g = value;
}
-static bool pc_machine_get_vmport(Object *obj, Error **errp)
+static void pc_machine_get_vmport(Object *obj, Visitor *v, void *opaque,
+ const char *name, Error **errp)
{
PCMachineState *pcms = PC_MACHINE(obj);
+int vmport = pcms-vmport;
-return pcms-vmport;
+visit_type_enum(v, vmport, vmport_lookup, NULL, name, errp);
}
-static void pc_machine_set_vmport(Object *obj, bool value, Error **errp)
+static void pc_machine_set_vmport(Object *obj, Visitor *v, void *opaque,
+ const char *name, Error **errp)
{
PCMachineState *pcms = PC_MACHINE(obj);
+int vmport;
-pcms-vmport = value;
+visit_type_enum(v, vmport, vmport_lookup, NULL, name, errp);
+pcms-vmport = vmport;
}
static void pc_machine_initfn(Object *obj)
@@ -1737,11 +1742,11 @@ static void pc_machine_initfn(Object *obj)
pc_machine_get_max_ram_below_4g,
pc_machine_set_max_ram_below_4g,
NULL, NULL, NULL);
-pcms-vmport = !xen_enabled();
-object_property_add_bool(obj, PC_MACHINE_VMPORT,
- pc_machine_get_vmport,
- pc_machine_set_vmport,
- NULL);
+pcms-vmport = VMPORT_AUTO;
+object_property_add(obj, PC_MACHINE_VMPORT, str,
+pc_machine_get_vmport,
+pc_machine_set_vmport,
+NULL, NULL, NULL);
}
static void pc_machine_class_init(ObjectClass *oc, void *data)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 7bb97a4..81a7b83 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -101,6 +101,7 @@ static void pc_init1(MachineState *machine,
FWCfgState *fw_cfg = NULL;
PcGuestInfo *guest_info;
ram_addr_t lowmem;
+bool no_vmport;
/* Check whether RAM fits below 4G (leaving 1/2 GByte for IO memory).
* If it doesn't, we need to split it in chunks below and above 4G.
@@ -234,9 +235,33 @@ static void pc_init1(MachineState *machine,
pc_vga_init(isa_bus, pci_enabled ? pci_bus : NULL);
+if (xen_enabled()) {
+switch (pc_machine-vmport) {
+case VMPORT_MAX:
+case VMPORT_AUTO:
+case VMPORT_OFF:
+no_vmport = true;
+break;
+case VMPORT_ON:
+no_vmport = false;
+break;
+}
+} else {
+switch (pc_machine-vmport) {
+case VMPORT_MAX:
+case VMPORT_OFF:
+no_vmport = true;
+break;
+case VMPORT_AUTO:
+case VMPORT_ON:
+no_vmport = false;
+break;
+}
+}
+
/* init basic PC hardware */
pc_basic_device_init(isa_bus, gsi, rtc_state, floppy,
- !pc_machine-vmport, 0x4);
+ no_vmport, 0x4);
pc_nic_init(isa_bus, pci_bus);
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 598e679..4f932d6 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -88,6 +88,7 @@ static void pc_q35_init(MachineState *machine)
PcGuestInfo *guest_info;
ram_addr_t lowmem;
DriveInfo *hd[MAX_SATA_PORTS];
+bool no_vmport;
/* Check whether RAM fits below 4G (leaving 1/2 GByte for IO memory
* and 256 Mbytes for PCI Express Enhanced Configuration Access Mapping
@@ -242,9 +243,33 @@ static void pc_q35_init(MachineState *machine)
pc_register_ferr_irq(gsi[13]);
+if (xen_enabled()) {
+switch (pc_machine-vmport) {
+case VMPORT_MAX:
+case VMPORT_AUTO:
+case VMPORT_OFF:
+no_vmport = true;
+break;
+case VMPORT_ON:
+no_vmport = false;
+break;
+}
+} else {
+switch (pc_machine-vmport) {
+case VMPORT_MAX:
+case VMPORT_OFF:
+no_vmport = true;
+break;
+case VMPORT_AUTO:
+case VMPORT_ON:
+no_vmport =
Re: [Qemu-devel] [BUGFIX][PATCH for 2.2 v3 1/1] -machine vmport=auto: Fix handling of VMWare ioport emulation for xen
On Wed, Nov 19, 2014 at 07:38:10PM -0500, Don Slutz wrote:
[...]
@@ -234,9 +235,33 @@ static void pc_init1(MachineState *machine,
pc_vga_init(isa_bus, pci_enabled ? pci_bus : NULL);
+if (xen_enabled()) {
+switch (pc_machine-vmport) {
+case VMPORT_MAX:
+case VMPORT_AUTO:
+case VMPORT_OFF:
+no_vmport = true;
+break;
+case VMPORT_ON:
+no_vmport = false;
+break;
+}
+} else {
+switch (pc_machine-vmport) {
+case VMPORT_MAX:
+case VMPORT_OFF:
+no_vmport = true;
+break;
+case VMPORT_AUTO:
+case VMPORT_ON:
+no_vmport = false;
+break;
+}
+}
+
/* init basic PC hardware */
pc_basic_device_init(isa_bus, gsi, rtc_state, floppy,
- !pc_machine-vmport, 0x4);
+ no_vmport, 0x4);
pc_nic_init(isa_bus, pci_bus);
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 598e679..4f932d6 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -88,6 +88,7 @@ static void pc_q35_init(MachineState *machine)
PcGuestInfo *guest_info;
ram_addr_t lowmem;
DriveInfo *hd[MAX_SATA_PORTS];
+bool no_vmport;
/* Check whether RAM fits below 4G (leaving 1/2 GByte for IO memory
* and 256 Mbytes for PCI Express Enhanced Configuration Access Mapping
@@ -242,9 +243,33 @@ static void pc_q35_init(MachineState *machine)
pc_register_ferr_irq(gsi[13]);
+if (xen_enabled()) {
+switch (pc_machine-vmport) {
+case VMPORT_MAX:
+case VMPORT_AUTO:
+case VMPORT_OFF:
+no_vmport = true;
+break;
+case VMPORT_ON:
+no_vmport = false;
+break;
+}
+} else {
+switch (pc_machine-vmport) {
+case VMPORT_MAX:
+case VMPORT_OFF:
+no_vmport = true;
+break;
+case VMPORT_AUTO:
+case VMPORT_ON:
+no_vmport = false;
+break;
+}
+}
What about simplifying those 24 lines to the following 5 lines:
if (pc_machine-vmport == VMPORT_AUTO) {
no_vmport = xen_enabled();
} else {
no_vmport = (pc_machine-vmport == VMPORT_ON);
}
diff --git a/qapi-schema.json b/qapi-schema.json
index d0926d9..f7ee3ad 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3513,3 +3513,19 @@
# Since: 2.1
##
{ 'command': 'rtc-reset-reinjection' }
+
+##
+# @vmport
+#
+# An enumeration of the options on enabling of VMWare ioport emulation
+#
+# @auto: system selects the old default
+#
+# @on: provide the vmport device
+#
+# @off: do not provide the vmport device
+#
+# Since: 2.2
+##
+{ 'enum': 'vmport',
Coding style for enum typedefs is CamelCase.
Probably something more descriptive than just VMPort would be better.
This enum does not describe the vmport itself, but just the on/off/auto
setting. Maybe VMPortConfig?
+ 'data': [ 'auto', 'on', 'off' ] }
I believe there may be other properties with exactly the same behavior.
Maybe we could use a generic enum name, like Tristate?
But we have a problem, here: the existing code accepts on, yes and
true for boolean values. Now it is going to accept only on' and
off. This breaks compatibility.
Maybe my enum suggestion was not a very good one? I would like to hear
opinions from others.
In either case, I am sure your previous bugfix is more appropriate for
2.2. Changing the data type of the vmport property is much more
intrusive than just allowing it to return an incorrect value temporarily
between instance_init and machine-init().
--
Eduardo
Re: [Qemu-devel] [BUGFIX][PATCH for 2.2 v3 1/1] -machine vmport=auto: Fix handling of VMWare ioport emulation for xen
On 11/19/2014 05:38 PM, Don Slutz wrote:
c/s 9b23cfb76b3a5e9eb5cc899eaf2f46bc46d33ba4
or
c/s b154537ad07598377ebf98252fb7d2aff127983b
moved the testing of xen_enabled() from pc_init1() to
pc_machine_initfn().
xen_enabled() does not return the correct value in
pc_machine_initfn().
Changed vmport from a bool to an enum. Added the value auto to do
the old way.
+++ b/qapi-schema.json
@@ -3513,3 +3513,19 @@
# Since: 2.1
##
{ 'command': 'rtc-reset-reinjection' }
+
+##
+# @vmport
+#
+# An enumeration of the options on enabling of VMWare ioport emulation
+#
+# @auto: system selects the old default
+#
+# @on: provide the vmport device
+#
+# @off: do not provide the vmport device
+#
+# Since: 2.2
+##
+{ 'enum': 'vmport',
All other enums in .json files are named in StudlyCaps. Please name
this starting with a capital letter, and reserve all-lower-case names
for commands and built-in types.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [BUGFIX][PATCH for 2.2 v3 1/1] -machine vmport=auto: Fix handling of VMWare ioport emulation for xen
On 20/11/2014 01:58, Eduardo Habkost wrote:
if (pc_machine-vmport == VMPORT_AUTO) {
no_vmport = xen_enabled();
} else {
no_vmport = (pc_machine-vmport == VMPORT_ON);
}
I'm still not sure why the configuration should differ for -M pc
depending on whether xen is enabled.
Paolo
