Re: [PATCH v3 5/6] hw/arm/virt: kvm: Check the chosen gic version is supported by the host

[Auger Eric <eric.auger@redhat.com>]

Hi Peter,

On 3/9/20 2:28 PM, Peter Maydell wrote:
> On Mon, 2 Mar 2020 at 10:55, Eric Auger <eric.auger@redhat.com> wrote:
>>
>> Restructure the finalize_gic_version with switch cases and, in
>> KVM mode, explictly check whether the chosen version is supported
>> by the host.
>>
>> if the end-user explicitly sets v2/v3 and this is not supported by
>> the host, then the user gets an explicit error message.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> Reviewed-by: Andrew Jones <drjones@redhat.com>
>>
>> ---
>>
>> v2 -> v3:
>> - explictly list V2 and V3 in the switch/case
>> - fix indent
>> ---
>>  hw/arm/virt.c | 77 +++++++++++++++++++++++++++++++++++----------------
>>  1 file changed, 53 insertions(+), 24 deletions(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index eb8c57c85e..aeb6c45e51 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -1542,33 +1542,62 @@ static void virt_set_memmap(VirtMachineState *vms)
>>   */
>>  static void finalize_gic_version(VirtMachineState *vms)
>>  {
>> -    if (vms->gic_version == VIRT_GIC_VERSION_HOST ||
>> -        vms->gic_version == VIRT_GIC_VERSION_MAX) {
>> -        if (!kvm_enabled()) {
>> -            if (vms->gic_version == VIRT_GIC_VERSION_HOST) {
>> -                error_report("gic-version=host requires KVM");
>> -                exit(1);
>> -            } else {
>> -                /* "max": currently means 3 for TCG */
>> -                vms->gic_version = VIRT_GIC_VERSION_3;
>> -            }
>> -        } else {
>> -            int probe_bitmap = kvm_arm_vgic_probe();
>> +    if (kvm_enabled()) {
>> +        int probe_bitmap = kvm_arm_vgic_probe();
> 
> Previously we would only do kvm_arm_vgic_probe() if the
> user asked for 'host' or 'max'. Now we do it always,
> which means that if the user is on a really old kernel
> where the CREATE_DEVICE ioctl doesn't exist then we
> will now fail if the user specifically asked for gicv2,
> where previously we (probably) would have succeeded.
> I don't think we should put too much weight on continuing
> to theoretically support ancient kernels which we're not
> actually testing against, but it does seem a bit odd to
> probe even if we don't need to know the answer.
> 
> More relevant to actual plausible use cases, if
> kvm_irqchip_in_kernel() == false, we shouldn't be
> probing the kernel to ask what kind of GIC to use.
I think the existing code also does the same:
kvm_arm_vgic_probe() gets called as soon as vms->gic_version <= 0 &&
kvm_enabled() whatever the state of kvm_irqchip_in_kernel().

So in case the host only supports GICv2, in kvm mode with userspace
irqchip we would use GICV2 in host/max mode. If host supports GICv3 we
would select GICv3 which is not supported in !kvm_irqchip_in_kernel().

So do I understand correctly that you want me to change that behavior
and always set v2 in KVM/!kvm_irqchip_in_kernel() max/host mode?

Thanks

Eric

> 
> thanks
> -- PMM
>