[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 09/26] conf: Move index number checking to drivers



On 06/12/2017 10:08 PM, Laine Stump wrote:
> On 06/02/2017 12:07 PM, Andrea Bolognani wrote:
>> pSeries guests will soon be allowed to have multiple
>> PHBs (pci-root controllers), which of course means that
>> all but one of them will have a non-zero index; hence,
>> we'll need to relax the current check.
>>
>> However, right now the check is performed in the conf
>> module, which is generic rather than tied to the QEMU
>> driver, and where we don't have information such as the
>> guest machine type available.
>>
>> To make this change of behavior possible down the line,
>> we need to move the check from the XML parser to the
>> driver. Unfortunately, this means duplicating code :(\
> 
> 
> Maybe instead we should just eliminate this check (since the pSeries
> case shows that it's an invalid assumption). And since the index is
> really just something used internally by libvirt, we really don't even
> need to verify that one of the pci controllers has index=0. All we
> *really* care about is that there is at least one "root" PCI bus, and
> that no device includes a bus# in its PCI address that isn't represented
> by the index in one of the PCI controllers.
> 
> (But for the purposes of this patch, I think we can just remove the
> validation in conf and not worry about adding it elsewhere).\\

After looking at the next patch, I may have changed my mind. If we
remove the validation here, then we would still have to add in
validation when the commandline is generated. But I suppose it's better
to give an error at domain definition time rather than domain runtime,
so this makes sense. I don't think all of those drivers actually use PCI
controllers though, do they? (Look for which ones actually call the
functions to assign PCI addresses).


> 
>> ---
>>  src/bhyve/bhyve_domain.c   | 15 +++++++++++++++
>>  src/conf/domain_conf.c     |  6 ------
>>  src/libxl/libxl_domain.c   | 14 ++++++++++++++
>>  src/lxc/lxc_domain.c       | 14 ++++++++++++++
>>  src/openvz/openvz_driver.c | 14 ++++++++++++++
>>  src/qemu/qemu_domain.c     |  9 +++++++++
>>  src/uml/uml_driver.c       | 14 ++++++++++++++
>>  src/vz/vz_driver.c         | 14 ++++++++++++++
>>  src/xen/xen_driver.c       | 14 ++++++++++++++
>>  9 files changed, 108 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/bhyve/bhyve_domain.c b/src/bhyve/bhyve_domain.c
>> index 76b4fac..05c1508 100644
>> --- a/src/bhyve/bhyve_domain.c
>> +++ b/src/bhyve/bhyve_domain.c
>> @@ -122,6 +122,21 @@ bhyveDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
>>              bhyveDomainDiskDefAssignAddress(driver, disk, def) < 0)
>>              return -1;
>>      }
>> +
>> +    if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) {
>> +        virDomainControllerDefPtr cont = dev->data.controller;
>> +
>> +        if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
>> +            (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT ||
>> +             cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) &&
>> +            cont->idx != 0) {
>> +            virReportError(VIR_ERR_XML_ERROR, "%s",
>> +                           _("pci-root and pcie-root controllers "
>> +                             "should have index 0"));
>> +            return -1;
>> +        }
>> +    }
>> +
>>      return 0;
>>  }
>>  
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index c7e20b8..d5dff8e 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -9059,12 +9059,6 @@ virDomainControllerDefParseXML(xmlNodePtr node,
>>                                   "have an address"));
>>                  goto error;
>>              }
>> -            if (def->idx > 0) {
>> -                virReportError(VIR_ERR_XML_ERROR, "%s",
>> -                               _("pci-root and pcie-root controllers "
>> -                                 "should have index 0"));
>> -                goto error;
>> -            }
>>              if ((rc = virDomainParseScaledValue("./pcihole64", NULL,
>>                                                  ctxt, &bytes, 1024,
>>                                                  1024ULL * ULONG_MAX, false)) < 0)
>> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
>> index 256cf1d..59ef2fb 100644
>> --- a/src/libxl/libxl_domain.c
>> +++ b/src/libxl/libxl_domain.c
>> @@ -379,6 +379,20 @@ libxlDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
>>              virDomainDiskSetFormat(disk, VIR_STORAGE_FILE_RAW);
>>      }
>>  
>> +    if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) {
>> +        virDomainControllerDefPtr cont = dev->data.controller;
>> +
>> +        if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
>> +            (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT ||
>> +             cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) &&
>> +            cont->idx != 0) {
>> +            virReportError(VIR_ERR_XML_ERROR, "%s",
>> +                           _("pci-root and pcie-root controllers "
>> +                             "should have index 0"));
>> +            return -1;
>> +        }
>> +    }
>> +
>>      return 0;
>>  }
>>  
>> diff --git a/src/lxc/lxc_domain.c b/src/lxc/lxc_domain.c
>> index 3a7404f..a50f6fe 100644
>> --- a/src/lxc/lxc_domain.c
>> +++ b/src/lxc/lxc_domain.c
>> @@ -389,6 +389,20 @@ virLXCDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
>>          dev->data.chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_NONE)
>>          dev->data.chr->targetType = VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_LXC;
>>  
>> +    if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) {
>> +        virDomainControllerDefPtr cont = dev->data.controller;
>> +
>> +        if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
>> +            (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT ||
>> +             cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) &&
>> +            cont->idx != 0) {
>> +            virReportError(VIR_ERR_XML_ERROR, "%s",
>> +                           _("pci-root and pcie-root controllers "
>> +                             "should have index 0"));
>> +            return -1;
>> +        }
>> +    }
>> +
>>      return 0;
>>  }
>>  
>> diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c
>> index 9c4a196..058d830 100644
>> --- a/src/openvz/openvz_driver.c
>> +++ b/src/openvz/openvz_driver.c
>> @@ -127,6 +127,20 @@ openvzDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
>>          return -1;
>>      }
>>  
>> +    if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) {
>> +        virDomainControllerDefPtr cont = dev->data.controller;
>> +
>> +        if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
>> +            (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT ||
>> +             cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) &&
>> +            cont->idx != 0) {
>> +            virReportError(VIR_ERR_XML_ERROR, "%s",
>> +                           _("pci-root and pcie-root controllers "
>> +                             "should have index 0"));
>> +            return -1;
>> +        }
>> +    }
>> +
>>      return 0;
>>  }
>>  
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 0a85ee9..18512cb 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -3404,6 +3404,15 @@ qemuDomainControllerDefPostParse(virDomainControllerDefPtr cont,
>>          break;
>>  
>>      case VIR_DOMAIN_CONTROLLER_TYPE_PCI:
>> +        if  ((cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT ||
>> +              cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) &&
>> +             cont->idx != 0) {
>> +            virReportError(VIR_ERR_XML_ERROR, "%s",
>> +                           _("pci-root and pcie-root controllers "
>> +                             "should have index 0"));
>> +            return -1;
>> +        }
>> +
>>          if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS &&
>>              !qemuDomainIsI440FX(def)) {
>>              virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c
>> index 03edc89..d9df3c5 100644
>> --- a/src/uml/uml_driver.c
>> +++ b/src/uml/uml_driver.c
>> @@ -426,6 +426,20 @@ umlDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
>>          return -1;
>>      }
>>  
>> +    if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) {
>> +        virDomainControllerDefPtr cont = dev->data.controller;
>> +
>> +        if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
>> +            (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT ||
>> +             cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) &&
>> +            cont->idx != 0) {
>> +            virReportError(VIR_ERR_XML_ERROR, "%s",
>> +                           _("pci-root and pcie-root controllers "
>> +                             "should have index 0"));
>> +            return -1;
>> +        }
>> +    }
>> +
>>      return 0;
>>  }
>>  
>> diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
>> index ef7b453..e988e7c 100644
>> --- a/src/vz/vz_driver.c
>> +++ b/src/vz/vz_driver.c
>> @@ -283,6 +283,20 @@ vzDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
>>          VIR_STRDUP(dev->data.net->model, "e1000") < 0)
>>          return -1;
>>  
>> +    if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) {
>> +        virDomainControllerDefPtr cont = dev->data.controller;
>> +
>> +        if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
>> +            (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT ||
>> +             cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) &&
>> +            cont->idx != 0) {
>> +            virReportError(VIR_ERR_XML_ERROR, "%s",
>> +                           _("pci-root and pcie-root controllers "
>> +                             "should have index 0"));
>> +            return -1;
>> +        }
>> +    }
>> +
>>      return 0;
>>  }
>>  
>> diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c
>> index 4d14089..b681764 100644
>> --- a/src/xen/xen_driver.c
>> +++ b/src/xen/xen_driver.c
>> @@ -362,6 +362,20 @@ xenDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
>>          }
>>      }
>>  
>> +    if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) {
>> +        virDomainControllerDefPtr cont = dev->data.controller;
>> +
>> +        if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
>> +            (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT ||
>> +             cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) &&
>> +            cont->idx != 0) {
>> +            virReportError(VIR_ERR_XML_ERROR, "%s",
>> +                           _("pci-root and pcie-root controllers "
>> +                             "should have index 0"));
>> +            return -1;
>> +        }
>> +    }
>> +
>>      return 0;
>>  }
>>  
>>
> 
> --
> libvir-list mailing list
> libvir-list redhat com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]