Re: [libvirt] [PATCH REBASE 2/2] qemu: Clean up PCI controller options

2018-02-12 Thread Andrea Bolognani 
On Sun, 2018-02-11 at 08:22 -0500, John Ferlan wrote:
> On 02/05/2018 11:08 AM, Andrea Bolognani wrote:
> > Most of the options are only applicable to one or two controller
> > types, so they should be filtered out everywhere else.
> > 
> > This will reduce user confusion and, in at least one corner case,
> > prevent guests from disappearing on daemon restart.
> > 
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1483816
> 
> So, if I'm reading things correctly - rather than fail because someone
> set an option on a controller (and sometimes for a machine) for which
> it's not supported, the choice is to ignore the value and enforce the
> default.
> 
> This does seem to go against what's been traditionally done (at least my
> recollection of it) for other XML options being wrongly applied to some
> other device.
> 
> Even the bz is a big vague:
> 
> Expected results:
> Schema for the 'target' field in  should
> not accept 'chassis' and 'port' parameters for 'q35' machine type
> 
> But I'd read that as some sort of failure is expected rather than
> acceptance and quietly changing.

Yeah, I'm not sure why I implemented it that way myself. I'll
move everything to a Validate() sub-callback and error out if any
unexpected option is set for a controller.

That will make the test cases added in 1/2 slightly less useful,
though, as it will error out on the first such option instead of
showing that it's resetting all of them.

> > +static void
> > +qemuDomainPCIControllerCleanupOpts(const virDomainDef *def,
> > +   virDomainControllerDefPtr cont)
> > +{
> > +if (cont->type != VIR_DOMAIN_CONTROLLER_TYPE_PCI)
> > +return;
> 
> This is redundant with [1]
> 
> > @@ -4799,6 +4914,8 @@ 
> > qemuDomainControllerDefPostParse(virDomainControllerDefPtr cont,
> >  
> >  case VIR_DOMAIN_CONTROLLER_TYPE_PCI:
> 
> [1] only called when type is PCI...

I prefer not embedding in a function knowledge about its callers,
because callers change all the time. In this specific case, I guess
the name is explicit enough that nobody would try calling it on a
USB controller, so we can probably skip the check.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH REBASE 2/2] qemu: Clean up PCI controller options

2018-02-11 Thread John Ferlan 


On 02/05/2018 11:08 AM, Andrea Bolognani wrote:
> Most of the options are only applicable to one or two controller
> types, so they should be filtered out everywhere else.
> 
> This will reduce user confusion and, in at least one corner case,
> prevent guests from disappearing on daemon restart.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1483816
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  src/qemu/qemu_domain.c | 117 
> +
>  .../pseries-controllers-pciopts.xml|   1 +
>  .../i440fx-controllers-pciopts.xml |   7 +-
>  tests/qemuxml2xmloutdata/pcie-expander-bus.xml |   4 +-
>  .../pseries-controllers-pciopts.xml|   8 +-
>  .../qemuxml2xmloutdata/q35-controllers-pciopts.xml |  21 +---
>  6 files changed, 128 insertions(+), 30 deletions(-)
> 

So, if I'm reading things correctly - rather than fail because someone
set an option on a controller (and sometimes for a machine) for which
it's not supported, the choice is to ignore the value and enforce the
default.

This does seem to go against what's been traditionally done (at least my
recollection of it) for other XML options being wrongly applied to some
other device.

Even the bz is a big vague:

Expected results:
Schema for the 'target' field in  should
not accept 'chassis' and 'port' parameters for 'q35' machine type

But I'd read that as some sort of failure is expected rather than
acceptance and quietly changing.

Curious what other thoughts on this exist?  At the very least be a bit
more pointed in the commit message rather than just saying "filtered" it
should be pointed out that if one is set, it's going to be cleared.

I didn't see any of these in the ABI Stability tests so that's good!

> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index df433c2f0..867f805bd 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4717,6 +4717,121 @@ qemuDomainShmemDefPostParse(virDomainShmemDefPtr shm)
>  #define QEMU_USB_XHCI_MAXPORTS 15
>  
>  
> +/**
> + * qemuDomainPCIControllerCleanupOpts:
> + * @cont: controller
> + *
> + * Clean up PCI options for @cont so that only options applicable to
> + * the controller model will actually be set.
> + */
> +static void
> +qemuDomainPCIControllerCleanupOpts(const virDomainDef *def,
> +   virDomainControllerDefPtr cont)
> +{
> +if (cont->type != VIR_DOMAIN_CONTROLLER_TYPE_PCI)
> +return;

This is redundant with [1]

> +
> +/* pcihole64 and targetIndex */
> +switch ((virDomainControllerModelPCI) cont->model) {
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
> +/* These controllers support all options; however,
> + * targetIndex is only supported for pSeries guests and
> + * pcihole64 is only supported on x86 */
> +if (!qemuDomainIsPSeries(def))
> +cont->opts.pciopts.targetIndex = -1;
> +if (!ARCH_IS_X86(def->os.arch)) {
> +cont->opts.pciopts.pcihole64 = false;
> +cont->opts.pciopts.pcihole64size = 0;
> +}
> +break;
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
> +case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS:
> +/* These controllers don't support any option */
> +cont->opts.pciopts.pcihole64 = false;
> +cont->opts.pciopts.pcihole64size = 0;
> +ATTRIBUTE_FALLTHROUGH;
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
> +/* These controllers support all options except targetIndex */
> +cont->opts.pciopts.targetIndex = -1;
> +break;
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
> +break;
> +}
> +
> +/* busNr and numaNode */
> +switch ((virDomainControllerModelPCI) cont->model) {
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS:
> +/* These controllers support all options */
> +break;
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
> +case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT:
> +/* These controllers don't support any option */
> +cont->opts.pciopts.numaNode = -1;
> +ATTRIBUTE_FALLTHROUGH;
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
> +/* These controllers support all options except busNr; however,
> + * numaNode is only supported

[libvirt] [PATCH REBASE 2/2] qemu: Clean up PCI controller options

2018-02-05 Thread Andrea Bolognani 
Most of the options are only applicable to one or two controller
types, so they should be filtered out everywhere else.

This will reduce user confusion and, in at least one corner case,
prevent guests from disappearing on daemon restart.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1483816

Signed-off-by: Andrea Bolognani 
---
 src/qemu/qemu_domain.c | 117 +
 .../pseries-controllers-pciopts.xml|   1 +
 .../i440fx-controllers-pciopts.xml |   7 +-
 tests/qemuxml2xmloutdata/pcie-expander-bus.xml |   4 +-
 .../pseries-controllers-pciopts.xml|   8 +-
 .../qemuxml2xmloutdata/q35-controllers-pciopts.xml |  21 +---
 6 files changed, 128 insertions(+), 30 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index df433c2f0..867f805bd 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4717,6 +4717,121 @@ qemuDomainShmemDefPostParse(virDomainShmemDefPtr shm)
 #define QEMU_USB_XHCI_MAXPORTS 15
 
 
+/**
+ * qemuDomainPCIControllerCleanupOpts:
+ * @cont: controller
+ *
+ * Clean up PCI options for @cont so that only options applicable to
+ * the controller model will actually be set.
+ */
+static void
+qemuDomainPCIControllerCleanupOpts(const virDomainDef *def,
+   virDomainControllerDefPtr cont)
+{
+if (cont->type != VIR_DOMAIN_CONTROLLER_TYPE_PCI)
+return;
+
+/* pcihole64 and targetIndex */
+switch ((virDomainControllerModelPCI) cont->model) {
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
+/* These controllers support all options; however,
+ * targetIndex is only supported for pSeries guests and
+ * pcihole64 is only supported on x86 */
+if (!qemuDomainIsPSeries(def))
+cont->opts.pciopts.targetIndex = -1;
+if (!ARCH_IS_X86(def->os.arch)) {
+cont->opts.pciopts.pcihole64 = false;
+cont->opts.pciopts.pcihole64size = 0;
+}
+break;
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
+case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS:
+/* These controllers don't support any option */
+cont->opts.pciopts.pcihole64 = false;
+cont->opts.pciopts.pcihole64size = 0;
+ATTRIBUTE_FALLTHROUGH;
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
+/* These controllers support all options except targetIndex */
+cont->opts.pciopts.targetIndex = -1;
+break;
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
+break;
+}
+
+/* busNr and numaNode */
+switch ((virDomainControllerModelPCI) cont->model) {
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS:
+/* These controllers support all options */
+break;
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
+case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT:
+/* These controllers don't support any option */
+cont->opts.pciopts.numaNode = -1;
+ATTRIBUTE_FALLTHROUGH;
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
+/* These controllers support all options except busNr; however,
+ * numaNode is only supported for pSeries guests */
+cont->opts.pciopts.busNr = -1;
+if (!qemuDomainIsPSeries(def))
+cont->opts.pciopts.numaNode = -1;
+break;
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
+break;
+}
+
+/* chassis and port */
+switch ((virDomainControllerModelPCI) cont->model) {
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT:
+/* These controllers support all options */
+break;
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
+case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS:
+/* These controllers don't support any option */
+cont->opts.pciopts.chassis = -1;
+cont->opts.pciopts.port = -1;
+break;
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
+break;
+}
+
+/* chassisNr */
+switch