Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1312460 > unrolled thread

Re: [PATCH 1/3] irqchip/GIC: Add workaround for aliased GIC400

Back to article view | Back to linux.kernel

Contents

  Re: [PATCH 1/3] irqchip/GIC: Add workaround for aliased GIC400 Duc Dang <dhdang@apm.com> - 2016-01-19 20:20 +0100
    Re: [PATCH 1/3] irqchip/GIC: Add workaround for aliased GIC400 Marc Zyngier <marc.zyngier@arm.com> - 2016-01-20 10:10 +0100
      Re: [PATCH 1/3] irqchip/GIC: Add workaround for aliased GIC400 Duc Dang <dhdang@apm.com> - 2016-01-23 00:30 +0100

#1312460 — Re: [PATCH 1/3] irqchip/GIC: Add workaround for aliased GIC400

On Sun, Sep 13, 2015 at 4:14 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>
> The GICv2 architecture mandates that the two 4kB GIC regions are
> contiguous, and on two separate physical pages (so that access to
> the second page can be trapped by a hypervisor). This doesn't work
> very well when PAGE_SIZE is 64kB.
>
> A relatively common hack^Wway to work around this is to alias each
> 4kB region over its own 64kB page. Of course in this case, the base
> address you want to use is not really the begining of the region,
> but base + 60kB (so that you get a contiguous 8kB region over two
> distinct pages).
>
> Normally, this would be described in DT with a new property, but
> some HW is already out there, and the firmware makes sure that
> it will override whatever you put in the GIC node. Duh. And of course,
> said firmware source code is not available, despite being based
> on u-boot.
>
> The workaround is to detect the case where the CPU interface size
> is set to 128kB, and verify the aliasing by checking that the ID
> register for GIC400 (which is the only GIC wired this way so far)
> is the same at base and base + 0xF000. In this case, we update
> the GIC base address and let it roll.

Hi Marc,

When booting ACPI with X-Gene Mustang, I saw it hangs when EOI mode is
enabled, should we have ACPI version for gic_check_eoimode as well?

Regards,
Duc Dang.
>
> And if you feel slightly sick by looking at this, rest assured that
> I do too...
>
> Reported-by: Julien Grall <julien.grall@citrix.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  drivers/irqchip/irq-gic.c | 44 +++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 39 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index e6b7ed5..a947057 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -1119,12 +1119,49 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
>  #ifdef CONFIG_OF
>  static int gic_cnt __initdata;
>
> +static bool gic_check_eoimode(struct device_node *node, void __iomem **base)
> +{
> +       struct resource cpuif_res;
> +
> +       of_address_to_resource(node, 1, &cpuif_res);
> +
> +       if (!is_hyp_mode_available())
> +               return false;
> +       if (resource_size(&cpuif_res) < SZ_8K)
> +               return false;
> +       if (resource_size(&cpuif_res) == SZ_128K) {
> +               u32 val_low, val_high;
> +
> +               /*
> +                * Verify that we have the first 4kB of a GIC400
> +                * aliased over the first 64kB by checking the
> +                * GICC_IIDR register on both ends.
> +                */
> +               val_low = readl_relaxed(*base + GIC_CPU_IDENT);
> +               val_high = readl_relaxed(*base + GIC_CPU_IDENT + 0xf000);
> +               if ((val_low & 0xffff0fff) != 0x0202043B ||
> +                   val_low != val_high)
> +                       return false;
> +
> +               /*
> +                * Move the base up by 60kB, so that we have a 8kB
> +                * contiguous region, which allows us to use GICC_DIR
> +                * at its normal offset. Please pass me that bucket.
> +                */
> +               *base += 0xf000;
> +               cpuif_res.start += 0xf000;
> +               pr_warn("GIC: Adjusting CPU interface base to %pa",
> +                       &cpuif_res.start);
> +       }
> +
> +       return true;
> +}
> +
>  static int __init
>  gic_of_init(struct device_node *node, struct device_node *parent)
>  {
>         void __iomem *cpu_base;
>         void __iomem *dist_base;
> -       struct resource cpu_res;
>         u32 percpu_offset;
>         int irq;
>
> @@ -1137,14 +1174,11 @@ gic_of_init(struct device_node *node, struct device_node *parent)
>         cpu_base = of_iomap(node, 1);
>         WARN(!cpu_base, "unable to map gic cpu registers\n");
>
> -       of_address_to_resource(node, 1, &cpu_res);
> -
>         /*
>          * Disable split EOI/Deactivate if either HYP is not available
>          * or the CPU interface is too small.
>          */
> -       if (gic_cnt == 0 && (!is_hyp_mode_available() ||
> -                            resource_size(&cpu_res) < SZ_8K))
> +       if (gic_cnt == 0 && !gic_check_eoimode(node, &cpu_base))
>                 static_key_slow_dec(&supports_deactivate);
>
>         if (of_property_read_u32(node, "cpu-offset", &percpu_offset))
> --
> 2.1.4
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[toc] | [next] | [standalone]

#1313006

Hi Duc,

On Tue, 19 Jan 2016 11:12:15 -0800
Duc Dang <dhdang@apm.com> wrote:

> On Sun, Sep 13, 2015 at 4:14 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> >
> > The GICv2 architecture mandates that the two 4kB GIC regions are
> > contiguous, and on two separate physical pages (so that access to
> > the second page can be trapped by a hypervisor). This doesn't work
> > very well when PAGE_SIZE is 64kB.
> >
> > A relatively common hack^Wway to work around this is to alias each
> > 4kB region over its own 64kB page. Of course in this case, the base
> > address you want to use is not really the begining of the region,
> > but base + 60kB (so that you get a contiguous 8kB region over two
> > distinct pages).
> >
> > Normally, this would be described in DT with a new property, but
> > some HW is already out there, and the firmware makes sure that
> > it will override whatever you put in the GIC node. Duh. And of course,
> > said firmware source code is not available, despite being based
> > on u-boot.
> >
> > The workaround is to detect the case where the CPU interface size
> > is set to 128kB, and verify the aliasing by checking that the ID
> > register for GIC400 (which is the only GIC wired this way so far)
> > is the same at base and base + 0xF000. In this case, we update
> > the GIC base address and let it roll.
> 
> Hi Marc,
> 
> When booting ACPI with X-Gene Mustang, I saw it hangs when EOI mode is
> enabled, should we have ACPI version for gic_check_eoimode as well?

ACPI doesn't provide the size of the CPU interface, so you cannot
perform the same kind of check and bug workaround. I'm afraid you have
to fix your ACPI tables (which shouldn't be a problem since nobody is
using ACPI in production so far...).

Thanks,

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

[toc] | [prev] | [next] | [standalone]

#1315345

On Wed, Jan 20, 2016 at 1:06 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> Hi Duc,
>
> On Tue, 19 Jan 2016 11:12:15 -0800
> Duc Dang <dhdang@apm.com> wrote:
>
>> On Sun, Sep 13, 2015 at 4:14 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> >
>> > The GICv2 architecture mandates that the two 4kB GIC regions are
>> > contiguous, and on two separate physical pages (so that access to
>> > the second page can be trapped by a hypervisor). This doesn't work
>> > very well when PAGE_SIZE is 64kB.
>> >
>> > A relatively common hack^Wway to work around this is to alias each
>> > 4kB region over its own 64kB page. Of course in this case, the base
>> > address you want to use is not really the begining of the region,
>> > but base + 60kB (so that you get a contiguous 8kB region over two
>> > distinct pages).
>> >
>> > Normally, this would be described in DT with a new property, but
>> > some HW is already out there, and the firmware makes sure that
>> > it will override whatever you put in the GIC node. Duh. And of course,
>> > said firmware source code is not available, despite being based
>> > on u-boot.
>> >
>> > The workaround is to detect the case where the CPU interface size
>> > is set to 128kB, and verify the aliasing by checking that the ID
>> > register for GIC400 (which is the only GIC wired this way so far)
>> > is the same at base and base + 0xF000. In this case, we update
>> > the GIC base address and let it roll.
>>
>> Hi Marc,
>>
>> When booting ACPI with X-Gene Mustang, I saw it hangs when EOI mode is
>> enabled, should we have ACPI version for gic_check_eoimode as well?
>
> ACPI doesn't provide the size of the CPU interface, so you cannot
> perform the same kind of check and bug workaround. I'm afraid you have
> to fix your ACPI tables (which shouldn't be a problem since nobody is
> using ACPI in production so far...).
>
Thanks, Marc.

We will go with your suggestion.

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web