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 <abologna@xxxxxxxxxx> > --- > 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 <controller model='pci-bridge'> 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 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 ((virDomainControllerModelPCI) cont->model) { > + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: > + /* 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_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.chassisNr = -1; > + break; > + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: > + break; > + } > +} > + > + > static int > qemuDomainControllerDefPostParse(virDomainControllerDefPtr cont, > const virDomainDef *def, > @@ -4799,6 +4914,8 @@ qemuDomainControllerDefPostParse(virDomainControllerDefPtr cont, > > case VIR_DOMAIN_CONTROLLER_TYPE_PCI: > [1] only called when type is PCI... John > + qemuDomainPCIControllerCleanupOpts(def, cont); > + > /* pSeries guests can have multiple pci-root controllers, > * but other machine types only support a single one */ > if (!qemuDomainIsPSeries(def) && > diff --git a/tests/qemuxml2argvdata/pseries-controllers-pciopts.xml b/tests/qemuxml2argvdata/pseries-controllers-pciopts.xml > index 43353cba6..a3237ff2c 100644 > --- a/tests/qemuxml2argvdata/pseries-controllers-pciopts.xml > +++ b/tests/qemuxml2argvdata/pseries-controllers-pciopts.xml > @@ -16,6 +16,7 @@ > <controller type='pci' index='0' model='pci-root'> > <!-- Only targetIndex should be preserved --> > <target busNr='1' chassis='1' chassisNr='1' index='0' port='1'/> > + <pcihole64 unit='KiB'>1024</pcihole64> > </controller> > <controller type='pci' index='1' model='pci-root'> > <!-- Only numaNode and targetIndex should be preserved --> > diff --git a/tests/qemuxml2xmloutdata/i440fx-controllers-pciopts.xml b/tests/qemuxml2xmloutdata/i440fx-controllers-pciopts.xml > index d171d1370..d1868fdc2 100644 > --- a/tests/qemuxml2xmloutdata/i440fx-controllers-pciopts.xml > +++ b/tests/qemuxml2xmloutdata/i440fx-controllers-pciopts.xml > @@ -20,19 +20,16 @@ > <devices> > <emulator>/usr/bin/qemu-system-x86_64</emulator> > <controller type='pci' index='0' model='pci-root'> > - <target chassisNr='1' chassis='1' port='0x1' busNr='1' index='0'/> > <pcihole64 unit='KiB'>1024</pcihole64> > </controller> > <controller type='pci' index='1' model='pci-bridge'> > <model name='pci-bridge'/> > - <target chassisNr='2' chassis='2' port='0x2' busNr='2' index='2'> > - <node>0</node> > - </target> > + <target chassisNr='2'/> > <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> > </controller> > <controller type='pci' index='2' model='pci-expander-bus'> > <model name='pxb'/> > - <target chassisNr='3' chassis='3' port='0x3' busNr='3' index='3'> > + <target busNr='3'> > <node>0</node> > </target> > <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> > diff --git a/tests/qemuxml2xmloutdata/pcie-expander-bus.xml b/tests/qemuxml2xmloutdata/pcie-expander-bus.xml > index aaac423ca..b6498fd2e 100644 > --- a/tests/qemuxml2xmloutdata/pcie-expander-bus.xml > +++ b/tests/qemuxml2xmloutdata/pcie-expander-bus.xml > @@ -36,9 +36,7 @@ > </controller> > <controller type='pci' index='2' model='pcie-root-port'> > <model name='ioh3420'/> > - <target chassis='2' port='0x0' busNr='220'> > - <node>1</node> > - </target> > + <target chassis='2' port='0x0'/> > <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> > </controller> > <controller type='pci' index='3' model='pcie-switch-upstream-port'> > diff --git a/tests/qemuxml2xmloutdata/pseries-controllers-pciopts.xml b/tests/qemuxml2xmloutdata/pseries-controllers-pciopts.xml > index bbe360e25..b5d646765 100644 > --- a/tests/qemuxml2xmloutdata/pseries-controllers-pciopts.xml > +++ b/tests/qemuxml2xmloutdata/pseries-controllers-pciopts.xml > @@ -21,19 +21,17 @@ > <emulator>/usr/bin/qemu-system-ppc64</emulator> > <controller type='pci' index='0' model='pci-root'> > <model name='spapr-pci-host-bridge'/> > - <target chassisNr='1' chassis='1' port='0x1' busNr='1' index='0'/> > + <target index='0'/> > </controller> > <controller type='pci' index='1' model='pci-root'> > <model name='spapr-pci-host-bridge'/> > - <target chassisNr='2' chassis='2' port='0x2' busNr='2' index='1'> > + <target index='1'> > <node>0</node> > </target> > </controller> > <controller type='pci' index='2' model='pci-bridge'> > <model name='pci-bridge'/> > - <target chassisNr='3' chassis='3' port='0x3' busNr='3' index='3'> > - <node>0</node> > - </target> > + <target chassisNr='3'/> > <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> > </controller> > <controller type='usb' index='0' model='none'/> > diff --git a/tests/qemuxml2xmloutdata/q35-controllers-pciopts.xml b/tests/qemuxml2xmloutdata/q35-controllers-pciopts.xml > index 5ef7aa564..d8a014853 100644 > --- a/tests/qemuxml2xmloutdata/q35-controllers-pciopts.xml > +++ b/tests/qemuxml2xmloutdata/q35-controllers-pciopts.xml > @@ -20,49 +20,36 @@ > <devices> > <emulator>/usr/bin/qemu-system-x86_64</emulator> > <controller type='pci' index='0' model='pcie-root'> > - <target chassisNr='1' chassis='1' port='0x1' busNr='1' index='0'/> > <pcihole64 unit='KiB'>1024</pcihole64> > </controller> > <controller type='pci' index='1' model='dmi-to-pci-bridge'> > <model name='i82801b11-bridge'/> > - <target chassisNr='2' chassis='2' port='0x2' busNr='2' index='2'> > - <node>0</node> > - </target> > <address type='pci' domain='0x0000' bus='0x00' slot='0x1e' function='0x0'/> > </controller> > <controller type='pci' index='2' model='pci-bridge'> > <model name='pci-bridge'/> > - <target chassisNr='3' chassis='3' port='0x3' busNr='3' index='3'> > - <node>0</node> > - </target> > + <target chassisNr='3'/> > <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> > </controller> > <controller type='pci' index='3' model='pcie-expander-bus'> > <model name='pxb-pcie'/> > - <target chassisNr='4' chassis='4' port='0x4' busNr='4' index='4'> > + <target busNr='4'> > <node>0</node> > </target> > <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> > </controller> > <controller type='pci' index='4' model='pcie-root-port'> > <model name='pcie-root-port'/> > - <target chassisNr='5' chassis='5' port='0x5' busNr='5' index='5'> > - <node>0</node> > - </target> > + <target chassis='5' port='0x5'/> > <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> > </controller> > <controller type='pci' index='5' model='pcie-switch-upstream-port'> > <model name='x3130-upstream'/> > - <target chassisNr='6' chassis='6' port='0x6' busNr='6' index='6'> > - <node>0</node> > - </target> > <address type='pci' domain='0x0000' bus='0x04' slot='0x00' function='0x0'/> > </controller> > <controller type='pci' index='6' model='pcie-switch-downstream-port'> > <model name='xio3130-downstream'/> > - <target chassisNr='7' chassis='7' port='0x7' busNr='7' index='7'> > - <node>0</node> > - </target> > + <target chassis='7' port='0x7'/> > <address type='pci' domain='0x0000' bus='0x05' slot='0x00' function='0x0'/> > </controller> > <controller type='usb' index='0' model='none'/> > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
