Re: [libvirt] [PATCHv4 5/5] qemu: auto-add bridges and allow using them

2013-04-25 Thread Ján Tomko 
On 04/25/2013 02:39 AM, Eric Blake wrote:
 On 04/23/2013 06:47 AM, Ján Tomko wrote:
 @@ -1326,15 +1368,53 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
  qemuDomainObjPrivatePtr priv = NULL;
  
  if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
 +int max_idx = -1;
 
 So let's trace what happens if I have XML with no controller but I do
 use 33 PCI devices and have a capable qemu:
 
 max_idx starts at -1,
 
  int nbuses = 0;
  int i;
 +int rv;
  
  for (i = 0; i  def-ncontrollers; i++) {
 -if (def-controllers[i]-type == VIR_DOMAIN_CONTROLLER_TYPE_PCI)
 -nbuses++;
 +if (def-controllers[i]-type == 
 VIR_DOMAIN_CONTROLLER_TYPE_PCI) {
 +if (def-controllers[i]-idx  max_idx)
 +max_idx = def-controllers[i]-idx;
 +}
 +}
 
 If no controllers were specified, it is still at -1,
 
 +
 +nbuses = max_idx + 1;
 
 so nbuses is now 0,
 
 +
 +if (nbuses  0 
 +virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCI_BRIDGE)) {
 
 therefore we skip this if,
 
 +virDomainDeviceInfo info;
 +/* 1st pass to figure out how many PCI bridges we need */
 +if (!(addrs = qemuDomainPCIAddressSetCreate(def, nbuses, true)))
 +goto cleanup;
 +if (qemuAssignDevicePCISlots(def, qemuCaps, addrs)  0)
 +goto cleanup;
 +/* Reserve 1 extra slot for a (potential) bridge */
 +if (qemuDomainPCIAddressSetNextAddr(addrs, info)  0)
 +goto cleanup;
 +
 +for (i = 1; i  addrs-nbuses; i++) {
 +if ((rv = virDomainDefMaybeAddController(
 +def, VIR_DOMAIN_CONTROLLER_TYPE_PCI,
 +i, VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE))  0)
 +goto cleanup;
 +/* If we added a new bridge, we will need one more address 
 */
 +if (rv  0  qemuDomainPCIAddressSetNextAddr(addrs, info) 
  0)
 +goto cleanup;
 +}
 +nbuses = addrs-nbuses;
 +qemuDomainPCIAddressSetFree(addrs);
 +addrs = NULL;
 +
 +} else if (max_idx  0) {
 
 we don't error out,
 
 +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, %s,
 +   _(PCI bridges are not supported 
 + by this QEMU binary));
 +goto cleanup;
  }
 
 but we also didn't auto-instantiate any bridges, even if the capability
 is supported.  Is that a problem?

No, if the machine has no buses, we would have no place to put the
bridge in. (Unless it's a PCI express machine, but libvirt doesn't know
how to use those yet)

And we'll error out anyway with:
error: XML error: No PCI buses available
either in GetNextSlot called by AssignPCISlots for devices without an
address, or in PCIAddressValidate called by CollectPCIAddress for
explicitly specified PCI addresses.

Jan

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv4 5/5] qemu: auto-add bridges and allow using them

2013-04-24 Thread Eric Blake 
On 04/23/2013 06:47 AM, Ján Tomko wrote:
 Add a dry run address allocation to figure out how many bridges
 will be needed for all the devices without explicit addresses.
 
 Auto-add just enough bridges to put all the devices on, or up to the
 bridge with the largest specified index.
 ---
 
 v4:
 Moved the check for duplicate controller indexes to a separate patch
 Simplified nbuses and max_idx computation
 Only does two-pass allocation of PCI addresses if the machine has a PCI bus
 Does not contain traces of rebasing or spurious whitespace changes
 Tests auto-adding PCI bridges in xml-argv and xml-xml tests.
 
 @@ -1233,9 +1236,45 @@ static bool 
 qemuPCIAddressValidate(qemuDomainPCIAddressSetPtr addrs ATTRIBUTE_UN
 QEMU_PCI_ADDRESS_SLOT_LAST);
  return false;
  }
 +if (addr-slot == 0) {
 +if (addr-bus) {
 +virReportError(VIR_ERR_XML_ERROR, %s,
 +   _(Slot 0 is unusable on PCI bridges));
 +} else {
 +virReportError(VIR_ERR_XML_ERROR, %s,
 +  _(Slot 0 on bus 0 is reserved for the host 
 bridge));

Indentation is off.

 @@ -1326,15 +1368,53 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
  qemuDomainObjPrivatePtr priv = NULL;
  
  if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
 +int max_idx = -1;

So let's trace what happens if I have XML with no controller but I do
use 33 PCI devices and have a capable qemu:

max_idx starts at -1,

  int nbuses = 0;
  int i;
 +int rv;
  
  for (i = 0; i  def-ncontrollers; i++) {
 -if (def-controllers[i]-type == VIR_DOMAIN_CONTROLLER_TYPE_PCI)
 -nbuses++;
 +if (def-controllers[i]-type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) 
 {
 +if (def-controllers[i]-idx  max_idx)
 +max_idx = def-controllers[i]-idx;
 +}
 +}

If no controllers were specified, it is still at -1,

 +
 +nbuses = max_idx + 1;

so nbuses is now 0,

 +
 +if (nbuses  0 
 +virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCI_BRIDGE)) {

therefore we skip this if,

 +virDomainDeviceInfo info;
 +/* 1st pass to figure out how many PCI bridges we need */
 +if (!(addrs = qemuDomainPCIAddressSetCreate(def, nbuses, true)))
 +goto cleanup;
 +if (qemuAssignDevicePCISlots(def, qemuCaps, addrs)  0)
 +goto cleanup;
 +/* Reserve 1 extra slot for a (potential) bridge */
 +if (qemuDomainPCIAddressSetNextAddr(addrs, info)  0)
 +goto cleanup;
 +
 +for (i = 1; i  addrs-nbuses; i++) {
 +if ((rv = virDomainDefMaybeAddController(
 +def, VIR_DOMAIN_CONTROLLER_TYPE_PCI,
 +i, VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE))  0)
 +goto cleanup;
 +/* If we added a new bridge, we will need one more address */
 +if (rv  0  qemuDomainPCIAddressSetNextAddr(addrs, info) 
  0)
 +goto cleanup;
 +}
 +nbuses = addrs-nbuses;
 +qemuDomainPCIAddressSetFree(addrs);
 +addrs = NULL;
 +
 +} else if (max_idx  0) {

we don't error out,

 +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, %s,
 +   _(PCI bridges are not supported 
 + by this QEMU binary));
 +goto cleanup;
  }

but we also didn't auto-instantiate any bridges, even if the capability
is supported.  Is that a problem?

ACK if you can answer my question and fix the minor issues.

-- 
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

[libvirt] [PATCHv4 5/5] qemu: auto-add bridges and allow using them

2013-04-23 Thread Ján Tomko 
Add a dry run address allocation to figure out how many bridges
will be needed for all the devices without explicit addresses.

Auto-add just enough bridges to put all the devices on, or up to the
bridge with the largest specified index.
---

v4:
Moved the check for duplicate controller indexes to a separate patch
Simplified nbuses and max_idx computation
Only does two-pass allocation of PCI addresses if the machine has a PCI bus
Does not contain traces of rebasing or spurious whitespace changes
Tests auto-adding PCI bridges in xml-argv and xml-xml tests.


 src/qemu/qemu_command.c| 210 +
 src/qemu/qemu_command.h|   3 +-
 .../qemuxml2argv-pci-autoadd-addr.args |  12 ++
 .../qemuxml2argv-pci-autoadd-addr.xml  |  44 +
 .../qemuxml2argv-pci-autoadd-idx.args  |  13 ++
 .../qemuxml2argv-pci-autoadd-idx.xml   |  45 +
 tests/qemuxml2argvdata/qemuxml2argv-pci-bridge.xml | 210 +
 tests/qemuxml2argvtest.c   |   3 +
 .../qemuxml2xmlout-pci-autoadd-addr.xml|  44 +
 .../qemuxml2xmlout-pci-autoadd-idx.xml |  45 +
 tests/qemuxml2xmltest.c|   3 +
 11 files changed, 593 insertions(+), 39 deletions(-)
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pci-autoadd-addr.args
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pci-autoadd-addr.xml
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pci-autoadd-idx.args
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pci-autoadd-idx.xml
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pci-bridge.xml
 create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-pci-autoadd-addr.xml
 create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-pci-autoadd-idx.xml

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 3787ff1..30c967c 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1197,6 +1197,8 @@ struct _qemuDomainPCIAddressSet {
 qemuDomainPCIAddressBus *used;
 virDevicePCIAddress lastaddr;
 size_t nbuses;/* allocation of 'used' */
+bool dryRun;  /* on a dry run, new buses are auto-added
+ and addresses aren't saved in device infos */
 };
 
 
@@ -1216,9 +1218,10 @@ static bool 
qemuPCIAddressValidate(qemuDomainPCIAddressSetPtr addrs ATTRIBUTE_UN
_(Only PCI domain 0 is available));
 return false;
 }
-if (addr-bus != 0) {
-virReportError(VIR_ERR_XML_ERROR, %s,
-   _(Only PCI bus 0 is available));
+if (addr-bus = addrs-nbuses) {
+virReportError(VIR_ERR_XML_ERROR,
+   _(Only PCI buses up to %zu are available),
+   addrs-nbuses - 1);
 return false;
 }
 if (addr-function = QEMU_PCI_ADDRESS_FUNCTION_LAST) {
@@ -1233,9 +1236,45 @@ static bool 
qemuPCIAddressValidate(qemuDomainPCIAddressSetPtr addrs ATTRIBUTE_UN
QEMU_PCI_ADDRESS_SLOT_LAST);
 return false;
 }
+if (addr-slot == 0) {
+if (addr-bus) {
+virReportError(VIR_ERR_XML_ERROR, %s,
+   _(Slot 0 is unusable on PCI bridges));
+} else {
+virReportError(VIR_ERR_XML_ERROR, %s,
+  _(Slot 0 on bus 0 is reserved for the host 
bridge));
+}
+return false;
+}
 return true;
 }
 
+/* Ensure addr fits in the address set, by expanding it if needed.
+ * Return value:
+ * -1 = OOM
+ *  0 = no action performed
+ * 0 = number of buses added
+ */
+static int
+qemuDomainPCIAddressSetGrow(qemuDomainPCIAddressSetPtr addrs,
+virDevicePCIAddressPtr addr)
+{
+int add, i;
+
+add = addr-bus - addrs-nbuses + 1;
+i = addrs-nbuses;
+if (add = 0)
+return 0;
+if (VIR_EXPAND_N(addrs-used, addrs-nbuses, add)  0) {
+virReportOOMError();
+return -1;
+}
+/* reserve slot 0 on the new buses */
+for (; i  addrs-nbuses; i++)
+addrs-used[i][0] = 0xFF;
+return add;
+}
+
 
 static char *qemuPCIAddressAsString(virDevicePCIAddressPtr addr)
 {
@@ -1273,6 +1312,9 @@ static int qemuCollectPCIAddress(virDomainDefPtr def 
ATTRIBUTE_UNUSED,
 return 0;
 }
 
+if (addrs-dryRun  qemuDomainPCIAddressSetGrow(addrs, addr)  0)
+return -1;
+
 if (!qemuPCIAddressValidate(addrs, addr))
 return -1;
 
@@ -1326,15 +1368,53 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
 qemuDomainObjPrivatePtr priv = NULL;
 
 if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
+int max_idx = -1;
 int nbuses = 0;
 int i;
+int rv;
 
 for (i = 0; i  def-ncontrollers; i++) {
-if (def-controllers[i]-type == VIR_DOMAIN_CONTROLLER_TYPE_PCI)
-nbuses++;
+if