Message-ID: <85bb389a75f347d29ab3f75e4cfae060@huawei.com>
Date:   Mon, 30 Nov 2020 13:55:49 +0000
From:   Shameerali Kolothum Thodi <shameerali.kolothum.thodi@...wei.com>
To:     Marc Zyngier <maz@...nel.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "eric.auger@...hat.com" <eric.auger@...hat.com>,
        Linuxarm <linuxarm@...wei.com>
Subject: RE: [PATCH] irqchip/gic-v3: Check SRE bit for GICv2 legacy support

Hi Marc,

> -----Original Message-----
> From: Marc Zyngier [mailto:maz@...nel.org]
> Sent: 30 November 2020 12:28
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@...wei.com>
> Cc: linux-kernel@...r.kernel.org; linux-arm-kernel@...ts.infradead.org;
> eric.auger@...hat.com; Linuxarm <linuxarm@...wei.com>
> Subject: Re: [PATCH] irqchip/gic-v3: Check SRE bit for GICv2 legacy support
> 
> Hi Shameer,
> 
> On 2020-11-30 10: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)
> 
> That'd be true of *any* guest using GICv2, not just when using QEMU as
> the VMM, right?

Yes, I would think so.

> > hangs when firmware falsely reports this address on systems that don't
> > have support for legacy mode.
> 
> And I guess it isn't just the guest that hangs, but the whole system can
> go south (it would be totally legitimate for the HW to deliver a
> SError).

So far I haven’t seen that happening. I was able to kill the Guest and recover.
But the annoying thing is Guest boot hangs at random places without any
error reported and people end up spending lot of time only to be told later
that gic-version=3 is missing from their scripts.  
 
> > 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@...wei.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.
> 
> What a bore. Is this glorious firmware really out in the wild?
 
:(. I am afraid it is. 

> > 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).
> 
> I don't think there is any other reliable way, but see below.
> 
> >
> > 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 */
> > +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;
> 
> The problem is that this gic_gicv2_compatible() comes in way too late,
> and you are disabling SRE while we already have configured tons of
> stuff.
> This has the potential for breaking things unexpectedly.

Ok. Makes sense. 

> How about this instead? Completely untested, of course.

Thanks for that. I just tested and it works.

Shameer

> Thanks,
> 
>          M.
> 
> diff --git a/arch/arm64/kernel/cpufeature.c
> b/arch/arm64/kernel/cpufeature.c
> index dcc165b3fc04..c42f39154af9 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -1200,7 +1200,7 @@ static bool has_useable_gicv3_cpuif(const struct
> arm64_cpu_capabilities *entry,
>   	if (!has_cpuid_feature(entry, scope))
>   		return false;
> 
> -	has_sre = gic_enable_sre();
> +	has_sre = gic_enable_sre(NULL);
>   	if (!has_sre)
>   		pr_warn_once("%s present but disabled by higher exception level\n",
>   			     entry->desc);
> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> index 09c96f57818c..20a7102612da 100644
> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -182,7 +182,7 @@ static void init_gic_priority_masking(void)
>   {
>   	u32 cpuflags;
> 
> -	if (WARN_ON(!gic_enable_sre()))
> +	if (WARN_ON(!gic_enable_sre(NULL)))
>   		return;
> 
>   	cpuflags = read_sysreg(daif);
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 16fecc0febe8..db5ce3dd01c7 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -103,6 +103,8 @@ EXPORT_SYMBOL(gic_nonsecure_priorities);
>   /* ppi_nmi_refs[n] == number of cpus having ppi[n + 16] set as NMI */
>   static refcount_t *ppi_nmi_refs;
> 
> +static bool has_v2_compat;
> +
>   static struct gic_kvm_info gic_v3_kvm_info;
>   static DEFINE_PER_CPU(bool, has_rss);
> 
> @@ -915,7 +917,7 @@ static void gic_cpu_sys_reg_init(void)
>   	 *
>   	 * Kindly inform the luser.
>   	 */
> -	if (!gic_enable_sre())
> +	if (!gic_enable_sre(&has_v2_compat))
>   		pr_err("GIC: unable to set SRE (disabled at EL2), panic ahead\n");
> 
>   	pribits = gic_get_pribits();
> @@ -1853,7 +1855,7 @@ static void __init gic_of_setup_kvm_info(struct
> device_node *node)
> 
>   	gicv_idx += 3;	/* Also skip GICD, GICC, GICH */
>   	ret = of_address_to_resource(node, gicv_idx, &r);
> -	if (!ret)
> +	if (has_v2_compat && !ret)
>   		gic_v3_kvm_info.vcpu = r;
> 
>   	gic_v3_kvm_info.has_v4 = gic_data.rdists.has_vlpis;
> @@ -2164,7 +2166,7 @@ static void __init gic_acpi_setup_kvm_info(void)
> 
>   	gic_v3_kvm_info.maint_irq = irq;
> 
> -	if (acpi_data.vcpu_base) {
> +	if (has_v2_compat && acpi_data.vcpu_base) {
>   		struct resource *vcpu = &gic_v3_kvm_info.vcpu;
> 
>   		vcpu->flags = IORESOURCE_MEM;
> diff --git a/include/linux/irqchip/arm-gic-v3.h
> b/include/linux/irqchip/arm-gic-v3.h
> index f6d092fdb93d..bdc390529367 100644
> --- a/include/linux/irqchip/arm-gic-v3.h
> +++ b/include/linux/irqchip/arm-gic-v3.h
> @@ -693,11 +693,22 @@ int its_init(struct fwnode_handle *handle, struct
> rdists *rdists,
>   	     struct irq_domain *domain);
>   int mbi_init(struct fwnode_handle *fwnode, struct irq_domain *parent);
> 
> -static inline bool gic_enable_sre(void)
> +static inline bool gic_enable_sre(bool *has_v2)
>   {
>   	u32 val;
> 
>   	val = gic_read_sre();
> +	if (has_v2) {
> +		if (!(val & ICC_SRE_EL1_SRE)) {
> +			*has_v2 = true;
> +		} else {
> +			val &= ~ICC_SRE_EL1_SRE;
> +			gic_write_sre(val);
> +			val = gic_read_sre();
> +			*has_v2 = !(val & ICC_SRE_EL1_SRE);
> +		}
> +	}
> +
>   	if (val & ICC_SRE_EL1_SRE)
>   		return true;
> 
> --
> Jazz is not dead. It just smells funny...

Powered by blists - more mailing lists