On 05/10/2016 08:46 AM, Andrea Bolognani wrote:
> When the <gic/> element in not present in the domain XML, use the
> domain capabilities to figure out what GIC version is usable and
> choose that one automatically.
>
> This allows guests to be created on hardware that only supports
> GIC v3 without having to update virt-manager and similar tools.
>
> Keep using the default GIC version if the <gic/> element has been
> added to the domain XML but no version has been specified, as not
> to break existing guests.
> ---
> src/conf/domain_capabilities.c | 25 ++++++++++++++++
> src/conf/domain_capabilities.h | 8 +++++
> src/libvirt_private.syms | 1 +
> src/qemu/qemu_domain.c | 66 ++++++++++++++++++++++++++++++++++--------
> 4 files changed, 88 insertions(+), 12 deletions(-)
>
> diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c
> index 1676f0e..f9adeb9 100644
> --- a/src/conf/domain_capabilities.c
> +++ b/src/conf/domain_capabilities.c
> @@ -130,6 +130,31 @@ virDomainCapsEnumSet(virDomainCapsEnumPtr capsEnum,
> }
>
>
> +bool
> +virDomainCapsEnumIsSet(const virDomainCapsEnum *capsEnum,
> + const char *capsEnumName,
> + unsigned int value)
> +{
> + unsigned int val = 1 << value;
> + bool ret = false;
> +
> +
Extra newline
> + if (!val) {
> + /* Integer overflow */
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("integer overflow on %s. Please contact the "
> + "libvirt development team at libvir-list@xxxxxxxxxx"),
> + capsEnumName);
> + goto cleanup;
> + }
> +
Heh that's new to me, but I see you're just following the template of
virDomainCapsEnumSet, so okay
> + ret = (capsEnum->values & val);
> +
> + cleanup:
> + return ret;
> +}
> +
> +
> void
> virDomainCapsEnumClear(virDomainCapsEnumPtr capsEnum)
> {
> diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h
> index 492a9cf..8ed4111 100644
> --- a/src/conf/domain_capabilities.h
> +++ b/src/conf/domain_capabilities.h
> @@ -137,10 +137,18 @@ virDomainCapsPtr virDomainCapsNew(const char *path,
> __nvalues, __values); \
> } while (0)
>
> +# define VIR_DOMAIN_CAPS_ENUM_IS_SET(capsEnum, value) \
> + virDomainCapsEnumIsSet(&(capsEnum), #capsEnum, value) \
> +
> int virDomainCapsEnumSet(virDomainCapsEnumPtr capsEnum,
> const char *capsEnumName,
> size_t nvalues,
> unsigned int *values);
> +
> +bool virDomainCapsEnumIsSet(const virDomainCapsEnum *capsEnum,
> + const char *capsEnumName,
> + unsigned int value);
> +
> void virDomainCapsEnumClear(virDomainCapsEnumPtr capsEnum);
>
> char * virDomainCapsFormat(virDomainCapsPtr const caps);
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index fff8c30..183e84a 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -141,6 +141,7 @@ virDomainAuditVcpu;
>
> # conf/domain_capabilities.h
> virDomainCapsEnumClear;
> +virDomainCapsEnumIsSet;
> virDomainCapsEnumSet;
> virDomainCapsFormat;
> virDomainCapsNew;
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 93f0a01..d34450f 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -1921,26 +1921,67 @@ qemuDomainDefAddDefaultDevices(virDomainDefPtr def,
> * Make sure that features that should be enabled by default are actually
> * enabled and configure default values related to those features.
> */
> -static void
> -qemuDomainDefEnableDefaultFeatures(virDomainDefPtr def)
> +static int
> +qemuDomainDefEnableDefaultFeatures(virDomainDefPtr def,
> + virQEMUCapsPtr qemuCaps)
> {
> - switch (def->os.arch) {
> - case VIR_ARCH_ARMV7L:
> - case VIR_ARCH_AARCH64:
> - if (qemuDomainMachineIsVirt(def)) {
> - /* GIC is always available to ARM virt machines */
> - def->features[VIR_DOMAIN_FEATURE_GIC] = VIR_TRISTATE_SWITCH_ON;
> + virDomainCapsPtr caps = NULL;
> + virDomainCapsFeatureGICPtr gic = NULL;
> + int ret = -1;
> +
> + /* The virt machine type always uses GIC: if the relevant element
> + * was not included in the domain XML, we need to choose a suitable
> + * GIC version ourselves */
> + if ((def->os.arch == VIR_ARCH_ARMV7L ||
> + def->os.arch == VIR_ARCH_AARCH64) &&
> + qemuDomainMachineIsVirt(def) &&
> + def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ABSENT) {
> +
> + if (!(caps = virDomainCapsNew(def->emulator,
> + def->os.machine,
> + def->os.arch,
> + def->virtType)))
> + goto cleanup;
> +
Call this domCaps ? 'caps' name is usually used for virCapabilities
> + if (virQEMUCapsFillDomainCaps(caps, qemuCaps, NULL, 0) < 0)
> + goto cleanup;
> +
> + gic = &(caps->gic);
> +
> + /* Pick the best GIC version from those available */
> + if (gic->supported) {
> + virGICVersion version;
> +
> + VIR_DEBUG("Looking for usable GIC version in domain capabilities");
> + for (version = VIR_GIC_VERSION_LAST - 1;
> + version > VIR_GIC_VERSION_NONE;
> + version--) {
> + if (VIR_DOMAIN_CAPS_ENUM_IS_SET(gic->version, version)) {
> +
> + VIR_DEBUG("Using GIC version %s",
> + virGICVersionTypeToString(version));
> + def->gic_version = version;
> + break;
> + }
> + }
> }
Hmm that's a bit of a new pattern... it seems the only thing you really need
from domcaps is the bit of logic we encode via
virQEMUCapsFillDomainFeatureGICCaps. Maybe break that logic out into a public
function and call it here, rather than spinning up domcaps for a small bit of
info? Or is there more to it?
- Cole
> - break;
>
> - default:
> - break;
> + /* Even if we haven't found a usable GIC version in the domain
> + * capabilities, we still want to enable this */
> + def->features[VIR_DOMAIN_FEATURE_GIC] = VIR_TRISTATE_SWITCH_ON;
> }
>
> /* 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_DEFAULT;
> +
> + ret = 0;
> +
> + cleanup:
> + virObjectUnref(caps);
> +
> + return ret;
> }
>
>
> @@ -2033,7 +2074,8 @@ qemuDomainDefPostParse(virDomainDefPtr def,
> if (qemuCanonicalizeMachine(def, qemuCaps) < 0)
> goto cleanup;
>
> - qemuDomainDefEnableDefaultFeatures(def);
> + if (qemuDomainDefEnableDefaultFeatures(def, qemuCaps) < 0)
> + goto cleanup;
>
> qemuDomainRecheckInternalPaths(def, cfg, parseFlags);
>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list