On Fri, 2011-12-09 at 15:14 -0700, Eric Blake wrote:
> On 12/07/2011 11:41 PM, Michael Ellerman wrote:
> > From: Michael Ellerman <michael@xxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxx>
> > ---
> > src/qemu/qemu_command.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++
> > 1 files changed, 87 insertions(+), 0 deletions(-)
>
> Depends on patch 4, but I'll review anyway...
Thanks.
> > 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.
Will fix.
> > + 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.
Zero is valid, so as I said I think has_reg is the best solution.
> > + /* 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?
Actually we're planning on not doing anything in QEMU. So this will be
the only code that does magical address assignment. Users who want to
use bare QEMU and specify their own devices will just have to assign reg
values themselves.
> > +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.
Yeah they do. I'll add a comment to that effect.
cheers
Attachment:
signature.asc
Description: This is a digitally signed message part
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list