Re: [libvirt] [PATCH 03/11] qemu: Move GIC checks to qemuDomainDefValidateFeatures()

2018-02-13 Thread Andrea Bolognani 
On Mon, 2018-02-12 at 14:59 -0500, John Ferlan wrote:
> > @@ -3252,6 +3252,16 @@ qemuDomainDefValidateFeatures(const virDomainDef 
> > *def)
> >  }
> >  break;
> >  
> > +case VIR_DOMAIN_FEATURE_GIC:
> > +if (def->features[i] == VIR_TRISTATE_SWITCH_ON &&
> > +!qemuDomainIsVirt(def)) {
> > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +   _("The '%s' feature is only supported for 
> > %s guests"),
> 
> s/for %s guests/for '%s' guests/ ??

Sure, why not :)

> > +   featureName, "mach-virt");
> 
> Not that I think it matters greatly, the error message changes from ARM
> specifically to mach-virt... I guess I'm just used to seeing 'ARM' or
> 'aarch64' and not 'mach-virt' (although the XML would be AIUI " arch='aarch64' machine='virt'>"). Suffice to say it caused me to wonder
> and I have to imagine it would do the same for anyone getting that message.
> 
> I don't have a strong opinion one way or other and it's not really
> easily "decode-able" based on someone just reading the " machine=''..." in the docs.

Yeah, we're being quite inconsistent throughout both the code and
the test suite.

The root of the problem is that, while other architectures use very
recognizable or at least distinct names for the machine types we
care about, such as pseries or q35, on aarch64 the machine type is
just... virt. Which is not really a great name. But we can't just
use the architecture name either, because there are a lot of arm
and aarch64 machine types out there.

Using mach-virt is an attempt to sidestep the problem, and it's
used (at least internally) in QEMU. But I agree it can be confusing.

Maybe the solution in this case is to use the approach originally
used for the  feature and just say

  The '%s' feature is not supported for architecture '%s' or
  machine type '%s'

though that's slightly less helpful for users...

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 03/11] qemu: Move GIC checks to qemuDomainDefValidateFeatures()

2018-02-12 Thread John Ferlan 


On 02/06/2018 11:42 AM, Andrea Bolognani wrote:
> Keep them along with other arch/machine type checks for
> features instead of waiting until command line generation
> time.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  src/qemu/qemu_command.c  |  7 ---
>  src/qemu/qemu_domain.c   | 11 ++-
>  tests/qemuxml2argvtest.c | 12 ++--
>  3 files changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 24b434a45..529079be0 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -7192,13 +7192,6 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
>  
>  if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON) 
> {
>  if (def->gic_version != VIR_GIC_VERSION_NONE) {
> -if (!qemuDomainIsVirt(def)) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -   _("gic-version option is available "
> - "only for ARM virt machine"));
> -goto cleanup;
> -}
> -
>  /* The default GIC version (GICv2) should not be specified on
>   * the QEMU commandline for backwards compatibility reasons 
> */
>  if (def->gic_version != VIR_GIC_VERSION_2) {
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index dd36b42eb..98cba8789 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -3252,6 +3252,16 @@ qemuDomainDefValidateFeatures(const virDomainDef *def)
>  }
>  break;
>  
> +case VIR_DOMAIN_FEATURE_GIC:
> +if (def->features[i] == VIR_TRISTATE_SWITCH_ON &&
> +!qemuDomainIsVirt(def)) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("The '%s' feature is only supported for %s 
> guests"),

s/for %s guests/for '%s' guests/ ??

> +   featureName, "mach-virt");

Not that I think it matters greatly, the error message changes from ARM
specifically to mach-virt... I guess I'm just used to seeing 'ARM' or
'aarch64' and not 'mach-virt' (although the XML would be AIUI ""). Suffice to say it caused me to wonder
and I have to imagine it would do the same for anyone getting that message.

I don't have a strong opinion one way or other and it's not really
easily "decode-able" based on someone just reading the "

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 03/11] qemu: Move GIC checks to qemuDomainDefValidateFeatures()

2018-02-06 Thread Andrea Bolognani 
Keep them along with other arch/machine type checks for
features instead of waiting until command line generation
time.

Signed-off-by: Andrea Bolognani 
---
 src/qemu/qemu_command.c  |  7 ---
 src/qemu/qemu_domain.c   | 11 ++-
 tests/qemuxml2argvtest.c | 12 ++--
 3 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 24b434a45..529079be0 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7192,13 +7192,6 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
 
 if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON) {
 if (def->gic_version != VIR_GIC_VERSION_NONE) {
-if (!qemuDomainIsVirt(def)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("gic-version option is available "
- "only for ARM virt machine"));
-goto cleanup;
-}
-
 /* The default GIC version (GICv2) should not be specified on
  * the QEMU commandline for backwards compatibility reasons */
 if (def->gic_version != VIR_GIC_VERSION_2) {
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index dd36b42eb..98cba8789 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3252,6 +3252,16 @@ qemuDomainDefValidateFeatures(const virDomainDef *def)
 }
 break;
 
+case VIR_DOMAIN_FEATURE_GIC:
+if (def->features[i] == VIR_TRISTATE_SWITCH_ON &&
+!qemuDomainIsVirt(def)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("The '%s' feature is only supported for %s 
guests"),
+   featureName, "mach-virt");
+return -1;
+}
+break;
+
 case VIR_DOMAIN_FEATURE_ACPI:
 case VIR_DOMAIN_FEATURE_APIC:
 case VIR_DOMAIN_FEATURE_PAE:
@@ -3264,7 +3274,6 @@ qemuDomainDefValidateFeatures(const virDomainDef *def)
 case VIR_DOMAIN_FEATURE_CAPABILITIES:
 case VIR_DOMAIN_FEATURE_PMU:
 case VIR_DOMAIN_FEATURE_VMPORT:
-case VIR_DOMAIN_FEATURE_GIC:
 case VIR_DOMAIN_FEATURE_SMM:
 case VIR_DOMAIN_FEATURE_VMCOREINFO:
 case VIR_DOMAIN_FEATURE_LAST:
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index c8739909d..b7afb6980 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -2656,12 +2656,12 @@ mymain(void)
 DO_TEST_PARSE_ERROR("aarch64-gic-invalid",
 QEMU_CAPS_KVM, QEMU_CAPS_MACHINE_OPT,
 QEMU_CAPS_MACH_VIRT_GIC_VERSION);
-DO_TEST_FAILURE("aarch64-gic-not-virt",
-QEMU_CAPS_KVM, QEMU_CAPS_MACHINE_OPT,
-QEMU_CAPS_MACH_VIRT_GIC_VERSION);
-DO_TEST_FAILURE("aarch64-gic-not-arm",
-QEMU_CAPS_KVM, QEMU_CAPS_MACHINE_OPT,
-QEMU_CAPS_MACH_VIRT_GIC_VERSION);
+DO_TEST_PARSE_ERROR("aarch64-gic-not-virt",
+QEMU_CAPS_KVM, QEMU_CAPS_MACHINE_OPT,
+QEMU_CAPS_MACH_VIRT_GIC_VERSION);
+DO_TEST_PARSE_ERROR("aarch64-gic-not-arm",
+QEMU_CAPS_KVM, QEMU_CAPS_MACHINE_OPT,
+QEMU_CAPS_MACH_VIRT_GIC_VERSION);
 DO_TEST("aarch64-kvm-32-on-64",
 QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_DEVICE_VIRTIO_MMIO,
 QEMU_CAPS_DEVICE_PL011,
-- 
2.14.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list