2:23 p.m.
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
2:23 p.m.
New subject: [libvirt] [PATCH 1/6] conf: move virDomainDeviceInfo definition from domain_conf.h to device_conf.h
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
12:20 p.m.
New subject: [libvirt] [PATCH 1/6] conf: move virDomainDeviceInfo definition from domain_conf.h to device_conf.h
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(-)
12:32 p.m.
New subject: [libvirt] [PATCH 1/6] conf: move virDomainDeviceInfo definition from domain_conf.h to device_conf.h
On 05/19/2016 01:20 PM, Ján Tomko wrote:
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.
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 :-)
2:23 p.m.
New subject: [libvirt] [PATCH 2/6] conf: new functions to check if PCI address is wanted/present
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
12:21 p.m.
New subject: [libvirt] [PATCH 2/6] conf: new functions to check if PCI address is wanted/present
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
12:45 p.m.
New subject: [libvirt] [PATCH 2/6] conf: new functions to check if PCI address is wanted/present
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).
2:23 p.m.
New subject: [libvirt] [PATCH 3/6] conf: allow type='pci' addresses with no address attributes specified
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
2:23 p.m.
New subject: [libvirt] [PATCH 4/6] bhyve: auto-assign addresses when <address type='pci'/> is specified
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
10:42 a.m.
New subject: [libvirt] [PATCH 4/6] bhyve: auto-assign addresses when <address type='pci'/> is specified
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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Roman Bogorodskiy
10:05 p.m.
New subject: [libvirt] [PATCH 4/6] bhyve: auto-assign addresses when <address type='pci'/> is specified
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(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
Roman Bogorodskiy
7:13 a.m.
New subject: [libvirt] [PATCH 4/6] bhyve: auto-assign addresses when <address type='pci'/> is specified
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(a)redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
> Roman Bogorodskiy
Roman Bogorodskiy
2:23 p.m.
New subject: [libvirt] [PATCH 5/6] qemu: auto-assign addresses when <address type='pci'/> is specified
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
2:23 p.m.
New subject: [libvirt] [PATCH 6/6] tests: qemu: test <address type='pci'/> with aarch64
From: Cole Robinson <crobinso(a)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
12:15 p.m.
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
12:23 p.m.
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
4:14 p.m.
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.
1:32 a.m.
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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
12:50 p.m.
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.
2:27 p.m.
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.
2:33 p.m.
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
7:30 p.m.
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".
3040
days inactive
3142
days old
21 comments
4 participants
participants (4)
-
Cole Robinson -
Ján Tomko -
Laine Stump -
Roman Bogorodskiy