Re: [Qemu-devel] [RFC PATCH 16/30] q35/xen: Add Xen platform device support for Q35
On Mon, 12 Mar 2018 18:44:02 -0300
Eduardo Habkost 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 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
>> >> ---
>> >[...]
>> >> 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()).
Re: [Qemu-devel] [RFC PATCH 16/30] q35/xen: Add Xen platform device support for Q35
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
> ---
> 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"
Re: [Qemu-devel] [RFC PATCH 16/30] q35/xen: Add Xen platform device support for Q35
On Tue, Mar 13, 2018 at 06:56:37AM +1000, Alexey G wrote:
> On Mon, 12 Mar 2018 16:44:06 -0300
> Eduardo Habkost 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
> >> ---
> >[...]
> >> 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()).
--
Eduardo
Re: [Qemu-devel] [RFC PATCH 16/30] q35/xen: Add Xen platform device support for Q35
On Mon, 12 Mar 2018 16:44:06 -0300
Eduardo Habkost 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
>> ---
>[...]
>> 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.
Re: [Qemu-devel] [RFC PATCH 16/30] q35/xen: Add Xen platform device support for Q35
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
> ---
[...]
> 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.
--
Eduardo
[Qemu-devel] [RFC PATCH 16/30] q35/xen: Add Xen platform device support for Q35
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
---
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
