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.
>
> For QEMU PPC64 we have a machine type ("pseries") which has a virtual
> bus called "spapr-vio". We need to be able to create devices on this
> bus, and as such need a way to specify the address for those devices.
>
> This patch adds a new address type "spapr-vio", which achieves this.
>
> The addressing is specified with a "reg" property in the address
> definition. The reg is optional, if it is not specified QEMU will
> auto-assign an address for the device.
>
> Signed-off-by: Michael Ellerman <michael@xxxxxxxxxxxxxx>
> ---
> src/conf/domain_conf.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
> src/conf/domain_conf.h | 9 +++++++++
> src/qemu/qemu_command.c | 5 +++++
> 3 files changed, 56 insertions(+), 1 deletions(-)
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.
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.
I've pushed 1-3, and we'll wait for v2 for 4-6.
>
> 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'/>). 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.
>
> 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?
> 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? If so, we can tighten up the
pattern I propose in the rng to require it, and the parser should be
fixed to require it as well. 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.
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.
diff --git i/docs/schemas/domaincommon.rng w/docs/schemas/domaincommon.rng
index 27ec1da..dc85329 100644
--- i/docs/schemas/domaincommon.rng
+++ w/docs/schemas/domaincommon.rng
@@ -2263,6 +2263,11 @@
</attribute>
</optional>
</define>
+ <define name='spaprvioaddress'>
+ <attribute name='reg'>
+ <ref name='spaprvioAddr'/>
+ </attribute>
+ </define>
<!--
Devices attached to a domain.
Sub-elements such as <alias> are not documented here, as they
@@ -2577,6 +2582,12 @@
</attribute>
<ref name="usbportaddress"/>
</group>
+ <group>
+ <attribute name='type'>
+ <value>spapr-vio</value>
+ </attribute>
+ <ref name='spaprvioaddress'/>
+ </group>
</choice>
</element>
</define>
@@ -2826,6 +2837,11 @@
<param name="pattern">(0x)?[0-7]</param>
</data>
</define>
+ <define name='spaprvioAddr'>
+ <data type='string'>
+ <param name='pattern'>(0x)?[0-9a-fA-F]{1,16}</param>
+ </data>
+ </define>
<define name="driveController">
<data type="string">
<param name="pattern">[0-9]{1,2}</param>
--
Eric Blake eblake@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list