On 04/27/2015 09:07 AM, Michal Privoznik wrote:

Some platforms, like aarch64, don't have APIC but GIC. So there's no reason to have <apic/> feature turned on. However, we are

s/have/have the/ s/turned on/enabled/

still missing <gic/> feature. This commit introduces the feature to XML parser and formatter, adds documentation and updates RNG

s/to/to the/

schema.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatdomain.html.in | 9 +++++++++ docs/schemas/domaincommon.rng | 11 ++++++++++- src/conf/domain_conf.c | 34 +++++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 2 ++ 4 files changed, 54 insertions(+), 2 deletions(-)

Does aarch64 just ignore APIC? Makes me wonder how things work today... Is this also an architecture capability feature that needs to be added? IOW: How does one determine whether their architecture has GIC instead of APIC.

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index e921749..27883fe 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1401,6 +1401,7 @@ <hidden state='on'/> </kvm> <pvspinlock/> + <gic version='2'/>

</features> ...</pre> @@ -1501,6 +1502,14 @@ performance monitoring unit for the guest. <span class="since">Since 1.2.12</span> </dd> + <dt><code>gic</code></dt> + <dd>Some architectures don't have <code>APIC</code> but have + <code>GIC</code> <i>(Generic Interrupt Controller)</i>. For example + aarch64 is one of those architectures. If the guest belongs to the + set, you may want to turn on this feature instead of + <code>APIC</code>. Optional attribute <code>version</code> specifies + version of the controller, however may not be supported by all + hypervisors.</dd>

Enable for architectures using a General Interrupt Controller instead of APIC in order to handle interrupts. For example, the 'aarch64' architecture uses <code>gic</code> instead of <code>apic</code>. The optional attribute <code>version</code> specifies the GIC version; however, it may not be supported by all hypervisors. <span class="since">Since 1.2.16</span> </dd> NOTE1: I didn't put <code> </code> around the capitalized GIC or APIC since to me that would infer the feature must be capitalized. NOTE2: That last sentance is awkward - what would happen if it were supplied, but the hypervisor didn't support/recognize it? NOTE3: I added the <span> and put the </dd> on a separate line like the previous <dd>...</dd> - not that it matters.

</dl>

<h3><a name="elementsTime">Time keeping</a></h3> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 19461f5..b1d6f6b 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3953,7 +3953,7 @@ </element> </define> <!-- - A set of optional features: PAE, APIC, ACPI, + A set of optional features: PAE, APIC, ACPI, GIC, HyperV Enlightenment, KVM features, paravirtual spinlocks and HAP support --> <define name="features"> @@ -4014,6 +4014,15 @@ <optional> <ref name="pmu"/> </optional> + <optional> + <element name="gic"> + <optional> + <attribute name="version"> + <ref name="positiveInteger"/> + </attribute> + </optional> + </element> + </optional> </interleave> </element> </optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 03710cb..466163e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -144,7 +144,8 @@ VIR_ENUM_IMPL(virDomainFeature, VIR_DOMAIN_FEATURE_LAST, "kvm", "pvspinlock", "capabilities", - "pmu") + "pmu", + "gic")

VIR_ENUM_IMPL(virDomainCapabilitiesPolicy, VIR_DOMAIN_CAPABILITIES_POLICY_LAST, "default", @@ -14361,6 +14362,23 @@ virDomainDefParseXML(xmlDocPtr xml, ctxt->node = node; break;

+ case VIR_DOMAIN_FEATURE_GIC: + node = ctxt->node; + ctxt->node = nodes[i]; + if ((tmp = virXPathString("string(./@version)", ctxt))) { + if (virStrToLong_ui(tmp, NULL, 10, &def->gic_version) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("malformed gic version: %s"), tmp); + goto error; + } + VIR_FREE(tmp);

I think you wan to use virStrToLong_uip... and would probably need to check for an invalid value of zero (that way you can use that...)

+ } else { + def->gic_version = 2; /* By default, GIC version is 2 */

If not provided, then we're setting to the magic number of 2, which means when we format it will be written out. Is that expected/desired? I would think we should be able to handle things when the version is not supplied. Furthermore, since it's possible (from the docs) that a hypervisor doesn't support it, then setting a default could cause an issue.

+ } + def->features[val] = VIR_TRISTATE_SWITCH_ON; + ctxt->node = node; + break; + /* coverity[dead_error_begin] */ case VIR_DOMAIN_FEATURE_LAST: break; @@ -16443,6 +16461,14 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src, return false; }

+ /* GIC version */ + if (src->gic_version != dst->gic_version) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Source GIC version '%u' does not match destination '%u'"), + src->gic_version, dst->gic_version); + return false; + } +

Obviously if the gic_version is enabled, so is gic; however, what if gic_

/* hyperv */ if (src->features[VIR_DOMAIN_FEATURE_HYPERV] == VIR_TRISTATE_SWITCH_ON) { for (i = 0; i < VIR_DOMAIN_HYPERV_LAST; i++) { @@ -20996,6 +21022,12 @@ virDomainDefFormatInternal(virDomainDefPtr def, virBufferAddLit(buf, "</capabilities>\n"); break;

+ case VIR_DOMAIN_FEATURE_GIC: + if (def->features[i] == VIR_TRISTATE_SWITCH_ON) + virBufferAsprintf(buf, "<gic version='%u'/>\n", + def->gic_version); + break; +

Here's where I'd think that since gic_version is optional we'd have to have a way to just print "<gic/>"; otherwise, it's not optional. John

/* coverity[dead_error_begin] */ case VIR_DOMAIN_FEATURE_LAST: break; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 9955052..a79f0b8 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1649,6 +1649,7 @@ typedef enum { VIR_DOMAIN_FEATURE_PVSPINLOCK, VIR_DOMAIN_FEATURE_CAPABILITIES, VIR_DOMAIN_FEATURE_PMU, + VIR_DOMAIN_FEATURE_GIC,

VIR_DOMAIN_FEATURE_LAST } virDomainFeature; @@ -2171,6 +2172,7 @@ struct _virDomainDef { int hyperv_features[VIR_DOMAIN_HYPERV_LAST]; int kvm_features[VIR_DOMAIN_KVM_LAST]; unsigned int hyperv_spinlocks; + unsigned int gic_version; /* by default 2 */

/* These options are of type virTristateSwitch: ON = keep, OFF = drop */ int caps_features[VIR_DOMAIN_CAPS_FEATURE_LAST];

On 04/30/2015 07:53 AM, Michal Privoznik wrote: ...

...

...

+ } + def->features[val] = VIR_TRISTATE_SWITCH_ON; + ctxt->node = node; + break; + /* coverity[dead_error_begin] */ case VIR_DOMAIN_FEATURE_LAST: break; @@ -16443,6 +16461,14 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src, return false; }

+ /* GIC version */ + if (src->gic_version != dst->gic_version) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Source GIC version '%u' does not match destination '%u'"), + src->gic_version, dst->gic_version); + return false; + } +

Obviously if the gic_version is enabled, so is gic; however, what if gic_

Er, what?

Must've been distracted and lost my train of thought. I was probably going for the if gic_version was 0 (as in optionally not set), then is the GIC bit checked... I probably started off into the code and then got distracted. But I see it is so it's a nothing. John

...

...

/* hyperv */ if (src->features[VIR_DOMAIN_FEATURE_HYPERV] == VIR_TRISTATE_SWITCH_ON) { for (i = 0; i < VIR_DOMAIN_HYPERV_LAST; i++) { @@ -20996,6 +21022,12 @@ virDomainDefFormatInternal(virDomainDefPtr def, virBufferAddLit(buf, "</capabilities>\n"); break;

+ case VIR_DOMAIN_FEATURE_GIC: + if (def->features[i] == VIR_TRISTATE_SWITCH_ON) + virBufferAsprintf(buf, "<gic version='%u'/>\n", + def->gic_version); + break; +

Michal

On 04/27/2015 09:07 AM, Michal Privoznik wrote:

The only version that's supported in QEMU is version 2, currently. Fortunately, it is enabled by aarch64 automatically, so there's nothing for us that needs to be put onto command line.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 13 +++++++++++ .../qemuxml2argvdata/qemuxml2argv-aarch64-gic.args | 6 +++++ .../qemuxml2argvdata/qemuxml2argv-aarch64-gic.xml | 26 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 2 ++ 4 files changed, 47 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic.xml

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index a54f3a3..0341300 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7199,6 +7199,19 @@ qemuBuildCpuArgStr(virQEMUDriverPtr driver, have_cpu = true; }

+ if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON) { + if (def->gic_version != 2) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("gic version '%u' is not supported"), + def->gic_version); + goto cleanup; + }

This is where I'd expect the "if gic_version != 0" type logic to go. That way the future could handle version==2, version==3, etc. Eventually one would think that magic number would be replaced by something else. So after this I see that it doesn't matter "today", but could matter "someday" since nothing gets sent or built up in the command line.

+ + /* There's no command line argument currently to turn on/off GIC. It's + * done automatically by qemu-system-aarch64. But if this changes, lets + * put the code here. */ + } + if (virBufferCheckError(&buf) < 0) goto cleanup;

diff --git a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic.args b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic.args new file mode 100644 index 0000000..e61cd1e --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic.args @@ -0,0 +1,6 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-aarch64 -S -M virt -no-kvm -cpu cortex-a53 -m 1024 -smp 1 \ +-nographic -nodefaults -monitor unix:/tmp/test-monitor,server,nowait -boot c \ +-kernel /aarch64.kernel -initrd /aarch64.initrd -append console=ttyAMA0 -usb \ +-net nic,macaddr=52:54:00:09:a4:37,vlan=0,model=virtio,name=net0 \ +-net user,vlan=0,name=hostnet0 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic.xml b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic.xml new file mode 100644 index 0000000..08d3d71 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic.xml @@ -0,0 +1,26 @@ +<domain type="qemu"> + <name>aarch64test</name> + <uuid>6ba410c5-1e5c-4d57-bee7-2228e7ffa32f</uuid> + <memory>1048576</memory> + <currentMemory>1048576</currentMemory> + <vcpu>1</vcpu> + <features> + <acpi/> + <gic version='2'/>

Both enabled? Probably could remove acpi, right? John

+ </features> + <cpu match='exact'> + <model>cortex-a53</model> + </cpu> + <os> + <type arch="aarch64" machine="virt">hvm</type> + <kernel>/aarch64.kernel</kernel> + <initrd>/aarch64.initrd</initrd> + <cmdline>console=ttyAMA0</cmdline> + </os> + <devices> + <emulator>/usr/bin/qemu-system-aarch64</emulator> + <interface type='user'> + <mac address='52:54:00:09:a4:37'/> + </interface> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 055ceee..ce5a7e8 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1531,6 +1531,8 @@ mymain(void) DO_TEST("aarch64-cpu-model-host", QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE, QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_DEVICE_VIRTIO_MMIO, QEMU_CAPS_CPU_HOST, QEMU_CAPS_KVM); + DO_TEST("aarch64-gic", QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE, + QEMU_CAPS_KVM);

DO_TEST("kvm-pit-device", QEMU_CAPS_KVM_PIT_TICK_POLICY); DO_TEST("kvm-pit-delay", QEMU_CAPS_NO_KVM_PIT);