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@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxx as my canonical address.
I'm not sure why the mails are coming from michael@xxxxxxxxxx, 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
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