Re: [PATCH] arm64: dts: Fix GIC reg sizes for APM X-Gene

[Pranavkumar Sawargaonkar <psawargaonkar@apm.com>]

Hi Rob,

On Mon, Feb 23, 2015 at 10:09 PM, Rob Herring <robherring2@gmail.com> wrote:
> On Mon, Feb 23, 2015 at 6:07 AM, Christoffer Dall
> <christoffer.dall@linaro.org> wrote:
>> On Sat, Feb 21, 2015 at 03:56:17PM -0600, Rob Herring wrote:
>>> On Thu, Feb 19, 2015 at 1:03 PM, Christoffer Dall
>>> <christoffer.dall@linaro.org> wrote:
>>> > On Thu, Feb 19, 2015 at 12:23:15PM -0600, Rob Herring wrote:
>>> >> On Tue, Jan 27, 2015 at 1:03 AM, Pranavkumar Sawargaonkar
>>> >> <psawargaonkar@apm.com> wrote:
>>> >> > In APM X-Gene, GIC register space is 64K aligned while the sizes mentioned
>>> >> > in the dt are 4K aligned. This breaks KVM when kernel is built with 64K page
>>> >> > size due to size alignment checking in vgic driver for VCPU Control and
>>> >> > VCPU register.
>>> >> >
>>> >> > This patch corrects the sizes to be inline with the hardware spec.
>>> >>
>>> >> This does not make sense. The GIC regions are still only 4 or 8KB and
>>> >> the h/w description should reflect that. For implementations using
>>> >> gic-400 and the addressing decode trick, the rest of the register
>>> >> range is also not safe to access given it is multiple mapped. Also,
>>> >> this wastes virtual space, but I guess we don't care on 64-bit.
>>> >>
>>> >> KVM should be fixed to only check base address alignment. Size
>>> >> alignment does not matter (if it does, then you need to fix all
>>> >> register blocks).
>>> >>
>>> > It matters if you want to ensure that the 64K page you are assigning to
>>> > a guest for the GIC virtual CPU interface contains only GIC virtual CPU
>>> > mappings, and not other random stuff that the guest is not allowed to
>>> > touch.
>>>
>>> Good point.
>>>
>>> > How else should this be enforced?
>>>
>>> Rely on correct h/w design? You'll have to repeat this every time you
>>> want to do pass-thru of a device.
>>>
>>> What do you do if 64K mapping is not supported? Fallback to emulation
>>> of the CPU interface?
>>
>> Agree with Peter on these two points.
>>
>>>
>>> Are there other DTSs that need to be fixed?
>>>
>> Not sure really, AMD Seattle works with 64K pages IIRC.
>
> Well, looks we have been inconsistent here:
>
> arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi-           reg = <0x0
> 0xe1110000 0 0x1000>,
> arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi-                 <0x0
> 0xe112f000 0 0x2000>,
> arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi-                 <0x0
> 0xe1140000 0 0x10000>,
> arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi-                 <0x0
> 0xe1160000 0 0x10000>;
>
> arch/arm64/boot/dts/arm/juno.dts-               reg = <0x0 0x2c010000 0 0x1000>,
> arch/arm64/boot/dts/arm/juno.dts-                     <0x0 0x2c02f000 0 0x2000>,
> arch/arm64/boot/dts/arm/juno.dts-                     <0x0 0x2c04f000 0 0x2000>,
> arch/arm64/boot/dts/arm/juno.dts-                     <0x0 0x2c06f000 0 0x2000>;
>
> If we are going to use 64K sizes, can we have some consistency here
> please. Which ranges really need 64KB sizes? It should only be the
> VCPU interface. right? Why does XGene need 128K? If XGene is doing
> address swizzling, then the CPU and VCPU base addresses are wrong.
> Seattle is also wrong for the VCPU, but no one has noticed because we
> don't use the DIR register IIRC.
>
> XGene should also add an "arm,gic-400" compatible string or something
> XGene specific if in fact it is not GIC-400.

X-Gene has gic-400 as an interrupt controller.
Only thing is GIC pages are mapped at 64K boundary (with 64K page size)
Hence CPU, VCPU interfaces has a size of 128K (2GIC pages)
Regarding GICC_DIR, yes there is a problem which needs to be solved
since the first page size is 64K.
In XEN we already have a small fix to access GICC_DIR with 64K page
offset instead of standard 4K.
I remember a small discussion in this regard in past
(http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/266468.html)
which was deferred at that time.
Once this patch is accepted we can post RFC patch to address GICC_DIR
and discuss further.

>
> I think perhaps we need a specific compatible property to indicate a
> GIC-400 with address swizzling. While we could get away with using the
> aliased addresses, that seems to be hard to get right and we may
> regret not doing it in the long term. It would indicate at least it is
> 64K page safe for example.
>
> Rob

Thanks,
Pranav