Re: [libvirt] [PATCH 6/6] qemu: Add spapr-vio address assignment

On 12/07/2011 11:41 PM, Michael Ellerman wrote: > From: Michael Ellerman <michael(a)ellerman.id.au&gt; > > Add logic to assign addresses for devices with spapr-vio addresses. > > We also do validation of addresses specified by the user, ie. ensuring > that there are not duplicate addresses on the bus. > > Signed-off-by: Michael Ellerman <michael(a)ellerman.id.au&gt; > --- > src/qemu/qemu_command.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++ > 1 files changed, 87 insertions(+), 0 deletions(-) Depends on patch 4, but I'll review anyway...

> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index 32ee8d8..62a1258 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -684,6 +684,87 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virBitmapPtr qemuCaps) > return -1; > } > > +static int > +qemuSpaprVIOFindByReg(virDomainDefPtr def ATTRIBUTE_UNUSED, > + virDomainDeviceInfoPtr info, void *opaque) > +{ > + virDomainDeviceInfoPtr target = (virDomainDeviceInfoPtr)opaque; The cast isn't strictly necessary in C.

> + if (info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO) > + return 0; > + > + /* Match a dev that has a reg, is not us, and has a matching reg */ > + if (info->addr.spaprvio.has_reg && info != target && As I asked in 4, can a valid reg ever be 0, or must it be non-zero? If the latter, then you don't need has_reg.

> + /* Check if the user has assigned the reg already, if so use it */ > + user_reg = info->addr.spaprvio.has_reg; > + if (!user_reg) { > + info->addr.spaprvio.reg = default_reg; > + info->addr.spaprvio.has_reg = true; > + } > + > + rc = virDomainDeviceInfoIterate(def, qemuSpaprVIOFindByReg, info); > + while (rc != 0) { > + if (user_reg) { > + qemuReportError(VIR_ERR_XML_ERROR, > + _("spapr-vio address %#llx already in use"), > + info->addr.spaprvio.reg); > + return -EEXIST; > + } > + > + /* We assigned the reg, so try a new value */ > + info->addr.spaprvio.reg += 0x1000; > + rc = virDomainDeviceInfoIterate(def, qemuSpaprVIOFindByReg, info); > + } Looks reasonable at either honoring the user's choice, or at iterating by 0x1000 until a free slot is found. Does that match the qemu algorithm?

> +static int qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def) > +{ > + int i, rc; > + > + for (i = 0 ; i < def->nnets; i++) { > + rc = qemuAssignSpaprVIOAddress(def, &def->nets[i]->info, > + 0x1000ul); > + if (rc) > + return rc; > + } > + > + for (i = 0 ; i < def->ncontrollers; i++) { > + rc = qemuAssignSpaprVIOAddress(def, &def->controllers[i]->info, > + 0x2000ul); > + if (rc) > + return rc; > + } > + > + for (i = 0 ; i < def->nserials; i++) { > + rc = qemuAssignSpaprVIOAddress(def, &def->serials[i]->info, > + 0x30000000ul); Again, I assume these three defaults match qemu.