[libvirt] [PATCH 0/6] auto-assign addresses when <address type='pci'/> is specified
This is an alternative to Cole's series that permits <address type='pci'/> to force assignment of a PCI address, which is particularly useful on platforms that could connect the same device in different ways (e.g. aarch64/virt). Here is Cole's last iteration of the series: https://www.redhat.com/archives/libvir-list/2016-May/msg01088.html I had expressed a dislike of the "auto_allocate" flag that his series temporarily adds to the address (while simultaneously changing the address type to NONE) and suggested just changing all the necessary places to check for a valid PCI address instead of just checking the address type. He replied that this wasn't as simple as it looked, so I decided to try it; turns out he's right. But I still like this method better because it's not playing tricks with the address type, or adding a temporary private attribute to what should be pure config data. Your opinion may vary though :-) Note that patch 5/6 incorporates the same test case that Cole used in his penultimate patch, and I've added his patch to check the case of aarch64 at the end as well. Cole Robinson (1): tests: qemu: test <address type='pci'/> with aarch64 Laine Stump (5): conf: move virDomainDeviceInfo definition from domain_conf.h to device_conf.h conf: new functions to check if PCI address is wanted/present conf: allow type='pci' addresses with no address attributes specified bhyve: auto-assign addresses when <address type='pci'/> is specified qemu: auto-assign addresses when <address type='pci'/> is specified docs/schemas/basictypes.rng | 8 +- src/bhyve/bhyve_device.c | 10 +- src/conf/device_conf.c | 6 +- src/conf/device_conf.h | 132 ++++++++++++++++++++- src/conf/domain_addr.c | 2 +- src/conf/domain_conf.c | 13 +- src/conf/domain_conf.h | 129 -------------------- src/qemu/qemu_domain_address.c | 64 +++++----- ...l2argv-aarch64-virtio-pci-manual-addresses.args | 4 +- ...ml2argv-aarch64-virtio-pci-manual-addresses.xml | 5 + .../qemuxml2argv-pci-autofill-addr.args | 25 ++++ .../qemuxml2argv-pci-autofill-addr.xml | 35 ++++++ tests/qemuxml2argvtest.c | 1 + ...2xmlout-aarch64-virtio-pci-manual-addresses.xml | 5 + .../qemuxml2xmlout-pci-autofill-addr.xml | 41 +++++++ tests/qemuxml2xmltest.c | 1 + 16 files changed, 298 insertions(+), 183 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pci-autofill-addr.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pci-autofill-addr.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-pci-autofill-addr.xml -- 2.5.5
Also moves all the subordinate structs. This is necessary due to a new inline function that will be defined in device_conf.h, and also makes sense, because it is the *device* info that's in the struct. (Actually a lot more stuff from domain_conf.h could move to this newer file, but I didn't want to disturb any more than necessary). --- src/conf/device_conf.h | 111 +++++++++++++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 129 ------------------------------------------------- 2 files changed, 110 insertions(+), 130 deletions(-) diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h index fc0f4ab..46c720d 100644 --- a/src/conf/device_conf.h +++ b/src/conf/device_conf.h @@ -1,7 +1,7 @@ /* * device_conf.h: device XML handling entry points * - * Copyright (C) 2006-2012, 2014-2015 Red Hat, Inc. + * Copyright (C) 2006-2012, 2014-2016 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -72,6 +72,115 @@ typedef enum { VIR_ENUM_DECL(virNetDevFeature) +typedef enum { + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE, + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI, + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE, + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL, + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCID, + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB, + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO, + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390, + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW, + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO, + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_ISA, + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM, + + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST +} virDomainDeviceAddressType; + +typedef struct _virDomainDeviceDriveAddress { + unsigned int controller; + unsigned int bus; + unsigned int target; + unsigned int unit; +} virDomainDeviceDriveAddress, *virDomainDeviceDriveAddressPtr; + +typedef struct _virDomainDeviceVirtioSerialAddress { + unsigned int controller; + unsigned int bus; + unsigned int port; +} virDomainDeviceVirtioSerialAddress, *virDomainDeviceVirtioSerialAddressPtr; + +# define VIR_DOMAIN_DEVICE_CCW_MAX_CSSID 254 +# define VIR_DOMAIN_DEVICE_CCW_MAX_SSID 3 +# define VIR_DOMAIN_DEVICE_CCW_MAX_DEVNO 65535 + +typedef struct _virDomainDeviceCCWAddress { + unsigned int cssid; + unsigned int ssid; + unsigned int devno; + bool assigned; +} virDomainDeviceCCWAddress, *virDomainDeviceCCWAddressPtr; + +typedef struct _virDomainDeviceCcidAddress { + unsigned int controller; + unsigned int slot; +} virDomainDeviceCcidAddress, *virDomainDeviceCcidAddressPtr; + +typedef struct _virDomainDeviceUSBAddress { + unsigned int bus; + char *port; +} virDomainDeviceUSBAddress, *virDomainDeviceUSBAddressPtr; + +typedef struct _virDomainDeviceSpaprVioAddress { + unsigned long long reg; + bool has_reg; +} virDomainDeviceSpaprVioAddress, *virDomainDeviceSpaprVioAddressPtr; + +typedef enum { + VIR_DOMAIN_CONTROLLER_MASTER_NONE, + VIR_DOMAIN_CONTROLLER_MASTER_USB, + + VIR_DOMAIN_CONTROLLER_MASTER_LAST +} virDomainControllerMaster; + +typedef struct _virDomainDeviceUSBMaster { + unsigned int startport; +} virDomainDeviceUSBMaster, *virDomainDeviceUSBMasterPtr; + +typedef struct _virDomainDeviceISAAddress { + unsigned int iobase; + unsigned int irq; +} virDomainDeviceISAAddress, *virDomainDeviceISAAddressPtr; + +typedef struct _virDomainDeviceDimmAddress { + unsigned int slot; + unsigned long long base; +} virDomainDeviceDimmAddress, *virDomainDeviceDimmAddressPtr; + +typedef struct _virDomainDeviceInfo { + /* If adding to this struct, ensure that + * virDomainDeviceInfoIsSet() is updated + * to consider the new fields + */ + char *alias; + int type; /* virDomainDeviceAddressType */ + union { + virPCIDeviceAddress pci; + virDomainDeviceDriveAddress drive; + virDomainDeviceVirtioSerialAddress vioserial; + virDomainDeviceCcidAddress ccid; + virDomainDeviceUSBAddress usb; + virDomainDeviceSpaprVioAddress spaprvio; + virDomainDeviceCCWAddress ccw; + virDomainDeviceISAAddress isa; + virDomainDeviceDimmAddress dimm; + } addr; + int mastertype; + union { + virDomainDeviceUSBMaster usb; + } master; + /* rombar and romfile are only used for pci hostdev and network + * devices. */ + int rombar; /* enum virTristateSwitch */ + char *romfile; + /* bootIndex is only used for disk, network interface, hostdev + * and redirdev devices */ + unsigned int bootIndex; +} virDomainDeviceInfo, *virDomainDeviceInfoPtr; + + int virPCIDeviceAddressIsValid(virPCIDeviceAddressPtr addr, bool report); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 4e21826..82e581b 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -243,135 +243,6 @@ typedef enum { VIR_ENUM_DECL(virDomainOS) -typedef enum { - VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE, - VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI, - VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE, - VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL, - VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCID, - VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB, - VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO, - VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390, - VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW, - VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO, - VIR_DOMAIN_DEVICE_ADDRESS_TYPE_ISA, - VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM, - - VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST -} virDomainDeviceAddressType; - -typedef struct _virDomainDeviceDriveAddress virDomainDeviceDriveAddress; -typedef virDomainDeviceDriveAddress *virDomainDeviceDriveAddressPtr; -struct _virDomainDeviceDriveAddress { - unsigned int controller; - unsigned int bus; - unsigned int target; - unsigned int unit; -}; - -typedef struct _virDomainDeviceVirtioSerialAddress virDomainDeviceVirtioSerialAddress; -typedef virDomainDeviceVirtioSerialAddress *virDomainDeviceVirtioSerialAddressPtr; -struct _virDomainDeviceVirtioSerialAddress { - unsigned int controller; - unsigned int bus; - unsigned int port; -}; - -# define VIR_DOMAIN_DEVICE_CCW_MAX_CSSID 254 -# define VIR_DOMAIN_DEVICE_CCW_MAX_SSID 3 -# define VIR_DOMAIN_DEVICE_CCW_MAX_DEVNO 65535 - -typedef struct _virDomainDeviceCCWAddress virDomainDeviceCCWAddress; -typedef virDomainDeviceCCWAddress *virDomainDeviceCCWAddressPtr; -struct _virDomainDeviceCCWAddress { - unsigned int cssid; - unsigned int ssid; - unsigned int devno; - bool assigned; -}; - -typedef struct _virDomainDeviceCcidAddress virDomainDeviceCcidAddress; -typedef virDomainDeviceCcidAddress *virDomainDeviceCcidAddressPtr; -struct _virDomainDeviceCcidAddress { - unsigned int controller; - unsigned int slot; -}; - -typedef struct _virDomainDeviceUSBAddress virDomainDeviceUSBAddress; -typedef virDomainDeviceUSBAddress *virDomainDeviceUSBAddressPtr; -struct _virDomainDeviceUSBAddress { - unsigned int bus; - char *port; -}; - -typedef struct _virDomainDeviceSpaprVioAddress virDomainDeviceSpaprVioAddress; -typedef virDomainDeviceSpaprVioAddress *virDomainDeviceSpaprVioAddressPtr; -struct _virDomainDeviceSpaprVioAddress { - unsigned long long reg; - bool has_reg; -}; - -typedef enum { - VIR_DOMAIN_CONTROLLER_MASTER_NONE, - VIR_DOMAIN_CONTROLLER_MASTER_USB, - - VIR_DOMAIN_CONTROLLER_MASTER_LAST -} virDomainControllerMaster; - -typedef struct _virDomainDeviceUSBMaster virDomainDeviceUSBMaster; -typedef virDomainDeviceUSBMaster *virDomainDeviceUSBMasterPtr; -struct _virDomainDeviceUSBMaster { - unsigned int startport; -}; - -typedef struct _virDomainDeviceISAAddress virDomainDeviceISAAddress; -typedef virDomainDeviceISAAddress *virDomainDeviceISAAddressPtr; -struct _virDomainDeviceISAAddress { - unsigned int iobase; - unsigned int irq; -}; - -typedef struct _virDomainDeviceDimmAddress virDomainDeviceDimmAddress; -typedef virDomainDeviceDimmAddress *virDomainDeviceDimmAddressPtr; -struct _virDomainDeviceDimmAddress { - unsigned int slot; - unsigned long long base; -}; - -typedef struct _virDomainDeviceInfo virDomainDeviceInfo; -typedef virDomainDeviceInfo *virDomainDeviceInfoPtr; -struct _virDomainDeviceInfo { - /* If adding to this struct, ensure that - * virDomainDeviceInfoIsSet() is updated - * to consider the new fields - */ - char *alias; - int type; /* virDomainDeviceAddressType */ - union { - virPCIDeviceAddress pci; - virDomainDeviceDriveAddress drive; - virDomainDeviceVirtioSerialAddress vioserial; - virDomainDeviceCcidAddress ccid; - virDomainDeviceUSBAddress usb; - virDomainDeviceSpaprVioAddress spaprvio; - virDomainDeviceCCWAddress ccw; - virDomainDeviceISAAddress isa; - virDomainDeviceDimmAddress dimm; - } addr; - int mastertype; - union { - virDomainDeviceUSBMaster usb; - } master; - /* rombar and romfile are only used for pci hostdev and network - * devices. */ - int rombar; /* enum virTristateSwitch */ - char *romfile; - /* bootIndex is only used for disk, network interface, hostdev - * and redirdev devices */ - unsigned int bootIndex; -}; - - typedef struct _virDomainHostdevOrigStates virDomainHostdevOrigStates; typedef virDomainHostdevOrigStates *virDomainHostdevOrigStatesPtr; struct _virDomainHostdevOrigStates { -- 2.5.5
On Wed, May 18, 2016 at 03:23:49PM -0400, Laine Stump wrote:
Also moves all the subordinate structs. This is necessary due to a new inline function that will be defined in device_conf.h, and also makes sense, because it is the *device* info that's in the struct. (Actually a lot more stuff from domain_conf.h could move to this newer file, but I didn't want to disturb any more than necessary).
This would be easier to read as a pure code motion, with the typedef style change being separate. Jan
--- src/conf/device_conf.h | 111 +++++++++++++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 129 ------------------------------------------------- 2 files changed, 110 insertions(+), 130 deletions(-)
On 05/19/2016 01:20 PM, Ján Tomko wrote:
Also moves all the subordinate structs. This is necessary due to a new inline function that will be defined in device_conf.h, and also makes sense, because it is the *device* info that's in the struct. (Actually a lot more stuff from domain_conf.h could move to this newer file, but I didn't want to disturb any more than necessary). This would be easier to read as a pure code motion, with the typedef
On Wed, May 18, 2016 at 03:23:49PM -0400, Laine Stump wrote: style change being separate.
I gave that a short thought, but since the original patch would end up being a huge chunk of +'s followed by a huge chunk of -'s anyway, I decided to save the extra bit of git history / extra time of reviewing one more patch. Compilation success is also a good indicator that nothing else changed :-)
In order to allow <address type='pci'/> with no other attributes to mean "I want a PCI address, but any PCI address will do" (just as having no <address> at all usually indicates), we will need to change several places in the code from a simple "info->type == (or !=) VIR_DOMAIN_DEVICE_ADDRESS_TYPE_(PCI|NONE)" into something slightly more complex, this patch adds to new functions that take a virDomainDeviceInfoPtr and return true/false depending on 1) whether the current state of the info indicates that we "want" a PCI address for this device (virDeviceInfoPCIAddressWanted()) and 2) whether this device already has a valid PCI address (virDeviceInfoPCIAddressPresent()). Both of these functions required the simpler check for whether a pci address is "empty" (i.e. all of its attributes are 0, which can never happen in a real PCI address, since slot 0 of bus 0 of domain 0 is always reserved), so that function is also added. --- src/conf/device_conf.h | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h index 46c720d..847564b 100644 --- a/src/conf/device_conf.h +++ b/src/conf/device_conf.h @@ -184,6 +184,27 @@ typedef struct _virDomainDeviceInfo { int virPCIDeviceAddressIsValid(virPCIDeviceAddressPtr addr, bool report); +static inline bool +virPCIDeviceAddressIsEmpty(const virPCIDeviceAddress *addr) +{ + return !(addr->domain || addr->bus || addr->slot); +} + +static inline bool +virDeviceInfoPCIAddressWanted(const virDomainDeviceInfo *info) +{ + return info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE || + (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && + virPCIDeviceAddressIsEmpty(&info->addr.pci)); +} + +static inline bool +virDeviceInfoPCIAddressPresent(const virDomainDeviceInfo *info) +{ + return info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && + !virPCIDeviceAddressIsEmpty(&info->addr.pci); +} + int virPCIDeviceAddressParseXML(xmlNodePtr node, virPCIDeviceAddressPtr addr); -- 2.5.5
On Wed, May 18, 2016 at 03:23:50PM -0400, Laine Stump wrote:
In order to allow <address type='pci'/> with no other attributes to mean "I want a PCI address, but any PCI address will do" (just as having no <address> at all usually indicates), we will need to change several places in the code from a simple "info->type == (or !=) VIR_DOMAIN_DEVICE_ADDRESS_TYPE_(PCI|NONE)" into something slightly more complex, this patch adds to new functions that take a virDomainDeviceInfoPtr and return true/false depending on 1) whether the current state of the info indicates that we "want" a PCI address for this device (virDeviceInfoPCIAddressWanted()) and 2) whether this device already has a valid PCI address (virDeviceInfoPCIAddressPresent()).
Both of these functions required the simpler check for whether a pci address is "empty" (i.e. all of its attributes are 0, which can never happen in a real PCI address, since slot 0 of bus 0 of domain 0 is always reserved), so that function is also added. --- src/conf/device_conf.h | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)
diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h index 46c720d..847564b 100644 --- a/src/conf/device_conf.h +++ b/src/conf/device_conf.h @@ -184,6 +184,27 @@ typedef struct _virDomainDeviceInfo { int virPCIDeviceAddressIsValid(virPCIDeviceAddressPtr addr, bool report);
+static inline bool
I think inline keywords are pointless nowadays. Some compilers will inline the functions even if you don't want them to. Jan
On 05/19/2016 01:21 PM, Ján Tomko wrote:
On Wed, May 18, 2016 at 03:23:50PM -0400, Laine Stump wrote:
In order to allow <address type='pci'/> with no other attributes to mean "I want a PCI address, but any PCI address will do" (just as having no <address> at all usually indicates), we will need to change several places in the code from a simple "info->type == (or !=) VIR_DOMAIN_DEVICE_ADDRESS_TYPE_(PCI|NONE)" into something slightly more complex, this patch adds to new functions that take a virDomainDeviceInfoPtr and return true/false depending on 1) whether the current state of the info indicates that we "want" a PCI address for this device (virDeviceInfoPCIAddressWanted()) and 2) whether this device already has a valid PCI address (virDeviceInfoPCIAddressPresent()).
Both of these functions required the simpler check for whether a pci address is "empty" (i.e. all of its attributes are 0, which can never happen in a real PCI address, since slot 0 of bus 0 of domain 0 is always reserved), so that function is also added. --- src/conf/device_conf.h | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)
diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h index 46c720d..847564b 100644 --- a/src/conf/device_conf.h +++ b/src/conf/device_conf.h @@ -184,6 +184,27 @@ typedef struct _virDomainDeviceInfo { int virPCIDeviceAddressIsValid(virPCIDeviceAddressPtr addr, bool report);
+static inline bool I think inline keywords are pointless nowadays. Some compilers will inline the functions even if you don't want them to.
If you remove "inline" from a function defined in a .h file, then it becomes a simple "static bool f()...", which gives you this error when compiling any .c file that includes the .h but doesn't use the function: In file included from util/virnetdev.h:34:0, from util/virnetdevveth.c:36: ./conf/device_conf.h:194:1: error: 'virDeviceInfoPCIAddressWanted' defined but not used [-Werror=unused-function] virDeviceInfoPCIAddressWanted(const virDomainDeviceInfo *info) ^ And if you remove both static *and* inline, you get this: In file included from ./conf/domain_conf.h:48:0, from ./conf/virdomainobjlist.h:28, from util/virclosecallbacks.h:28, from util/virclosecallbacks.c:28: ./conf/device_conf.h:188:1: error: no previous prototype for 'virPCIDeviceAddressIsEmpty' [-Werror=missing-prototypes] virPCIDeviceAddressIsEmpty(const virPCIDeviceAddress *addr) ^ (and if you disabled that warning and got far enough to link, you could end up with a linker error due to multiple definitions of the same function in multiple .o's).
Prior to this, <address type='pci'/> wasn't allowed when parsing (domain+bus+slot+function needed to be a "valid" PCI address, meaning that at least one of domain/bus/slot had to be non-0), the RNG required bus to be specified, and if type was set to PCI when formatting, domain+bus+slot+function would always be output. This makes all the address attributes optional during parse and RNG validation, and suppresses domain+bus+slot+function if domain+bus+slot are all 0 (NB: if d+b+s are all 0, any value for function is nonsensical as that will never happen in the real world, and after the next patch we will always assign a real working address to any empty PCI address before it is ever output to anywhere). Note that explicitly setting all attributes to 0 is equivalent to setting none of them, which is okay, since 0000:00:00 is reserved in any PCI bus setup, and can't be used anyway. --- docs/schemas/basictypes.rng | 8 +++++--- src/conf/device_conf.c | 6 +++--- src/conf/domain_conf.c | 13 ++++++++----- 3 files changed, 16 insertions(+), 11 deletions(-) diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng index e2936d8..83fd4ec 100644 --- a/docs/schemas/basictypes.rng +++ b/docs/schemas/basictypes.rng @@ -83,9 +83,11 @@ <ref name="pciDomain"/> </attribute> </optional> - <attribute name="bus"> - <ref name="pciBus"/> - </attribute> + <optional> + <attribute name="bus"> + <ref name="pciBus"/> + </attribute> + </optional> <optional> <attribute name="slot"> <ref name="pciSlot"/> diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c index 9d9f6a7..4280513 100644 --- a/src/conf/device_conf.c +++ b/src/conf/device_conf.c @@ -1,7 +1,7 @@ /* * device_conf.c: device XML handling * - * Copyright (C) 2006-2015 Red Hat, Inc. + * Copyright (C) 2006-2016 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -90,7 +90,7 @@ int virPCIDeviceAddressIsValid(virPCIDeviceAddressPtr addr, addr->function); return 0; } - if (!(addr->domain || addr->bus || addr->slot)) { + if (virPCIDeviceAddressIsEmpty(addr)) { if (report) virReportError(VIR_ERR_XML_ERROR, "%s", _("Invalid PCI address 0000:00:00, at least " @@ -152,7 +152,7 @@ virPCIDeviceAddressParseXML(xmlNodePtr node, goto cleanup; } - if (!virPCIDeviceAddressIsValid(addr, true)) + if (!virPCIDeviceAddressIsEmpty(addr) && !virPCIDeviceAddressIsValid(addr, true)) goto cleanup; ret = 0; diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1dc45f4..531e660 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4522,11 +4522,14 @@ virDomainDeviceInfoFormat(virBufferPtr buf, switch ((virDomainDeviceAddressType) info->type) { case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI: - virBufferAsprintf(buf, " domain='0x%.4x' bus='0x%.2x' slot='0x%.2x' function='0x%.1x'", - info->addr.pci.domain, - info->addr.pci.bus, - info->addr.pci.slot, - info->addr.pci.function); + if (!virPCIDeviceAddressIsEmpty(&info->addr.pci)) { + virBufferAsprintf(buf, " domain='0x%.4x' bus='0x%.2x' " + "slot='0x%.2x' function='0x%.1x'", + info->addr.pci.domain, + info->addr.pci.bus, + info->addr.pci.slot, + info->addr.pci.function); + } if (info->addr.pci.multi) { virBufferAsprintf(buf, " multifunction='%s'", virTristateSwitchTypeToString(info->addr.pci.multi)); -- 2.5.5
Rather than only assigning a PCI address when no address is given at all, also do it when the config says that the address type is 'pci', but it gives no address. --- src/bhyve/bhyve_device.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/bhyve/bhyve_device.c b/src/bhyve/bhyve_device.c index 3eb2956..8373a5f 100644 --- a/src/bhyve/bhyve_device.c +++ b/src/bhyve/bhyve_device.c @@ -98,7 +98,7 @@ bhyveAssignDevicePCISlots(virDomainDefPtr def, goto error; for (i = 0; i < def->nnets; i++) { - if (def->nets[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) + if (!virDeviceInfoPCIAddressWanted(&def->nets[i]->info)) continue; if (virDomainPCIAddressReserveNextSlot(addrs, &def->nets[i]->info, @@ -107,8 +107,7 @@ bhyveAssignDevicePCISlots(virDomainDefPtr def, } for (i = 0; i < def->ndisks; i++) { - if (def->disks[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && - def->disks[i]->info.addr.pci.slot != 0) + if (!virDeviceInfoPCIAddressWanted(&def->disks[i]->info)) continue; if (virDomainPCIAddressReserveNextSlot(addrs, &def->disks[i]->info, @@ -118,9 +117,8 @@ bhyveAssignDevicePCISlots(virDomainDefPtr def, for (i = 0; i < def->ncontrollers; i++) { if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { - if (def->controllers[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) - continue; - if (def->controllers[i]->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) + if (def->controllers[i]->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT || + !virDeviceInfoPCIAddressWanted(&def->controllers[i]->info)) continue; if (virDomainPCIAddressReserveNextSlot(addrs, -- 2.5.5
Laine Stump wrote:
Rather than only assigning a PCI address when no address is given at all, also do it when the config says that the address type is 'pci', but it gives no address. --- src/bhyve/bhyve_device.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/src/bhyve/bhyve_device.c b/src/bhyve/bhyve_device.c index 3eb2956..8373a5f 100644 --- a/src/bhyve/bhyve_device.c +++ b/src/bhyve/bhyve_device.c @@ -98,7 +98,7 @@ bhyveAssignDevicePCISlots(virDomainDefPtr def, goto error;
for (i = 0; i < def->nnets; i++) { - if (def->nets[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) + if (!virDeviceInfoPCIAddressWanted(&def->nets[i]->info)) continue; if (virDomainPCIAddressReserveNextSlot(addrs, &def->nets[i]->info, @@ -107,8 +107,7 @@ bhyveAssignDevicePCISlots(virDomainDefPtr def, }
for (i = 0; i < def->ndisks; i++) { - if (def->disks[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && - def->disks[i]->info.addr.pci.slot != 0) + if (!virDeviceInfoPCIAddressWanted(&def->disks[i]->info)) continue;
I just noticed that this change breaks address allocation for disks in some cases. E.g. for the following piece virDeviceInfoPCIAddressWanted() returns false because it expects info.type to be either VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE or VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI, but we have VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE in this case. <disk type='file' device='cdrom'> <driver name='file' type='raw'/> <source file='/home/test/foobar.iso'/> <target dev='hdc' bus='sata'/> <readonly/> </disk> Therefore address is not allocated, it stays 0:0 that's reserved for the hostbridge and therefore VM fails to start. Adding <address type='pci'/> fixes that, but that's not very obvious thing for users. Generally, it makes sense, but not in the bhyve driver currently, because it's been using a scheme where each disk resides on its own controller, so we didn't have to bother with disk addressing. Not so long ago a possibility to have multiple disk on a single controller was introduced to bhyve: https://svnweb.freebsd.org/base?view=revision&revision=302459 (thanks to Fabian for the link!) and we'd need to implement proper disk addressing for it. However, for the upcoming release I'm wondering if we should go with a simple solution/workaround similar to this one: diff --git a/src/bhyve/bhyve_device.c b/src/bhyve/bhyve_device.c index 8373a5f..f662012 100644 --- a/src/bhyve/bhyve_device.c +++ b/src/bhyve/bhyve_device.c @@ -107,7 +107,8 @@ bhyveAssignDevicePCISlots(virDomainDefPtr def, } for (i = 0; i < def->ndisks; i++) { - if (!virDeviceInfoPCIAddressWanted(&def->disks[i]->info)) + if (def->disks[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && + !virPCIDeviceAddressIsEmpty(&def->disks[i]->info.addr.pci)) continue; if (virDomainPCIAddressReserveNextSlot(addrs, &def->disks[i]->info, Thoughts?
if (virDomainPCIAddressReserveNextSlot(addrs, &def->disks[i]->info, @@ -118,9 +117,8 @@ bhyveAssignDevicePCISlots(virDomainDefPtr def,
for (i = 0; i < def->ncontrollers; i++) { if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { - if (def->controllers[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) - continue; - if (def->controllers[i]->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) + if (def->controllers[i]->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT || + !virDeviceInfoPCIAddressWanted(&def->controllers[i]->info)) continue;
if (virDomainPCIAddressReserveNextSlot(addrs, -- 2.5.5
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Roman Bogorodskiy
On 08/27/2016 11:42 AM, Roman Bogorodskiy wrote:
Laine Stump wrote:
Rather than only assigning a PCI address when no address is given at all, also do it when the config says that the address type is 'pci', but it gives no address. --- src/bhyve/bhyve_device.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/src/bhyve/bhyve_device.c b/src/bhyve/bhyve_device.c index 3eb2956..8373a5f 100644 --- a/src/bhyve/bhyve_device.c +++ b/src/bhyve/bhyve_device.c @@ -98,7 +98,7 @@ bhyveAssignDevicePCISlots(virDomainDefPtr def, goto error;
for (i = 0; i < def->nnets; i++) { - if (def->nets[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) + if (!virDeviceInfoPCIAddressWanted(&def->nets[i]->info)) continue; if (virDomainPCIAddressReserveNextSlot(addrs, &def->nets[i]->info, @@ -107,8 +107,7 @@ bhyveAssignDevicePCISlots(virDomainDefPtr def, }
for (i = 0; i < def->ndisks; i++) { - if (def->disks[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && - def->disks[i]->info.addr.pci.slot != 0) + if (!virDeviceInfoPCIAddressWanted(&def->disks[i]->info)) continue; I just noticed that this change breaks address allocation for disks in some cases.
E.g. for the following piece virDeviceInfoPCIAddressWanted() returns false because it expects info.type to be either VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE or VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI, but we have VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE in this case.
<disk type='file' device='cdrom'> <driver name='file' type='raw'/> <source file='/home/test/foobar.iso'/> <target dev='hdc' bus='sata'/> <readonly/> </disk>
Therefore address is not allocated, it stays 0:0 that's reserved for the hostbridge and therefore VM fails to start.
Adding <address type='pci'/> fixes that, but that's not very obvious thing for users.
Generally, it makes sense, but not in the bhyve driver currently, because it's been using a scheme where each disk resides on its own controller, so we didn't have to bother with disk addressing.
Not so long ago a possibility to have multiple disk on a single controller was introduced to bhyve: https://svnweb.freebsd.org/base?view=revision&revision=302459 (thanks to Fabian for the link!) and we'd need to implement proper disk addressing for it.
However, for the upcoming release I'm wondering if we should go with a simple solution/workaround similar to this one:
diff --git a/src/bhyve/bhyve_device.c b/src/bhyve/bhyve_device.c index 8373a5f..f662012 100644 --- a/src/bhyve/bhyve_device.c +++ b/src/bhyve/bhyve_device.c @@ -107,7 +107,8 @@ bhyveAssignDevicePCISlots(virDomainDefPtr def, }
for (i = 0; i < def->ndisks; i++) { - if (!virDeviceInfoPCIAddressWanted(&def->disks[i]->info)) + if (def->disks[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && + !virPCIDeviceAddressIsEmpty(&def->disks[i]->info.addr.pci)) continue; if (virDomainPCIAddressReserveNextSlot(addrs, &def->disks[i]->info,
Thoughts?
Because bus='sata', the address type is set to "drive" during the device post-parse for disk devices. And when the address type is 'drive', it's a bug to assign a PCI address to it. Using the shortcut of describing a drive plus a controller in a single device by misusing the address type may have seemed expedient, but it's incorrect and needs to be fixed. And the longer you wait to fix it, the worse the consequences will be. On the other hand, it's been broken (but working) for a long time already, whereas it's been *non-working* for a shorter time, so it makes sense to temporarily restore the broken-but-working behavior for this release, then start working on a permanent solution. Your proposed change is technically not correct, though.You really should only be calling virPCIDeviceAddressIsEmpty() if the address type='pci'. If I understand correctly, you currently give *all* disk devices a PCI address, so what you really want is this: if(def->disks[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && !virPCIDeviceAddressIsEmpty(&def->disks[i]->info.addr.pci)) continue; This way you will always assign a PCI address to every disk, unless it already has an address assigned. I'm okay with ACKing this as a temporary fix until you can fix it correctly (anyway, really the final word is up to you, since you're the bhyve maintainer :-) So, beyond the temporary fix, how do these disks appear inside the guest? Are they connected to a SATA controller and using the guest OS' sata driver? Or are they connected directly to the PCI bus and using some other driver (similar to virtio-blk-pci)? The XML should reflect what the emulated hardware really looks like, whereas right now what you end up with after the addressing is done, is this: <disk type='file' device='cdrom'> <driver name='file' type='raw'/> <source file='/home/test/foobar.iso'/> <target dev='hdc' bus='sata'/> <address type='pci' domain='0x0000' bus='0x00' slot='0xNN' function='0x0'/> <readonly/> </disk> I think the first thing you need to start doing (after this release) is to separate that into: <controller type='sata' index='${I}'> <address type='pci' domain='0x0000' bus='0x00' slot='0xNN' function='0x0'/> </controller> <disk type='file' device='cdrom'> <driver name='file' type='raw'/> <source file='/home/test/foobar.iso'/> <target dev='hdc' bus='sata'/> <address type='drive' controller='${I}' bus='0' target='0' unit='0'/> <readonly/> </disk> Even if the version of bhyve only supports a single disk per sata controller, you should still do this. Later, when support is added for multiple disks on the same SATA controller, you can allow unit to increment for each disk on a particular controller (0-5). Follow the rules in the VIR_DOMAIN_DISK_BUS_SATA case of virDomainDiskDefAssignAddress - target and bus are always 0, controller is the same as the "index" attribute of the desired controller, and unit is 0-5 (6 disks per controller).
if (virDomainPCIAddressReserveNextSlot(addrs, &def->disks[i]->info, @@ -118,9 +117,8 @@ bhyveAssignDevicePCISlots(virDomainDefPtr def,
for (i = 0; i < def->ncontrollers; i++) { if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { - if (def->controllers[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) - continue; - if (def->controllers[i]->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) + if (def->controllers[i]->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT || + !virDeviceInfoPCIAddressWanted(&def->controllers[i]->info)) continue;
if (virDomainPCIAddressReserveNextSlot(addrs, -- 2.5.5
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Roman Bogorodskiy
Laine Stump wrote:
On 08/27/2016 11:42 AM, Roman Bogorodskiy wrote:
Laine Stump wrote:
Rather than only assigning a PCI address when no address is given at all, also do it when the config says that the address type is 'pci', but it gives no address. --- src/bhyve/bhyve_device.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/src/bhyve/bhyve_device.c b/src/bhyve/bhyve_device.c index 3eb2956..8373a5f 100644 --- a/src/bhyve/bhyve_device.c +++ b/src/bhyve/bhyve_device.c @@ -98,7 +98,7 @@ bhyveAssignDevicePCISlots(virDomainDefPtr def, goto error;
for (i = 0; i < def->nnets; i++) { - if (def->nets[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) + if (!virDeviceInfoPCIAddressWanted(&def->nets[i]->info)) continue; if (virDomainPCIAddressReserveNextSlot(addrs, &def->nets[i]->info, @@ -107,8 +107,7 @@ bhyveAssignDevicePCISlots(virDomainDefPtr def, }
for (i = 0; i < def->ndisks; i++) { - if (def->disks[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && - def->disks[i]->info.addr.pci.slot != 0) + if (!virDeviceInfoPCIAddressWanted(&def->disks[i]->info)) continue; I just noticed that this change breaks address allocation for disks in some cases.
E.g. for the following piece virDeviceInfoPCIAddressWanted() returns false because it expects info.type to be either VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE or VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI, but we have VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE in this case.
<disk type='file' device='cdrom'> <driver name='file' type='raw'/> <source file='/home/test/foobar.iso'/> <target dev='hdc' bus='sata'/> <readonly/> </disk>
Therefore address is not allocated, it stays 0:0 that's reserved for the hostbridge and therefore VM fails to start.
Adding <address type='pci'/> fixes that, but that's not very obvious thing for users.
Generally, it makes sense, but not in the bhyve driver currently, because it's been using a scheme where each disk resides on its own controller, so we didn't have to bother with disk addressing.
Not so long ago a possibility to have multiple disk on a single controller was introduced to bhyve: https://svnweb.freebsd.org/base?view=revision&revision=302459 (thanks to Fabian for the link!) and we'd need to implement proper disk addressing for it.
However, for the upcoming release I'm wondering if we should go with a simple solution/workaround similar to this one:
diff --git a/src/bhyve/bhyve_device.c b/src/bhyve/bhyve_device.c index 8373a5f..f662012 100644 --- a/src/bhyve/bhyve_device.c +++ b/src/bhyve/bhyve_device.c @@ -107,7 +107,8 @@ bhyveAssignDevicePCISlots(virDomainDefPtr def, }
for (i = 0; i < def->ndisks; i++) { - if (!virDeviceInfoPCIAddressWanted(&def->disks[i]->info)) + if (def->disks[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && + !virPCIDeviceAddressIsEmpty(&def->disks[i]->info.addr.pci)) continue; if (virDomainPCIAddressReserveNextSlot(addrs, &def->disks[i]->info,
Thoughts?
Because bus='sata', the address type is set to "drive" during the device post-parse for disk devices. And when the address type is 'drive', it's a bug to assign a PCI address to it. Using the shortcut of describing a drive plus a controller in a single device by misusing the address type may have seemed expedient, but it's incorrect and needs to be fixed. And the longer you wait to fix it, the worse the consequences will be.
Yes, that's a bug, though bhyve didn't have a notion of disk addressing within controller, e.g. until recently it had only: -s $slot,virtio-blk,/my/image Where $slot is: slot pcislot[:function] bus:pcislot:function So a similar scheme was used in the driver. Now bhyve supports a more flexible disk management like this: -s 1:0,ahci,hd:/images/disk.1,hd:/images/disk.2,\ hd:/images/disk.3,hd:/images/disk.4,\ hd:/images/disk.5,hd:/images/disk.6,\ hd:/images/disk.7,hd:/images/disk.8,\ cd:/images/install.iso \ So it's a good time to re-do that in the bhyve driver in a proper way.
On the other hand, it's been broken (but working) for a long time already, whereas it's been *non-working* for a shorter time, so it makes sense to temporarily restore the broken-but-working behavior for this release, then start working on a permanent solution. Your proposed change is technically not correct, though.You really should only be calling virPCIDeviceAddressIsEmpty() if the address type='pci'. If I understand correctly, you currently give *all* disk devices a PCI address, so what you really want is this:
if(def->disks[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && !virPCIDeviceAddressIsEmpty(&def->disks[i]->info.addr.pci)) continue;
This way you will always assign a PCI address to every disk, unless it already has an address assigned. I'm okay with ACKing this as a temporary fix until you can fix it correctly (anyway, really the final word is up to you, since you're the bhyve maintainer :-)
So, beyond the temporary fix, how do these disks appear inside the guest? Are they connected to a SATA controller and using the guest OS' sata driver? Or are they connected directly to the PCI bus and using some other driver (similar to virtio-blk-pci)? The XML should reflect what the emulated hardware really looks like, whereas right now what you end up with after the addressing is done, is this:
<disk type='file' device='cdrom'> <driver name='file' type='raw'/> <source file='/home/test/foobar.iso'/> <target dev='hdc' bus='sata'/> <address type='pci' domain='0x0000' bus='0x00' slot='0xNN' function='0x0'/> <readonly/> </disk>
They look like controllers with disks inside the guest. E.g.: <disk type='file' device='cdrom'> <driver name='file' type='raw'/> <source file='/home/novel/FreeBSD-11.0-CURRENT-amd64-20160217-r295683-disc1.iso'/> <backingStore/> <target dev='hdc' bus='sata'/> <readonly/> <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> </disk> Looks like this in the guest: ahci0@pci0:0:3:0: class=0x010601 card=0x00000000 chip=0x28218086 rev=0x00 hdr=0x00 vendor = 'Intel Corporation' device = '82801HR/HO/HH (ICH8R/DO/DH) 6 port SATA Controller [AHCI mode]' class = mass storage subclass = SATA And the actual cdrom device there: root@fbsdvm01:~ # dmesg | grep -E "(ahci0|cd0)" ahci0: <Intel ICH8 AHCI SATA controller> mem 0xc0002000-0xc00023ff irq 17 at device 3.0 on pci0 ahci0: AHCI v1.30 with 6 6Gbps ports, Port Multiplier not supported ahcich0: <AHCI channel> at channel 0 on ahci0 cd0 at ahcich0 bus 0 scbus0 target 0 lun 0 cd0: <BHYVE BHYVE DVD-ROM 001> Removable CD-ROM SCSI device cd0: Serial Number BHYVE-13E5-4C93-29EB cd0: 600.000MB/s transfers (SATA 3.x, UDMA6, ATAPI 12bytes, PIO 8192bytes) cd0: 811MB (415600 2048 byte sectors) root@fbsdvm01:~ #
I think the first thing you need to start doing (after this release) is to separate that into:
<controller type='sata' index='${I}'> <address type='pci' domain='0x0000' bus='0x00' slot='0xNN' function='0x0'/> </controller> <disk type='file' device='cdrom'> <driver name='file' type='raw'/> <source file='/home/test/foobar.iso'/> <target dev='hdc' bus='sata'/> <address type='drive' controller='${I}' bus='0' target='0' unit='0'/> <readonly/> </disk>
Even if the version of bhyve only supports a single disk per sata controller, you should still do this. Later, when support is added for multiple disks on the same SATA controller, you can allow unit to increment for each disk on a particular controller (0-5). Follow the rules in the VIR_DOMAIN_DISK_BUS_SATA case of virDomainDiskDefAssignAddress - target and bus are always 0, controller is the same as the "index" attribute of the desired controller, and unit is 0-5 (6 disks per controller).
Yes, hopefully we'll do that before the October release. In the meantime I'll submit a fix to get back to 'buggy-but-working' approach for the next release.
if (virDomainPCIAddressReserveNextSlot(addrs, &def->disks[i]->info, @@ -118,9 +117,8 @@ bhyveAssignDevicePCISlots(virDomainDefPtr def,
for (i = 0; i < def->ncontrollers; i++) { if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { - if (def->controllers[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) - continue; - if (def->controllers[i]->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) + if (def->controllers[i]->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT || + !virDeviceInfoPCIAddressWanted(&def->controllers[i]->info)) continue;
if (virDomainPCIAddressReserveNextSlot(addrs, -- 2.5.5
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Roman Bogorodskiy
Roman Bogorodskiy
Rather than only assigning a PCI address when no address is given at all, also do it when the config says that the address type is 'pci', but it gives no address (virDeviceInfoPCIAddressWanted()). There are also several places after parsing but prior to address assignment where code previously expected that any info with address type='pci' would have a *valid* PCI address, which isn't always the case - now we check not only for type='pci', but also for a valid address (virDeviceInfoPCIAddressPresent()). The test case added in this patch was directly copied from Cole's patch titled: qemu: Wire up address type=pci auto_allocate --- src/conf/domain_addr.c | 2 +- src/qemu/qemu_domain_address.c | 64 ++++++++++------------ .../qemuxml2argv-pci-autofill-addr.args | 25 +++++++++ .../qemuxml2argv-pci-autofill-addr.xml | 35 ++++++++++++ tests/qemuxml2argvtest.c | 1 + .../qemuxml2xmlout-pci-autofill-addr.xml | 41 ++++++++++++++ tests/qemuxml2xmltest.c | 1 + 7 files changed, 134 insertions(+), 35 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pci-autofill-addr.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pci-autofill-addr.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-pci-autofill-addr.xml diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index acd8ce6..794270d 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -471,7 +471,7 @@ virDomainPCIAddressEnsureAddr(virDomainPCIAddressSetPtr addrs, if (!(addrStr = virDomainPCIAddressAsString(&dev->addr.pci))) goto cleanup; - if (dev->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { + if (virDeviceInfoPCIAddressPresent(dev)) { /* We do not support hotplug multi-function PCI device now, so we should * reserve the whole slot. The function of the PCI device must be 0. */ diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 9d09b3a..7bd8ee5 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -431,9 +431,9 @@ qemuDomainCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, virDomainPCIConnectFlags flags = (VIR_PCI_CONNECT_HOTPLUGGABLE | VIR_PCI_CONNECT_TYPE_PCI_DEVICE); - if ((info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) - || ((device->type == VIR_DOMAIN_DEVICE_HOSTDEV) && - (device->data.hostdev->parent.type != VIR_DOMAIN_DEVICE_NONE))) { + if (!virDeviceInfoPCIAddressPresent(info) || + ((device->type == VIR_DOMAIN_DEVICE_HOSTDEV) && + (device->data.hostdev->parent.type != VIR_DOMAIN_DEVICE_NONE))) { /* If a hostdev has a parent, its info will be a part of the * parent, and will have its address collected during the scan * of the parent's device type. @@ -633,7 +633,7 @@ qemuDomainValidateDevicePCISlotsPIIX3(virDomainDefPtr def, /* First IDE controller lives on the PIIX3 at slot=1, function=1 */ if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE && def->controllers[i]->idx == 0) { - if (def->controllers[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { + if (virDeviceInfoPCIAddressPresent(&def->controllers[i]->info)) { if (def->controllers[i]->info.addr.pci.domain != 0 || def->controllers[i]->info.addr.pci.bus != 0 || def->controllers[i]->info.addr.pci.slot != 1 || @@ -653,7 +653,7 @@ qemuDomainValidateDevicePCISlotsPIIX3(virDomainDefPtr def, def->controllers[i]->idx == 0 && (def->controllers[i]->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI || def->controllers[i]->model == -1)) { - if (def->controllers[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { + if (virDeviceInfoPCIAddressPresent(&def->controllers[i]->info)) { if (def->controllers[i]->info.addr.pci.domain != 0 || def->controllers[i]->info.addr.pci.bus != 0 || def->controllers[i]->info.addr.pci.slot != 1 || @@ -690,7 +690,7 @@ qemuDomainValidateDevicePCISlotsPIIX3(virDomainDefPtr def, * at slot 2. */ virDomainVideoDefPtr primaryVideo = def->videos[0]; - if (primaryVideo->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { + if (virDeviceInfoPCIAddressWanted(&primaryVideo->info)) { memset(&tmp_addr, 0, sizeof(tmp_addr)); tmp_addr.slot = 2; @@ -769,7 +769,7 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def, * address. */ if (def->controllers[i]->idx == 0) { - if (def->controllers[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { + if (virDeviceInfoPCIAddressPresent(&def->controllers[i]->info)) { if (def->controllers[i]->info.addr.pci.domain != 0 || def->controllers[i]->info.addr.pci.bus != 0 || def->controllers[i]->info.addr.pci.slot != 0x1F || @@ -881,7 +881,7 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def, * on pc machinetypes). */ virDomainVideoDefPtr primaryVideo = def->videos[0]; - if (primaryVideo->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { + if (virDeviceInfoPCIAddressWanted(&primaryVideo->info)) { memset(&tmp_addr, 0, sizeof(tmp_addr)); tmp_addr.slot = 1; @@ -1030,9 +1030,9 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def, if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { virDomainControllerModelPCI model = def->controllers[i]->model; - if (def->controllers[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE || - model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT || - model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) + if (model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT || + model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT || + !virDeviceInfoPCIAddressWanted(&def->controllers[i]->info)) continue; /* convert the type of controller into a "CONNECT_TYPE" @@ -1055,7 +1055,7 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def, flags = VIR_PCI_CONNECT_HOTPLUGGABLE | VIR_PCI_CONNECT_TYPE_PCI_DEVICE; for (i = 0; i < def->nfss; i++) { - if (def->fss[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) + if (!virDeviceInfoPCIAddressWanted(&def->fss[i]->info)) continue; /* Only support VirtIO-9p-pci so far. If that changes, @@ -1072,7 +1072,7 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def, * instead of here. */ if ((def->nets[i]->type == VIR_DOMAIN_NET_TYPE_HOSTDEV) || - (def->nets[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)) { + !virDeviceInfoPCIAddressWanted(&def->nets[i]->info)) { continue; } if (virDomainPCIAddressReserveNextSlot(addrs, &def->nets[i]->info, @@ -1082,7 +1082,7 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def, /* Sound cards */ for (i = 0; i < def->nsounds; i++) { - if (def->sounds[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) + if (!virDeviceInfoPCIAddressWanted(&def->sounds[i]->info)) continue; /* Skip ISA sound card, PCSPK and usb-audio */ if (def->sounds[i]->model == VIR_DOMAIN_SOUND_MODEL_SB16 || @@ -1117,7 +1117,7 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def, def->controllers[i]->idx == 0) continue; - if (def->controllers[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) + if (!virDeviceInfoPCIAddressWanted(&def->controllers[i]->info)) continue; /* USB2 needs special handling to put all companions in the same slot */ @@ -1129,8 +1129,7 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def, for (j = 0; j < def->ncontrollers; j++) { if (IS_USB2_CONTROLLER(def->controllers[j]) && def->controllers[j]->idx == def->controllers[i]->idx && - def->controllers[j]->info.type - == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { + virDeviceInfoPCIAddressPresent(&def->controllers[j]->info)) { addr = def->controllers[j]->info.addr.pci; foundAddr = true; break; @@ -1191,7 +1190,7 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def, continue; /* don't touch s390 devices */ - if (def->disks[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI || + if (virDeviceInfoPCIAddressPresent(&def->disks[i]->info) || def->disks[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390 || def->disks[i]->info.type == @@ -1204,7 +1203,7 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def, virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_MMIO)) continue; - if (def->disks[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { + if (!virDeviceInfoPCIAddressWanted(&def->disks[i]->info)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("virtio disk cannot have an address of type '%s'"), virDomainDeviceAddressTypeToString(def->disks[i]->info.type)); @@ -1218,7 +1217,7 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def, /* Host PCI devices */ for (i = 0; i < def->nhostdevs; i++) { - if (def->hostdevs[i]->info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) + if (!virDeviceInfoPCIAddressWanted(def->hostdevs[i]->info)) continue; if (def->hostdevs[i]->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS || def->hostdevs[i]->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) @@ -1233,7 +1232,7 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def, /* VirtIO balloon */ if (def->memballoon && def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO && - def->memballoon->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { + virDeviceInfoPCIAddressWanted(&def->memballoon->info)) { if (virDomainPCIAddressReserveNextSlot(addrs, &def->memballoon->info, flags) < 0) @@ -1243,7 +1242,7 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def, /* VirtIO RNG */ for (i = 0; i < def->nrngs; i++) { if (def->rngs[i]->model != VIR_DOMAIN_RNG_MODEL_VIRTIO || - def->rngs[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) + !virDeviceInfoPCIAddressWanted(&def->rngs[i]->info)) continue; if (virDomainPCIAddressReserveNextSlot(addrs, @@ -1254,7 +1253,7 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def, /* A watchdog - check if it is a PCI device */ if (def->watchdog && def->watchdog->model == VIR_DOMAIN_WATCHDOG_MODEL_I6300ESB && - def->watchdog->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { + virDeviceInfoPCIAddressWanted(&def->watchdog->info)) { if (virDomainPCIAddressReserveNextSlot(addrs, &def->watchdog->info, flags) < 0) goto error; @@ -1263,7 +1262,7 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def, /* Assign a PCI slot to the primary video card if there is not an * assigned address. */ if (def->nvideos > 0 && - def->videos[0]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { + virDeviceInfoPCIAddressWanted(&def->videos[0]->info)) { if (virDomainPCIAddressReserveNextSlot(addrs, &def->videos[0]->info, flags) < 0) goto error; @@ -1276,7 +1275,7 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def, _("non-primary video device must be type of 'qxl'")); goto error; } - if (def->videos[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) + if (!virDeviceInfoPCIAddressWanted(&def->videos[i]->info)) continue; if (virDomainPCIAddressReserveNextSlot(addrs, &def->videos[i]->info, flags) < 0) @@ -1285,7 +1284,7 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def, /* Shared Memory */ for (i = 0; i < def->nshmems; i++) { - if (def->shmems[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) + if (!virDeviceInfoPCIAddressWanted(&def->shmems[i]->info)) continue; if (virDomainPCIAddressReserveNextSlot(addrs, @@ -1293,9 +1292,8 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def, goto error; } for (i = 0; i < def->ninputs; i++) { - if (def->inputs[i]->bus != VIR_DOMAIN_INPUT_BUS_VIRTIO) - continue; - if (def->inputs[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) + if (def->inputs[i]->bus != VIR_DOMAIN_INPUT_BUS_VIRTIO || + !virDeviceInfoPCIAddressWanted(&def->inputs[i]->info)) continue; if (virDomainPCIAddressReserveNextSlot(addrs, @@ -1308,10 +1306,8 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def, for (i = 0; i < def->nserials; i++) { virDomainChrDefPtr chr = def->serials[i]; - if (chr->targetType != VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI) - continue; - - if (chr->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) + if (chr->targetType != VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI || + !virDeviceInfoPCIAddressWanted(&chr->info)) continue; if (virDomainPCIAddressReserveNextSlot(addrs, &chr->info, flags) < 0) @@ -1677,7 +1673,7 @@ qemuDomainReleaseDeviceAddress(virDomainObjPtr vm, virDomainCCWAddressReleaseAddr(priv->ccwaddrs, info) < 0) VIR_WARN("Unable to release CCW address on %s", NULLSTR(devstr)); - else if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && + else if (virDeviceInfoPCIAddressPresent(info) && virDomainPCIAddressReleaseSlot(priv->pciaddrs, &info->addr.pci) < 0) VIR_WARN("Unable to release PCI address on %s", diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pci-autofill-addr.args b/tests/qemuxml2argvdata/qemuxml2argv-pci-autofill-addr.args new file mode 100644 index 0000000..ddb8c8d --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pci-autofill-addr.args @@ -0,0 +1,25 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/libexec/qemu-kvm \ +-name fdr-br \ +-S \ +-M pc-1.2 \ +-m 2048 \ +-smp 2 \ +-uuid 3ec6cbe1-b5a2-4515-b800-31a61855df41 \ +-nographic \ +-nodefaults \ +-monitor unix:/tmp/lib/domain--1-fdr-br/monitor.sock,server,nowait \ +-no-acpi \ +-boot c \ +-usb \ +-drive file=/var/iso/f18kde.iso,format=raw,if=none,media=cdrom,\ +id=drive-virtio-disk0 \ +-device virtio-blk-pci,bus=pci.0,addr=0x3,drive=drive-virtio-disk0,\ +id=virtio-disk0 \ +-vga cirrus \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pci-autofill-addr.xml b/tests/qemuxml2argvdata/qemuxml2argv-pci-autofill-addr.xml new file mode 100644 index 0000000..e5256fe --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pci-autofill-addr.xml @@ -0,0 +1,35 @@ +<domain type='qemu'> + <name>fdr-br</name> + <uuid>3ec6cbe1-b5a2-4515-b800-31a61855df41</uuid> + <memory unit='KiB'>2097152</memory> + <currentMemory unit='KiB'>2097152</currentMemory> + <vcpu placement='static' cpuset='0-1'>2</vcpu> + <os> + <type arch='x86_64' machine='pc-1.2'>hvm</type> + <boot dev='hd'/> + </os> + <devices> + <emulator>/usr/libexec/qemu-kvm</emulator> + <disk type='file' device='cdrom'> + <driver name='qemu' type='raw'/> + <source file='/var/iso/f18kde.iso'/> + <target dev='vda' bus='virtio'/> + <readonly/> + <address type='pci'/> + </disk> + <controller type='usb' index='0'> + <address type='pci'/> + </controller> + <controller type='ide' index='0'> + <address type='pci'/> + </controller> + <input type='mouse' bus='ps2'/> + <video> + <model type='cirrus' vram='16384' heads='1'/> + <address type='pci'/> + </video> + <memballoon model='virtio'> + <address type='pci'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 3bfb5c4..cd4fe22 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1522,6 +1522,7 @@ mymain(void) DO_TEST("pci-autoadd-addr", QEMU_CAPS_DEVICE_PCI_BRIDGE); DO_TEST("pci-autoadd-idx", QEMU_CAPS_DEVICE_PCI_BRIDGE); + DO_TEST("pci-autofill-addr", NONE); DO_TEST("pci-many", QEMU_CAPS_DEVICE_PCI_BRIDGE); DO_TEST("pci-bridge-many-disks", diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pci-autofill-addr.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pci-autofill-addr.xml new file mode 100644 index 0000000..6259044 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pci-autofill-addr.xml @@ -0,0 +1,41 @@ +<domain type='qemu'> + <name>fdr-br</name> + <uuid>3ec6cbe1-b5a2-4515-b800-31a61855df41</uuid> + <memory unit='KiB'>2097152</memory> + <currentMemory unit='KiB'>2097152</currentMemory> + <vcpu placement='static' cpuset='0-1'>2</vcpu> + <os> + <type arch='x86_64' machine='pc-1.2'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/libexec/qemu-kvm</emulator> + <disk type='file' device='cdrom'> + <driver name='qemu' type='raw'/> + <source file='/var/iso/f18kde.iso'/> + <target dev='vda' bus='virtio'/> + <readonly/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </disk> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <video> + <model type='cirrus' vram='16384' heads='1' primary='yes'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </video> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 30cf3e8..24852d2 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -591,6 +591,7 @@ mymain(void) QEMU_CAPS_DEVICE_PCI_BRIDGE); DO_TEST_FULL("pci-autoadd-idx", WHEN_ACTIVE, GIC_NONE, QEMU_CAPS_DEVICE_PCI_BRIDGE); + DO_TEST("pci-autofill-addr"); DO_TEST_FULL("q35", WHEN_ACTIVE, GIC_NONE, QEMU_CAPS_DEVICE_PCI_BRIDGE, -- 2.5.5
From: Cole Robinson <crobinso@redhat.com> This is an interesting test case since PCI isn't the default for aarch64. --- .../qemuxml2argv-aarch64-virtio-pci-manual-addresses.args | 4 +++- .../qemuxml2argv-aarch64-virtio-pci-manual-addresses.xml | 5 +++++ .../qemuxml2xmlout-aarch64-virtio-pci-manual-addresses.xml | 5 +++++ 3 files changed, 13 insertions(+), 1 deletion(-) diff --git a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-virtio-pci-manual-addresses.args b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-virtio-pci-manual-addresses.args index 2dfcd1e..0234404 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-virtio-pci-manual-addresses.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-virtio-pci-manual-addresses.args @@ -29,4 +29,6 @@ QEMU_AUDIO_DRV=none \ -device scsi-disk,bus=scsi0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,\ id=scsi0-0-0-0 \ -device virtio-net-pci,vlan=0,id=net0,mac=52:54:00:09:a4:37,bus=pcie.0,addr=0x2 \ --net user,vlan=0,name=hostnet0 +-net user,vlan=0,name=hostnet0 \ +-device virtio-net-pci,vlan=1,id=net1,mac=52:54:00:09:a4:38,bus=pci.2,addr=0x1 \ +-net user,vlan=1,name=hostnet1 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-virtio-pci-manual-addresses.xml b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-virtio-pci-manual-addresses.xml index 6a44f19..bf0f249 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-virtio-pci-manual-addresses.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-virtio-pci-manual-addresses.xml @@ -39,5 +39,10 @@ <model type='virtio'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> </interface> + <interface type='user'> + <mac address='52:54:00:09:a4:38'/> + <model type='virtio'/> + <address type='pci'/> + </interface> </devices> </domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-aarch64-virtio-pci-manual-addresses.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-aarch64-virtio-pci-manual-addresses.xml index 0d69169..4fdedac 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-aarch64-virtio-pci-manual-addresses.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-aarch64-virtio-pci-manual-addresses.xml @@ -50,5 +50,10 @@ <model type='virtio'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> </interface> + <interface type='user'> + <mac address='52:54:00:09:a4:38'/> + <model type='virtio'/> + <address type='pci' domain='0x0000' bus='0x02' slot='0x01' function='0x0'/> + </interface> </devices> </domain> -- 2.5.5
On 05/18/2016 03:23 PM, Laine Stump wrote:
This is an alternative to Cole's series that permits <address type='pci'/> to force assignment of a PCI address, which is particularly useful on platforms that could connect the same device in different ways (e.g. aarch64/virt).
Here is Cole's last iteration of the series:
https://www.redhat.com/archives/libvir-list/2016-May/msg01088.html
I had expressed a dislike of the "auto_allocate" flag that his series temporarily adds to the address (while simultaneously changing the address type to NONE) and suggested just changing all the necessary places to check for a valid PCI address instead of just checking the address type. He replied that this wasn't as simple as it looked, so I decided to try it; turns out he's right. But I still like this method better because it's not playing tricks with the address type, or adding a temporary private attribute to what should be pure config data.
Your opinion may vary though :-)
Note that patch 5/6 incorporates the same test case that Cole used in his penultimate patch, and I've added his patch to check the case of aarch64 at the end as well.
ACK series, but it's missing formatdomain.html.in changes. You can grab the check from my patch #2 I'm fine with this approach but I'll just list the downsides - Less generalizable, for adding additional address types in the future, but that's hypothetical at this point - We don't raise an explicit error for drivers that don't support this type of address allocation, like libxl. If it matters we could add a domain XML parse feature to throw an explicit error though - checking info->type == DEVICE_ADDRESS_TYPE_PCI is now a suspect pattern and needs to be considered carefully in other parts of the code upsides: - less magic - I think it will make allocating address at hotplug time simpler as well Thanks, Cole
Cole Robinson (1): tests: qemu: test <address type='pci'/> with aarch64
Laine Stump (5): conf: move virDomainDeviceInfo definition from domain_conf.h to device_conf.h conf: new functions to check if PCI address is wanted/present conf: allow type='pci' addresses with no address attributes specified bhyve: auto-assign addresses when <address type='pci'/> is specified qemu: auto-assign addresses when <address type='pci'/> is specified
docs/schemas/basictypes.rng | 8 +- src/bhyve/bhyve_device.c | 10 +- src/conf/device_conf.c | 6 +- src/conf/device_conf.h | 132 ++++++++++++++++++++- src/conf/domain_addr.c | 2 +- src/conf/domain_conf.c | 13 +- src/conf/domain_conf.h | 129 -------------------- src/qemu/qemu_domain_address.c | 64 +++++----- ...l2argv-aarch64-virtio-pci-manual-addresses.args | 4 +- ...ml2argv-aarch64-virtio-pci-manual-addresses.xml | 5 + .../qemuxml2argv-pci-autofill-addr.args | 25 ++++ .../qemuxml2argv-pci-autofill-addr.xml | 35 ++++++ tests/qemuxml2argvtest.c | 1 + ...2xmlout-aarch64-virtio-pci-manual-addresses.xml | 5 + .../qemuxml2xmlout-pci-autofill-addr.xml | 41 +++++++ tests/qemuxml2xmltest.c | 1 + 16 files changed, 298 insertions(+), 183 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pci-autofill-addr.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pci-autofill-addr.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-pci-autofill-addr.xml
On Thu, May 19, 2016 at 01:15:28PM -0400, Cole Robinson wrote:
On 05/18/2016 03:23 PM, Laine Stump wrote:
This is an alternative to Cole's series that permits <address type='pci'/> to force assignment of a PCI address, which is particularly useful on platforms that could connect the same device in different ways (e.g. aarch64/virt).
Here is Cole's last iteration of the series:
https://www.redhat.com/archives/libvir-list/2016-May/msg01088.html
I had expressed a dislike of the "auto_allocate" flag that his series temporarily adds to the address (while simultaneously changing the address type to NONE) and suggested just changing all the necessary places to check for a valid PCI address instead of just checking the address type. He replied that this wasn't as simple as it looked, so I decided to try it; turns out he's right. But I still like this method better because it's not playing tricks with the address type, or adding a temporary private attribute to what should be pure config data.
Your opinion may vary though :-)
I like this series more than counting XML elements and temporarily changing the types.
Note that patch 5/6 incorporates the same test case that Cole used in his penultimate patch, and I've added his patch to check the case of aarch64 at the end as well.
ACK series, but it's missing formatdomain.html.in changes. You can grab the check from my patch #2
I'm fine with this approach but I'll just list the downsides
- Less generalizable, for adding additional address types in the future, but that's hypothetical at this point - We don't raise an explicit error for drivers that don't support this type of address allocation, like libxl. If it matters we could add a domain XML parse feature to throw an explicit error though - checking info->type == DEVICE_ADDRESS_TYPE_PCI is now a suspect pattern and needs to be considered carefully in other parts of the code
upsides: - less magic - I think it will make allocating address at hotplug time simpler as well
Yes, qemu_hotplug.c has a few of those places using == DEVICE_ADDRESS_TYPE_PCI untouched by this series. Jan
On 05/19/2016 01:23 PM, Ján Tomko wrote:
On Thu, May 19, 2016 at 01:15:28PM -0400, Cole Robinson wrote:
On 05/18/2016 03:23 PM, Laine Stump wrote:
This is an alternative to Cole's series that permits <address type='pci'/> to force assignment of a PCI address, which is particularly useful on platforms that could connect the same device in different ways (e.g. aarch64/virt).
Here is Cole's last iteration of the series:
https://www.redhat.com/archives/libvir-list/2016-May/msg01088.html
I had expressed a dislike of the "auto_allocate" flag that his series temporarily adds to the address (while simultaneously changing the address type to NONE) and suggested just changing all the necessary places to check for a valid PCI address instead of just checking the address type. He replied that this wasn't as simple as it looked, so I decided to try it; turns out he's right. But I still like this method better because it's not playing tricks with the address type, or adding a temporary private attribute to what should be pure config data.
Your opinion may vary though :-)
I like this series more than counting XML elements and temporarily changing the types.
Note that patch 5/6 incorporates the same test case that Cole used in his penultimate patch, and I've added his patch to check the case of aarch64 at the end as well.
ACK series, but it's missing formatdomain.html.in changes. You can grab the check from my patch #2
I'm fine with this approach but I'll just list the downsides
- Less generalizable, for adding additional address types in the future, but that's hypothetical at this point - We don't raise an explicit error for drivers that don't support this type of address allocation, like libxl. If it matters we could add a domain XML parse feature to throw an explicit error though - checking info->type == DEVICE_ADDRESS_TYPE_PCI is now a suspect pattern and needs to be considered carefully in other parts of the code
upsides: - less magic - I think it will make allocating address at hotplug time simpler as well
Yes, qemu_hotplug.c has a few of those places using == DEVICE_ADDRESS_TYPE_PCI untouched by this series.
Since they happen after parse is finished, it should be safe. And anyway, looking at those uses, I think what most of them are doing (calling virDomainPCIAddressEnsureAddr()) is 100% unnecessary, since it's now already done when addresses are assigned in qemuDomainDefAssignAddresses(), which has already been called. (Back in Jan 2010 when the calls to qemuDomainPCIAddressEnsureAddr() were added (commit d8acc4), this was not the case - they were needed in order for the new devices to get an address assigned, but a lot has changed since then - even before Cole put in the postparse callback address assignment stuff and called it when attaching a device, we had already been assigning addresses during the higher level qemuDomainAttachDeviceConfig for a long time (since commit f5dd58a in June 2012; long enough that the calls to qemuDomainCCWAddressAssign() that are also sprinkled throughout qemu_hotplug.c in the same vicinity as the calls to qemuDomainPCIAddressEnsureAddr() weren't needed *even at the time they were added!* (commit f94646, March 2013). This was purely cargo-cult coding, caused by commit f5dd58a failing to remove the calls to ...EnsureAddr(). The interesting bit is that both of these commits were put in in support of s390 virtio devices.). (Well, *that* was a nice trip down git "memory lane" (aka "blame") So, the result is that most of the code in qemu_hotplug that requires checking for address type is unneeded, and I'm going to send a patch to remove it.
On Thu, May 19, 2016 at 05:14:33PM -0400, Laine Stump wrote:
On 05/19/2016 01:23 PM, Ján Tomko wrote:
Yes, qemu_hotplug.c has a few of those places using == DEVICE_ADDRESS_TYPE_PCI untouched by this series.
Since they happen after parse is finished, it should be safe.
And anyway, looking at those uses, I think what most of them are doing (calling virDomainPCIAddressEnsureAddr()) is 100% unnecessary, since it's now already done when addresses are assigned in qemuDomainDefAssignAddresses(), which has already been called.
Actually, virDomainPCIAddressEnsureAddr is the place where the address gets assigned. But only when address == NONE or PCI, which is OK because virDomainPCIAddressEnsureAddr calls PCIAddressIsPresent to decide whether it needs to allocate a new one. qemuDomainDefAssignAddresses is only called after parsing the whole domain, not just one device.
(Back in Jan 2010 when the calls to qemuDomainPCIAddressEnsureAddr() were added (commit d8acc4), this was not the case - they were needed in order for the new devices to get an address assigned, but a lot has changed since then - even before Cole put in the postparse callback address assignment stuff and called it when attaching a device, we had already been assigning addresses during the higher level qemuDomainAttachDeviceConfig for a long time (since commit f5dd58a in June 2012;
qemuDomainAttachDeviceConfig is for changing the persistent definition. Hotplug in qemuDomainAttachDeviceLive does not call any other address assignment function. Jan
long enough that the calls to qemuDomainCCWAddressAssign() that are also sprinkled throughout qemu_hotplug.c in the same vicinity as the calls to qemuDomainPCIAddressEnsureAddr() weren't needed *even at the time they were added!* (commit f94646, March 2013). This was purely cargo-cult coding, caused by commit f5dd58a failing to remove the calls to ...EnsureAddr(). The interesting bit is that both of these commits were put in in support of s390 virtio devices.).
(Well, *that* was a nice trip down git "memory lane" (aka "blame")
So, the result is that most of the code in qemu_hotplug that requires checking for address type is unneeded, and I'm going to send a patch to remove it.
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 05/20/2016 02:32 AM, Ján Tomko wrote:
On Thu, May 19, 2016 at 05:14:33PM -0400, Laine Stump wrote:
On 05/19/2016 01:23 PM, Ján Tomko wrote:
Yes, qemu_hotplug.c has a few of those places using == DEVICE_ADDRESS_TYPE_PCI untouched by this series. Since they happen after parse is finished, it should be safe.
And anyway, looking at those uses, I think what most of them are doing (calling virDomainPCIAddressEnsureAddr()) is 100% unnecessary, since it's now already done when addresses are assigned in qemuDomainDefAssignAddresses(), which has already been called. Actually, virDomainPCIAddressEnsureAddr is the place where the address gets assigned.
Exactly. But I was looking only at the _CONFIG version of hotplug, and saw that the PCI address had already been assigned by the time we called EnsureAddr, so it was superfluous. I didn't catch the bit that you point out below - in the case of live-only that assignment isn't done.
But only when address == NONE or PCI, which is OK because virDomainPCIAddressEnsureAddr calls PCIAddressIsPresent to decide whether it needs to allocate a new one.
qemuDomainDefAssignAddresses is only called after parsing the whole domain, not just one device.
Cole pushed a patch yesterday to cause it to be called during attach-device as well, but only in the case of _CONFIG, so your point still stands. I guess the patch to remove the other EnsureAddr stuff needs to be combined with moving that AssignAddresses call to a point that is common for live and config (which makes sense - they really do both need the same address)
(Back in Jan 2010 when the calls to qemuDomainPCIAddressEnsureAddr() were added (commit d8acc4), this was not the case - they were needed in order for the new devices to get an address assigned, but a lot has changed since then - even before Cole put in the postparse callback address assignment stuff and called it when attaching a device, we had already been assigning addresses during the higher level qemuDomainAttachDeviceConfig for a long time (since commit f5dd58a in June 2012; qemuDomainAttachDeviceConfig is for changing the persistent definition. Hotplug in qemuDomainAttachDeviceLive does not call any other address assignment function.
Yeah, I missed that in my haste.
On 05/19/2016 01:15 PM, Cole Robinson wrote:
On 05/18/2016 03:23 PM, Laine Stump wrote:
This is an alternative to Cole's series that permits <address type='pci'/> to force assignment of a PCI address, which is particularly useful on platforms that could connect the same device in different ways (e.g. aarch64/virt).
Here is Cole's last iteration of the series:
https://www.redhat.com/archives/libvir-list/2016-May/msg01088.html
I had expressed a dislike of the "auto_allocate" flag that his series temporarily adds to the address (while simultaneously changing the address type to NONE) and suggested just changing all the necessary places to check for a valid PCI address instead of just checking the address type. He replied that this wasn't as simple as it looked, so I decided to try it; turns out he's right. But I still like this method better because it's not playing tricks with the address type, or adding a temporary private attribute to what should be pure config data.
Your opinion may vary though :-)
Note that patch 5/6 incorporates the same test case that Cole used in his penultimate patch, and I've added his patch to check the case of aarch64 at the end as well.
ACK series, but it's missing formatdomain.html.in changes. You can grab the check from my patch #2
Right. I forgot that. I'll grab your doc bits and squash them in.
I'm fine with this approach but I'll just list the downsides
- Less generalizable, for adding additional address types in the future, but that's hypothetical at this point
Actually I was thinking the opposite :-) (not that it makes too much difference - how many different device types will we ever be able to actually choose between?)
- We don't raise an explicit error for drivers that don't support this type of address allocation, like libxl.
Is that specific to my method of supporting <address type='pci'/> ? Since they don't use any of the common address assignment functions, I hadn't even looked at what they do.
If it matters we could add a domain XML parse feature to throw an explicit error though - checking info->type == DEVICE_ADDRESS_TYPE_PCI is now a suspect pattern and needs to be considered carefully in other parts of the code
Yes and no. I did check all uses of it. It really only needs extra qualification in code that is part of the parser or postparse callbacks (of course, in order to only check those parts, you need to know where/what they are!). Once address assignment is done, anything with ADDRESS_TYPE_PCI is guaranteed to have a valid PCI address.
upsides: - less magic - I think it will make allocating address at hotplug time simpler as well
I hadn't thought about that yet - more of the "90% of the coding effort going to 0.005% of the users" :-P Thanks for putting up with my opinionated opinions :-) (and for reviewing, and for the test cases, and ....) I'll try to push it a bit later today.
On 05/19/2016 03:27 PM, Laine Stump wrote:
On 05/19/2016 01:15 PM, Cole Robinson wrote:
On 05/18/2016 03:23 PM, Laine Stump wrote:
This is an alternative to Cole's series that permits <address type='pci'/> to force assignment of a PCI address, which is particularly useful on platforms that could connect the same device in different ways (e.g. aarch64/virt).
Here is Cole's last iteration of the series:
https://www.redhat.com/archives/libvir-list/2016-May/msg01088.html
I had expressed a dislike of the "auto_allocate" flag that his series temporarily adds to the address (while simultaneously changing the address type to NONE) and suggested just changing all the necessary places to check for a valid PCI address instead of just checking the address type. He replied that this wasn't as simple as it looked, so I decided to try it; turns out he's right. But I still like this method better because it's not playing tricks with the address type, or adding a temporary private attribute to what should be pure config data.
Your opinion may vary though :-)
Note that patch 5/6 incorporates the same test case that Cole used in his penultimate patch, and I've added his patch to check the case of aarch64 at the end as well.
ACK series, but it's missing formatdomain.html.in changes. You can grab the check from my patch #2
Right. I forgot that. I'll grab your doc bits and squash them in.
I'm fine with this approach but I'll just list the downsides
- Less generalizable, for adding additional address types in the future, but that's hypothetical at this point
Actually I was thinking the opposite :-) (not that it makes too much difference - how many different device types will we ever be able to actually choose between?)
- We don't raise an explicit error for drivers that don't support this type of address allocation, like libxl.
Is that specific to my method of supporting <address type='pci'/> ? Since they don't use any of the common address assignment functions, I hadn't even looked at what they do.
My patches had an explicit PostParse check in generic code that would throw an error if <address type='pci'/> was never correctly filled in by the hypervisor, so <address type='pci/> would explicitly fail for all non-qemu drivers. It's a minor point
If it matters we could add a domain XML parse feature to throw an explicit error though - checking info->type == DEVICE_ADDRESS_TYPE_PCI is now a suspect pattern and needs to be considered carefully in other parts of the code
Yes and no. I did check all uses of it. It really only needs extra qualification in code that is part of the parser or postparse callbacks (of course, in order to only check those parts, you need to know where/what they are!). Once address assignment is done, anything with ADDRESS_TYPE_PCI is guaranteed to have a valid PCI address.
Agreed, my point was just that we need to be cognizant of the distinction whenever new code is added
upsides: - less magic - I think it will make allocating address at hotplug time simpler as well
I hadn't thought about that yet - more of the "90% of the coding effort going to 0.005% of the users" :-P
Yeah it's certainly not a blocker for this series, but hotplug will be relevant for aarch64 eventually
Thanks for putting up with my opinionated opinions :-) (and for reviewing, and for the test cases, and ....) I'll try to push it a bit later today.
No worries, thanks for implementing it :) - Cole
On 05/19/2016 03:33 PM, Cole Robinson wrote:
My patches had an explicit PostParse check in generic code that would throw an error if <address type='pci'/> was never correctly filled in by the hypervisor, so <address type='pci/> would explicitly fail for all non-qemu drivers. It's a minor point
That sounds like a good idea, and should be trivial to add to the tail end of your address assignment callback. I'll make a followup patch for it.
upsides: - less magic - I think it will make allocating address at hotplug time simpler as well
I hadn't thought about that yet - more of the "90% of the coding effort going to 0.005% of the users" :-P
Yeah it's certainly not a blocker for this series, but hotplug will be relevant for aarch64 eventually
Actually it should "just work".
participants (4)
-
Cole Robinson -
Ján Tomko -
Laine Stump -
Roman Bogorodskiy