| Message ID | 1416443890-20263-1-git-send-email-dslutz@verizon.com |
|---|---|
| State | New |
| Headers | show |
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().
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.
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
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 <dslutz@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 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; > + } > + } > + > /* init basic PC hardware */ > pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy, > - !pc_machine->vmport, 0xff0104); > + no_vmport, 0xff0104); > > /* connect pm stuff to lpc */ > ich9_lpc_pm_init(lpc); > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h > index 7c3731f..d7d8f30 100644 > --- a/include/hw/i386/pc.h > +++ b/include/hw/i386/pc.h > @@ -37,7 +37,7 @@ struct PCMachineState { > ISADevice *rtc; > > uint64_t max_ram_below_4g; > - bool vmport; > + vmport vmport; > }; > > #define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device" > 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', vmport as type name violates our coding style. Should be VMPort. But in fact, this is a generic OnOffAuto type, isn't it? Maybe it should be named like this, and go into qapi/common.json? I think it might be a good idea but it's not a must. > + 'data': [ 'auto', 'on', 'off' ] } > diff --git a/qemu-options.hx b/qemu-options.hx > index da9851d..64af16d 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -33,7 +33,7 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \ > " property accel=accel1[:accel2[:...]] selects accelerator\n" > " supported accelerators are kvm, xen, tcg (default: tcg)\n" > " kernel_irqchip=on|off controls accelerated irqchip support\n" > - " vmport=on|off controls emulation of vmport (default: on)\n" > + " vmport=on|off|auto controls emulation of vmport (default: auto)\n" > " kvm_shadow_mem=size of KVM shadow MMU\n" > " dump-guest-core=on|off include guest memory in a core dump (default=on)\n" > " mem-merge=on|off controls memory merge support (default: on)\n" > @@ -52,8 +52,10 @@ than one accelerator specified, the next one is used if the previous one fails > to initialize. > @item kernel_irqchip=on|off > Enables in-kernel irqchip support for the chosen accelerator when available. > -@item vmport=on|off > -Enables emulation of VMWare IO port, for vmmouse etc. (enabled by default) > +@item vmport=on|off|auto > +Enables emulation of VMWare IO port, for vmmouse etc. auto says to select the > +value based on accel. For accel=xen the default is off otherwise the default > +is on. > @item kvm_shadow_mem=size > Defines the size of the KVM shadow MMU. > @item dump-guest-core=on|off > diff --git a/vl.c b/vl.c > index f4a6e5e..eb89d62 100644 > --- a/vl.c > +++ b/vl.c > @@ -381,7 +381,7 @@ static QemuOptsList qemu_machine_opts = { > .help = "maximum ram below the 4G boundary (32bit boundary)", > }, { > .name = PC_MACHINE_VMPORT, > - .type = QEMU_OPT_BOOL, > + .type = QEMU_OPT_STRING, > .help = "Enable vmport (pc & q35)", > },{ > .name = "iommu", > -- > 1.8.4
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 >
* Paolo Bonzini (pbonzini@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 1611977c3d8fdbdac6090cbd1f5555cee4aed6d9 Author: Anthony PERARD <anthony.perard@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.perard@citrix.com> Signed-off-by: Alexander Graf <agraf@suse.de> Dave -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
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 1611977c3d8fdbdac6090cbd1f5555cee4aed6d9 > Author: Anthony PERARD <anthony.perard@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.perard@citrix.com> > Signed-off-by: Alexander Graf <agraf@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
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 1611977c3d8fdbdac6090cbd1f5555cee4aed6d9 > > Author: Anthony PERARD <anthony.perard@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.perard@citrix.com> > > Signed-off-by: Alexander Graf <agraf@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.
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
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.fantoni@m2r.biz> To: xen-devel <xen-devel@lists.xensource.com>, qemu-devel@nongnu.org <qemu-devel@nongnu.org>, spice-devel@lists.freedesktop.org CC: anthony PERARD <anthony.perard@citrix.com>, dslutz@verizon.com, Stefano Stabellini <Stefano.Stabellini@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. > 0x0000555555689b07 in vmport_ioport_read (opaque=0x5555564443a0, 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 0x0000555555689b07 in vmport_ioport_read (opaque=0x5555564443a0, > addr=0, > size=4) at /mnt/vm/xen/Xen/tools/qemu-xen-dir/hw/misc/vmport.c:73 > s = 0x5555564443a0 > cs = 0x0 > cpu = 0x0 > __func__ = "vmport_ioport_read" > env = 0x8250 > command = 0 '\000' > eax = 0 > #1 0x0000555555655fc4 in memory_region_read_accessor (mr=0x555556444428, > addr=0, value=0x7fffffffd8d0, size=4, shift=0, mask=4294967295) > at /mnt/vm/xen/Xen/tools/qemu-xen-dir/memory.c:410 > tmp = 0 > #2 0x00005555556562b7 in access_with_adjusted_size (addr=0, > value=0x7fffffffd8d0, size=4, access_size_min=4, access_size_max=4, > access=0x555555655f62 <memory_region_read_accessor>, > mr=0x555556444428) > at /mnt/vm/xen/Xen/tools/qemu-xen-dir/memory.c:480 > access_mask = 4294967295 > access_size = 4 > i = 0 > #3 0x00005555556590e9 in memory_region_dispatch_read1 > (mr=0x555556444428, > addr=0, size=4) at /mnt/vm/xen/Xen/tools/qemu-xen-dir/memory.c:1077 > data = 0 > #4 0x00005555556591b1 in memory_region_dispatch_read (mr=0x555556444428, > addr=0, pval=0x7fffffffd9a8, 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
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
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 >> >
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.
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 1611977c3d8fdbdac6090cbd1f5555cee4aed6d9 >> Author: Anthony PERARD <anthony.perard@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.perard@citrix.com> >> Signed-off-by: Alexander Graf <agraf@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
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 <dslutz@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.
On 11/20/14 03:44, 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 <dslutz@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. Ok, added the assert. >> + 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. > I feel that the what Eduardo Habkost provided: + assert(pc_machine->vmport != ON_OFF_AUTO_MAX); + if (pc_machine->vmport == ON_OFF_AUTO_AUTO) { + no_vmport = xen_enabled(); + } else { + no_vmport = (pc_machine->vmport == ON_OFF_AUTO_ON); + } is short enough to not need it's own function. But I can do this if still needed. >> /* 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; >> + } >> + } >> + >> /* init basic PC hardware */ >> pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy, >> - !pc_machine->vmport, 0xff0104); >> + no_vmport, 0xff0104); >> >> /* connect pm stuff to lpc */ >> ich9_lpc_pm_init(lpc); >> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h >> index 7c3731f..d7d8f30 100644 >> --- a/include/hw/i386/pc.h >> +++ b/include/hw/i386/pc.h >> @@ -37,7 +37,7 @@ struct PCMachineState { >> ISADevice *rtc; >> >> uint64_t max_ram_below_4g; >> - bool vmport; >> + vmport vmport; >> }; >> >> #define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device" >> 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', > > vmport as type name violates our coding style. > Should be VMPort. > > But in fact, this is a generic OnOffAuto type, isn't it? > Maybe it should be named like this, and go into qapi/common.json? > I think it might be a good idea but it's not a must. > This looks like the name I will go with. -Don Slutz >> + 'data': [ 'auto', 'on', 'off' ] } >> diff --git a/qemu-options.hx b/qemu-options.hx >> index da9851d..64af16d 100644 >> --- a/qemu-options.hx >> +++ b/qemu-options.hx >> @@ -33,7 +33,7 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \ >> " property accel=accel1[:accel2[:...]] selects accelerator\n" >> " supported accelerators are kvm, xen, tcg (default: tcg)\n" >> " kernel_irqchip=on|off controls accelerated irqchip support\n" >> - " vmport=on|off controls emulation of vmport (default: on)\n" >> + " vmport=on|off|auto controls emulation of vmport (default: auto)\n" >> " kvm_shadow_mem=size of KVM shadow MMU\n" >> " dump-guest-core=on|off include guest memory in a core dump (default=on)\n" >> " mem-merge=on|off controls memory merge support (default: on)\n" >> @@ -52,8 +52,10 @@ than one accelerator specified, the next one is used if the previous one fails >> to initialize. >> @item kernel_irqchip=on|off >> Enables in-kernel irqchip support for the chosen accelerator when available. >> -@item vmport=on|off >> -Enables emulation of VMWare IO port, for vmmouse etc. (enabled by default) >> +@item vmport=on|off|auto >> +Enables emulation of VMWare IO port, for vmmouse etc. auto says to select the >> +value based on accel. For accel=xen the default is off otherwise the default >> +is on. >> @item kvm_shadow_mem=size >> Defines the size of the KVM shadow MMU. >> @item dump-guest-core=on|off >> diff --git a/vl.c b/vl.c >> index f4a6e5e..eb89d62 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -381,7 +381,7 @@ static QemuOptsList qemu_machine_opts = { >> .help = "maximum ram below the 4G boundary (32bit boundary)", >> }, { >> .name = PC_MACHINE_VMPORT, >> - .type = QEMU_OPT_BOOL, >> + .type = QEMU_OPT_STRING, >> .help = "Enable vmport (pc & q35)", >> },{ >> .name = "iommu", >> -- >> 1.8.4
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 = false; + break; + } + } + /* init basic PC hardware */ pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy, - !pc_machine->vmport, 0xff0104); + no_vmport, 0xff0104); /* connect pm stuff to lpc */ ich9_lpc_pm_init(lpc); diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h index 7c3731f..d7d8f30 100644 --- a/include/hw/i386/pc.h +++ b/include/hw/i386/pc.h @@ -37,7 +37,7 @@ struct PCMachineState { ISADevice *rtc; uint64_t max_ram_below_4g; - bool vmport; + vmport vmport; }; #define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device" 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', + 'data': [ 'auto', 'on', 'off' ] } diff --git a/qemu-options.hx b/qemu-options.hx index da9851d..64af16d 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -33,7 +33,7 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \ " property accel=accel1[:accel2[:...]] selects accelerator\n" " supported accelerators are kvm, xen, tcg (default: tcg)\n" " kernel_irqchip=on|off controls accelerated irqchip support\n" - " vmport=on|off controls emulation of vmport (default: on)\n" + " vmport=on|off|auto controls emulation of vmport (default: auto)\n" " kvm_shadow_mem=size of KVM shadow MMU\n" " dump-guest-core=on|off include guest memory in a core dump (default=on)\n" " mem-merge=on|off controls memory merge support (default: on)\n" @@ -52,8 +52,10 @@ than one accelerator specified, the next one is used if the previous one fails to initialize. @item kernel_irqchip=on|off Enables in-kernel irqchip support for the chosen accelerator when available. -@item vmport=on|off -Enables emulation of VMWare IO port, for vmmouse etc. (enabled by default) +@item vmport=on|off|auto +Enables emulation of VMWare IO port, for vmmouse etc. auto says to select the +value based on accel. For accel=xen the default is off otherwise the default +is on. @item kvm_shadow_mem=size Defines the size of the KVM shadow MMU. @item dump-guest-core=on|off diff --git a/vl.c b/vl.c index f4a6e5e..eb89d62 100644 --- a/vl.c +++ b/vl.c @@ -381,7 +381,7 @@ static QemuOptsList qemu_machine_opts = { .help = "maximum ram below the 4G boundary (32bit boundary)", }, { .name = PC_MACHINE_VMPORT, - .type = QEMU_OPT_BOOL, + .type = QEMU_OPT_STRING, .help = "Enable vmport (pc & q35)", },{ .name = "iommu",
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 <dslutz@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(-)