[libvirt] [PATCH] docs: List possible GIC versions
Recent changes to the handling of GIC version, specifically commit 2a7b11eafb67, have clearly defined what values are acceptable for the version attribute of the <gic> element. Update the documentation accordingly. --- docs/formatdomain.html.in | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index e96798f..faee603 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1577,7 +1577,8 @@ 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; + attribute <code>version</code> specifies the GIC version (one of + <code>2</code>, <code>3</code> and <code>host</code>); however, it may not be supported by all hypervisors. <span class="since">Since 1.2.16</span> </dd> -- 2.5.0
GIC v2 is the default, but checking against that specific version when we want to know whether the default has been selected is potentially error prone; using an alias instead makes it safer. --- src/qemu/qemu_command.c | 7 +++---- src/qemu/qemu_domain.c | 4 ++-- src/util/virgic.h | 3 +++ 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b751f04..287a2b9 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5306,10 +5306,9 @@ qemuBuildMachineCommandLine(virCommandPtr cmd, return -1; } - /* 2 is the default, so we don't put it as option for - * backwards compatibility - */ - if (def->gic_version != VIR_GIC_VERSION_2) { + /* The default GIC version should not be specified on the + * QEMU commandline for backwards compatibility reasons */ + if (def->gic_version != VIR_GIC_VERSION_DEFAULT) { if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACH_VIRT_GIC_VERSION)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index c6ff0da..99ef696 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1264,10 +1264,10 @@ qemuDomainDefEnableDefaultFeatures(virDomainDefPtr def) break; } - /* Default to GIC v2 if no version was specified */ + /* Use the default GIC version if no version was specified */ if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON && def->gic_version == VIR_GIC_VERSION_NONE) - def->gic_version = VIR_GIC_VERSION_2; + def->gic_version = VIR_GIC_VERSION_DEFAULT; } diff --git a/src/util/virgic.h b/src/util/virgic.h index a2ba300..470ce95 100644 --- a/src/util/virgic.h +++ b/src/util/virgic.h @@ -35,4 +35,7 @@ typedef enum { VIR_ENUM_DECL(virGICVersion); +/* Consider GIC v2 the default */ +# define VIR_GIC_VERSION_DEFAULT VIR_GIC_VERSION_2 + #endif /* __VIR_GIC_H__ */ -- 2.5.0
This was of course not meant to be a reply to the other patch, but to be a separate thread... Sigh. -- Andrea Bolognani Software Engineer - Virtualization Team
On 02/18/2016 10:28 AM, Andrea Bolognani wrote:
GIC v2 is the default, but checking against that specific version when we want to know whether the default has been selected is potentially error prone; using an alias instead makes it safer. --- src/qemu/qemu_command.c | 7 +++---- src/qemu/qemu_domain.c | 4 ++-- src/util/virgic.h | 3 +++ 3 files changed, 8 insertions(+), 6 deletions(-)
This works, but so does I think changing VIR_GIC_VERSION_2 in virgic.h to VIR_GIC_VERSION_DEFAULT ACK either way, but I think changing _2 to _DEFAULT would be better... John
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b751f04..287a2b9 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5306,10 +5306,9 @@ qemuBuildMachineCommandLine(virCommandPtr cmd, return -1; }
- /* 2 is the default, so we don't put it as option for - * backwards compatibility - */ - if (def->gic_version != VIR_GIC_VERSION_2) { + /* The default GIC version should not be specified on the + * QEMU commandline for backwards compatibility reasons */ + if (def->gic_version != VIR_GIC_VERSION_DEFAULT) { if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACH_VIRT_GIC_VERSION)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index c6ff0da..99ef696 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1264,10 +1264,10 @@ qemuDomainDefEnableDefaultFeatures(virDomainDefPtr def) break; }
- /* Default to GIC v2 if no version was specified */ + /* Use the default GIC version if no version was specified */ if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON && def->gic_version == VIR_GIC_VERSION_NONE) - def->gic_version = VIR_GIC_VERSION_2; + def->gic_version = VIR_GIC_VERSION_DEFAULT; }
diff --git a/src/util/virgic.h b/src/util/virgic.h index a2ba300..470ce95 100644 --- a/src/util/virgic.h +++ b/src/util/virgic.h @@ -35,4 +35,7 @@ typedef enum {
VIR_ENUM_DECL(virGICVersion);
+/* Consider GIC v2 the default */ +# define VIR_GIC_VERSION_DEFAULT VIR_GIC_VERSION_2 + #endif /* __VIR_GIC_H__ */
On Fri, 2016-02-19 at 11:05 -0500, John Ferlan wrote:
On 02/18/2016 10:28 AM, Andrea Bolognani wrote:
GIC v2 is the default, but checking against that specific version when we want to know whether the default has been selected is potentially error prone; using an alias instead makes it safer. --- src/qemu/qemu_command.c | 7 +++---- src/qemu/qemu_domain.c | 4 ++-- src/util/virgic.h | 3 +++ 3 files changed, 8 insertions(+), 6 deletions(-)
This works, but so does I think changing VIR_GIC_VERSION_2 in virgic.h to VIR_GIC_VERSION_DEFAULT
ACK either way, but I think changing _2 to _DEFAULT would be better...
That would hide the fact that v2 is the default version, though. I've pushed it as is, thanks for the review. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
On 02/18/2016 10:28 AM, Andrea Bolognani wrote:
Recent changes to the handling of GIC version, specifically commit 2a7b11eafb67, have clearly defined what values are acceptable for the version attribute of the <gic> element. Update the documentation accordingly. --- docs/formatdomain.html.in | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index e96798f..faee603 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1577,7 +1577,8 @@ 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; + attribute <code>version</code> specifies the GIC version (one of + <code>2</code>, <code>3</code> and <code>host</code>);
Rather than parentheses before the semi-colon of however... Consider a sentence after "all hypervisors.": Accepted values are <code>2</code>, <code>3</code>, or <code>host</code>). ACK with that John
however, it may not be supported by all hypervisors. <span class="since">Since 1.2.16</span> </dd>
On Fri, 2016-02-19 at 10:56 -0500, John Ferlan wrote:
On 02/18/2016 10:28 AM, Andrea Bolognani wrote:
Recent changes to the handling of GIC version, specifically commit 2a7b11eafb67, have clearly defined what values are acceptable for the version attribute of the <gic> element. Update the documentation accordingly. --- docs/formatdomain.html.in | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index e96798f..faee603 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1577,7 +1577,8 @@ 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; + attribute <code>version</code> specifies the GIC version (one of + <code>2</code>, <code>3</code> and <code>host</code>);
Rather than parentheses before the semi-colon of however...
Consider a sentence after "all hypervisors.":
Accepted values are <code>2</code>, <code>3</code>, or <code>host</code>).
ACK with that
Amended and pushed. Thanks. -- Andrea Bolognani Software Engineer - Virtualization Team
participants (2)
-
Andrea Bolognani -
John Ferlan