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@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@ellerman.id.au as my canonical address. I'm not sure why the mails are coming from michael@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