Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 8bit
Date:   Mon, 30 Nov 2020 13:16:49 +0000
From:   Marc Zyngier <maz@kernel.org>
To:     Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
Cc:     yuzenghui <yuzenghui@huawei.com>, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        Linuxarm <linuxarm@huawei.com>, eric.auger@redhat.com
Subject: Re: [PATCH] irqchip/gic-v3: Check SRE bit for GICv2 legacy support
In-Reply-To: <ae78e69ded9a45bf82832241560bcee0@huawei.com>
References: <20201130102639.7504-1-shameerali.kolothum.thodi@huawei.com>
 <f6e8ee71-f76b-428c-cd56-5bc93b1afafe@huawei.com>
 <ae78e69ded9a45bf82832241560bcee0@huawei.com>
User-Agent: Roundcube Webmail/1.4.9
Message-ID: <7422841eba7a0243c8aaefb6c0f044bf@kernel.org>
Precedence: bulk

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 <shameerali.kolothum.thodi@huawei.com>;
>> linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
>> Cc: maz@kernel.org; Linuxarm <linuxarm@huawei.com>;
>> eric.auger@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@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.

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.
-- 
Jazz is not dead. It just smells funny...