| Message ID | 1404484784-30659-1-git-send-email-stefano.stabellini@eu.citrix.com |
|---|---|
| State | New |
| Headers | show |
Hi Stefano, On 04/07/14 15:39, Stefano Stabellini wrote: > GICH_LR_HW doesn't work as expected on X-Gene: request maintenance > interrupts and perform EOIs in the hypervisor for hardware interrupts as > a workaround. Trigger this behaviour with a per platform option. > > This patch assumes that GICC_DIR can be written on any pcpu for a given > SGI, not matter where GICC_IAR has been read before. > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> Acked-by: Julien Grall <julien.grall@linaro.org> Regards,
On Fri, 2014-07-04 at 15:39 +0100, Stefano Stabellini wrote: > GICH_LR_HW doesn't work as expected on X-Gene: request maintenance > interrupts and perform EOIs in the hypervisor for hardware interrupts as > a workaround. Trigger this behaviour with a per platform option. > > This patch assumes that GICC_DIR can be written on any pcpu for a given > SGI, not matter where GICC_IAR has been read before. Did you really mean SGI here? Those are per-cpu, I suspect you meant SPI? Ack to the actual patch though. Ian.
Hi Stefano/ Ian, On Wed, Jul 9, 2014 at 6:11 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote: > On Fri, 2014-07-04 at 15:39 +0100, Stefano Stabellini wrote: >> GICH_LR_HW doesn't work as expected on X-Gene: request maintenance >> interrupts and perform EOIs in the hypervisor for hardware interrupts as >> a workaround. Trigger this behaviour with a per platform option. >> >> This patch assumes that GICC_DIR can be written on any pcpu for a given >> SGI, not matter where GICC_IAR has been read before. > > Did you really mean SGI here? Those are per-cpu, I suspect you meant > SPI? > > Ack to the actual patch though. > > Ian. > We have found clean fix for this issue in u-boot. The issue is that X-Gene does not implement security extensions but the GIC-400 present in X-Gene has security extensions. To take care of this situation, APM HW designers have provided two sets of GIC register addresses: one for accessing GIC secured registers, and another for accessing GIC non-secured registers. Currently, we are only accessing GIC secured register for Linux, Xen, and KVM. This works fine in most cases but does not work for GICH_LRn.HW bit because we can only auto-deactivate non-secured interrupts using GICH_LRn.HW bit. To fix this issue, we have updated u-boot to initialize GIC secured register to make all interrupts as non-secured and we will need to access GIC non-secured registers from Linux, Xen, KVM, and everywhere else. For now, you can go ahead with this patch but once we have updated u-boot released by APM then we will need to disable the quirk for X-Gene Mustang. We would be also having a patch for Linux to fix the GIC addresses in X-Gene Storm DTS file. Thanks, Pranav
On Wed, 9 Jul 2014, Pranavkumar Sawargaonkar wrote: > Hi Stefano/ Ian, > > On Wed, Jul 9, 2014 at 6:11 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote: > > On Fri, 2014-07-04 at 15:39 +0100, Stefano Stabellini wrote: > >> GICH_LR_HW doesn't work as expected on X-Gene: request maintenance > >> interrupts and perform EOIs in the hypervisor for hardware interrupts as > >> a workaround. Trigger this behaviour with a per platform option. > >> > >> This patch assumes that GICC_DIR can be written on any pcpu for a given > >> SGI, not matter where GICC_IAR has been read before. > > > > Did you really mean SGI here? Those are per-cpu, I suspect you meant > > SPI? > > > > Ack to the actual patch though. > > > > Ian. > > > > We have found clean fix for this issue in u-boot. > > The issue is that X-Gene does not implement security extensions > but the GIC-400 present in X-Gene has security extensions. To take > care of this situation, APM HW designers have provided two sets > of GIC register addresses: one for accessing GIC secured registers, > and another for accessing GIC non-secured registers. Currently, we > are only accessing GIC secured register for Linux, Xen, and KVM. > This works fine in most cases but does not work for GICH_LRn.HW > bit because we can only auto-deactivate non-secured interrupts using > GICH_LRn.HW bit. > > To fix this issue, we have updated u-boot to initialize GIC secured > register to make all interrupts as non-secured and we will need to > access GIC non-secured registers from Linux, Xen, KVM, and > everywhere else. > > For now, you can go ahead with this patch but once we have updated > u-boot released by APM then we will need to disable the quirk for > X-Gene Mustang. We would be also having a patch for Linux to fix > the GIC addresses in X-Gene Storm DTS file. It's great that you managed to fix the problem! How can we detect whether the Mustang board we are running on has an up-to-date uboot? Would it be possible for u-boot to add a flag somewhere to tell that the GICH_LRn.HW is safe to use? Then we could check that flag and only enable PLATFORM_QUIRK_GUEST_PIRQ_NEED_EOI if the flag is missing.
On Wed, Jul 9, 2014 at 7:49 PM, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote: > On Wed, 9 Jul 2014, Pranavkumar Sawargaonkar wrote: >> Hi Stefano/ Ian, >> >> On Wed, Jul 9, 2014 at 6:11 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote: >> > On Fri, 2014-07-04 at 15:39 +0100, Stefano Stabellini wrote: >> >> GICH_LR_HW doesn't work as expected on X-Gene: request maintenance >> >> interrupts and perform EOIs in the hypervisor for hardware interrupts as >> >> a workaround. Trigger this behaviour with a per platform option. >> >> >> >> This patch assumes that GICC_DIR can be written on any pcpu for a given >> >> SGI, not matter where GICC_IAR has been read before. >> > >> > Did you really mean SGI here? Those are per-cpu, I suspect you meant >> > SPI? >> > >> > Ack to the actual patch though. >> > >> > Ian. >> > >> >> We have found clean fix for this issue in u-boot. >> >> The issue is that X-Gene does not implement security extensions >> but the GIC-400 present in X-Gene has security extensions. To take >> care of this situation, APM HW designers have provided two sets >> of GIC register addresses: one for accessing GIC secured registers, >> and another for accessing GIC non-secured registers. Currently, we >> are only accessing GIC secured register for Linux, Xen, and KVM. >> This works fine in most cases but does not work for GICH_LRn.HW >> bit because we can only auto-deactivate non-secured interrupts using >> GICH_LRn.HW bit. >> >> To fix this issue, we have updated u-boot to initialize GIC secured >> register to make all interrupts as non-secured and we will need to >> access GIC non-secured registers from Linux, Xen, KVM, and >> everywhere else. >> >> For now, you can go ahead with this patch but once we have updated >> u-boot released by APM then we will need to disable the quirk for >> X-Gene Mustang. We would be also having a patch for Linux to fix >> the GIC addresses in X-Gene Storm DTS file. > > It's great that you managed to fix the problem! > > How can we detect whether the Mustang board we are running on has an > up-to-date uboot? Would it be possible for u-boot to add a flag > somewhere to tell that the GICH_LRn.HW is safe to use? Then we could > check that flag and only enable PLATFORM_QUIRK_GUEST_PIRQ_NEED_EOI if > the flag is missing. I think we should check GIC Dist address passed in DTS by u-boot to Xen. If GIC Dist address is secured address then enable the quirk otherwise disable the qurik. Regards, Anup
On Wed, 2014-07-09 at 19:54 +0530, Anup Patel wrote: > On Wed, Jul 9, 2014 at 7:49 PM, Stefano Stabellini > <stefano.stabellini@eu.citrix.com> wrote: > > On Wed, 9 Jul 2014, Pranavkumar Sawargaonkar wrote: > >> Hi Stefano/ Ian, > >> > >> On Wed, Jul 9, 2014 at 6:11 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote: > >> > On Fri, 2014-07-04 at 15:39 +0100, Stefano Stabellini wrote: > >> >> GICH_LR_HW doesn't work as expected on X-Gene: request maintenance > >> >> interrupts and perform EOIs in the hypervisor for hardware interrupts as > >> >> a workaround. Trigger this behaviour with a per platform option. > >> >> > >> >> This patch assumes that GICC_DIR can be written on any pcpu for a given > >> >> SGI, not matter where GICC_IAR has been read before. > >> > > >> > Did you really mean SGI here? Those are per-cpu, I suspect you meant > >> > SPI? > >> > > >> > Ack to the actual patch though. > >> > > >> > Ian. > >> > > >> > >> We have found clean fix for this issue in u-boot. > >> > >> The issue is that X-Gene does not implement security extensions > >> but the GIC-400 present in X-Gene has security extensions. To take > >> care of this situation, APM HW designers have provided two sets > >> of GIC register addresses: one for accessing GIC secured registers, > >> and another for accessing GIC non-secured registers. Currently, we > >> are only accessing GIC secured register for Linux, Xen, and KVM. > >> This works fine in most cases but does not work for GICH_LRn.HW > >> bit because we can only auto-deactivate non-secured interrupts using > >> GICH_LRn.HW bit. > >> > >> To fix this issue, we have updated u-boot to initialize GIC secured > >> register to make all interrupts as non-secured and we will need to > >> access GIC non-secured registers from Linux, Xen, KVM, and > >> everywhere else. > >> > >> For now, you can go ahead with this patch but once we have updated > >> u-boot released by APM then we will need to disable the quirk for > >> X-Gene Mustang. We would be also having a patch for Linux to fix > >> the GIC addresses in X-Gene Storm DTS file. > > > > It's great that you managed to fix the problem! > > > > How can we detect whether the Mustang board we are running on has an > > up-to-date uboot? Would it be possible for u-boot to add a flag > > somewhere to tell that the GICH_LRn.HW is safe to use? Then we could > > check that flag and only enable PLATFORM_QUIRK_GUEST_PIRQ_NEED_EOI if > > the flag is missing. > > I think we should check GIC Dist address passed in DTS by > u-boot to Xen. If GIC Dist address is secured address then > enable the quirk otherwise disable the qurik. Isn't IGROUPR readable from NS? In which case we should be able to spot the difference, I think? Stefano, you might want to arrange in your patch to cache the value of the quirk in the gicv2 struct -- otherwise all the calls to platform_has_quirk are going to add up. Ian.
On 07/09/2014 03:40 PM, Ian Campbell wrote: >> I think we should check GIC Dist address passed in DTS by >> u-boot to Xen. If GIC Dist address is secured address then >> enable the quirk otherwise disable the qurik. > > Isn't IGROUPR readable from NS? In which case we should be able to spot > the difference, I think? > > Stefano, you might want to arrange in your patch to cache the value of > the quirk in the gicv2 struct -- otherwise all the calls to > platform_has_quirk are going to add up. Can't we cache the value in the platform code? I don't think it's too expensive to call the platform_has_quirk in this case. Regards,
On Wed, 9 Jul 2014, Julien Grall wrote: > On 07/09/2014 03:40 PM, Ian Campbell wrote: > >> I think we should check GIC Dist address passed in DTS by > >> u-boot to Xen. If GIC Dist address is secured address then > >> enable the quirk otherwise disable the qurik. > > > > Isn't IGROUPR readable from NS? In which case we should be able to spot > > the difference, I think? Anup, can you confirm that we can use IGROUPR to spot the difference? Otherwise is there another register we can use? In any case given that only you can test it, I think that it should be one of you guys that come up with a way to detect the change and submit a patch to conditionally disable the workaround. > > Stefano, you might want to arrange in your patch to cache the value of > > the quirk in the gicv2 struct -- otherwise all the calls to > > platform_has_quirk are going to add up. > > Can't we cache the value in the platform code? I don't think it's too > expensive to call the platform_has_quirk in this case. Caching the value in gic-v2.c seems a bit overkill. But we could turn platform->quirks() into a bitfield, then we can significantly reduce memory accesses when checking for quirks, from 3 to just 1.
On Wed, 2014-07-09 at 16:11 +0100, Stefano Stabellini wrote: > > > Stefano, you might want to arrange in your patch to cache the value of > > > the quirk in the gicv2 struct -- otherwise all the calls to > > > platform_has_quirk are going to add up. > > > > Can't we cache the value in the platform code? I don't think it's too > > expensive to call the platform_has_quirk in this case. > > Caching the value in gic-v2.c seems a bit overkill. But we could turn > platform->quirks() into a bitfield, then we can significantly reduce > memory accesses when checking for quirks, from 3 to just 1. Hrm, yes. That does have the disadvantage of potentially un-consting the platform ops. Anyway, this patch is correct for now and the detection of the fixed platform can be done later. So applied with s/SGI/SPI in the commit log as discussed IRL. Ian.
On Wed, Jul 9, 2014 at 8:41 PM, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote: > On Wed, 9 Jul 2014, Julien Grall wrote: >> On 07/09/2014 03:40 PM, Ian Campbell wrote: >> >> I think we should check GIC Dist address passed in DTS by >> >> u-boot to Xen. If GIC Dist address is secured address then >> >> enable the quirk otherwise disable the qurik. >> > >> > Isn't IGROUPR readable from NS? In which case we should be able to spot >> > the difference, I think? > > Anup, can you confirm that we can use IGROUPR to spot the difference? > Otherwise is there another register we can use? Yes, IGROUPR of the secured GIC registers would be correct register to spot the difference. More precisely, if IGROUPR0 == 0xFFFFFFFF in GIC secured registers then don't use the quirk for X-Gene. > > In any case given that only you can test it, I think that it should be > one of you guys that come up with a way to detect the change and submit > a patch to conditionally disable the workaround. Sure, most likely Pranav will send a patch for Xen once APM releases updated u-boot binary. Thanks, Anup > > >> > Stefano, you might want to arrange in your patch to cache the value of >> > the quirk in the gicv2 struct -- otherwise all the calls to >> > platform_has_quirk are going to add up. >> >> Can't we cache the value in the platform code? I don't think it's too >> expensive to call the platform_has_quirk in this case. > > Caching the value in gic-v2.c seems a bit overkill. But we could turn > platform->quirks() into a bitfield, then we can significantly reduce > memory accesses when checking for quirks, from 3 to just 1.
diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c index c3d2853..66ad069 100644 --- a/xen/arch/arm/gic-v2.c +++ b/xen/arch/arm/gic-v2.c @@ -361,8 +361,13 @@ static void gicv2_update_lr(int lr, const struct pending_irq *p, ((p->irq & GICH_V2_LR_VIRTUAL_MASK) << GICH_V2_LR_VIRTUAL_SHIFT)); if ( p->desc != NULL ) - lr_reg |= GICH_V2_LR_HW | ((p->desc->irq & GICH_V2_LR_PHYSICAL_MASK ) - << GICH_V2_LR_PHYSICAL_SHIFT); + { + if ( platform_has_quirk(PLATFORM_QUIRK_GUEST_PIRQ_NEED_EOI) ) + lr_reg |= GICH_V2_LR_MAINTENANCE_IRQ; + else + lr_reg |= GICH_V2_LR_HW | ((p->desc->irq & GICH_V2_LR_PHYSICAL_MASK ) + << GICH_V2_LR_PHYSICAL_SHIFT); + } writel_relaxed(lr_reg, GICH + GICH_LR + lr * 4); } diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index 37b08c2..7fa8a0f 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -369,7 +369,11 @@ static void gic_update_one_lr(struct vcpu *v, int i) clear_bit(i, &this_cpu(lr_mask)); if ( p->desc != NULL ) + { p->desc->status &= ~IRQ_INPROGRESS; + if ( platform_has_quirk(PLATFORM_QUIRK_GUEST_PIRQ_NEED_EOI) ) + gic_hw_ops->deactivate_irq(p->desc); + } clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status); clear_bit(GIC_IRQ_GUEST_ACTIVE, &p->status); p->lr = GIC_INVALID_LR; diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c index c9dd63c..837d8e6 100644 --- a/xen/arch/arm/platforms/xgene-storm.c +++ b/xen/arch/arm/platforms/xgene-storm.c @@ -37,7 +37,7 @@ static bool reset_vals_valid = false; static uint32_t xgene_storm_quirks(void) { - return PLATFORM_QUIRK_GIC_64K_STRIDE; + return PLATFORM_QUIRK_GIC_64K_STRIDE|PLATFORM_QUIRK_GUEST_PIRQ_NEED_EOI; } static int map_one_mmio(struct domain *d, const char *what, diff --git a/xen/include/asm-arm/platform.h b/xen/include/asm-arm/platform.h index bcd2097..eefaca6 100644 --- a/xen/include/asm-arm/platform.h +++ b/xen/include/asm-arm/platform.h @@ -55,6 +55,11 @@ struct platform_desc { */ #define PLATFORM_QUIRK_GIC_64K_STRIDE (1 << 0) +/* + * Quirk for platforms where GICH_LR_HW does not work as expected. + */ +#define PLATFORM_QUIRK_GUEST_PIRQ_NEED_EOI (1 << 1) + void __init platform_init(void); int __init platform_init_time(void); int __init platform_specific_mapping(struct domain *d);
GICH_LR_HW doesn't work as expected on X-Gene: request maintenance interrupts and perform EOIs in the hypervisor for hardware interrupts as a workaround. Trigger this behaviour with a per platform option. This patch assumes that GICC_DIR can be written on any pcpu for a given SGI, not matter where GICC_IAR has been read before. Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> CC: psawargaonkar@apm.com CC: apatel@apm.com --- Changes in v3: - do not request maintenance interrupts for virtual interrupts. Changes in v2: - s/PLATFORM_QUIRK_NEED_EOI/PLATFORM_QUIRK_GUEST_PIRQ_NEED_EOI/g; - improved commit message. --- xen/arch/arm/gic-v2.c | 9 +++++++-- xen/arch/arm/gic.c | 4 ++++ xen/arch/arm/platforms/xgene-storm.c | 2 +- xen/include/asm-arm/platform.h | 5 +++++ 4 files changed, 17 insertions(+), 3 deletions(-)