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

Commit Message

Shameerali Kolothum Thodi Nov. 30, 2020, 10:26 a.m. UTC

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. 

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 <[e-mail]>
---
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).

Thanks,
Shameer

---
 drivers/irqchip/irq-gic-v3.c | 33 ++++++++++++++++++++++++++++-----
 1 file changed, 28 insertions(+), 5 deletions(-)

Comments

Zenghui Yu Nov. 30, 2020, 11:50 a.m. UTC | #1

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.

> 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 <[e-mail]>
> ---
> 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.

> 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.

> +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

Shameerali Kolothum Thodi Nov. 30, 2020, 12:06 p.m. UTC | #2

Hi Zenghui,

> -----Original Message-----
> From: yuzenghui
> Sent: 30 November 2020 11:51
> To: Shameerali Kolothum Thodi <[e-mail]>;
> [e-mail]; [e-mail]
> Cc: [e-mail]; Linuxarm <[e-mail]>;
> [e-mail]
> 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 <[e-mail]>
> > ---
> > 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

Marc Zyngier Nov. 30, 2020, 12:28 p.m. UTC | #3

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?

> 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).

> 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 <[e-mail]>
> ---
> 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?

> 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.

How about this instead? Completely untested, of course.

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;

Marc Zyngier Nov. 30, 2020, 1:16 p.m. UTC | #4

On 2020-11-30 12:06, Shameerali Kolothum Thodi wrote:
> Hi Zenghui,
> 
>> -----Original Message-----
>> From: yuzenghui
>> Sent: 30 November 2020 11:51
>> To: Shameerali Kolothum Thodi <[e-mail]>;
>> [e-mail]; [e-mail]
>> Cc: [e-mail]; Linuxarm <[e-mail]>;
>> [e-mail]
>> 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 <[e-mail]>
>> > ---
>> > 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.

Note: *GICv2*, not GICv3 with v2 compatibility. I still think the 
firmware
should be fixed. But that also relies on finding out whether the broken
FW is in the wild or not. If it is already, we need something in the 
kernel.

>> 
>> > 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 :)

- If we cannot clear SRE, then we cannot use v2 compat, and we're good.

- If we can clear SRE and that there is no GICV region, we're goo too.

- If we can clear SRE and that there is a *bogus* GICV region, there
   is nothing we can do and the machine will explode when the guest
   pokes at it.

Using ARE would be tempting, but AFAIKT it is only relevant to the
physical side of the GIC, and has no bearing on the virtual side
(since the distributor is itself virtual).

Thanks,

         M.

Shameerali Kolothum Thodi Nov. 30, 2020, 1:55 p.m. UTC | #5

Hi Marc,

> -----Original Message-----
> From: Marc Zyngier [mailto:[e-mail]]
> Sent: 30 November 2020 12:28
> To: Shameerali Kolothum Thodi <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; Linuxarm <[e-mail]>
> 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 <[e-mail]>
> > ---
> > 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...

Marc Zyngier Nov. 30, 2020, 2:56 p.m. UTC | #6

Hi Shameer,

On 2020-11-30 13:55, Shameerali Kolothum Thodi wrote:
> Hi Marc,
> 
>> -----Original Message-----
>> From: Marc Zyngier [mailto:[e-mail]]
>> Sent: 30 November 2020 12:28
>> To: Shameerali Kolothum Thodi <[e-mail]>
>> Cc: [e-mail]; 
>> [e-mail];
>> [e-mail]; Linuxarm <[e-mail]>
>> 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.

That's pretty lucky. The guest has been reading/writing to random 
places,
and depending on where this maps in the physical space, anything can
happen. Out  of (morbid) curiosity, what is at the address pointed to by
GICC in MADT?

> 
>> > 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 <[e-mail]>
>> > ---
>> > 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.

Meh. We'll have to paper over it then. How urgent is that?

[...]

>> How about this instead? Completely untested, of course.
> 
> Thanks for that. I just tested and it works.

OK. I'll rework it a bit and post it as a complete patch. Is there an
erratum number on your side?

Thanks,

         M.

Shameerali Kolothum Thodi Nov. 30, 2020, 4:20 p.m. UTC | #7

> -----Original Message-----
> From: Marc Zyngier [mailto:[e-mail]]
> Sent: 30 November 2020 14:57
> To: Shameerali Kolothum Thodi <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; Linuxarm <[e-mail]>
> Subject: Re: [PATCH] irqchip/gic-v3: Check SRE bit for GICv2 legacy support
> 
> Hi Shameer,
> 
> On 2020-11-30 13:55, Shameerali Kolothum Thodi wrote:
> > Hi Marc,
> >
> >> -----Original Message-----
> >> From: Marc Zyngier [mailto:[e-mail]]
> >> Sent: 30 November 2020 12:28
> >> To: Shameerali Kolothum Thodi <[e-mail]>
> >> Cc: [e-mail];
> >> [e-mail];
> >> [e-mail]; Linuxarm <[e-mail]>
> >> 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.
> 
> That's pretty lucky. The guest has been reading/writing to random
> places,
> and depending on where this maps in the physical space, anything can
> happen. Out  of (morbid) curiosity, what is at the address pointed to by
> GICC in MADT?

This is what it reports,

[02Ch 0044   1]                Subtable Type : 0B [Generic Interrupt Controller]
[02Dh 0045   1]                       Length : 50
...
[04Ch 0076   8]                 Base Address : 000000009B000000
[054h 0084   8]     Virtual GIC Base Address : 000000009B020000
[05Ch 0092   8]  Hypervisor GIC Base Address : 000000009B010000
[064h 0100   4]        Virtual GIC Interrupt : 00000019
[068h 0104   8]   Redistributor Base Address : 00000000AE100000
[070h 0112   8]                    ARM MPIDR : 0000000000080000
[078h 0120   1]             Efficiency Class : 15
[079h 0121   3]                     Reserved : 001500

> >
> >> > 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
> <[e-mail]>
> >> > ---
> >> > 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.
> 
> Meh. We'll have to paper over it then. How urgent is that?

It is not that urgent urgent but 5.10 support would be nice :)

> 
> [...]
> 
> >> How about this instead? Completely untested, of course.
> >
> > Thanks for that. I just tested and it works.
> 
> OK. I'll rework it a bit and post it as a complete patch. Is there an
> erratum number on your side?

Sure. I am not sure on erratum, but will check internally and get back to you
if there is one.

Thanks,
Shameer
> 
> Thanks,
> 
>          M.
> --
> Jazz is not dead. It just smells funny...

Ard Biesheuvel Nov. 30, 2020, 6:31 p.m. UTC | #8

On Mon, 30 Nov 2020 at 17:22, Shameerali Kolothum Thodi
<[e-mail]> wrote:
>
>
>
> > -----Original Message-----
> > From: Marc Zyngier [mailto:[e-mail]]
> > Sent: 30 November 2020 14:57
> > To: Shameerali Kolothum Thodi <[e-mail]>
> > Cc: [e-mail]; [e-mail];
> > [e-mail]; Linuxarm <[e-mail]>
> > Subject: Re: [PATCH] irqchip/gic-v3: Check SRE bit for GICv2 legacy support
> >
> > Hi Shameer,
> >
> > On 2020-11-30 13:55, Shameerali Kolothum Thodi wrote:
> > > Hi Marc,
> > >
> > >> -----Original Message-----
> > >> From: Marc Zyngier [mailto:[e-mail]]
> > >> Sent: 30 November 2020 12:28
> > >> To: Shameerali Kolothum Thodi <[e-mail]>
> > >> Cc: [e-mail];
> > >> [e-mail];
> > >> [e-mail]; Linuxarm <[e-mail]>
> > >> 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.
> >
> > That's pretty lucky. The guest has been reading/writing to random
> > places,
> > and depending on where this maps in the physical space, anything can
> > happen. Out  of (morbid) curiosity, what is at the address pointed to by
> > GICC in MADT?
>
> This is what it reports,
>
> [02Ch 0044   1]                Subtable Type : 0B [Generic Interrupt Controller]
> [02Dh 0045   1]                       Length : 50
> ...
> [04Ch 0076   8]                 Base Address : 000000009B000000
> [054h 0084   8]     Virtual GIC Base Address : 000000009B020000
> [05Ch 0092   8]  Hypervisor GIC Base Address : 000000009B010000
> [064h 0100   4]        Virtual GIC Interrupt : 00000019
> [068h 0104   8]   Redistributor Base Address : 00000000AE100000
> [070h 0112   8]                    ARM MPIDR : 0000000000080000
> [078h 0120   1]             Efficiency Class : 15
> [079h 0121   3]                     Reserved : 001500
>
> > >
> > >> > 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
> > <[e-mail]>
> > >> > ---
> > >> > 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.
> >
> > Meh. We'll have to paper over it then. How urgent is that?
>
> It is not that urgent urgent but 5.10 support would be nice :)
>
> >
> > [...]
> >
> > >> How about this instead? Completely untested, of course.
> > >
> > > Thanks for that. I just tested and it works.
> >
> > OK. I'll rework it a bit and post it as a complete patch. Is there an
> > erratum number on your side?
>
> Sure. I am not sure on erratum, but will check internally and get back to you
> if there is one.
>

Any clue why production D06 firmware deviates from the D06 port that
exists in Tianocore's edk2-platforms repository? Because that version
does not have this bug, and I wonder why that code was upstreamed at
all if a substantially different version gets shipped with production
hardware.

Shameerali Kolothum Thodi Dec. 2, 2020, 8:23 a.m. UTC | #9

[+]

> -----Original Message-----
> From: Ard Biesheuvel [mailto:[e-mail]]
> Sent: 30 November 2020 18:32
> To: Shameerali Kolothum Thodi <[e-mail]>
> Cc: Marc Zyngier <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; Linuxarm
> <[e-mail]>
> Subject: Re: [PATCH] irqchip/gic-v3: Check SRE bit for GICv2 legacy support
> 
...

> 
> Any clue why production D06 firmware deviates from the D06 port that
> exists in Tianocore's edk2-platforms repository? Because that version
> does not have this bug, and I wonder why that code was upstreamed at
> all if a substantially different version gets shipped with production
> hardware.

Ok. Thanks for pointing this out. I have informed our UEFI team about this.
They will check Internally and clarify.

Regards,
Shameer

wanghuiqiang Dec. 15, 2020, 7:49 a.m. UTC | #10

Sorry response late.
Hi Shameer & Ard,

Could you let me know which firmware you are using? If the difference is Madt table vGIC your pointed , they are the same. We changed the vGIC memory base address at very early design stage.

Thanks！ 

-----邮件原件-----
发件人: Shameerali Kolothum Thodi 
发送时间: 2020年12月2日 16:23
收件人: Ard Biesheuvel <[e-mail]>
抄送: Marc Zyngier <[e-mail]>; [e-mail]; [e-mail]; [e-mail]; Linuxarm <[e-mail]>; wanghuiqiang <[e-mail]>; xuwei (O) <[e-mail]>
主题: RE: [PATCH] irqchip/gic-v3: Check SRE bit for GICv2 legacy support

[+]

> -----Original Message-----
> From: Ard Biesheuvel [mailto:[e-mail]]
> Sent: 30 November 2020 18:32
> To: Shameerali Kolothum Thodi <[e-mail]>
> Cc: Marc Zyngier <[e-mail]>; [e-mail]; 
> [e-mail]; [e-mail]; 
> Linuxarm <[e-mail]>
> Subject: Re: [PATCH] irqchip/gic-v3: Check SRE bit for GICv2 legacy 
> support
> 
...

> 
> Any clue why production D06 firmware deviates from the D06 port that 
> exists in Tianocore's edk2-platforms repository? Because that version 
> does not have this bug, and I wonder why that code was upstreamed at 
> all if a substantially different version gets shipped with production 
> hardware.

Ok. Thanks for pointing this out. I have informed our UEFI team about this.
They will check Internally and clarify.

Regards,
Shameer

Shameerali Kolothum Thodi Dec. 15, 2020, 10 a.m. UTC | #11

Hi Wanghuiqiang,


> -----Original Message-----
> From: wanghuiqiang
> Sent: 15 December 2020 07:49
> To: Shameerali Kolothum Thodi <[e-mail]>;
> Ard Biesheuvel <[e-mail]>
> Cc: Marc Zyngier <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; Linuxarm
> <[e-mail]>; xuwei (O) <[e-mail]>
> Subject: 答复: [PATCH] irqchip/gic-v3: Check SRE bit for GICv2 legacy support
> 
> Sorry response late.
> Hi Shameer & Ard,
> 
> Could you let me know which firmware you are using? If the difference is Madt
> table vGIC your pointed , they are the same. We changed the vGIC memory
> base address at very early design stage.

I checked the below ones and all these boards has the issue,

Openlab-Board - 69009,

DMI: Huawei TaiShan 2280 V2/BC82AMDC, BIOS 2280-V2 CS V3.B270.01 05/08/2020

Openlab-Board-69008,

DMI: Huawei TaiShan 2280 V2/BC82AMDC, BIOS 2280-V2 CS V5.B030.01 07/03/2020

UK-D06CS-board,

Boot firmware (version 2280-V2 CS V3.B220.01 built at 03/19/2020  16:52)

Thanks,
Shameer

> Thanks！
> 
> -----邮件原件-----
> 发件人: Shameerali Kolothum Thodi
> 发送时间: 2020年12月2日 16:23
> 收件人: Ard Biesheuvel <[e-mail]>
> 抄送: Marc Zyngier <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; Linuxarm
> <[e-mail]>; wanghuiqiang <[e-mail]>; xuwei
> (O) <[e-mail]>
> 主题: RE: [PATCH] irqchip/gic-v3: Check SRE bit for GICv2 legacy support
> 
> [+]
> 
> > -----Original Message-----
> > From: Ard Biesheuvel [mailto:[e-mail]]
> > Sent: 30 November 2020 18:32
> > To: Shameerali Kolothum Thodi <[e-mail]>
> > Cc: Marc Zyngier <[e-mail]>; [e-mail];
> > [e-mail]; [e-mail];
> > Linuxarm <[e-mail]>
> > Subject: Re: [PATCH] irqchip/gic-v3: Check SRE bit for GICv2 legacy
> > support
> >
> ...
> 
> >
> > Any clue why production D06 firmware deviates from the D06 port that
> > exists in Tianocore's edk2-platforms repository? Because that version
> > does not have this bug, and I wonder why that code was upstreamed at
> > all if a substantially different version gets shipped with production
> > hardware.
> 
> Ok. Thanks for pointing this out. I have informed our UEFI team about this.
> They will check Internally and clarify.
> 
> Regards,
> Shameer

wanghuiqiang Jan. 6, 2021, 9:22 a.m. UTC | #12

Hi Ard and all,

The issue is root caused, it is introduced by BIOS new feature implemented. 
With old BIOS，we use static MADT table and the GICV/GICH is set to 0 and reported this table to OS. But we added new features which will dynamic update MADT table based on some external input, the developer is set GICV/GICH as what we have done like previous generation chipset code did. But in fact, there is different compared with old generation chipset code.
I'll let my internal team know this and fix this issue in later BIOS release.

Thanks!  

-----邮件原件-----
发件人: wanghuiqiang 
发送时间: 2020年12月15日 15:49
收件人: Shameerali Kolothum Thodi <[e-mail]>; Ard Biesheuvel <[e-mail]>
抄送: Marc Zyngier <[e-mail]>; [e-mail]; [e-mail]; [e-mail]; Linuxarm <[e-mail]>; xuwei (O) <[e-mail]>
主题: 答复: [PATCH] irqchip/gic-v3: Check SRE bit for GICv2 legacy support

Sorry response late.
Hi Shameer & Ard,

Could you let me know which firmware you are using? If the difference is Madt table vGIC your pointed , they are the same. We changed the vGIC memory base address at very early design stage.

Thanks！ 

-----邮件原件-----
发件人: Shameerali Kolothum Thodi
发送时间: 2020年12月2日 16:23
收件人: Ard Biesheuvel <[e-mail]>
抄送: Marc Zyngier <[e-mail]>; [e-mail]; [e-mail]; [e-mail]; Linuxarm <[e-mail]>; wanghuiqiang <[e-mail]>; xuwei (O) <[e-mail]>
主题: RE: [PATCH] irqchip/gic-v3: Check SRE bit for GICv2 legacy support

[+]

> -----Original Message-----
> From: Ard Biesheuvel [mailto:[e-mail]]
> Sent: 30 November 2020 18:32
> To: Shameerali Kolothum Thodi <[e-mail]>
> Cc: Marc Zyngier <[e-mail]>; [e-mail]; 
> [e-mail]; [e-mail];
> Linuxarm <[e-mail]>
> Subject: Re: [PATCH] irqchip/gic-v3: Check SRE bit for GICv2 legacy 
> support
> 
...

> 
> Any clue why production D06 firmware deviates from the D06 port that 
> exists in Tianocore's edk2-platforms repository? Because that version 
> does not have this bug, and I wonder why that code was upstreamed at 
> all if a substantially different version gets shipped with production 
> hardware.

Ok. Thanks for pointing this out. I have informed our UEFI team about this.
They will check Internally and clarify.

Regards,
Shameer

Shameerali Kolothum Thodi Jan. 7, 2021, 2:42 p.m. UTC | #13

> -----Original Message-----
> From: wanghuiqiang
> Sent: 06 January 2021 09:22
> To: Shameerali Kolothum Thodi <[e-mail]>;
> 'Ard Biesheuvel' <[e-mail]>
> Cc: 'Marc Zyngier' <[e-mail]>; '[e-mail]'
> <[e-mail]>; '[e-mail]'
> <[e-mail]>; '[e-mail]'
> <[e-mail]>; Linuxarm <[e-mail]>;
> xuwei (O) <[e-mail]>
> Subject: 答复: [PATCH] irqchip/gic-v3: Check SRE bit for GICv2 legacy support
> 
> Hi Ard and all,
> 
> The issue is root caused, it is introduced by BIOS new feature implemented.
> With old BIOS，we use static MADT table and the GICV/GICH is set to 0 and
> reported this table to OS. But we added new features which will dynamic
> update MADT table based on some external input, the developer is set
> GICV/GICH as what we have done like previous generation chipset code did.
> But in fact, there is different compared with old generation chipset code.
> I'll let my internal team know this and fix this issue in later BIOS release.

Thanks Wanghuiqiang for your efforts and confirming the issue.

Hi Marc,

Considering the fact that we have systems out there with the faulty BIOS, and it is
not necessarily everyone will be keen to update the BIOS, I think it is better to
address this in kernel as well.

As discussed earlier, please consider the SRE bit based solution to make the logic
more robust irrespective of what BIOS provides.

(I don’t have an erratum id for this as I am told we keep that for Hardware issues
only, but we are using DTS202101070OAGUIP1L00 to track the issue and can be
used as reference).

Thanks,
Shameer 

> Thanks!
> 
> -----邮件原件-----
> 发件人: wanghuiqiang
> 发送时间: 2020年12月15日 15:49
> 收件人: Shameerali Kolothum Thodi
> <[e-mail]>; Ard Biesheuvel
> <[e-mail]>
> 抄送: Marc Zyngier <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; Linuxarm
> <[e-mail]>; xuwei (O) <[e-mail]>
> 主题: 答复: [PATCH] irqchip/gic-v3: Check SRE bit for GICv2 legacy support
> 
> Sorry response late.
> Hi Shameer & Ard,
> 
> Could you let me know which firmware you are using? If the difference is Madt
> table vGIC your pointed , they are the same. We changed the vGIC memory
> base address at very early design stage.
> 
> Thanks！
> 
> -----邮件原件-----
> 发件人: Shameerali Kolothum Thodi
> 发送时间: 2020年12月2日 16:23
> 收件人: Ard Biesheuvel <[e-mail]>
> 抄送: Marc Zyngier <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; Linuxarm
> <[e-mail]>; wanghuiqiang <[e-mail]>; xuwei
> (O) <[e-mail]>
> 主题: RE: [PATCH] irqchip/gic-v3: Check SRE bit for GICv2 legacy support
> 
> [+]
> 
> > -----Original Message-----
> > From: Ard Biesheuvel [mailto:[e-mail]]
> > Sent: 30 November 2020 18:32
> > To: Shameerali Kolothum Thodi <[e-mail]>
> > Cc: Marc Zyngier <[e-mail]>; [e-mail];
> > [e-mail]; [e-mail];
> > Linuxarm <[e-mail]>
> > Subject: Re: [PATCH] irqchip/gic-v3: Check SRE bit for GICv2 legacy
> > support
> >
> ...
> 
> >
> > Any clue why production D06 firmware deviates from the D06 port that
> > exists in Tianocore's edk2-platforms repository? Because that version
> > does not have this bug, and I wonder why that code was upstreamed at
> > all if a substantially different version gets shipped with production
> > hardware.
> 
> Ok. Thanks for pointing this out. I have informed our UEFI team about this.
> They will check Internally and clarify.
> 
> Regards,
> Shameer

Patch

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;