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

[Peter Maydell]

On Tue, 10 Mar 2020 at 18:02, Auger Eric <address@hidden> wrote:
>
> Hi Peter,
>
> On 3/9/20 2:28 PM, Peter Maydell wrote:
> > On Mon, 2 Mar 2020 at 10:55, Eric Auger <address@hidden> 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 <address@hidden>
> >> Reviewed-by: Richard Henderson <address@hidden>
> >> Reviewed-by: Andrew Jones <address@hidden>
> >>
> >> ---
> >>
> >> 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().

Yes, but your change here makes it call kvm_arm_vgic_probe()
even if the gic_version was explicitly set to 2 or 3
by the user, doesn't it ? That's what I was commenting on.

> 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?

I think:
(1) we should retain the current behaviour that if the user
asked for userspace-irqchip and specifically chose gic
version 2, then we don't probe the kernel for its capabilities,
we just create a userspace gicv2.

(2) we should also retain the current behaviour that we
default to "2" if the user requests userspace-irqchip but
doesn't specify the gic-version.

(3) the ideal-world behaviour would be that we correctly
handle the user asking for userspace-irqchip plus either "max"
(choose '2') or "3" (produce a useful error message if we
don't do so already). I'm not sure what "host" should mean
here. But this point (3) is separate from what this series is
doing, I think, and is basically fixing the bug that we didn't
think about the userspace-irqchip case when we implemented
"host" and "max" or when we added GICv3 support.

thanks
-- PMM