Re: [PATCH] arm64: dts: Fix GIC reg sizes for APM X-Gene
Hi Marc, On Wed, Mar 11, 2015 at 11:47 PM, Marc Zyngier marc.zyng...@arm.com wrote: On 11/03/15 17:57, Feng Kan wrote: On Wed, Mar 11, 2015 at 10:31 AM, Marc Zyngier marc.zyng...@arm.com wrote: On 11/03/15 17:19, Feng Kan wrote: On Wed, Mar 11, 2015 at 7:53 AM, Marc Zyngier marc.zyng...@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... -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] arm64: dts: Fix GIC reg sizes for APM X-Gene
Hi Rob, On Tue, Feb 24, 2015 at 8:00 PM, Rob Herring robherri...@gmail.com wrote: On Tue, Feb 24, 2015 at 12:34 AM, Pranavkumar Sawargaonkar psawargaon...@apm.com wrote: Hi Rob, On Mon, Feb 23, 2015 at 10:09 PM, Rob Herring robherri...@gmail.com wrote: On Mon, Feb 23, 2015 at 6:07 AM, Christoffer Dall christoffer.d...@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.d...@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 psawargaon...@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 0xe111 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 0xe114 0 0x1, arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi- 0x0 0xe116 0 0x1; arch/arm64/boot/dts/arm/juno.dts- reg = 0x0 0x2c01 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
Re: [PATCH] arm64: dts: Fix GIC reg sizes for APM X-Gene
Hi Rob, On Mon, Feb 23, 2015 at 10:09 PM, Rob Herring robherri...@gmail.com wrote: On Mon, Feb 23, 2015 at 6:07 AM, Christoffer Dall christoffer.d...@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.d...@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 psawargaon...@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 0xe111 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 0xe114 0 0x1, arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi- 0x0 0xe116 0 0x1; arch/arm64/boot/dts/arm/juno.dts- reg = 0x0 0x2c01 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 -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] arm64: dts: Fix GIC reg sizes for APM X-Gene
Hi, On Tue, Jan 27, 2015 at 3:02 PM, Jon Masters j...@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-ker...@lists.infradead.org CC: kvm...@lists.cs.columbia.edu CC: a...@arndb.de CC: marc.zyng...@arm.com CC: christoffer.d...@linaro.org CC: j...@redhat.com Signed-off-by: Pranavkumar Sawargaonkar psawargaon...@apm.com Signed-off-by: Tushar Jagad tja...@apm.com Signed-off-by: Feng Kan f...@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 0x7801 0x0 0x1000, /* GIC Dist */ - 0x0 0x7802 0x0 0x1000, /* GIC CPU */ - 0x0 0x7804 0x0 0x2000, /* GIC VCPU Control */ - 0x0 0x7806 0x0 0x2000; /* GIC VCPU */ + reg = 0x0 0x7801 0x0 0x1, /* GIC Dist */ + 0x0 0x7802 0x0 0x2, /* GIC CPU */ + 0x0 0x7804 0x0 0x1, /* GIC VCPU Control */ + 0x0 0x7806 0x0 0x2; /* 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 -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] arm64: dts: Fix GIC reg sizes for APM X-Gene
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-ker...@lists.infradead.org CC: kvm...@lists.cs.columbia.edu CC: a...@arndb.de CC: marc.zyng...@arm.com CC: christoffer.d...@linaro.org CC: j...@redhat.com Signed-off-by: Pranavkumar Sawargaonkar psawargaon...@apm.com Signed-off-by: Tushar Jagad tja...@apm.com Signed-off-by: Feng Kan f...@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 0x7801 0x0 0x1000, /* GIC Dist */ - 0x0 0x7802 0x0 0x1000, /* GIC CPU */ - 0x0 0x7804 0x0 0x2000, /* GIC VCPU Control */ - 0x0 0x7806 0x0 0x2000; /* GIC VCPU */ + reg = 0x0 0x7801 0x0 0x1, /* GIC Dist */ + 0x0 0x7802 0x0 0x2, /* GIC CPU */ + 0x0 0x7804 0x0 0x1, /* GIC VCPU Control */ + 0x0 0x7806 0x0 0x2; /* GIC VCPU */ interrupts = 1 9 0xf04; /* GIC Maintenence IRQ */ }; -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
