[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC 2/2] hw/arm/virt: kvm: allow gicv3 by default if host does not
From: |
Auger Eric |
Subject: |
Re: [RFC 2/2] hw/arm/virt: kvm: allow gicv3 by default if host does not support v2 |
Date: |
Thu, 27 Feb 2020 09:12:05 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 |
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 <address@hidden>
>> Reported-by: Dr. David Alan Gilbert <address@hidden>
>> ---
>> 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
>
>