[Qemu-devel] [PATCH] target/arm/kvm: gic: Prevent creating userspace GICv3 with KVM

[Qemu-devel] [PATCH] target/arm/kvm: gic: Prevent creating userspace GICv3 with KVM

[Christoffer Dall]

KVM doesn't support emulating a GICv3 in userspace, only GICv2.  We
currently attempt this anyway, and as a result a KVM guest doesn't
receive interrupts and the user is left wondering why.  Report an error
to the user if this particular combination is requested.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 target/arm/kvm_arm.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
index ff53e9fafb..cfb7e5af72 100644
--- a/target/arm/kvm_arm.h
+++ b/target/arm/kvm_arm.h
@@ -234,6 +234,10 @@ static inline const char *gicv3_class_name(void)
         exit(1);
 #endif
     } else {
+        if (kvm_enabled()) {
+            error_report("Userspace GICv3 is not supported with KVM");
+            exit(1);
+        }
         return "arm-gicv3";
     }
 }
-- 
2.14.2

Re: [Qemu-devel] [PATCH] target/arm/kvm: gic: Prevent creating userspace GICv3 with KVM

[Philippe Mathieu-Daudé]

[-- Attachment #1: Type: text/plain, Size: 1554 bytes --]

Hi Christoffer,

On 02/01/2018 05:53 PM, Christoffer Dall wrote:
> KVM doesn't support emulating a GICv3 in userspace, only GICv2.  We
> currently attempt this anyway, and as a result a KVM guest doesn't
> receive interrupts and the user is left wondering why.  Report an error
> to the user if this particular combination is requested.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  target/arm/kvm_arm.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
> index ff53e9fafb..cfb7e5af72 100644
> --- a/target/arm/kvm_arm.h
> +++ b/target/arm/kvm_arm.h
> @@ -234,6 +234,10 @@ static inline const char *gicv3_class_name(void)

Any reason why these functions are inlined in this include?

Also - not related to your patch - while gicv3_class_name() is
self-explicit, gic_class_name() isn't (to me at least).

There are many check for gic_version 1/2/3 in virt.c, which we could
clean using more generic functions such gic_class_name(int gic_version)
and Co.

Alistair/Edgar:
Why the ZynqMP is not using the GICv3 for the APU? I remember hearing
something about not fully implemented so not working correctly.

>          exit(1);
>  #endif
>      } else {
> +        if (kvm_enabled()) {
> +            error_report("Userspace GICv3 is not supported with KVM");
> +            exit(1);

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> +        }
>          return "arm-gicv3";
>      }
>  }
> 

Regards,

Phil.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH] target/arm/kvm: gic: Prevent creating userspace GICv3 with KVM

[Alistair Francis]

On Mon, Feb 5, 2018 at 4:10 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi Christoffer,
>
> On 02/01/2018 05:53 PM, Christoffer Dall wrote:
>> KVM doesn't support emulating a GICv3 in userspace, only GICv2.  We
>> currently attempt this anyway, and as a result a KVM guest doesn't
>> receive interrupts and the user is left wondering why.  Report an error
>> to the user if this particular combination is requested.
>>
>> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
>> ---
>>  target/arm/kvm_arm.h | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
>> index ff53e9fafb..cfb7e5af72 100644
>> --- a/target/arm/kvm_arm.h
>> +++ b/target/arm/kvm_arm.h
>> @@ -234,6 +234,10 @@ static inline const char *gicv3_class_name(void)
>
> Any reason why these functions are inlined in this include?
>
> Also - not related to your patch - while gicv3_class_name() is
> self-explicit, gic_class_name() isn't (to me at least).
>
> There are many check for gic_version 1/2/3 in virt.c, which we could
> clean using more generic functions such gic_class_name(int gic_version)
> and Co.
>
> Alistair/Edgar:
> Why the ZynqMP is not using the GICv3 for the APU? I remember hearing
> something about not fully implemented so not working correctly.

The ZynqMP only has a GICv2.

Maybe you are thinking of the virtualisation features? Mainline
doesn't have those fully implemented so they don't work correctly.

Alistair

>
>>          exit(1);
>>  #endif
>>      } else {
>> +        if (kvm_enabled()) {
>> +            error_report("Userspace GICv3 is not supported with KVM");
>> +            exit(1);
>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
>> +        }
>>          return "arm-gicv3";
>>      }
>>  }
>>
>
> Regards,
>
> Phil.
>

Re: [Qemu-devel] [PATCH] target/arm/kvm: gic: Prevent creating userspace GICv3 with KVM

[Christoffer Dall]

[-- Attachment #1: Type: text/plain, Size: 1997 bytes --]

Hi Philippe,

On Mon, Feb 05, 2018 at 09:10:48PM -0300, Philippe Mathieu-Daudé wrote:
> 
> On 02/01/2018 05:53 PM, Christoffer Dall wrote:
> > KVM doesn't support emulating a GICv3 in userspace, only GICv2.  We
> > currently attempt this anyway, and as a result a KVM guest doesn't
> > receive interrupts and the user is left wondering why.  Report an error
> > to the user if this particular combination is requested.
> > 
> > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> > ---
> >  target/arm/kvm_arm.h | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
> > index ff53e9fafb..cfb7e5af72 100644
> > --- a/target/arm/kvm_arm.h
> > +++ b/target/arm/kvm_arm.h
> > @@ -234,6 +234,10 @@ static inline const char *gicv3_class_name(void)
> 
> Any reason why these functions are inlined in this include?

Not really sure what you mean?  What do you think would be a better
approach?

> 
> Also - not related to your patch - while gicv3_class_name() is
> self-explicit, gic_class_name() isn't (to me at least).

I think I tend to see 'gic' as GICv2 and 'gicv3' as GICv3 in QEMU, but I
don't remember the details of how you make the GIC be a proper GICv2 vs.
the 11mpcore version thingy.

> 
> There are many check for gic_version 1/2/3 in virt.c, which we could
> clean using more generic functions such gic_class_name(int gic_version)
> and Co.

Possibly.  Do note, that checking "which version of the GIC does this
board contain" is a separate thing from "which particular mechanism do I
use to emulate the GIC".

> 
> >          exit(1);
> >  #endif
> >      } else {
> > +        if (kvm_enabled()) {
> > +            error_report("Userspace GICv3 is not supported with KVM");
> > +            exit(1);
> 
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> > +        }
> >          return "arm-gicv3";
> >      }
> >  }
> > 
> 
Thanks!
-Christoffer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH] target/arm/kvm: gic: Prevent creating userspace GICv3 with KVM

[Peter Maydell]

On 1 February 2018 at 20:53, Christoffer Dall
<christoffer.dall@linaro.org> wrote:
> KVM doesn't support emulating a GICv3 in userspace, only GICv2.  We
> currently attempt this anyway, and as a result a KVM guest doesn't
> receive interrupts and the user is left wondering why.  Report an error
> to the user if this particular combination is requested.
>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  target/arm/kvm_arm.h | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
> index ff53e9fafb..cfb7e5af72 100644
> --- a/target/arm/kvm_arm.h
> +++ b/target/arm/kvm_arm.h
> @@ -234,6 +234,10 @@ static inline const char *gicv3_class_name(void)
>          exit(1);
>  #endif
>      } else {
> +        if (kvm_enabled()) {
> +            error_report("Userspace GICv3 is not supported with KVM");
> +            exit(1);
> +        }
>          return "arm-gicv3";
>      }



Applied to target-arm.next, thanks.

-- PMM