All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] hw/intc/arm_gic_kvm: Don't assume kernel can provide a GICv2
@ 2020-02-25 18:24 Peter Maydell
  2020-02-25 18:42 ` Philippe Mathieu-Daudé
  2020-02-26  8:52 ` Andrew Jones
  0 siblings, 2 replies; 7+ messages in thread
From: Peter Maydell @ 2020-02-25 18:24 UTC (permalink / raw)
  To: qemu-arm, qemu-devel; +Cc: Marc Zyngier

In our KVM GICv2 realize function, we try to cope with old kernels
that don't provide the device control API (KVM_CAP_DEVICE_CTRL): we
try to use the device control, and if that fails we fall back to
assuming that the kernel has the old style KVM_CREATE_IRQCHIP and
that it will provide a GICv2.

This doesn't cater for the possibility of a kernel and hardware which
only provide a GICv3, which is very common now.  On that setup we
will abort() later on in kvm_arm_pmu_set_irq() when we try to wire up
an interrupt to the GIC we failed to create:

qemu-system-aarch64: PMU: KVM_SET_DEVICE_ATTR: Invalid argument
qemu-system-aarch64: failed to set irq for PMU
Aborted

If the kernel advertises KVM_CAP_DEVICE_CTRL we should trust it if it
says it can't create a GICv2, rather than assuming it has one.  We
can then produce a more helpful error message including a hint about
the most probable reason for the failure.

If the kernel doesn't advertise KVM_CAP_DEVICE_CTRL then it is truly
ancient by this point but we might as well still fall back to a
KVM_CREATE_IRQCHIP GICv2.

With this patch then the user misconfiguration which previously
caused an abort now prints:
qemu-system-aarch64: Initialization of device kvm-arm-gic failed: error creating in-kernel VGIC: No such device
Perhaps the host CPU does not support GICv2?

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
I spent a while wondering if the PMU code was broken before Marc
put me on the right track about what was going wrong (ie that
I hadn't put "-machine gic-version=host" on the commandline).

 hw/intc/arm_gic_kvm.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
index 9deb15e7e69..d7df423a7a3 100644
--- a/hw/intc/arm_gic_kvm.c
+++ b/hw/intc/arm_gic_kvm.c
@@ -551,7 +551,16 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error **errp)
                               KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true,
                               &error_abort);
         }
+    } else if (kvm_check_extension(kvm_state, KVM_CAP_DEVICE_CTRL)) {
+        error_setg_errno(errp, -ret, "error creating in-kernel VGIC");
+        error_append_hint(errp,
+                          "Perhaps the host CPU does not support GICv2?\n");
     } else if (ret != -ENODEV && ret != -ENOTSUP) {
+        /*
+         * Very ancient kernel without KVM_CAP_DEVICE_CTRL: assume that
+         * ENODEV or ENOTSUP mean "can't create GICv2 with KVM_CREATE_DEVICE",
+         * and that we will get a GICv2 via KVM_CREATE_IRQCHIP.
+         */
         error_setg_errno(errp, -ret, "error creating in-kernel VGIC");
         return;
     }
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] hw/intc/arm_gic_kvm: Don't assume kernel can provide a GICv2
  2020-02-25 18:24 [PATCH] hw/intc/arm_gic_kvm: Don't assume kernel can provide a GICv2 Peter Maydell
@ 2020-02-25 18:42 ` Philippe Mathieu-Daudé
  2020-02-26  8:52 ` Andrew Jones
  1 sibling, 0 replies; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-02-25 18:42 UTC (permalink / raw)
  To: Peter Maydell, qemu-arm, qemu-devel; +Cc: Marc Zyngier

On 2/25/20 7:24 PM, Peter Maydell wrote:
> In our KVM GICv2 realize function, we try to cope with old kernels
> that don't provide the device control API (KVM_CAP_DEVICE_CTRL): we
> try to use the device control, and if that fails we fall back to
> assuming that the kernel has the old style KVM_CREATE_IRQCHIP and
> that it will provide a GICv2.
> 
> This doesn't cater for the possibility of a kernel and hardware which
> only provide a GICv3, which is very common now.  On that setup we
> will abort() later on in kvm_arm_pmu_set_irq() when we try to wire up
> an interrupt to the GIC we failed to create:
> 
> qemu-system-aarch64: PMU: KVM_SET_DEVICE_ATTR: Invalid argument
> qemu-system-aarch64: failed to set irq for PMU
> Aborted
> 
> If the kernel advertises KVM_CAP_DEVICE_CTRL we should trust it if it
> says it can't create a GICv2, rather than assuming it has one.  We
> can then produce a more helpful error message including a hint about
> the most probable reason for the failure.
> 
> If the kernel doesn't advertise KVM_CAP_DEVICE_CTRL then it is truly
> ancient by this point but we might as well still fall back to a
> KVM_CREATE_IRQCHIP GICv2.
> 
> With this patch then the user misconfiguration which previously
> caused an abort now prints:
> qemu-system-aarch64: Initialization of device kvm-arm-gic failed: error creating in-kernel VGIC: No such device
> Perhaps the host CPU does not support GICv2?
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I spent a while wondering if the PMU code was broken before Marc
> put me on the right track about what was going wrong (ie that
> I hadn't put "-machine gic-version=host" on the commandline).
> 
>   hw/intc/arm_gic_kvm.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
> index 9deb15e7e69..d7df423a7a3 100644
> --- a/hw/intc/arm_gic_kvm.c
> +++ b/hw/intc/arm_gic_kvm.c
> @@ -551,7 +551,16 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error **errp)
>                                 KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true,
>                                 &error_abort);
>           }
> +    } else if (kvm_check_extension(kvm_state, KVM_CAP_DEVICE_CTRL)) {
> +        error_setg_errno(errp, -ret, "error creating in-kernel VGIC");
> +        error_append_hint(errp,
> +                          "Perhaps the host CPU does not support GICv2?\n");
>       } else if (ret != -ENODEV && ret != -ENOTSUP) {
> +        /*
> +         * Very ancient kernel without KVM_CAP_DEVICE_CTRL: assume that
> +         * ENODEV or ENOTSUP mean "can't create GICv2 with KVM_CREATE_DEVICE",
> +         * and that we will get a GICv2 via KVM_CREATE_IRQCHIP.
> +         */
>           error_setg_errno(errp, -ret, "error creating in-kernel VGIC");
>           return;
>       }
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] hw/intc/arm_gic_kvm: Don't assume kernel can provide a GICv2
  2020-02-25 18:24 [PATCH] hw/intc/arm_gic_kvm: Don't assume kernel can provide a GICv2 Peter Maydell
  2020-02-25 18:42 ` Philippe Mathieu-Daudé
@ 2020-02-26  8:52 ` Andrew Jones
  2020-02-26  8:56   ` Peter Maydell
  1 sibling, 1 reply; 7+ messages in thread
From: Andrew Jones @ 2020-02-26  8:52 UTC (permalink / raw)
  To: Peter Maydell; +Cc: Marc Zyngier, qemu-arm, qemu-devel

On Tue, Feb 25, 2020 at 06:24:35PM +0000, Peter Maydell wrote:
> In our KVM GICv2 realize function, we try to cope with old kernels
> that don't provide the device control API (KVM_CAP_DEVICE_CTRL): we
> try to use the device control, and if that fails we fall back to
> assuming that the kernel has the old style KVM_CREATE_IRQCHIP and
> that it will provide a GICv2.
> 
> This doesn't cater for the possibility of a kernel and hardware which
> only provide a GICv3, which is very common now.  On that setup we
> will abort() later on in kvm_arm_pmu_set_irq() when we try to wire up
> an interrupt to the GIC we failed to create:
> 
> qemu-system-aarch64: PMU: KVM_SET_DEVICE_ATTR: Invalid argument
> qemu-system-aarch64: failed to set irq for PMU
> Aborted
> 
> If the kernel advertises KVM_CAP_DEVICE_CTRL we should trust it if it
> says it can't create a GICv2, rather than assuming it has one.  We
> can then produce a more helpful error message including a hint about
> the most probable reason for the failure.
> 
> If the kernel doesn't advertise KVM_CAP_DEVICE_CTRL then it is truly
> ancient by this point but we might as well still fall back to a
> KVM_CREATE_IRQCHIP GICv2.
> 
> With this patch then the user misconfiguration which previously
> caused an abort now prints:
> qemu-system-aarch64: Initialization of device kvm-arm-gic failed: error creating in-kernel VGIC: No such device
> Perhaps the host CPU does not support GICv2?
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I spent a while wondering if the PMU code was broken before Marc
> put me on the right track about what was going wrong (ie that
> I hadn't put "-machine gic-version=host" on the commandline).
> 
>  hw/intc/arm_gic_kvm.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
> index 9deb15e7e69..d7df423a7a3 100644
> --- a/hw/intc/arm_gic_kvm.c
> +++ b/hw/intc/arm_gic_kvm.c
> @@ -551,7 +551,16 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error **errp)
>                                KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true,
>                                &error_abort);
>          }
> +    } else if (kvm_check_extension(kvm_state, KVM_CAP_DEVICE_CTRL)) {
> +        error_setg_errno(errp, -ret, "error creating in-kernel VGIC");
> +        error_append_hint(errp,
> +                          "Perhaps the host CPU does not support GICv2?\n");
>      } else if (ret != -ENODEV && ret != -ENOTSUP) {
> +        /*
> +         * Very ancient kernel without KVM_CAP_DEVICE_CTRL: assume that
> +         * ENODEV or ENOTSUP mean "can't create GICv2 with KVM_CREATE_DEVICE",
> +         * and that we will get a GICv2 via KVM_CREATE_IRQCHIP.
> +         */
>          error_setg_errno(errp, -ret, "error creating in-kernel VGIC");
>          return;
>      }
> -- 
> 2.20.1
> 
>

This is nice, as some QEMU command line users may now be able to more
easily correct their command lines. So,

Reviewed-by: Andrew Jones <drjones@redhat.com>
Tested-by: Andrew Jones <drjones@redhat.com>

Although, many QEMU command line users still won't know what to do
without an explicit "Try -machine gic-version=host" hint, so that
might be nice to add too.

Also, unless our command line compatibility policy is so strict that
we can't change failing command lines (which this patch does to some
degree as it fails with a new message now), then we might as well
just probe for a working GIC version when using KVM, as old command
lines that implicitly or explicitly selected '2' never worked with
KVM on gicv3-only hosts anyway. We just have to try '2' first in
order to continue providing a gicv2 to a guests on hosts that support
both, but if that fails, then we can try '3', and if that works, then
users don't have to try and correct their command lines at all.

Thanks,
drew



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] hw/intc/arm_gic_kvm: Don't assume kernel can provide a GICv2
  2020-02-26  8:52 ` Andrew Jones
@ 2020-02-26  8:56   ` Peter Maydell
  2020-02-26  9:17     ` Andrew Jones
  0 siblings, 1 reply; 7+ messages in thread
From: Peter Maydell @ 2020-02-26  8:56 UTC (permalink / raw)
  To: Andrew Jones; +Cc: Marc Zyngier, qemu-arm, QEMU Developers

On Wed, 26 Feb 2020 at 08:52, Andrew Jones <drjones@redhat.com> wrote:
> Although, many QEMU command line users still won't know what to do
> without an explicit "Try -machine gic-version=host" hint, so that
> might be nice to add too.

In the GIC code we don't know if the machine even has a
gic-version property, so we're not in the right place to try to
produce that message.

thanks
-- PMM


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] hw/intc/arm_gic_kvm: Don't assume kernel can provide a GICv2
  2020-02-26  8:56   ` Peter Maydell
@ 2020-02-26  9:17     ` Andrew Jones
  2020-02-26 11:58       ` Auger Eric
  0 siblings, 1 reply; 7+ messages in thread
From: Andrew Jones @ 2020-02-26  9:17 UTC (permalink / raw)
  To: Peter Maydell; +Cc: Marc Zyngier, qemu-arm, QEMU Developers

On Wed, Feb 26, 2020 at 08:56:03AM +0000, Peter Maydell wrote:
> On Wed, 26 Feb 2020 at 08:52, Andrew Jones <drjones@redhat.com> wrote:
> > Although, many QEMU command line users still won't know what to do
> > without an explicit "Try -machine gic-version=host" hint, so that
> > might be nice to add too.
> 
> In the GIC code we don't know if the machine even has a
> gic-version property, so we're not in the right place to try to
> produce that message.
>

Ah yes, we use qdev_init_nofail() in virt::create_gic(), so there's
no chance to append another hint at the machine level.

And what about when machine.gic-version is not provided and KVM is
in use? Shouldn't we try version '2', as we do now, but then also
'3', if '2' fails, before erroring out?

Thanks,
drew



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] hw/intc/arm_gic_kvm: Don't assume kernel can provide a GICv2
  2020-02-26  9:17     ` Andrew Jones
@ 2020-02-26 11:58       ` Auger Eric
  2020-02-26 17:09         ` Auger Eric
  0 siblings, 1 reply; 7+ messages in thread
From: Auger Eric @ 2020-02-26 11:58 UTC (permalink / raw)
  To: Andrew Jones, Peter Maydell; +Cc: Marc Zyngier, qemu-arm, QEMU Developers

Hi Peter,

On 2/26/20 10:17 AM, Andrew Jones wrote:
> On Wed, Feb 26, 2020 at 08:56:03AM +0000, Peter Maydell wrote:
>> On Wed, 26 Feb 2020 at 08:52, Andrew Jones <drjones@redhat.com> wrote:
>>> Although, many QEMU command line users still won't know what to do
>>> without an explicit "Try -machine gic-version=host" hint, so that
>>> might be nice to add too.
>>
>> In the GIC code we don't know if the machine even has a
>> gic-version property, so we're not in the right place to try to
>> produce that message.
>>
> 
> Ah yes, we use qdev_init_nofail() in virt::create_gic(), so there's
> no chance to append another hint at the machine level.
> 
> And what about when machine.gic-version is not provided and KVM is
> in use? Shouldn't we try version '2', as we do now, but then also
> '3', if '2' fails, before erroring out?

In case of KVM accelerated mode we could effectively probe v2 first and
if not supported choose v3, as mentioned by Drew.

Couldn't kvm_arm_vgic_probe() return a bitmap by calling
kvm_create_device on both versions in dryrun mode?

Thanks

Eric

> 
> Thanks,
> drew
> 
> 



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] hw/intc/arm_gic_kvm: Don't assume kernel can provide a GICv2
  2020-02-26 11:58       ` Auger Eric
@ 2020-02-26 17:09         ` Auger Eric
  0 siblings, 0 replies; 7+ messages in thread
From: Auger Eric @ 2020-02-26 17:09 UTC (permalink / raw)
  To: Andrew Jones, Peter Maydell; +Cc: Marc Zyngier, qemu-arm, QEMU Developers

Hi Peter,

On 2/26/20 12:58 PM, Auger Eric wrote:
> Hi Peter,
> 
> On 2/26/20 10:17 AM, Andrew Jones wrote:
>> On Wed, Feb 26, 2020 at 08:56:03AM +0000, Peter Maydell wrote:
>>> On Wed, 26 Feb 2020 at 08:52, Andrew Jones <drjones@redhat.com> wrote:
>>>> Although, many QEMU command line users still won't know what to do
>>>> without an explicit "Try -machine gic-version=host" hint, so that
>>>> might be nice to add too.
>>>
>>> In the GIC code we don't know if the machine even has a
>>> gic-version property, so we're not in the right place to try to
>>> produce that message.
>>>
>>
>> Ah yes, we use qdev_init_nofail() in virt::create_gic(), so there's
>> no chance to append another hint at the machine level.
>>
>> And what about when machine.gic-version is not provided and KVM is
>> in use? Shouldn't we try version '2', as we do now, but then also
>> '3', if '2' fails, before erroring out?
> 
> In case of KVM accelerated mode we could effectively probe v2 first and
> if not supported choose v3, as mentioned by Drew.
> 
> Couldn't kvm_arm_vgic_probe() return a bitmap by calling
> kvm_create_device on both versions in dryrun mode?

I sent an RFC prototyping what we had in mind - I think - together with
Drew.

[RFC 0/2] hw/arm/virt: kvm: allow gicv3 by default if host does not
support v2

This was discussed earlier in that thread:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg674469.html.

It should not break any compatibility as the only case we are supposed
to change here was aborting before.

Thanks

Eric
> 
> Thanks
> 
> Eric
> 
>>
>> Thanks,
>> drew
>>
>>



^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-02-26 17:10 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-25 18:24 [PATCH] hw/intc/arm_gic_kvm: Don't assume kernel can provide a GICv2 Peter Maydell
2020-02-25 18:42 ` Philippe Mathieu-Daudé
2020-02-26  8:52 ` Andrew Jones
2020-02-26  8:56   ` Peter Maydell
2020-02-26  9:17     ` Andrew Jones
2020-02-26 11:58       ` Auger Eric
2020-02-26 17:09         ` Auger Eric

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.