| Message ID | 4a65e8b30fe9d2a6c1c53b85ef2697f02e01d13f.1520867956.git.x1917x@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Mar 13, 2018 at 04:34:01AM +1000, Alexey Gerasimenko wrote: > Current Xen/QEMU method to control Xen Platform device on i440 is a bit > odd -- enabling/disabling Xen platform device actually modifies the QEMU > emulated machine type, namely xenfv <--> pc. > > In order to avoid multiplying machine types, use a new way to control Xen > Platform device for QEMU -- "xen-platform-dev" machine property (bool). > To maintain backward compatibility with existing Xen/QEMU setups, this > is only applicable to q35 machine currently. i440 emulation still uses the > old method (i.e. xenfv/pc machine selection) to control Xen Platform > device, this may be changed later to xen-platform-dev property as well. > > This way we can use a single machine type (q35) and change just > xen-platform-dev value to on/off to control Xen platform device. > > Signed-off-by: Alexey Gerasimenko <x1917x@gmail.com> > --- [...] > diff --git a/qemu-options.hx b/qemu-options.hx > index 6585058c6c..cee0b92028 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -38,6 +38,7 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \ > " 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" > " igd-passthru=on|off controls IGD GFX passthrough support (default=off)\n" > + " xen-platform-dev=on|off controls Xen Platform device (default=off)\n" > " aes-key-wrap=on|off controls support for AES key wrapping (default=on)\n" > " dea-key-wrap=on|off controls support for DEA key wrapping (default=on)\n" > " suppress-vmdesc=on|off disables self-describing migration (default=off)\n" What are the obstacles preventing "-device xen-platform" from working? It would be better than adding a new boolean option to -machine.
On Mon, 12 Mar 2018 16:44:06 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: >On Tue, Mar 13, 2018 at 04:34:01AM +1000, Alexey Gerasimenko wrote: >> Current Xen/QEMU method to control Xen Platform device on i440 is a >> bit odd -- enabling/disabling Xen platform device actually modifies >> the QEMU emulated machine type, namely xenfv <--> pc. >> >> In order to avoid multiplying machine types, use a new way to >> control Xen Platform device for QEMU -- "xen-platform-dev" machine >> property (bool). To maintain backward compatibility with existing >> Xen/QEMU setups, this is only applicable to q35 machine currently. >> i440 emulation still uses the old method (i.e. xenfv/pc machine >> selection) to control Xen Platform device, this may be changed later >> to xen-platform-dev property as well. >> >> This way we can use a single machine type (q35) and change just >> xen-platform-dev value to on/off to control Xen platform device. >> >> Signed-off-by: Alexey Gerasimenko <x1917x@gmail.com> >> --- >[...] >> diff --git a/qemu-options.hx b/qemu-options.hx >> index 6585058c6c..cee0b92028 100644 >> --- a/qemu-options.hx >> +++ b/qemu-options.hx >> @@ -38,6 +38,7 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \ >> " 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" " >> igd-passthru=on|off controls IGD GFX passthrough support >> (default=off)\n" >> + " xen-platform-dev=on|off controls Xen Platform >> device (default=off)\n" " aes-key-wrap=on|off >> controls support for AES key wrapping (default=on)\n" >> " dea-key-wrap=on|off controls support for DEA key >> wrapping (default=on)\n" " suppress-vmdesc=on|off >> disables self-describing migration (default=off)\n" > >What are the obstacles preventing "-device xen-platform" from >working? It would be better than adding a new boolean option to >-machine. I guess the initial assumption was that changing the xen_platform_device value in Xen's options may cause some additional changes in platform configuration besides adding (or not) the Xen Platform device, hence a completely different machine type was chosen (xenfv). At the moment pc,accel=xen/xenfv selection mostly governs only the Xen Platform device presence. Also setting max_cpus to HVM_MAX_VCPUS depends on it, but this doesn't applicable to a 'pc,accel=xen' machine for some reason. If applying HVM_MAX_VCPUS to max_cpus is really necessary I think it's better to set it unconditionally for all 'accel=xen' HVM machine types inside xen_enabled() block. Right now it's missing for pc,accel=xen and q35,accel=xen. I'll check if supplying the Xen platform device via the '-device' option will be ok for all usage cases.
On Tue, Mar 13, 2018 at 06:56:37AM +1000, Alexey G wrote: > On Mon, 12 Mar 2018 16:44:06 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > >On Tue, Mar 13, 2018 at 04:34:01AM +1000, Alexey Gerasimenko wrote: > >> Current Xen/QEMU method to control Xen Platform device on i440 is a > >> bit odd -- enabling/disabling Xen platform device actually modifies > >> the QEMU emulated machine type, namely xenfv <--> pc. > >> > >> In order to avoid multiplying machine types, use a new way to > >> control Xen Platform device for QEMU -- "xen-platform-dev" machine > >> property (bool). To maintain backward compatibility with existing > >> Xen/QEMU setups, this is only applicable to q35 machine currently. > >> i440 emulation still uses the old method (i.e. xenfv/pc machine > >> selection) to control Xen Platform device, this may be changed later > >> to xen-platform-dev property as well. > >> > >> This way we can use a single machine type (q35) and change just > >> xen-platform-dev value to on/off to control Xen platform device. > >> > >> Signed-off-by: Alexey Gerasimenko <x1917x@gmail.com> > >> --- > >[...] > >> diff --git a/qemu-options.hx b/qemu-options.hx > >> index 6585058c6c..cee0b92028 100644 > >> --- a/qemu-options.hx > >> +++ b/qemu-options.hx > >> @@ -38,6 +38,7 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \ > >> " 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" " > >> igd-passthru=on|off controls IGD GFX passthrough support > >> (default=off)\n" > >> + " xen-platform-dev=on|off controls Xen Platform > >> device (default=off)\n" " aes-key-wrap=on|off > >> controls support for AES key wrapping (default=on)\n" > >> " dea-key-wrap=on|off controls support for DEA key > >> wrapping (default=on)\n" " suppress-vmdesc=on|off > >> disables self-describing migration (default=off)\n" > > > >What are the obstacles preventing "-device xen-platform" from > >working? It would be better than adding a new boolean option to > >-machine. > > I guess the initial assumption was that changing the > xen_platform_device value in Xen's options may cause some additional > changes in platform configuration besides adding (or not) the Xen > Platform device, hence a completely different machine type was chosen > (xenfv). > > At the moment pc,accel=xen/xenfv selection mostly governs > only the Xen Platform device presence. Also setting max_cpus to > HVM_MAX_VCPUS depends on it, but this doesn't applicable to a > 'pc,accel=xen' machine for some reason. > > If applying HVM_MAX_VCPUS to max_cpus is really necessary I think it's > better to set it unconditionally for all 'accel=xen' HVM machine > types inside xen_enabled() block. Right now it's missing for > pc,accel=xen and q35,accel=xen. If you are talking about MachineClass::max_cpus, note that it is returned by query-machines, so it's supposed to be a static value. Changing it a runtime would mean the query-machines value is incorrect. Is HVM_MAX_CPUS higher or lower than 255? If it's higher, does it mean the current value on pc and q35 isn't accurate? > > I'll check if supplying the Xen platform device via the '-device' option > will be ok for all usage cases. Is HVM_MAX_CPUS something that needs to be enabled because of accel=xen or because or the xen-platform device? If it's just because of accel=xen, we could introduce a AccelClass::max_cpus() method (we also have KVM-imposed CPU count limits, currently implemented inside kvm_init()).
On Tue, Mar 13, 2018 at 04:34:01AM +1000, Alexey Gerasimenko wrote: > Current Xen/QEMU method to control Xen Platform device on i440 is a bit > odd -- enabling/disabling Xen platform device actually modifies the QEMU > emulated machine type, namely xenfv <--> pc. > > In order to avoid multiplying machine types, use a new way to control Xen > Platform device for QEMU -- "xen-platform-dev" machine property (bool). > To maintain backward compatibility with existing Xen/QEMU setups, this > is only applicable to q35 machine currently. i440 emulation still uses the > old method (i.e. xenfv/pc machine selection) to control Xen Platform > device, this may be changed later to xen-platform-dev property as well. The change you made to q35 is pretty tiny, so I imagine the equiv change to pc machine is equally small. IOW, I think you should just convert them both straight away rather than providing an inconsistent configuration approach for q35 vs pc. > This way we can use a single machine type (q35) and change just > xen-platform-dev value to on/off to control Xen platform device. > > Signed-off-by: Alexey Gerasimenko <x1917x@gmail.com> > --- > hw/core/machine.c | 21 +++++++++++++++++++++ > hw/i386/pc_q35.c | 14 ++++++++++++++ > include/hw/boards.h | 1 + > qemu-options.hx | 1 + > 4 files changed, 37 insertions(+) > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index 5e2bbcdace..205e7da3ce 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -290,6 +290,20 @@ static void machine_set_igd_gfx_passthru(Object *obj, bool value, Error **errp) > ms->igd_gfx_passthru = value; > } > > +static bool machine_get_xen_platform_dev(Object *obj, Error **errp) > +{ > + MachineState *ms = MACHINE(obj); > + > + return ms->xen_platform_dev; > +} > + > +static void machine_set_xen_platform_dev(Object *obj, bool value, Error **errp) > +{ > + MachineState *ms = MACHINE(obj); > + > + ms->xen_platform_dev = value; > +} > + > static char *machine_get_firmware(Object *obj, Error **errp) > { > MachineState *ms = MACHINE(obj); > @@ -595,6 +609,13 @@ static void machine_class_init(ObjectClass *oc, void *data) > object_class_property_set_description(oc, "igd-passthru", > "Set on/off to enable/disable igd passthrou", &error_abort); > > + object_class_property_add_bool(oc, "xen-platform-dev", > + machine_get_xen_platform_dev, > + machine_set_xen_platform_dev, &error_abort); > + object_class_property_set_description(oc, "xen-platform-dev", > + "Set on/off to enable/disable Xen Platform device", > + &error_abort); > + > object_class_property_add_str(oc, "firmware", > machine_get_firmware, machine_set_firmware, > &error_abort); > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c > index 0db670f6d7..62caf924cf 100644 > --- a/hw/i386/pc_q35.c > +++ b/hw/i386/pc_q35.c > @@ -56,6 +56,18 @@ > /* ICH9 AHCI has 6 ports */ > #define MAX_SATA_PORTS 6 > > +static void q35_xen_hvm_init(MachineState *machine) > +{ > + PCMachineState *pcms = PC_MACHINE(machine); > + > + if (xen_enabled()) { > + /* check if Xen Platform device is enabled */ > + if (machine->xen_platform_dev) { > + pci_create_simple(pcms->bus, -1, "xen-platform"); > + } > + } > +} > + > /* PC hardware initialisation */ > static void pc_q35_init(MachineState *machine) > { > @@ -207,6 +219,8 @@ static void pc_q35_init(MachineState *machine) > if (xen_enabled()) { > pci_bus_irqs(host_bus, xen_cmn_set_irq, xen_cmn_pci_slot_get_pirq, > ich9_lpc, ICH9_XEN_NUM_IRQ_SOURCES); > + > + q35_xen_hvm_init(machine); > } else { > pci_bus_irqs(host_bus, ich9_lpc_set_irq, ich9_lpc_map_irq, ich9_lpc, > ICH9_LPC_NB_PIRQS); > diff --git a/include/hw/boards.h b/include/hw/boards.h > index efb0a9edfd..f35fc1cc03 100644 > --- a/include/hw/boards.h > +++ b/include/hw/boards.h > @@ -238,6 +238,7 @@ struct MachineState { > bool usb; > bool usb_disabled; > bool igd_gfx_passthru; > + bool xen_platform_dev; > char *firmware; > bool iommu; > bool suppress_vmdesc; > diff --git a/qemu-options.hx b/qemu-options.hx > index 6585058c6c..cee0b92028 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -38,6 +38,7 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \ > " 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" > " igd-passthru=on|off controls IGD GFX passthrough support (default=off)\n" > + " xen-platform-dev=on|off controls Xen Platform device (default=off)\n" > " aes-key-wrap=on|off controls support for AES key wrapping (default=on)\n" > " dea-key-wrap=on|off controls support for DEA key wrapping (default=on)\n" > " suppress-vmdesc=on|off disables self-describing migration (default=off)\n" > -- > 2.11.0 > > Regards, Daniel
On Mon, 12 Mar 2018 18:44:02 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: >On Tue, Mar 13, 2018 at 06:56:37AM +1000, Alexey G wrote: >> On Mon, 12 Mar 2018 16:44:06 -0300 >> Eduardo Habkost <ehabkost@redhat.com> wrote: >> >> >On Tue, Mar 13, 2018 at 04:34:01AM +1000, Alexey Gerasimenko >> >wrote: >> >> Current Xen/QEMU method to control Xen Platform device on i440 is >> >> a bit odd -- enabling/disabling Xen platform device actually >> >> modifies the QEMU emulated machine type, namely xenfv <--> pc. >> >> >> >> In order to avoid multiplying machine types, use a new way to >> >> control Xen Platform device for QEMU -- "xen-platform-dev" machine >> >> property (bool). To maintain backward compatibility with existing >> >> Xen/QEMU setups, this is only applicable to q35 machine currently. >> >> i440 emulation still uses the old method (i.e. xenfv/pc machine >> >> selection) to control Xen Platform device, this may be changed >> >> later to xen-platform-dev property as well. >> >> >> >> This way we can use a single machine type (q35) and change just >> >> xen-platform-dev value to on/off to control Xen platform device. >> >> >> >> Signed-off-by: Alexey Gerasimenko <x1917x@gmail.com> >> >> --- >> >[...] >> >> diff --git a/qemu-options.hx b/qemu-options.hx >> >> index 6585058c6c..cee0b92028 100644 >> >> --- a/qemu-options.hx >> >> +++ b/qemu-options.hx >> >> @@ -38,6 +38,7 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \ >> >> " 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" " >> >> igd-passthru=on|off controls IGD GFX passthrough support >> >> (default=off)\n" >> >> + " xen-platform-dev=on|off controls Xen >> >> Platform device (default=off)\n" " >> >> aes-key-wrap=on|off controls support for AES key wrapping >> >> (default=on)\n" " dea-key-wrap=on|off controls >> >> support for DEA key wrapping (default=on)\n" " >> >> suppress-vmdesc=on|off disables self-describing migration >> >> (default=off)\n" >> > >> >What are the obstacles preventing "-device xen-platform" from >> >working? It would be better than adding a new boolean option to >> >-machine. >> >> I guess the initial assumption was that changing the >> xen_platform_device value in Xen's options may cause some additional >> changes in platform configuration besides adding (or not) the Xen >> Platform device, hence a completely different machine type was chosen >> (xenfv). >> >> At the moment pc,accel=xen/xenfv selection mostly governs >> only the Xen Platform device presence. Also setting max_cpus to >> HVM_MAX_VCPUS depends on it, but this doesn't applicable to a >> 'pc,accel=xen' machine for some reason. >> >> If applying HVM_MAX_VCPUS to max_cpus is really necessary I think >> it's better to set it unconditionally for all 'accel=xen' HVM machine >> types inside xen_enabled() block. Right now it's missing for >> pc,accel=xen and q35,accel=xen. > >If you are talking about MachineClass::max_cpus, note that it is >returned by query-machines, so it's supposed to be a static >value. Changing it a runtime would mean the query-machines value >is incorrect. > >Is HVM_MAX_CPUS higher or lower than 255? If it's higher, does >it mean the current value on pc and q35 isn't accurate? HVM_MAX_VCPUS is 128 currently, but there is an ongoing work from Intel to support more vcpus and >8bit APIC IDs, so this number will likely change soon. According to the code, using HVM_MAX_VCPUS in QEMU is a bit excessive as the maximum number of vcpus is controlled on Xen side anyway. Currently HVM_MAX_VCPUS is used in a one-time check for the maxcpus value (which itself comes from libxl). I think for future compatibility it's better to set mc->max_cpus to HVM_MAX_VCPUS for all accel=xen HVM-supported machine types, not just xenfv. The '-device' approach you suggested seems more preferable than a machine bool property, I'll try switching to it. >Is HVM_MAX_CPUS something that needs to be enabled because of >accel=xen or because or the xen-platform device? > >If it's just because of accel=xen, we could introduce a >AccelClass::max_cpus() method (we also have KVM-imposed CPU count >limits, currently implemented inside kvm_init()).
diff --git a/hw/core/machine.c b/hw/core/machine.c index 5e2bbcdace..205e7da3ce 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -290,6 +290,20 @@ static void machine_set_igd_gfx_passthru(Object *obj, bool value, Error **errp) ms->igd_gfx_passthru = value; } +static bool machine_get_xen_platform_dev(Object *obj, Error **errp) +{ + MachineState *ms = MACHINE(obj); + + return ms->xen_platform_dev; +} + +static void machine_set_xen_platform_dev(Object *obj, bool value, Error **errp) +{ + MachineState *ms = MACHINE(obj); + + ms->xen_platform_dev = value; +} + static char *machine_get_firmware(Object *obj, Error **errp) { MachineState *ms = MACHINE(obj); @@ -595,6 +609,13 @@ static void machine_class_init(ObjectClass *oc, void *data) object_class_property_set_description(oc, "igd-passthru", "Set on/off to enable/disable igd passthrou", &error_abort); + object_class_property_add_bool(oc, "xen-platform-dev", + machine_get_xen_platform_dev, + machine_set_xen_platform_dev, &error_abort); + object_class_property_set_description(oc, "xen-platform-dev", + "Set on/off to enable/disable Xen Platform device", + &error_abort); + object_class_property_add_str(oc, "firmware", machine_get_firmware, machine_set_firmware, &error_abort); diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c index 0db670f6d7..62caf924cf 100644 --- a/hw/i386/pc_q35.c +++ b/hw/i386/pc_q35.c @@ -56,6 +56,18 @@ /* ICH9 AHCI has 6 ports */ #define MAX_SATA_PORTS 6 +static void q35_xen_hvm_init(MachineState *machine) +{ + PCMachineState *pcms = PC_MACHINE(machine); + + if (xen_enabled()) { + /* check if Xen Platform device is enabled */ + if (machine->xen_platform_dev) { + pci_create_simple(pcms->bus, -1, "xen-platform"); + } + } +} + /* PC hardware initialisation */ static void pc_q35_init(MachineState *machine) { @@ -207,6 +219,8 @@ static void pc_q35_init(MachineState *machine) if (xen_enabled()) { pci_bus_irqs(host_bus, xen_cmn_set_irq, xen_cmn_pci_slot_get_pirq, ich9_lpc, ICH9_XEN_NUM_IRQ_SOURCES); + + q35_xen_hvm_init(machine); } else { pci_bus_irqs(host_bus, ich9_lpc_set_irq, ich9_lpc_map_irq, ich9_lpc, ICH9_LPC_NB_PIRQS); diff --git a/include/hw/boards.h b/include/hw/boards.h index efb0a9edfd..f35fc1cc03 100644 --- a/include/hw/boards.h +++ b/include/hw/boards.h @@ -238,6 +238,7 @@ struct MachineState { bool usb; bool usb_disabled; bool igd_gfx_passthru; + bool xen_platform_dev; char *firmware; bool iommu; bool suppress_vmdesc; diff --git a/qemu-options.hx b/qemu-options.hx index 6585058c6c..cee0b92028 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -38,6 +38,7 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \ " 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" " igd-passthru=on|off controls IGD GFX passthrough support (default=off)\n" + " xen-platform-dev=on|off controls Xen Platform device (default=off)\n" " aes-key-wrap=on|off controls support for AES key wrapping (default=on)\n" " dea-key-wrap=on|off controls support for DEA key wrapping (default=on)\n" " suppress-vmdesc=on|off disables self-describing migration (default=off)\n"
Current Xen/QEMU method to control Xen Platform device on i440 is a bit odd -- enabling/disabling Xen platform device actually modifies the QEMU emulated machine type, namely xenfv <--> pc. In order to avoid multiplying machine types, use a new way to control Xen Platform device for QEMU -- "xen-platform-dev" machine property (bool). To maintain backward compatibility with existing Xen/QEMU setups, this is only applicable to q35 machine currently. i440 emulation still uses the old method (i.e. xenfv/pc machine selection) to control Xen Platform device, this may be changed later to xen-platform-dev property as well. This way we can use a single machine type (q35) and change just xen-platform-dev value to on/off to control Xen platform device. Signed-off-by: Alexey Gerasimenko <x1917x@gmail.com> --- hw/core/machine.c | 21 +++++++++++++++++++++ hw/i386/pc_q35.c | 14 ++++++++++++++ include/hw/boards.h | 1 + qemu-options.hx | 1 + 4 files changed, 37 insertions(+)