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

[Pranavkumar Sawargaonkar]

Hi Rob,

On Tue, Feb 24, 2015 at 8:00 PM, Rob Herring <robherring2 at gmail.com> wrote:
> On Tue, Feb 24, 2015 at 12:34 AM, Pranavkumar Sawargaonkar
> <psawargaonkar at apm.com> wrote:
>> Hi Rob,
>>
>> On Mon, Feb 23, 2015 at 10:09 PM, Rob Herring <robherring2 at gmail.com> wrote:
>>> On Mon, Feb 23, 2015 at 6:07 AM, Christoffer Dall
>>> <christoffer.dall at 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 at 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 at 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.
>
> Right, and in order for this to work, you should use the last 4K alias
> for the cpu interface(s). This is why other platforms use xxxf000 as
> their cpu interface base.
>
> It is of course possible that xgene does not properly do the address
> swizzling and therefore you have to use 64K aligned addresses. But in
> that case you need a unique compatible string.
>
>> 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.
>
> No, let's get this right now and not keep changing the dts.
>

So should we add some string specific to apm/xgnene (something like
apm,cortex-a15-gic) or specific to 64K GIC page size
(arm,cortex-a15-gic-64Kpg) ?
Also till 3.19, I am not sure if any code is accessing GICC_DIR so for
now only thing which seems to be needed is a new dt string for 64K gic
pages.


Thanks,
Pranav

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