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

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

[Pranavkumar Sawargaonkar]

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.

CC: linux-arm-kernel at lists.infradead.org
CC: kvmarm at lists.cs.columbia.edu
CC: arnd at arndb.de
CC: marc.zyngier at arm.com
CC: christoffer.dall at linaro.org
CC: jcm at redhat.com
Signed-off-by: Pranavkumar Sawargaonkar <psawargaonkar@apm.com>
Signed-off-by: Tushar Jagad <tjagad@apm.com>
Signed-off-by: Feng Kan <fkan@apm.com>
---
 arch/arm64/boot/dts/apm/apm-storm.dtsi |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/boot/dts/apm/apm-storm.dtsi b/arch/arm64/boot/dts/apm/apm-storm.dtsi
index f1ad9c2..65f0e6d 100644
--- a/arch/arm64/boot/dts/apm/apm-storm.dtsi
+++ b/arch/arm64/boot/dts/apm/apm-storm.dtsi
@@ -81,10 +81,10 @@
 		compatible = "arm,cortex-a15-gic";
 		#interrupt-cells = <3>;
 		interrupt-controller;
-		reg = <0x0 0x78010000 0x0 0x1000>,	/* GIC Dist */
-		      <0x0 0x78020000 0x0 0x1000>,	/* GIC CPU */
-		      <0x0 0x78040000 0x0 0x2000>,	/* GIC VCPU Control */
-		      <0x0 0x78060000 0x0 0x2000>;	/* GIC VCPU */
+		reg = <0x0 0x78010000 0x0 0x10000>,	/* GIC Dist */
+		      <0x0 0x78020000 0x0 0x20000>,	/* GIC CPU */
+		      <0x0 0x78040000 0x0 0x10000>,	/* GIC VCPU Control */
+		      <0x0 0x78060000 0x0 0x20000>;	/* GIC VCPU */
 		interrupts = <1 9 0xf04>;	/* GIC Maintenence IRQ */
 	};

--
1.7.9.5

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

[Jon Masters]

On 01/27/2015 02:03 AM, Pranavkumar Sawargaonkar 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.
> 
> CC: linux-arm-kernel at lists.infradead.org
> CC: kvmarm at lists.cs.columbia.edu
> CC: arnd at arndb.de
> CC: marc.zyngier at arm.com
> CC: christoffer.dall at linaro.org
> CC: jcm at redhat.com
> Signed-off-by: Pranavkumar Sawargaonkar <psawargaonkar@apm.com>
> Signed-off-by: Tushar Jagad <tjagad@apm.com>
> Signed-off-by: Feng Kan <fkan@apm.com>
> ---
>  arch/arm64/boot/dts/apm/apm-storm.dtsi |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/apm/apm-storm.dtsi b/arch/arm64/boot/dts/apm/apm-storm.dtsi
> index f1ad9c2..65f0e6d 100644
> --- a/arch/arm64/boot/dts/apm/apm-storm.dtsi
> +++ b/arch/arm64/boot/dts/apm/apm-storm.dtsi
> @@ -81,10 +81,10 @@
>  		compatible = "arm,cortex-a15-gic";
>  		#interrupt-cells = <3>;
>  		interrupt-controller;
> -		reg = <0x0 0x78010000 0x0 0x1000>,	/* GIC Dist */
> -		      <0x0 0x78020000 0x0 0x1000>,	/* GIC CPU */
> -		      <0x0 0x78040000 0x0 0x2000>,	/* GIC VCPU Control */
> -		      <0x0 0x78060000 0x0 0x2000>;	/* GIC VCPU */
> +		reg = <0x0 0x78010000 0x0 0x10000>,	/* GIC Dist */
> +		      <0x0 0x78020000 0x0 0x20000>,	/* GIC CPU */
> +		      <0x0 0x78040000 0x0 0x10000>,	/* GIC VCPU Control */
> +		      <0x0 0x78060000 0x0 0x20000>;	/* GIC VCPU */
>  		interrupts = <1 9 0xf04>;	/* GIC Maintenence IRQ */
>  	};

Thanks. I confirm that we have tested this.

Jon.

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

[Pranavkumar Sawargaonkar]

Hi,

On Tue, Jan 27, 2015 at 3:02 PM, Jon Masters <jcm@redhat.com> wrote:
> On 01/27/2015 02:03 AM, Pranavkumar Sawargaonkar 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.
>>
>> CC: linux-arm-kernel at lists.infradead.org
>> CC: kvmarm at lists.cs.columbia.edu
>> CC: arnd at arndb.de
>> CC: marc.zyngier at arm.com
>> CC: christoffer.dall at linaro.org
>> CC: jcm at redhat.com
>> Signed-off-by: Pranavkumar Sawargaonkar <psawargaonkar@apm.com>
>> Signed-off-by: Tushar Jagad <tjagad@apm.com>
>> Signed-off-by: Feng Kan <fkan@apm.com>
>> ---
>>  arch/arm64/boot/dts/apm/apm-storm.dtsi |    8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/apm/apm-storm.dtsi b/arch/arm64/boot/dts/apm/apm-storm.dtsi
>> index f1ad9c2..65f0e6d 100644
>> --- a/arch/arm64/boot/dts/apm/apm-storm.dtsi
>> +++ b/arch/arm64/boot/dts/apm/apm-storm.dtsi
>> @@ -81,10 +81,10 @@
>>               compatible = "arm,cortex-a15-gic";
>>               #interrupt-cells = <3>;
>>               interrupt-controller;
>> -             reg = <0x0 0x78010000 0x0 0x1000>,      /* GIC Dist */
>> -                   <0x0 0x78020000 0x0 0x1000>,      /* GIC CPU */
>> -                   <0x0 0x78040000 0x0 0x2000>,      /* GIC VCPU Control */
>> -                   <0x0 0x78060000 0x0 0x2000>;      /* GIC VCPU */
>> +             reg = <0x0 0x78010000 0x0 0x10000>,     /* GIC Dist */
>> +                   <0x0 0x78020000 0x0 0x20000>,     /* GIC CPU */
>> +                   <0x0 0x78040000 0x0 0x10000>,     /* GIC VCPU Control */
>> +                   <0x0 0x78060000 0x0 0x20000>;     /* GIC VCPU */
>>               interrupts = <1 9 0xf04>;       /* GIC Maintenence IRQ */
>>       };

Any comments on this patch ?

>
> Thanks. I confirm that we have tested this.
>
> Jon.
>
>

Thanks,
Pranav

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

[Christoffer Dall]

On Tue, Jan 27, 2015 at 12:33:26PM +0530, Pranavkumar Sawargaonkar 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.
> 
> CC: linux-arm-kernel at lists.infradead.org
> CC: kvmarm at lists.cs.columbia.edu
> CC: arnd at arndb.de
> CC: marc.zyngier at arm.com
> CC: christoffer.dall at linaro.org
> CC: jcm at redhat.com
> Signed-off-by: Pranavkumar Sawargaonkar <psawargaonkar@apm.com>
> Signed-off-by: Tushar Jagad <tjagad@apm.com>
> Signed-off-by: Feng Kan <fkan@apm.com>
> ---
>  arch/arm64/boot/dts/apm/apm-storm.dtsi |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/apm/apm-storm.dtsi b/arch/arm64/boot/dts/apm/apm-storm.dtsi
> index f1ad9c2..65f0e6d 100644
> --- a/arch/arm64/boot/dts/apm/apm-storm.dtsi
> +++ b/arch/arm64/boot/dts/apm/apm-storm.dtsi
> @@ -81,10 +81,10 @@
>  		compatible = "arm,cortex-a15-gic";
>  		#interrupt-cells = <3>;
>  		interrupt-controller;
> -		reg = <0x0 0x78010000 0x0 0x1000>,	/* GIC Dist */
> -		      <0x0 0x78020000 0x0 0x1000>,	/* GIC CPU */
> -		      <0x0 0x78040000 0x0 0x2000>,	/* GIC VCPU Control */
> -		      <0x0 0x78060000 0x0 0x2000>;	/* GIC VCPU */
> +		reg = <0x0 0x78010000 0x0 0x10000>,	/* GIC Dist */
> +		      <0x0 0x78020000 0x0 0x20000>,	/* GIC CPU */
> +		      <0x0 0x78040000 0x0 0x10000>,	/* GIC VCPU Control */
> +		      <0x0 0x78060000 0x0 0x20000>;	/* GIC VCPU */
>  		interrupts = <1 9 0xf04>;	/* GIC Maintenence IRQ */
>  	};
> 
> --
> 1.7.9.5
> 

This looks good to me and works with 4K and 64K pages on the mustang I
have at hand.

Acked-by: Christoffer Dall <christoffer.dall@linaro.org>

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

[Rob Herring]

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).

Rob

>
> CC: linux-arm-kernel at lists.infradead.org
> CC: kvmarm at lists.cs.columbia.edu
> CC: arnd at arndb.de
> CC: marc.zyngier at arm.com
> CC: christoffer.dall at linaro.org
> CC: jcm at redhat.com
> Signed-off-by: Pranavkumar Sawargaonkar <psawargaonkar@apm.com>
> Signed-off-by: Tushar Jagad <tjagad@apm.com>
> Signed-off-by: Feng Kan <fkan@apm.com>
> ---
>  arch/arm64/boot/dts/apm/apm-storm.dtsi |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/apm/apm-storm.dtsi b/arch/arm64/boot/dts/apm/apm-storm.dtsi
> index f1ad9c2..65f0e6d 100644
> --- a/arch/arm64/boot/dts/apm/apm-storm.dtsi
> +++ b/arch/arm64/boot/dts/apm/apm-storm.dtsi
> @@ -81,10 +81,10 @@
>                 compatible = "arm,cortex-a15-gic";
>                 #interrupt-cells = <3>;
>                 interrupt-controller;
> -               reg = <0x0 0x78010000 0x0 0x1000>,      /* GIC Dist */
> -                     <0x0 0x78020000 0x0 0x1000>,      /* GIC CPU */
> -                     <0x0 0x78040000 0x0 0x2000>,      /* GIC VCPU Control */
> -                     <0x0 0x78060000 0x0 0x2000>;      /* GIC VCPU */
> +               reg = <0x0 0x78010000 0x0 0x10000>,     /* GIC Dist */
> +                     <0x0 0x78020000 0x0 0x20000>,     /* GIC CPU */
> +                     <0x0 0x78040000 0x0 0x10000>,     /* GIC VCPU Control */
> +                     <0x0 0x78060000 0x0 0x20000>;     /* GIC VCPU */
>                 interrupts = <1 9 0xf04>;       /* GIC Maintenence IRQ */
>         };
>
> --
> 1.7.9.5
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Christoffer Dall]

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.

How else should this be enforced?

-Christoffer

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

[Rob Herring]

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?

Are there other DTSs that need to be fixed?

Rob

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

[Peter Maydell]

On 22 February 2015 at 06:56, Rob Herring <robherring2@gmail.com> wrote:
> On Thu, Feb 19, 2015 at 1:03 PM, Christoffer Dall
> <christoffer.dall@linaro.org> wrote:
>> 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.

The idea is that the device tree lets us describe whether
the hardware design is correct or not :-)

> What do you do if 64K mapping is not supported? Fallback to emulation
> of the CPU interface?

In this case KVM can't be supported -- use 4K host pages with this
hardware, or don't use KVM...

-- PMM

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

[Christoffer Dall]

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.

Thanks,
-Christoffer

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

[Jon Masters]

Sorry about top post. Quick comment in reply to Seattle point below - yes, it does indeed support 64K and already has the right DTS.

-- 
Computer Architect | Sent from my #ARM Powered Mobile Device

On Feb 23, 2015 4:14 AM, Christoffer Dall <christoffer.dall@linaro.org> wrote:
>
> On Sat, Feb 21, 2015 at 03:56:17PM -0600, Rob Herring wrote: 
> > On Thu, Feb 1On 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.

Thanks,
-Christoffer

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

[Rob Herring]

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.

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

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

[Pranavkumar Sawargaonkar]

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

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

[Rob Herring]

On Tue, Feb 24, 2015 at 12:34 AM, Pranavkumar Sawargaonkar
<psawargaonkar@apm.com> wrote:
> 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.

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.

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

[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@gmail.com> wrote:
> On Tue, Feb 24, 2015 at 12:34 AM, Pranavkumar Sawargaonkar
> <psawargaonkar@apm.com> wrote:
>> 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.
>
> 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

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

[Marc Zyngier]

On 24/02/15 06:34, Pranavkumar Sawargaonkar wrote:
> 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've had a look at my X-Gene board, and noticed the following thing:

Mustang# md.l 0x78020000 0x10
78020000: 000001e7 00000080 00000002 000003ff    ................
78020010: 00000000 000000ff 000003ff 00000003    ................
78020020: 000003ff 00000000 000003ff 00000000    ................
78020030: 00000000 00000000 00000000 00000000    ................
Mustang# md.l 0x7802f000 0x10
7802f000: 000001e7 00000080 00000002 000003ff    ................
7802f010: 00000000 000000ff 000003ff 00000003    ................
7802f020: 000003ff 00000000 000003ff 00000000    ................
7802f030: 00000000 00000000 00000000 00000000    ................

Notice a pattern? Yes, X-Gene is aliasing the GICC region over the 64k
page, repeated 16 times, as it should.

So either the DT is fixed to show GICC and GICV at offset 0xf000 (and we
don't support 64k pages with it), or we extend the binding to present an
offset inside the 128K region.

My preference goes of course to the latter, because for once we seem to
have something designed as expected.

Rob, what do you think?

	M.
-- 
Jazz is not dead. It just smells funny...

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

[Marc Zyngier]

On 27/01/15 07:03, Pranavkumar Sawargaonkar 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 patch may be correct, but it is useless. The firmware on my APM
system (some version of u-boot) repaints the DT at boot time, negating
the effect of this patch.

Another system I can remove from my 64k-capable list.

	M.

> CC: linux-arm-kernel at lists.infradead.org
> CC: kvmarm at lists.cs.columbia.edu
> CC: arnd at arndb.de
> CC: marc.zyngier at arm.com
> CC: christoffer.dall at linaro.org
> CC: jcm at redhat.com
> Signed-off-by: Pranavkumar Sawargaonkar <psawargaonkar@apm.com>
> Signed-off-by: Tushar Jagad <tjagad@apm.com>
> Signed-off-by: Feng Kan <fkan@apm.com>
> ---
>  arch/arm64/boot/dts/apm/apm-storm.dtsi |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/apm/apm-storm.dtsi b/arch/arm64/boot/dts/apm/apm-storm.dtsi
> index f1ad9c2..65f0e6d 100644
> --- a/arch/arm64/boot/dts/apm/apm-storm.dtsi
> +++ b/arch/arm64/boot/dts/apm/apm-storm.dtsi
> @@ -81,10 +81,10 @@
>  		compatible = "arm,cortex-a15-gic";
>  		#interrupt-cells = <3>;
>  		interrupt-controller;
> -		reg = <0x0 0x78010000 0x0 0x1000>,	/* GIC Dist */
> -		      <0x0 0x78020000 0x0 0x1000>,	/* GIC CPU */
> -		      <0x0 0x78040000 0x0 0x2000>,	/* GIC VCPU Control */
> -		      <0x0 0x78060000 0x0 0x2000>;	/* GIC VCPU */
> +		reg = <0x0 0x78010000 0x0 0x10000>,	/* GIC Dist */
> +		      <0x0 0x78020000 0x0 0x20000>,	/* GIC CPU */
> +		      <0x0 0x78040000 0x0 0x10000>,	/* GIC VCPU Control */
> +		      <0x0 0x78060000 0x0 0x20000>;	/* GIC VCPU */
>  		interrupts = <1 9 0xf04>;	/* GIC Maintenence IRQ */
>  	};
> 
> --
> 1.7.9.5
> 
> 


-- 
Jazz is not dead. It just smells funny...

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

[Feng Kan]

On Wed, Mar 11, 2015 at 7:53 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 27/01/15 07:03, Pranavkumar Sawargaonkar 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 patch may be correct, but it is useless. The firmware on my APM
> system (some version of u-boot) repaints the DT at boot time, negating
> the effect of this patch.
We have updated u-boot to reflect this change. I can supply you with a updated
image if you wish.

>
> Another system I can remove from my 64k-capable list.
>
>         M.
>
>> CC: linux-arm-kernel at lists.infradead.org
>> CC: kvmarm at lists.cs.columbia.edu
>> CC: arnd at arndb.de
>> CC: marc.zyngier at arm.com
>> CC: christoffer.dall at linaro.org
>> CC: jcm at redhat.com
>> Signed-off-by: Pranavkumar Sawargaonkar <psawargaonkar@apm.com>
>> Signed-off-by: Tushar Jagad <tjagad@apm.com>
>> Signed-off-by: Feng Kan <fkan@apm.com>
>> ---
>>  arch/arm64/boot/dts/apm/apm-storm.dtsi |    8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/apm/apm-storm.dtsi b/arch/arm64/boot/dts/apm/apm-storm.dtsi
>> index f1ad9c2..65f0e6d 100644
>> --- a/arch/arm64/boot/dts/apm/apm-storm.dtsi
>> +++ b/arch/arm64/boot/dts/apm/apm-storm.dtsi
>> @@ -81,10 +81,10 @@
>>               compatible = "arm,cortex-a15-gic";
>>               #interrupt-cells = <3>;
>>               interrupt-controller;
>> -             reg = <0x0 0x78010000 0x0 0x1000>,      /* GIC Dist */
>> -                   <0x0 0x78020000 0x0 0x1000>,      /* GIC CPU */
>> -                   <0x0 0x78040000 0x0 0x2000>,      /* GIC VCPU Control */
>> -                   <0x0 0x78060000 0x0 0x2000>;      /* GIC VCPU */
>> +             reg = <0x0 0x78010000 0x0 0x10000>,     /* GIC Dist */
>> +                   <0x0 0x78020000 0x0 0x20000>,     /* GIC CPU */
>> +                   <0x0 0x78040000 0x0 0x10000>,     /* GIC VCPU Control */
>> +                   <0x0 0x78060000 0x0 0x20000>;     /* GIC VCPU */
>>               interrupts = <1 9 0xf04>;       /* GIC Maintenence IRQ */
>>       };
>>
>> --
>> 1.7.9.5
>>
>>
>
>
> --
> Jazz is not dead. It just smells funny...

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

[Marc Zyngier]

On 11/03/15 17:19, Feng Kan wrote:
> On Wed, Mar 11, 2015 at 7:53 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> On 27/01/15 07:03, Pranavkumar Sawargaonkar 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 patch may be correct, but it is useless. The firmware on my APM
>> system (some version of u-boot) repaints the DT at boot time, negating
>> the effect of this patch.
> We have updated u-boot to reflect this change. I can supply you with a updated
> image if you wish.

That would be useful, thanks.

But more importantly, why bother upstreaming your DT into the kernel
tree if your firmware is going to overwrite whatever we provide?

Either the firmware let the user provide its own DT (and doesn't touch
it other than to change the CPU enable method, insert a /memreserve/ or
similar things), or the firmware always provide its own DT, and doesn't
let the user provide its own. Corrupting the user DT is a disaster, as
we just found.

Thanks,

	M.

>>
>> Another system I can remove from my 64k-capable list.
>>
>>         M.
>>
>>> CC: linux-arm-kernel at lists.infradead.org
>>> CC: kvmarm at lists.cs.columbia.edu
>>> CC: arnd at arndb.de
>>> CC: marc.zyngier at arm.com
>>> CC: christoffer.dall at linaro.org
>>> CC: jcm at redhat.com
>>> Signed-off-by: Pranavkumar Sawargaonkar <psawargaonkar@apm.com>
>>> Signed-off-by: Tushar Jagad <tjagad@apm.com>
>>> Signed-off-by: Feng Kan <fkan@apm.com>
>>> ---
>>>  arch/arm64/boot/dts/apm/apm-storm.dtsi |    8 ++++----
>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/arm64/boot/dts/apm/apm-storm.dtsi b/arch/arm64/boot/dts/apm/apm-storm.dtsi
>>> index f1ad9c2..65f0e6d 100644
>>> --- a/arch/arm64/boot/dts/apm/apm-storm.dtsi
>>> +++ b/arch/arm64/boot/dts/apm/apm-storm.dtsi
>>> @@ -81,10 +81,10 @@
>>>               compatible = "arm,cortex-a15-gic";
>>>               #interrupt-cells = <3>;
>>>               interrupt-controller;
>>> -             reg = <0x0 0x78010000 0x0 0x1000>,      /* GIC Dist */
>>> -                   <0x0 0x78020000 0x0 0x1000>,      /* GIC CPU */
>>> -                   <0x0 0x78040000 0x0 0x2000>,      /* GIC VCPU Control */
>>> -                   <0x0 0x78060000 0x0 0x2000>;      /* GIC VCPU */
>>> +             reg = <0x0 0x78010000 0x0 0x10000>,     /* GIC Dist */
>>> +                   <0x0 0x78020000 0x0 0x20000>,     /* GIC CPU */
>>> +                   <0x0 0x78040000 0x0 0x10000>,     /* GIC VCPU Control */
>>> +                   <0x0 0x78060000 0x0 0x20000>;     /* GIC VCPU */
>>>               interrupts = <1 9 0xf04>;       /* GIC Maintenence IRQ */
>>>       };
>>>
>>> --
>>> 1.7.9.5
>>>
>>>
>>
>>
>> --
>> Jazz is not dead. It just smells funny...
> 


-- 
Jazz is not dead. It just smells funny...

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

[Feng Kan]

On Wed, Mar 11, 2015 at 10:31 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 11/03/15 17:19, Feng Kan wrote:
>> On Wed, Mar 11, 2015 at 7:53 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>> On 27/01/15 07:03, Pranavkumar Sawargaonkar 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 patch may be correct, but it is useless. The firmware on my APM
>>> system (some version of u-boot) repaints the DT at boot time, negating
>>> the effect of this patch.
>> We have updated u-boot to reflect this change. I can supply you with a updated
>> image if you wish.
>
> That would be useful, thanks.
>
> But more importantly, why bother upstreaming your DT into the kernel
> tree if your firmware is going to overwrite whatever we provide?
We did tried to submit a version upstream but was rejected.

>
> Either the firmware let the user provide its own DT (and doesn't touch
> it other than to change the CPU enable method, insert a /memreserve/ or
> similar things), or the firmware always provide its own DT, and doesn't
> let the user provide its own. Corrupting the user DT is a disaster, as
> we just found.
Yes, the intent of the change is listed in the link below. It is not a
justification by any means,
just the effects of things appearing in layers.
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/288574.html

>
> Thanks,
>
>         M.
>
>>>
>>> Another system I can remove from my 64k-capable list.
>>>
>>>         M.
>>>
>>>> CC: linux-arm-kernel at lists.infradead.org
>>>> CC: kvmarm at lists.cs.columbia.edu
>>>> CC: arnd at arndb.de
>>>> CC: marc.zyngier at arm.com
>>>> CC: christoffer.dall at linaro.org
>>>> CC: jcm at redhat.com
>>>> Signed-off-by: Pranavkumar Sawargaonkar <psawargaonkar@apm.com>
>>>> Signed-off-by: Tushar Jagad <tjagad@apm.com>
>>>> Signed-off-by: Feng Kan <fkan@apm.com>
>>>> ---
>>>>  arch/arm64/boot/dts/apm/apm-storm.dtsi |    8 ++++----
>>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/apm/apm-storm.dtsi b/arch/arm64/boot/dts/apm/apm-storm.dtsi
>>>> index f1ad9c2..65f0e6d 100644
>>>> --- a/arch/arm64/boot/dts/apm/apm-storm.dtsi
>>>> +++ b/arch/arm64/boot/dts/apm/apm-storm.dtsi
>>>> @@ -81,10 +81,10 @@
>>>>               compatible = "arm,cortex-a15-gic";
>>>>               #interrupt-cells = <3>;
>>>>               interrupt-controller;
>>>> -             reg = <0x0 0x78010000 0x0 0x1000>,      /* GIC Dist */
>>>> -                   <0x0 0x78020000 0x0 0x1000>,      /* GIC CPU */
>>>> -                   <0x0 0x78040000 0x0 0x2000>,      /* GIC VCPU Control */
>>>> -                   <0x0 0x78060000 0x0 0x2000>;      /* GIC VCPU */
>>>> +             reg = <0x0 0x78010000 0x0 0x10000>,     /* GIC Dist */
>>>> +                   <0x0 0x78020000 0x0 0x20000>,     /* GIC CPU */
>>>> +                   <0x0 0x78040000 0x0 0x10000>,     /* GIC VCPU Control */
>>>> +                   <0x0 0x78060000 0x0 0x20000>;     /* GIC VCPU */
>>>>               interrupts = <1 9 0xf04>;       /* GIC Maintenence IRQ */
>>>>       };
>>>>
>>>> --
>>>> 1.7.9.5
>>>>
>>>>
>>>
>>>
>>> --
>>> Jazz is not dead. It just smells funny...
>>
>
>
> --
> Jazz is not dead. It just smells funny...

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

[Marc Zyngier]

On 11/03/15 17:57, Feng Kan wrote:
> On Wed, Mar 11, 2015 at 10:31 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> On 11/03/15 17:19, Feng Kan wrote:
>>> On Wed, Mar 11, 2015 at 7:53 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>>> On 27/01/15 07:03, Pranavkumar Sawargaonkar 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 patch may be correct, but it is useless. The firmware on my APM
>>>> system (some version of u-boot) repaints the DT at boot time, negating
>>>> the effect of this patch.
>>> We have updated u-boot to reflect this change. I can supply you with a updated
>>> image if you wish.
>>
>> That would be useful, thanks.
>>
>> But more importantly, why bother upstreaming your DT into the kernel
>> tree if your firmware is going to overwrite whatever we provide?
> We did tried to submit a version upstream but was rejected.
> 
>>
>> Either the firmware let the user provide its own DT (and doesn't touch
>> it other than to change the CPU enable method, insert a /memreserve/ or
>> similar things), or the firmware always provide its own DT, and doesn't
>> let the user provide its own. Corrupting the user DT is a disaster, as
>> we just found.
> Yes, the intent of the change is listed in the link below. It is not a
> justification by any means,
> just the effects of things appearing in layers.
> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/288574.html

Yeah. This is as wrong as it can possibly be. Oh well...

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

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

[Pranavkumar Sawargaonkar]

Hi Marc,

On Wed, Mar 11, 2015 at 11:47 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 11/03/15 17:57, Feng Kan wrote:
>> On Wed, Mar 11, 2015 at 10:31 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>> On 11/03/15 17:19, Feng Kan wrote:
>>>> On Wed, Mar 11, 2015 at 7:53 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>>>> On 27/01/15 07:03, Pranavkumar Sawargaonkar 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 patch may be correct, but it is useless. The firmware on my APM
>>>>> system (some version of u-boot) repaints the DT at boot time, negating
>>>>> the effect of this patch.
>>>> We have updated u-boot to reflect this change. I can supply you with a updated
>>>> image if you wish.
>>>
>>> That would be useful, thanks.
>>>
>>> But more importantly, why bother upstreaming your DT into the kernel
>>> tree if your firmware is going to overwrite whatever we provide?
>> We did tried to submit a version upstream but was rejected.
>>
>>>
>>> Either the firmware let the user provide its own DT (and doesn't touch
>>> it other than to change the CPU enable method, insert a /memreserve/ or
>>> similar things), or the firmware always provide its own DT, and doesn't
>>> let the user provide its own. Corrupting the user DT is a disaster, as
>>> we just found.
>> Yes, the intent of the change is listed in the link below. It is not a
>> justification by any means,
>> just the effects of things appearing in layers.
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/288574.html
>
> Yeah. This is as wrong as it can possibly be. Oh well...

Yes there is an issue with u-boot patching the dt for end user who
wants his DT to be used, for this we can (in fact we should) provide
an option in u-boot (may be setting some environment variable) which
will make sure end user's DT does not get modified (apart from
standard things like patching mac-addresses) by u-boot.

Another point I want to reopen here is the how to handle 64K GIC page
size pointed out in this thread, what would be the best way to tackle
this (adding a new DT string or any other way) ?


Thanks,
Pranav

>
> Thanks,
>
>         M.
> --
> Jazz is not dead. It just smells funny...

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

[Marc Zyngier]

[adding RobH to the CC list, as he was commenting on the subject earlier]

Hi Pranav,

On 12/03/15 03:52, Pranavkumar Sawargaonkar wrote:
> Hi Marc,
> 
> On Wed, Mar 11, 2015 at 11:47 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> On 11/03/15 17:57, Feng Kan wrote:
>>> On Wed, Mar 11, 2015 at 10:31 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>>> On 11/03/15 17:19, Feng Kan wrote:
>>>>> On Wed, Mar 11, 2015 at 7:53 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>>>>> On 27/01/15 07:03, Pranavkumar Sawargaonkar 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 patch may be correct, but it is useless. The firmware on my APM
>>>>>> system (some version of u-boot) repaints the DT at boot time, negating
>>>>>> the effect of this patch.
>>>>> We have updated u-boot to reflect this change. I can supply you with a updated
>>>>> image if you wish.
>>>>
>>>> That would be useful, thanks.
>>>>
>>>> But more importantly, why bother upstreaming your DT into the kernel
>>>> tree if your firmware is going to overwrite whatever we provide?
>>> We did tried to submit a version upstream but was rejected.
>>>
>>>>
>>>> Either the firmware let the user provide its own DT (and doesn't touch
>>>> it other than to change the CPU enable method, insert a /memreserve/ or
>>>> similar things), or the firmware always provide its own DT, and doesn't
>>>> let the user provide its own. Corrupting the user DT is a disaster, as
>>>> we just found.
>>> Yes, the intent of the change is listed in the link below. It is not a
>>> justification by any means,
>>> just the effects of things appearing in layers.
>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/288574.html
>>
>> Yeah. This is as wrong as it can possibly be. Oh well...
> 
> Yes there is an issue with u-boot patching the dt for end user who
> wants his DT to be used, for this we can (in fact we should) provide
> an option in u-boot (may be setting some environment variable) which
> will make sure end user's DT does not get modified (apart from
> standard things like patching mac-addresses) by u-boot.

Definitely. I would even argue that not overwriting the DT should be the
default behaviour, and that you could have a compatibility mode that
repaint ancient DTs  if you want to. But at least having something would
be good. At the moment, my X-Gene board is screwed, as I don't have any
way to fix the bootloader (or even to download a binary).

> Another point I want to reopen here is the how to handle 64K GIC page
> size pointed out in this thread, what would be the best way to tackle
> this (adding a new DT string or any other way) ?

The main problem is that there is several flavours of brokenness:

- GICC_CTLR at 0, GICC_DIR at 0x10000, size 128kB (X-Gene)
- GICC_CTLR at 0, GICC_DIR at 0x1000, size 64kB (Seattle)
- GICC_CTL at 0xF000, GICC_DIR at 0x10000, size 8kB (Juno)
- GICC_CTL at 0, GICC_DIR at 0x1000, size 8kB (HiKey)

Yes, they all have a GIC400, and yet they are all irritatingly non
compliant with SBSA. As far as I can tell, nobody has correctly
implemented the expected aliasing that would have made it work.

So either we add new compatibility strings describing all the possible
way people can break things, or we introduce something like dir-offset
and ctlr-offset that would tell the driver where the two sub-regions are
placed. The default values would be:

- When size is < 8kB -> invalid configuration, this is not a GIC400.
- When size is = 8kB -> ctlr-offset = 0, dir-offset = 0x1000
- When size is = 128kB -> ctlr-offset = 0xf000, dir-offset = 0x10000

For the two first braindead systems above:
- ctlr-offset = 0, dir-offset = 0x10000 (X-Gene)
- ctlr-offset = 0, dir-offset = 0x1000; (Seattle)

and that's just enough to get bare metal going.

When it comes to virtualization, this is hell:
- We need an API to be able to expose these various offsets to
userspace, so that QEMU/kvmtool can place the GICV region at the right
location (offset within a page).
- We will also miss the capability to trap GICV_DIR independently from
the rest of the VCPU interface on systems like Seattle, which is rather bad.
- Systems that look like Juno or HiKey cannot use virtualization with
64k pages, end of story.

After writing this, I'm feeling slightly depressed...

	M.
-- 
Jazz is not dead. It just smells funny...