[libvirt] [PATCH 4/6] Add address type for SPAPR VIO devices

[Michael Ellerman]

On Fri, 2011-12-09 at 14:49 -0700, Eric Blake wrote:
> On 12/07/2011 11:41 PM, Michael Ellerman wrote:
> > From: Michael Ellerman <michaele at au1.ibm.com>
> 
> This is a different address than you used for patch 2 and 3 (and yet a
> third address compared to the email where you sent this patch).  We can
> cope with that, but it means picking a favorite address for AUTHORS,
> then listing alternate addresses in .mailmap.  You may want to change
> authorship of this patch when re-submitting (git commit --amend
> --author=address), or at least give instructions on which address(es)
> you want to be known by for your libvirt.git contributions.

Sorry that's a bit sloppy of me. They are all my addresses (obviously),
I use michael at ellerman.id.au as my canonical address.

I'm not sure why the mails are coming from michael at ozlabs.org, they
didn't use to and I haven't had time to debug it yet.

> I can't apply this patch without documentation.  Below, I'll include a
> first cut at the rng changes that I think match your code, as well as
> discuss what needs to be done in docs/formatdomain.html.in, before you
> send a v2 of the remainder of your series.

Ah sorry. We actually have some patches internally to do that but I
didn't write them so I didn't send them. They look similar to your
changes below so I'll sort that out for v2.

> A test case would also be nice, correlating the new XML to the new
> ppc64-specific command line, but that may entail writing separate
> patches to improve the testsuite to allow targetting a controlled ppc64
> target type.  The testsuite already hard-codes a fake x86_64 target, so
> that the suite can run tests even if on machines that lack an x86_64
> qemu emulator, so I think there's precedence on how to do it.

OK I'll have a look at it and see if I can come up with something.


> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index 5de33b9..665a0cd 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -139,7 +139,8 @@ VIR_ENUM_IMPL(virDomainDeviceAddress, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST,
> >                "drive",
> >                "virtio-serial",
> >                "ccid",
> > -              "usb")
> > +              "usb",
> > +              "spapr-vio")
> 
> Can spapr-vio ever be used as a controller type, or does that not make
> sense?  I'm guessing that since an <address type='spapr-vio'
> reg='0x1000'/> only has 'reg' as the differentiating address, and reg is
> 64-bits, that it is a flat addressing space, so you don't need a
> controller (contrast with <address type='pci' bus='0x0'/> corresponding
> to <controller type='pci' index='0'/>).  

No it can't be used as a controller. There's just a single instance of
the bus with a flat 64-bit address space.

> But that makes documenting
> things a bit harder - previously, we have documented <address> in a
> haphazard manner, under the particular devices that must live on a
> particular bus, and pointed back to the corresponding controller.  But
> if there is no corresponding controller, and since we now have an
> instance of which addressing to use depending on which architecture (for
> example, a console lives on <address type='pci'/> for x86_64 and
> <address type='spapr-vio'/> for ppc64), I think I have to do a pre-req
> patch that reorganizes the existing <address> documentation, to make it
> easier to insert in your new mode.

OK, I haven't looked at the help doco yet, I'll have a look, and let me
know what you want done there.

> >  VIR_ENUM_IMPL(virDomainDeviceAddressPciMulti,
> >                VIR_DOMAIN_DEVICE_ADDRESS_PCI_MULTI_LAST,
> > @@ -1909,6 +1910,11 @@ virDomainDeviceInfoFormat(virBufferPtr buf,
> >                            info->addr.usb.port);
> >          break;
> >  
> > +    case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO:
> > +        if (info->addr.spaprvio.has_reg)
> > +            virBufferAsprintf(buf, " reg='%#llx'", info->addr.spaprvio.reg);
> > +        break;
> 
> Is the intent that reg will always be in hex?  "%#llx" results in 0 or
> in 0x1000; would it be better to use "0x%llx" to get 0x0 vs. 0x1000 so
> that we always have a leading 0x?

It is usually specified in hex, but that's just a stylistic convention.
I guess it's better to switch to "0x%llx" though just for 100%
consistency.

> >  static int
> > +virDomainDeviceSpaprVioAddressParseXML(xmlNodePtr node,
> > +                                      virDomainDeviceSpaprVioAddressPtr addr)
> > +{
> > +    char *reg;
> > +    int ret;
> > +
> > +    memset(addr, 0, sizeof(*addr));
> > +    addr->has_reg = false;
> > +
> > +    reg = virXMLPropString(node, "reg");
> > +    if (reg) {
> > +        if (virStrToLong_ull(reg, NULL, 16, &addr->reg) < 0) {
> > +            virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > +                                 _("Cannot parse <address> 'reg' attribute"));
> > +            ret = -1;
> > +            goto cleanup;
> > +        }
> > +
> > +        addr->has_reg = true;
> 
> Looking at patch 6/6, I see you start default assignments at 0x1000,
> 0x2000, or 0x30000000 depending on which device type is getting
> assigned, and increment attempts by 0x1000 until you have no collision.
>  Must assignments be on a 0x1000 boundary?  

No they can take any value. 4K is just "neater".

> Can an assignment of reg='0' ever be
> valid?  If not, then we can use non-zero info->addr.spaprvio.reg as
> evidence of default assignment, rather than needing
> info->addr.spaprvio.has_reg.

No 0 is a valid address. Which is a pity because has_reg is a bit ugly.
But I think it's better than picking 0 or some other value as a magic
"unassigned" value - that might come back to bite us.


> As promised, I think this RNG matches your code, but it would need
> further tweaks if we declare that a valid address must always be a
> non-zero multiple of 0x1000.

Thanks, I'll incorporate that into v2.

cheers
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20111212/3ce90c23/attachment-0001.sig>