Re: [PATCH] kvm: arm64: vgic: fix hyp panic with 64k pages on juno platform

[Will Deacon <will.deacon@arm.com>]

Hi Joel,

On Fri, Jul 25, 2014 at 03:02:58PM +0100, Joel Schopp wrote:
> >>>> I can't think of any way of determining whether a particular
> >>>> system gets this right or wrong automatically, which suggests
> >>>> perhaps we need to allow the device tree to specify that the
> >>>> GICV is 64k-page-safe...
> >>> When we support such systems, I also think we'll need a device-tree change.
> >>> My main concern right now is stopping the ability to hose the entire machine
> >>> by trying to instantiate a virtual GIC.
> >> ...I don't see how your patch prevents instantiating a VGIC
> >> and hosing the machine on a system where the 64K
> >> with the GICV registers in it goes
> >>  [GICV registers] [machine blows up if you read this]
> >>  0K                      8K                                             64K
> > True, if such a machine existed, then this patch wouldn't detect it. I don't
> > think we support anything like that in mainline at the moment, but the
> > following additional diff should solve the problem, no?
> >
> > diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> > index fa9a95b3ed19..476d3bf540a8 100644
> > --- a/virt/kvm/arm/vgic.c
> > +++ b/virt/kvm/arm/vgic.c
> > @@ -1539,6 +1539,14 @@ int kvm_vgic_hyp_init(void)
> >                 goto out_unmap;
> >         }
> >  
> > +       if (!PAGE_ALIGNED(resource_size(&vcpu_res))) {
> > +               kvm_err("GICV size 0x%llx not a multiple of page size 0x%lx\n",
> > +                       (unsigned long long)resource_size(&vcpu_res),
> > +                       PAGE_SIZE);
> > +               ret = -ENXIO;
> > +               goto out_unmap;
> > +       }
> > +
> >         vgic_vcpu_base = vcpu_res.start;
> >  
> >         kvm_info("%s@%llx IRQ%d\n", vgic_node->name,
> This would break with my SOC device tree which looks like this.  Note
> this device tree works just fine without checks.
> 
>     gic: interrupt-controller@e1101000 {
>         compatible = "arm,gic-400-v2m";
>         #interrupt-cells = <3>;
>         #address-cells = <0>;
>         interrupt-controller;
>         msi-controller;
>         reg = <0x0 0xe1110000 0 0x1000>, /* gic dist */
>               <0x0 0xe112f000 0 0x2000>, /* gic cpu */
>               <0x0 0xe114f000 0 0x2000>, /* gic virtual ic*/
>               <0x0 0xe116f000 0 0x2000>, /* gic virtual cpu*/
>               <0x0 0xe1180000 0 0x1000>; /* gic msi */
>         interrupts = <1 8 0xf04>;
>     };

I appreciate it may work, but that's only because the kernel is actually
using an alias of GICV at 0xe1160000 by accident. I would say that you're
getting away with passing an incorrect description.

Will