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