Re: [libvirt] [PATCH v3 4/5] qemu: auto-add pci-root controller for pc machine types
On 04/22/2013 10:37 PM, Laine Stump wrote:
On 04/22/2013 02:43 PM, Ján Tomko wrote:
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index a7aabdf..ab99538 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -673,6 +673,37 @@ qemuDomainDefPostParse(virDomainDefPtr def,
!(def-emulator = virDomainDefGetDefaultEmulator(def, caps)))
return -1;
+/* Add implicit PCI root controller if the machine has one */
+switch (def-os.arch) {
+case VIR_ARCH_I686:
+case VIR_ARCH_X86_64:
+if (!def-os.machine)
+break;
+if (STRPREFIX(def-os.machine, pc-q35) ||
+STREQ(def-os.machine, q35) ||
+STREQ(def-os.machine, isapc))
+break;
+if (!STRPREFIX(def-os.machine, pc-0.)
+!STRPREFIX(def-os.machine, pc-1.)
+!STREQ(def-os.machine, pc)
+!STRPREFIX(def-os.machine, rhel))
+break;
If you're going to fall through to a different case like this, you
should put a comment in the code something like this:
/* FALL THROUGH TO NEXT CASE */
just so people don't have to think too hard :-)
However, I think it would be more easily expandable in the future if you
had a straight switch statement with all the cases, and just set a
needsPCIRoot boolean for those cases that need it, then an if
(needsPCIRoot) at the end. In the future when we want to add other
implicit devices, each case can be a mix of the appropriate needsThis
and needsThat, with the actual additions at the end.
...
ACK, with the addition of the FALLTHROUGH comment, or restructuring it
is as I suggested.
I'm squashing this in before pushing:
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index ab99538..98ac56f 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -668,6 +668,8 @@ qemuDomainDefPostParse(virDomainDefPtr def,
virCapsPtr caps,
void *opaque ATTRIBUTE_UNUSED)
{
+bool addPCIRoot = false;
+
/* check for emulator and create a default one if needed */
if (!def-emulator
!(def-emulator = virDomainDefGetDefaultEmulator(def, caps)))
@@ -688,6 +690,8 @@ qemuDomainDefPostParse(virDomainDefPtr def,
!STREQ(def-os.machine, pc)
!STRPREFIX(def-os.machine, rhel))
break;
+addPCIRoot = true;
+break;
case VIR_ARCH_ALPHA:
case VIR_ARCH_PPC:
@@ -695,15 +699,18 @@ qemuDomainDefPostParse(virDomainDefPtr def,
case VIR_ARCH_PPCEMB:
case VIR_ARCH_SH4:
case VIR_ARCH_SH4EB:
-if (virDomainDefMaybeAddController(
-def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 0,
-VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) 0)
-return -1;
+addPCIRoot = true;
break;
default:
break;
}
+if (addPCIRoot
+virDomainDefMaybeAddController(
+def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 0,
+VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) 0)
+return -1;
+
return 0;
}
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 4/5] qemu: auto-add pci-root controller for pc machine types
On 04/22/2013 02:43 PM, Ján Tomko wrote:
controller type='pci' index='0' model='pci-root'/
is auto-added to pc* machine types.
Without this controller PCI bus 0 is not available and
no PCI addresses are assigned by default.
Since older libvirt supported PCI bus 0 even without
this controller, it is removed from the XML when migrating.
---
src/conf/domain_conf.c | 2 +-
src/conf/domain_conf.h | 6 ++
src/libvirt_private.syms | 1 +
src/qemu/qemu_command.c| 57 --
src/qemu/qemu_command.h| 3 +-
src/qemu/qemu_domain.c | 67
+-
[ + tons of test xml files]
162 files changed, 269 insertions(+), 23 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 1e7de52..5740009 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -9762,7 +9762,7 @@ virDomainLookupVcpuPin(virDomainDefPtr def,
return NULL;
}
-static int
+int
virDomainDefMaybeAddController(virDomainDefPtr def,
int type,
int idx,
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 3cb626b..565f0f8 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2553,6 +2553,12 @@ int virDomainObjListExport(virDomainObjListPtr doms,
virDomainVcpuPinDefPtr virDomainLookupVcpuPin(virDomainDefPtr def,
int vcpuid);
+int
+virDomainDefMaybeAddController(virDomainDefPtr def,
+ int type,
+ int idx,
+ int model);
+
char *virDomainDefGetDefaultEmulator(virDomainDefPtr def, virCapsPtr caps);
#endif /* __DOMAIN_CONF_H */
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 32b4ae8..ca324de 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -116,6 +116,7 @@ virDomainDefFree;
virDomainDefGenSecurityLabelDef;
virDomainDefGetDefaultEmulator;
virDomainDefGetSecurityLabelDef;
+virDomainDefMaybeAddController;
virDomainDefParseFile;
virDomainDefParseNode;
virDomainDefParseString;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index f052a91..3787ff1 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1196,6 +1196,7 @@ typedef uint8_t
qemuDomainPCIAddressBus[QEMU_PCI_ADDRESS_SLOT_LAST];
struct _qemuDomainPCIAddressSet {
qemuDomainPCIAddressBus *used;
virDevicePCIAddress lastaddr;
+size_t nbuses;/* allocation of 'used' */
};
@@ -1206,6 +1207,10 @@ struct _qemuDomainPCIAddressSet {
static bool qemuPCIAddressValidate(qemuDomainPCIAddressSetPtr addrs
ATTRIBUTE_UNUSED,
virDevicePCIAddressPtr addr)
{
+if (addrs-nbuses == 0) {
+virReportError(VIR_ERR_XML_ERROR, %s, _(No PCI buses available));
+return false;
+}
if (addr-domain != 0) {
virReportError(VIR_ERR_XML_ERROR, %s,
_(Only PCI domain 0 is available));
@@ -1321,7 +1326,15 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
qemuDomainObjPrivatePtr priv = NULL;
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
-if (!(addrs = qemuDomainPCIAddressSetCreate(def)))
+int nbuses = 0;
+int i;
+
+for (i = 0; i def-ncontrollers; i++) {
+if (def-controllers[i]-type == VIR_DOMAIN_CONTROLLER_TYPE_PCI)
+nbuses++;
+}
+
+if (!(addrs = qemuDomainPCIAddressSetCreate(def, nbuses)))
goto cleanup;
if (qemuAssignDevicePCISlots(def, qemuCaps, addrs) 0)
@@ -1366,16 +1379,25 @@ int qemuDomainAssignAddresses(virDomainDefPtr def,
return qemuDomainAssignPCIAddresses(def, qemuCaps, obj);
}
-qemuDomainPCIAddressSetPtr qemuDomainPCIAddressSetCreate(virDomainDefPtr def)
+qemuDomainPCIAddressSetPtr qemuDomainPCIAddressSetCreate(virDomainDefPtr def,
+ unsigned int nbuses)
{
qemuDomainPCIAddressSetPtr addrs;
+int i;
if (VIR_ALLOC(addrs) 0)
goto no_memory;
-if (VIR_ALLOC_N(addrs-used, 1) 0)
+if (VIR_ALLOC_N(addrs-used, nbuses) 0)
goto no_memory;
+addrs-nbuses = nbuses;
+
+/* reserve slot 0 in every bus - it's used by the host bridge on bus 0
+ * and unusable on PCI bridges */
+for (i = 0; i nbuses; i++)
+addrs-used[i][0] = 0xFF;
+
if (virDomainDeviceInfoIterate(def, qemuCollectPCIAddress, addrs) 0)
goto error;
@@ -1604,12 +1626,6 @@ qemuAssignDevicePCISlots(virDomainDefPtr def,
virDevicePCIAddressPtr addrptr;
unsigned int *func = tmp_addr.function;
-
-/* Reserve slot 0 for
Re: [libvirt] [PATCH v3 4/5] qemu: auto-add pci-root controller for pc machine types
On 04/22/2013 02:37 PM, Laine Stump wrote:
On 04/22/2013 02:43 PM, Ján Tomko wrote:
controller type='pci' index='0' model='pci-root'/
is auto-added to pc* machine types.
Without this controller PCI bus 0 is not available and
no PCI addresses are assigned by default.
Since older libvirt supported PCI bus 0 even without
this controller, it is removed from the XML when migrating.
---
+/* Add implicit PCI root controller if the machine has one */
+switch (def-os.arch) {
+case VIR_ARCH_I686:
+case VIR_ARCH_X86_64:
+if (!def-os.machine)
+break;
+if (STRPREFIX(def-os.machine, pc-q35) ||
+STREQ(def-os.machine, q35) ||
+STREQ(def-os.machine, isapc))
+break;
+if (!STRPREFIX(def-os.machine, pc-0.)
+!STRPREFIX(def-os.machine, pc-1.)
+!STREQ(def-os.machine, pc)
+!STRPREFIX(def-os.machine, rhel))
+break;
If you're going to fall through to a different case like this, you
should put a comment in the code something like this:
/* FALL THROUGH TO NEXT CASE */
just so people don't have to think too hard :-)
That, and Coverity will ding you on unmarked fallthrough of a non-empty
case label. Coverity recognizes several spellings, but our code base
seems to prefer the spelling /* fallthrough */ as a form that works
across several static analyzers.
ACK, with the addition of the FALLTHROUGH comment, or restructuring it
is as I suggested.
Of course, restructuring it means you don't even have to worry about
magic comments :)
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
