[PATCH] irqchip/gic-v3: Check SRE bit for GICv2 legacy support

[Shameerali Kolothum Thodi]

Hi Zenghui,

> -----Original Message-----
> From: yuzenghui
> Sent: 30 November 2020 11:51
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi at huawei.com>;
> linux-kernel at vger.kernel.org; linux-arm-kernel at lists.infradead.org
> Cc: maz at kernel.org; Linuxarm <linuxarm at huawei.com>;
> eric.auger at redhat.com
> Subject: Re: [PATCH] irqchip/gic-v3: Check SRE bit for GICv2 legacy support
> 
> Hi Shameer,
> 
> On 2020/11/30 18:26, Shameer Kolothum wrote:
> > At present, the support for GICv2 backward compatibility on GICv3/v4
> > hardware is determined based on whether DT/ACPI provides a memory
> > mapped phys base address for GIC virtual CPU interface register(GICV).
> > This creates a problem that a Qemu guest boot with default GIC(GICv2)
> > hangs when firmware falsely reports this address on systems that don't
> > have support for legacy mode.
> 
> So the problem is that BIOS has provided us a bogus GICC Structure.

Yes. And kernel uses this field to determine the legacy support.

> 
> > As per GICv3/v4 spec, in an implementation that does not support legacy
> > operation, affinity routing and system register access are permanently
> > enabled. This means that the associated control bits are RAO/WI. Hence
> > use the ICC_SRE_EL1.SRE bit to decide whether hardware supports GICv2
> > mode in addition to the above firmware based check.
> >
> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi at huawei.com>
> > ---
> > On Hisilicon D06, UEFI sets the GIC MADT GICC gicv_base_address but the
> > GIC implementation on these boards doesn't have the GICv2 legacy support.
> > This results in, Guest boot hang when Qemu uses the default GIC option.
> >
> > With this patch, the Qemu Guest with GICv2 now gracefully exits,
> >   "qemu-system-aarch64: host does not support in-kernel GICv2 emulation"
> >
> > Not very sure there is a better way to detect this other than checking
> > the SRE bit as done in this patch(Of course, we will be fixing the UEFI
> > going forward).
> 
> Yes, I had seen the same problem on the D06. But I *do* think it's the
> firmware that actually needs to be fixed.

Well, I am not sure I agree with that. The ACPI spec 6.3, section 5.2.12.14, says,
"If the platform is not presenting a GICv2 with virtualization extensions this 
field *can* be 0". So don’t think it mandates that.

> 
> > Thanks,
> > Shameer
> >
> > ---
> >   drivers/irqchip/irq-gic-v3.c | 33 ++++++++++++++++++++++++++++-----
> >   1 file changed, 28 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> > index 16fecc0febe8..15fa1eea45e4 100644
> > --- a/drivers/irqchip/irq-gic-v3.c
> > +++ b/drivers/irqchip/irq-gic-v3.c
> > @@ -1835,6 +1835,27 @@ static void __init
> gic_populate_ppi_partitions(struct device_node *gic_node)
> >   	of_node_put(parts_node);
> >   }
> >
> > +/* SRE bit being RAO/WI implies no GICv2 legacy mode support */
> 
> I'm wondering if this is a mandate of the architecture.

As I mentioned above, I am not sure this is the best way, though,
section 1.3.5 of GICv3 spec, says(for no legacy support case "affinity
routing and system register access are permanently enabled. This means
that the associated control bits are RAO/WI"

But again later in the spec, it uses "might choose to
make this bit RAO/WI". So it is arguable that it mandates it or not.

I leave that to Marc :)

Thanks,
Shameer 

> > enabled.
> > +static bool __init gic_gicv2_compatible(void)
> > +{
> > +	u32 org, val;
> > +
> > +	org = gic_read_sre();
> > +	if (!(org & ICC_SRE_EL1_SRE))
> > +		return true;
> > +
> > +	val = org & ~ICC_SRE_EL1_SRE;
> > +	gic_write_sre(val);
> > +
> > +	val = gic_read_sre();
> > +	gic_write_sre(org);
> > +
> > +	if (val & ICC_SRE_EL1_SRE)
> > +		return false;
> > +
> > +	return true;
> > +}
> > +
> >   static void __init gic_of_setup_kvm_info(struct device_node *node)
> >   {
> >   	int ret;
> > @@ -1851,10 +1872,12 @@ static void __init gic_of_setup_kvm_info(struct
> device_node *node)
> >   				 &gicv_idx))
> >   		gicv_idx = 1;
> >
> > -	gicv_idx += 3;	/* Also skip GICD, GICC, GICH */
> > -	ret = of_address_to_resource(node, gicv_idx, &r);
> > -	if (!ret)
> > -		gic_v3_kvm_info.vcpu = r;
> > +	if (gic_gicv2_compatible()) {
> > +		gicv_idx += 3;	/* Also skip GICD, GICC, GICH */
> > +		ret = of_address_to_resource(node, gicv_idx, &r);
> > +		if (!ret)
> > +			gic_v3_kvm_info.vcpu = r;
> > +	}
> >
> >   	gic_v3_kvm_info.has_v4 = gic_data.rdists.has_vlpis;
> >   	gic_v3_kvm_info.has_v4_1 = gic_data.rdists.has_rvpeid;
> > @@ -2164,7 +2187,7 @@ static void __init gic_acpi_setup_kvm_info(void)
> >
> >   	gic_v3_kvm_info.maint_irq = irq;
> >
> > -	if (acpi_data.vcpu_base) {
> > +	if (gic_gicv2_compatible() && acpi_data.vcpu_base) {
> >   		struct resource *vcpu = &gic_v3_kvm_info.vcpu;
> >
> >   		vcpu->flags = IORESOURCE_MEM;
> 
> Thanks,
> Zenghui