| Message ID | 20200226170500.17028-3-eric.auger@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/arm/virt: kvm: allow gicv3 by default if host does not support v2 | expand |
On Wed, Feb 26, 2020 at 06:05:00PM +0100, Eric Auger wrote: > At the moment if the end-user does not specify the gic-version along > with KVM acceleration, v2 is set by default. However most of the > systems now have GICv3 and sometimes they do not support GICv2 > compatibility. In that case we end up with this error: > > qemu-system-aarch64: PMU: KVM_SET_DEVICE_ATTR: Invalid argument > qemu-system-aarch64: failed to set irq for PMU > and qemu aborts. > > This patch keeps the default v2 selection in all cases except > in the KVM accelerated mode when the host does not support v2. > This case did not work anyway so we do not break any compatibility. > Now we get v3 selected in such a case. > Also if the end-user explicitly > sets v2 whereas this latter is not supported, we also are > informed that v2 is not selected by thos host instead of getting the > above PMU related message. I would change the above to: Also, if the end-user explicitly sets v2 and this is not supported by the host, then the user gets a more informative error message than the PMU invalid argument message above. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > Reported-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > hw/arm/virt.c | 30 ++++++++++++++++++++++++++++++ > include/hw/arm/virt.h | 1 + > 2 files changed, 31 insertions(+) > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index a196bbf0d5..b37b0c40c1 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -1554,6 +1554,33 @@ static void machvirt_init(MachineState *machine) > } > } > } > + } else if (kvm_enabled()) { > + int probe_bitmap = kvm_arm_vgic_probe(); > + > + if (!probe_bitmap) { > + error_report( > + "Unable to determine GIC version supported by host"); > + exit(1); > + } > + if (!vms->gic_version_user_selected) { > + /* > + * by default v2 is supposed to be chosen: check it is > + * supported by the host. Otherwise take v3. > + */ > + if (probe_bitmap & KVM_ARM_VGIC_V2) { > + vms->gic_version = 2; > + } else if (probe_bitmap & KVM_ARM_VGIC_V3) { > + vms->gic_version = 3; > + } > + } else { /* user explicitly set the version to 2 or 3 */ > + if (vms->gic_version == 2 && !(probe_bitmap & KVM_ARM_VGIC_V2)) { > + error_report("GICv2 is not supported by the host"); > + exit(1); > + } else if (vms->gic_version == 3 && !(probe_bitmap & KVM_ARM_VGIC_V3)) { > + error_report("GICv3 is not supported by the host"); > + exit(1); > + } > + } > } > > if (!cpu_type_valid(machine->cpu_type)) { > @@ -1840,6 +1867,7 @@ static void virt_set_gic_version(Object *obj, const char *value, Error **errp) > { > VirtMachineState *vms = VIRT_MACHINE(obj); > > + vms->gic_version_user_selected = true; > if (!strcmp(value, "3")) { > vms->gic_version = 3; > } else if (!strcmp(value, "2")) { > @@ -1851,6 +1879,7 @@ static void virt_set_gic_version(Object *obj, const char *value, Error **errp) > } else { > error_setg(errp, "Invalid gic-version value"); > error_append_hint(errp, "Valid values are 3, 2, host, max.\n"); > + vms->gic_version_user_selected = false; > } > } > > @@ -2103,6 +2132,7 @@ static void virt_instance_init(Object *obj) > NULL); > /* Default GIC type is v2 */ > vms->gic_version = 2; > + vms->gic_version_user_selected = false; > object_property_add_str(obj, "gic-version", virt_get_gic_version, > virt_set_gic_version, NULL); > object_property_set_description(obj, "gic-version", > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h > index 71508bf40c..e363cde452 100644 > --- a/include/hw/arm/virt.h > +++ b/include/hw/arm/virt.h > @@ -124,6 +124,7 @@ typedef struct { > bool its; > bool virt; > int32_t gic_version; > + bool gic_version_user_selected; > VirtIOMMUType iommu; > struct arm_boot_info bootinfo; > MemMapEntry *memmap; > -- > 2.20.1 > > I just noticed that virt_get_gic_version() returns "2" if the user didn't explicitly request "3". I guess that hasn't been a problem for "max" and "host" because nobody is looking, at least not until after gic_version has been fully initialized. It is wrong though (for example, with TCG "max" should be "3", not "2"). Ideally virt_get_gic_version() would always return what will actually be used by the guest, but maybe pre-init it could return "max", "host", and "nosel" instead. We could then use the property value "nosel" in machvirt_init() to determine whether or not the user provided input, rather than adding gic_version_user_selected. Thanks, drew
Hi Drew, On 2/26/20 7:42 PM, Andrew Jones wrote: > On Wed, Feb 26, 2020 at 06:05:00PM +0100, Eric Auger wrote: >> At the moment if the end-user does not specify the gic-version along >> with KVM acceleration, v2 is set by default. However most of the >> systems now have GICv3 and sometimes they do not support GICv2 >> compatibility. In that case we end up with this error: >> >> qemu-system-aarch64: PMU: KVM_SET_DEVICE_ATTR: Invalid argument >> qemu-system-aarch64: failed to set irq for PMU >> and qemu aborts. >> >> This patch keeps the default v2 selection in all cases except >> in the KVM accelerated mode when the host does not support v2. >> This case did not work anyway so we do not break any compatibility. >> Now we get v3 selected in such a case. > >> Also if the end-user explicitly >> sets v2 whereas this latter is not supported, we also are >> informed that v2 is not selected by thos host instead of getting the >> above PMU related message. > > I would change the above to: > > Also, if the end-user explicitly sets v2 and this is not supported by > the host, then the user gets a more informative error message than > the PMU invalid argument message above. Sure > >> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> Reported-by: Dr. David Alan Gilbert <dgilbert@redhat.com> >> --- >> hw/arm/virt.c | 30 ++++++++++++++++++++++++++++++ >> include/hw/arm/virt.h | 1 + >> 2 files changed, 31 insertions(+) >> >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c >> index a196bbf0d5..b37b0c40c1 100644 >> --- a/hw/arm/virt.c >> +++ b/hw/arm/virt.c >> @@ -1554,6 +1554,33 @@ static void machvirt_init(MachineState *machine) >> } >> } >> } >> + } else if (kvm_enabled()) { >> + int probe_bitmap = kvm_arm_vgic_probe(); >> + >> + if (!probe_bitmap) { >> + error_report( >> + "Unable to determine GIC version supported by host"); >> + exit(1); >> + } >> + if (!vms->gic_version_user_selected) { >> + /* >> + * by default v2 is supposed to be chosen: check it is >> + * supported by the host. Otherwise take v3. >> + */ >> + if (probe_bitmap & KVM_ARM_VGIC_V2) { >> + vms->gic_version = 2; >> + } else if (probe_bitmap & KVM_ARM_VGIC_V3) { >> + vms->gic_version = 3; >> + } >> + } else { /* user explicitly set the version to 2 or 3 */ >> + if (vms->gic_version == 2 && !(probe_bitmap & KVM_ARM_VGIC_V2)) { >> + error_report("GICv2 is not supported by the host"); >> + exit(1); >> + } else if (vms->gic_version == 3 && !(probe_bitmap & KVM_ARM_VGIC_V3)) { >> + error_report("GICv3 is not supported by the host"); >> + exit(1); >> + } >> + } >> } >> >> if (!cpu_type_valid(machine->cpu_type)) { >> @@ -1840,6 +1867,7 @@ static void virt_set_gic_version(Object *obj, const char *value, Error **errp) >> { >> VirtMachineState *vms = VIRT_MACHINE(obj); >> >> + vms->gic_version_user_selected = true; >> if (!strcmp(value, "3")) { >> vms->gic_version = 3; >> } else if (!strcmp(value, "2")) { >> @@ -1851,6 +1879,7 @@ static void virt_set_gic_version(Object *obj, const char *value, Error **errp) >> } else { >> error_setg(errp, "Invalid gic-version value"); >> error_append_hint(errp, "Valid values are 3, 2, host, max.\n"); >> + vms->gic_version_user_selected = false; >> } >> } >> >> @@ -2103,6 +2132,7 @@ static void virt_instance_init(Object *obj) >> NULL); >> /* Default GIC type is v2 */ >> vms->gic_version = 2; >> + vms->gic_version_user_selected = false; >> object_property_add_str(obj, "gic-version", virt_get_gic_version, >> virt_set_gic_version, NULL); >> object_property_set_description(obj, "gic-version", >> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h >> index 71508bf40c..e363cde452 100644 >> --- a/include/hw/arm/virt.h >> +++ b/include/hw/arm/virt.h >> @@ -124,6 +124,7 @@ typedef struct { >> bool its; >> bool virt; >> int32_t gic_version; >> + bool gic_version_user_selected; >> VirtIOMMUType iommu; >> struct arm_boot_info bootinfo; >> MemMapEntry *memmap; >> -- >> 2.20.1 >> >> > > I just noticed that virt_get_gic_version() returns "2" if the user didn't > explicitly request "3". I guess that hasn't been a problem for "max" > and "host" because nobody is looking, at least not until after gic_version > has been fully initialized. It is wrong though (for example, with TCG > "max" should be "3", not "2"). That's correct indeed. Ideally virt_get_gic_version() would always > return what will actually be used by the guest, but maybe pre-init it > could return "max", "host", and "nosel" instead. We could then use the > property value "nosel" in machvirt_init() to determine whether or not the > user provided input, rather than adding gic_version_user_selected. OK I will try to address that as well. I also notice the property description for gic-version misses "max" Thanks Eric > > Thanks, > drew > >
On Wed, Feb 26, 2020 at 06:05:00PM +0100, Eric Auger wrote: > At the moment if the end-user does not specify the gic-version along > with KVM acceleration, v2 is set by default. However most of the > systems now have GICv3 and sometimes they do not support GICv2 > compatibility. In that case we end up with this error: > > qemu-system-aarch64: PMU: KVM_SET_DEVICE_ATTR: Invalid argument > qemu-system-aarch64: failed to set irq for PMU > and qemu aborts. > > This patch keeps the default v2 selection in all cases except > in the KVM accelerated mode when the host does not support v2. > This case did not work anyway so we do not break any compatibility. > Now we get v3 selected in such a case. Also if the end-user explicitly > sets v2 whereas this latter is not supported, we also are > informed that v2 is not selected by thos host instead of getting the > above PMU related message. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > Reported-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > hw/arm/virt.c | 30 ++++++++++++++++++++++++++++++ > include/hw/arm/virt.h | 1 + > 2 files changed, 31 insertions(+) > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index a196bbf0d5..b37b0c40c1 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -1554,6 +1554,33 @@ static void machvirt_init(MachineState *machine) > } > } > } > + } else if (kvm_enabled()) { > + int probe_bitmap = kvm_arm_vgic_probe(); > + > + if (!probe_bitmap) { > + error_report( > + "Unable to determine GIC version supported by host"); > + exit(1); > + } > + if (!vms->gic_version_user_selected) { > + /* > + * by default v2 is supposed to be chosen: check it is > + * supported by the host. Otherwise take v3. > + */ > + if (probe_bitmap & KVM_ARM_VGIC_V2) { How about 'if ((probe_bitmap & KVM_ARM_VGIC_V2) && max_cpus <= GIC_NCPU)', because even on gicv3 hosts that support gicv2 guests command lines that didn't specify a gic version and did specify more than 8 vcpus were also broken. We can now automagically allow those to work too. Thanks, drew
diff --git a/hw/arm/virt.c b/hw/arm/virt.c index a196bbf0d5..b37b0c40c1 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -1554,6 +1554,33 @@ static void machvirt_init(MachineState *machine) } } } + } else if (kvm_enabled()) { + int probe_bitmap = kvm_arm_vgic_probe(); + + if (!probe_bitmap) { + error_report( + "Unable to determine GIC version supported by host"); + exit(1); + } + if (!vms->gic_version_user_selected) { + /* + * by default v2 is supposed to be chosen: check it is + * supported by the host. Otherwise take v3. + */ + if (probe_bitmap & KVM_ARM_VGIC_V2) { + vms->gic_version = 2; + } else if (probe_bitmap & KVM_ARM_VGIC_V3) { + vms->gic_version = 3; + } + } else { /* user explicitly set the version to 2 or 3 */ + if (vms->gic_version == 2 && !(probe_bitmap & KVM_ARM_VGIC_V2)) { + error_report("GICv2 is not supported by the host"); + exit(1); + } else if (vms->gic_version == 3 && !(probe_bitmap & KVM_ARM_VGIC_V3)) { + error_report("GICv3 is not supported by the host"); + exit(1); + } + } } if (!cpu_type_valid(machine->cpu_type)) { @@ -1840,6 +1867,7 @@ static void virt_set_gic_version(Object *obj, const char *value, Error **errp) { VirtMachineState *vms = VIRT_MACHINE(obj); + vms->gic_version_user_selected = true; if (!strcmp(value, "3")) { vms->gic_version = 3; } else if (!strcmp(value, "2")) { @@ -1851,6 +1879,7 @@ static void virt_set_gic_version(Object *obj, const char *value, Error **errp) } else { error_setg(errp, "Invalid gic-version value"); error_append_hint(errp, "Valid values are 3, 2, host, max.\n"); + vms->gic_version_user_selected = false; } } @@ -2103,6 +2132,7 @@ static void virt_instance_init(Object *obj) NULL); /* Default GIC type is v2 */ vms->gic_version = 2; + vms->gic_version_user_selected = false; object_property_add_str(obj, "gic-version", virt_get_gic_version, virt_set_gic_version, NULL); object_property_set_description(obj, "gic-version", diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h index 71508bf40c..e363cde452 100644 --- a/include/hw/arm/virt.h +++ b/include/hw/arm/virt.h @@ -124,6 +124,7 @@ typedef struct { bool its; bool virt; int32_t gic_version; + bool gic_version_user_selected; VirtIOMMUType iommu; struct arm_boot_info bootinfo; MemMapEntry *memmap;
At the moment if the end-user does not specify the gic-version along with KVM acceleration, v2 is set by default. However most of the systems now have GICv3 and sometimes they do not support GICv2 compatibility. In that case we end up with this error: qemu-system-aarch64: PMU: KVM_SET_DEVICE_ATTR: Invalid argument qemu-system-aarch64: failed to set irq for PMU and qemu aborts. This patch keeps the default v2 selection in all cases except in the KVM accelerated mode when the host does not support v2. This case did not work anyway so we do not break any compatibility. Now we get v3 selected in such a case. Also if the end-user explicitly sets v2 whereas this latter is not supported, we also are informed that v2 is not selected by thos host instead of getting the above PMU related message. Signed-off-by: Eric Auger <eric.auger@redhat.com> Reported-by: Dr. David Alan Gilbert <dgilbert@redhat.com> --- hw/arm/virt.c | 30 ++++++++++++++++++++++++++++++ include/hw/arm/virt.h | 1 + 2 files changed, 31 insertions(+)