[PATCH] irq/gic-its: gicv4: set VPENDING table as inner-shareable

[PATCH] irq/gic-its: gicv4: set VPENDING table as inner-shareable

[Heyi Guo]

There is no special reason to set virtual LPI pending table as
non-shareable. If we choose to hard code the shareability without
probing, inner-shareable will be a better choice, for all the other
ITS/GICR tables prefer to be inner-shareable.

What's more, on Hisilicon hip08 it will trigger some kind of bus
warning when mixing use of different shareabilities.

Signed-off-by: Heyi Guo <guoheyi@huawei.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Marc Zyngier <maz@kernel.org>
---
 drivers/irqchip/irq-gic-v3-its.c   | 2 +-
 include/linux/irqchip/arm-gic-v3.h | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 787e8eec9a7f..d31e863bc9ef 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -2831,7 +2831,7 @@ static void its_vpe_schedule(struct its_vpe *vpe)
 	val  = virt_to_phys(page_address(vpe->vpt_page)) &
 		GENMASK_ULL(51, 16);
 	val |= GICR_VPENDBASER_RaWaWb;
-	val |= GICR_VPENDBASER_NonShareable;
+	val |= GICR_VPENDBASER_InnerShareable;
 	/*
 	 * There is no good way of finding out if the pending table is
 	 * empty as we can race against the doorbell interrupt very
diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
index 5cc10cf7cb3e..a184f875e451 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -289,6 +289,9 @@
 #define GICR_VPENDBASER_NonShareable					\
 	GIC_BASER_SHAREABILITY(GICR_VPENDBASER, NonShareable)
 
+#define GICR_VPENDBASER_InnerShareable					\
+	GIC_BASER_SHAREABILITY(GICR_VPENDBASER, InnerShareable)
+
 #define GICR_VPENDBASER_nCnB	GIC_BASER_CACHEABILITY(GICR_VPENDBASER, INNER, nCnB)
 #define GICR_VPENDBASER_nC 	GIC_BASER_CACHEABILITY(GICR_VPENDBASER, INNER, nC)
 #define GICR_VPENDBASER_RaWt	GIC_BASER_CACHEABILITY(GICR_VPENDBASER, INNER, RaWt)
-- 
2.19.1

Re: [PATCH] irq/gic-its: gicv4: set VPENDING table as inner-shareable

[Marc Zyngier]

On Sat, 30 Nov 2019 15:38:49 +0800
Heyi Guo <guoheyi@huawei.com> wrote:

> There is no special reason to set virtual LPI pending table as
> non-shareable. If we choose to hard code the shareability without
> probing, inner-shareable will be a better choice, for all the other
> ITS/GICR tables prefer to be inner-shareable.

One of the issues is that we have strictly no idea what the caches are
Inner Shareable with (I've been asking for such clarification for years
without getting anywhere). You can have as many disconnected inner
shareable domains as you want!

I suspect that in the grand scheme of things, the redistributors
ought to be in the same inner shareable domain, and that with a bit of
luck, the CPUs are there as well. Still, that's a massive guess.

> What's more, on Hisilicon hip08 it will trigger some kind of bus
> warning when mixing use of different shareabilities.

Do you have more information about what the bus is complaining about?
Is that because the CPUs have these pages mapped as inner shareable?

I'll give it a go on D05 (HIP07) to find out what changes there.

Thanks,

	M.

> Signed-off-by: Heyi Guo <guoheyi@huawei.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: Marc Zyngier <maz@kernel.org>
> ---
>  drivers/irqchip/irq-gic-v3-its.c   | 2 +-
>  include/linux/irqchip/arm-gic-v3.h | 3 +++
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 787e8eec9a7f..d31e863bc9ef 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -2831,7 +2831,7 @@ static void its_vpe_schedule(struct its_vpe *vpe)
>  	val  = virt_to_phys(page_address(vpe->vpt_page)) &
>  		GENMASK_ULL(51, 16);
>  	val |= GICR_VPENDBASER_RaWaWb;
> -	val |= GICR_VPENDBASER_NonShareable;
> +	val |= GICR_VPENDBASER_InnerShareable;
>  	/*
>  	 * There is no good way of finding out if the pending table is
>  	 * empty as we can race against the doorbell interrupt very
> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
> index 5cc10cf7cb3e..a184f875e451 100644
> --- a/include/linux/irqchip/arm-gic-v3.h
> +++ b/include/linux/irqchip/arm-gic-v3.h
> @@ -289,6 +289,9 @@
>  #define GICR_VPENDBASER_NonShareable					\
>  	GIC_BASER_SHAREABILITY(GICR_VPENDBASER, NonShareable)
>  
> +#define GICR_VPENDBASER_InnerShareable					\
> +	GIC_BASER_SHAREABILITY(GICR_VPENDBASER, InnerShareable)
> +
>  #define GICR_VPENDBASER_nCnB	GIC_BASER_CACHEABILITY(GICR_VPENDBASER, INNER, nCnB)
>  #define GICR_VPENDBASER_nC 	GIC_BASER_CACHEABILITY(GICR_VPENDBASER, INNER, nC)
>  #define GICR_VPENDBASER_RaWt	GIC_BASER_CACHEABILITY(GICR_VPENDBASER, INNER, RaWt)



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

Re: [PATCH] irq/gic-its: gicv4: set VPENDING table as inner-shareable

[Guoheyi]

在 2019/12/2 2:04, Marc Zyngier 写道:
> On Sat, 30 Nov 2019 15:38:49 +0800
> Heyi Guo <guoheyi@huawei.com> wrote:
>
>> There is no special reason to set virtual LPI pending table as
>> non-shareable. If we choose to hard code the shareability without
>> probing, inner-shareable will be a better choice, for all the other
>> ITS/GICR tables prefer to be inner-shareable.
> One of the issues is that we have strictly no idea what the caches are
> Inner Shareable with (I've been asking for such clarification for years
> without getting anywhere). You can have as many disconnected inner
> shareable domains as you want!

Hisilicon HIP07 and HIP08 are compliant with ARM SBSA and have only one 
inner shareable domain in the whole system.

What will happen if a system has multiple inner shareable domains? Will 
Linux still work on such system? Can we declare that Linux only supports 
one single inner shareable domain?

>
> I suspect that in the grand scheme of things, the redistributors
> ought to be in the same inner shareable domain, and that with a bit of
> luck, the CPUs are there as well. Still, that's a massive guess.
>
>> What's more, on Hisilicon hip08 it will trigger some kind of bus
>> warning when mixing use of different shareabilities.
> Do you have more information about what the bus is complaining about?
> Is that because the CPUs have these pages mapped as inner shareable?

Actually HIP08 L3 Cache will complain on any non-shareable cache entry, 
for the data coherence cannot be guarenteed for such configuration. This 
also implies VPENDING table will be allocated and snooped in L3 cache.

>
> I'll give it a go on D05 (HIP07) to find out what changes there.

Thanks,

HG

>
> Thanks,
>
> 	M.
>
>> Signed-off-by: Heyi Guo <guoheyi@huawei.com>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Jason Cooper <jason@lakedaemon.net>
>> Cc: Marc Zyngier <maz@kernel.org>
>> ---
>>   drivers/irqchip/irq-gic-v3-its.c   | 2 +-
>>   include/linux/irqchip/arm-gic-v3.h | 3 +++
>>   2 files changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>> index 787e8eec9a7f..d31e863bc9ef 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -2831,7 +2831,7 @@ static void its_vpe_schedule(struct its_vpe *vpe)
>>   	val  = virt_to_phys(page_address(vpe->vpt_page)) &
>>   		GENMASK_ULL(51, 16);
>>   	val |= GICR_VPENDBASER_RaWaWb;
>> -	val |= GICR_VPENDBASER_NonShareable;
>> +	val |= GICR_VPENDBASER_InnerShareable;
>>   	/*
>>   	 * There is no good way of finding out if the pending table is
>>   	 * empty as we can race against the doorbell interrupt very
>> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
>> index 5cc10cf7cb3e..a184f875e451 100644
>> --- a/include/linux/irqchip/arm-gic-v3.h
>> +++ b/include/linux/irqchip/arm-gic-v3.h
>> @@ -289,6 +289,9 @@
>>   #define GICR_VPENDBASER_NonShareable					\
>>   	GIC_BASER_SHAREABILITY(GICR_VPENDBASER, NonShareable)
>>   
>> +#define GICR_VPENDBASER_InnerShareable					\
>> +	GIC_BASER_SHAREABILITY(GICR_VPENDBASER, InnerShareable)
>> +
>>   #define GICR_VPENDBASER_nCnB	GIC_BASER_CACHEABILITY(GICR_VPENDBASER, INNER, nCnB)
>>   #define GICR_VPENDBASER_nC 	GIC_BASER_CACHEABILITY(GICR_VPENDBASER, INNER, nC)
>>   #define GICR_VPENDBASER_RaWt	GIC_BASER_CACHEABILITY(GICR_VPENDBASER, INNER, RaWt)
>
>

Re: [PATCH] irq/gic-its: gicv4: set VPENDING table as inner-shareable

[Marc Zyngier]

On 2019-12-02 11:07, Guoheyi wrote:
> 在 2019/12/2 2:04, Marc Zyngier 写道:
>> On Sat, 30 Nov 2019 15:38:49 +0800
>> Heyi Guo <guoheyi@huawei.com> wrote:
>>
>>> There is no special reason to set virtual LPI pending table as
>>> non-shareable. If we choose to hard code the shareability without
>>> probing, inner-shareable will be a better choice, for all the other
>>> ITS/GICR tables prefer to be inner-shareable.
>> One of the issues is that we have strictly no idea what the caches 
>> are
>> Inner Shareable with (I've been asking for such clarification for 
>> years
>> without getting anywhere). You can have as many disconnected inner
>> shareable domains as you want!
>
> Hisilicon HIP07 and HIP08 are compliant with ARM SBSA and have only
> one inner shareable domain in the whole system.

I'm glad these systems are well designed, but that's not what SBSA 
mandates.

All it requires is that the all PEs are part of the same IS domain, and
that PCIe is part of the same IS domain as the PEs. Nothing more, and
certainly nothing about the GIC. Or anything else.

> What will happen if a system has multiple inner shareable domains?
> Will Linux still work on such system? Can we declare that Linux only
> supports one single inner shareable domain?

Linux works just fine as long as all the PEs are in the same IS domain.
There is no architectural requirement for anything else to be in that
domain.

>> I suspect that in the grand scheme of things, the redistributors
>> ought to be in the same inner shareable domain, and that with a bit 
>> of
>> luck, the CPUs are there as well. Still, that's a massive guess.
>>
>>> What's more, on Hisilicon hip08 it will trigger some kind of bus
>>> warning when mixing use of different shareabilities.
>> Do you have more information about what the bus is complaining 
>> about?
>> Is that because the CPUs have these pages mapped as inner shareable?
>
> Actually HIP08 L3 Cache will complain on any non-shareable cache
> entry, for the data coherence cannot be guarenteed for such
> configuration. This also implies VPENDING table will be allocated and
> snooped in L3 cache.

It really looks odd that L3 would even contain non-shareable entries.

Anyway, I don't think that's a biggy. Given that GICv4 is almost
exclusively implemented on these two SoCs (unless someone revives
QC system), I think we can take this change after some testing.

Thanks,

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

Re: [PATCH] irq/gic-its: gicv4: set VPENDING table as inner-shareable

[Heyi Guo]

Hi Marc,

On 2019/12/2 2:04, Marc Zyngier wrote:
> On Sat, 30 Nov 2019 15:38:49 +0800
> Heyi Guo <guoheyi@huawei.com> wrote:
>
>> There is no special reason to set virtual LPI pending table as
>> non-shareable. If we choose to hard code the shareability without
>> probing, inner-shareable will be a better choice, for all the other
>> ITS/GICR tables prefer to be inner-shareable.
> One of the issues is that we have strictly no idea what the caches are
> Inner Shareable with (I've been asking for such clarification for years
> without getting anywhere). You can have as many disconnected inner
> shareable domains as you want!
>
> I suspect that in the grand scheme of things, the redistributors
> ought to be in the same inner shareable domain, and that with a bit of
> luck, the CPUs are there as well. Still, that's a massive guess.
>
>> What's more, on Hisilicon hip08 it will trigger some kind of bus
>> warning when mixing use of different shareabilities.
> Do you have more information about what the bus is complaining about?
> Is that because the CPUs have these pages mapped as inner shareable?
>
> I'll give it a go on D05 (HIP07) to find out what changes there.

How's your go on D05? Did you see any issues?

Thanks,

Heyi


>
> Thanks,
>
> 	M.
>
>> Signed-off-by: Heyi Guo <guoheyi@huawei.com>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Jason Cooper <jason@lakedaemon.net>
>> Cc: Marc Zyngier <maz@kernel.org>
>> ---
>>   drivers/irqchip/irq-gic-v3-its.c   | 2 +-
>>   include/linux/irqchip/arm-gic-v3.h | 3 +++
>>   2 files changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>> index 787e8eec9a7f..d31e863bc9ef 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -2831,7 +2831,7 @@ static void its_vpe_schedule(struct its_vpe *vpe)
>>   	val  = virt_to_phys(page_address(vpe->vpt_page)) &
>>   		GENMASK_ULL(51, 16);
>>   	val |= GICR_VPENDBASER_RaWaWb;
>> -	val |= GICR_VPENDBASER_NonShareable;
>> +	val |= GICR_VPENDBASER_InnerShareable;
>>   	/*
>>   	 * There is no good way of finding out if the pending table is
>>   	 * empty as we can race against the doorbell interrupt very
>> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
>> index 5cc10cf7cb3e..a184f875e451 100644
>> --- a/include/linux/irqchip/arm-gic-v3.h
>> +++ b/include/linux/irqchip/arm-gic-v3.h
>> @@ -289,6 +289,9 @@
>>   #define GICR_VPENDBASER_NonShareable					\
>>   	GIC_BASER_SHAREABILITY(GICR_VPENDBASER, NonShareable)
>>   
>> +#define GICR_VPENDBASER_InnerShareable					\
>> +	GIC_BASER_SHAREABILITY(GICR_VPENDBASER, InnerShareable)
>> +
>>   #define GICR_VPENDBASER_nCnB	GIC_BASER_CACHEABILITY(GICR_VPENDBASER, INNER, nCnB)
>>   #define GICR_VPENDBASER_nC 	GIC_BASER_CACHEABILITY(GICR_VPENDBASER, INNER, nC)
>>   #define GICR_VPENDBASER_RaWt	GIC_BASER_CACHEABILITY(GICR_VPENDBASER, INNER, RaWt)
>
>

Re: [PATCH] irq/gic-its: gicv4: set VPENDING table as inner-shareable

[Marc Zyngier]

Hi Heyi,

On 2020-02-24 02:22, Heyi Guo wrote:
> Hi Marc,
> 
> On 2019/12/2 2:04, Marc Zyngier wrote:
>> On Sat, 30 Nov 2019 15:38:49 +0800
>> Heyi Guo <guoheyi@huawei.com> wrote:
>> 
>>> There is no special reason to set virtual LPI pending table as
>>> non-shareable. If we choose to hard code the shareability without
>>> probing, inner-shareable will be a better choice, for all the other
>>> ITS/GICR tables prefer to be inner-shareable.
>> One of the issues is that we have strictly no idea what the caches are
>> Inner Shareable with (I've been asking for such clarification for 
>> years
>> without getting anywhere). You can have as many disconnected inner
>> shareable domains as you want!
>> 
>> I suspect that in the grand scheme of things, the redistributors
>> ought to be in the same inner shareable domain, and that with a bit of
>> luck, the CPUs are there as well. Still, that's a massive guess.
>> 
>>> What's more, on Hisilicon hip08 it will trigger some kind of bus
>>> warning when mixing use of different shareabilities.
>> Do you have more information about what the bus is complaining about?
>> Is that because the CPUs have these pages mapped as inner shareable?
>> 
>> I'll give it a go on D05 (HIP07) to find out what changes there.
> 
> How's your go on D05? Did you see any issues?

Sorry it took so long. I've given it a go on my D05, and didn't notice
anything bad (or rather, nothing worse than usual, since GICv4 on this
machine is pretty... funky).

I've now take this into the 5.7 queue.

Thanks,

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

[tip: irq/core] irqchip/gic-v4: Use Inner-Shareable attributes for virtual pending tables

[tip-bot2 for Heyi Guo]

The following commit has been merged into the irq/core branch of tip:

Commit-ID:     b2cb11f4f7643255b7703c0fcabc31a8ec478f3a
Gitweb:        https://git.kernel.org/tip/b2cb11f4f7643255b7703c0fcabc31a8ec478f3a
Author:        Heyi Guo <guoheyi@huawei.com>
AuthorDate:    Sat, 30 Nov 2019 15:38:49 +08:00
Committer:     Marc Zyngier <maz@kernel.org>
CommitterDate: Sat, 21 Mar 2020 09:40:47 

irqchip/gic-v4: Use Inner-Shareable attributes for virtual pending tables

There is no special reason to set virtual LPI pending table as
non-shareable. If we choose to hard code the shareability without
probing, Inner-Shareable is likely to be a better choice, as the
VPEs can move around and benefit from having the redistributors
snooping each other's cache, if that's something they can do.

Furthermore, Hisilicon hip08 ends up with unspecified errors when
mixing shareability attributes. So let's move to IS attributes for
the VPT. This has also been tested on D05 and didn't show any
regression.

Signed-off-by: Heyi Guo <guoheyi@huawei.com>
[maz: rewrote commit message]
Signed-off-by: Marc Zyngier <maz@kernel.org>
Tested-by: Marc Zyngier <maz@kernel.org>
Link: https://lore.kernel.org/r/20191130073849.38378-1-guoheyi@huawei.com
---
 drivers/irqchip/irq-gic-v3-its.c   | 2 +-
 include/linux/irqchip/arm-gic-v3.h | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index bb80285..bc5b3f6 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -3560,7 +3560,7 @@ static void its_vpe_schedule(struct its_vpe *vpe)
 	val  = virt_to_phys(page_address(vpe->vpt_page)) &
 		GENMASK_ULL(51, 16);
 	val |= GICR_VPENDBASER_RaWaWb;
-	val |= GICR_VPENDBASER_NonShareable;
+	val |= GICR_VPENDBASER_InnerShareable;
 	/*
 	 * There is no good way of finding out if the pending table is
 	 * empty as we can race against the doorbell interrupt very
diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
index 83439bf..85b105f 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -320,6 +320,9 @@
 #define GICR_VPENDBASER_NonShareable					\
 	GIC_BASER_SHAREABILITY(GICR_VPENDBASER, NonShareable)
 
+#define GICR_VPENDBASER_InnerShareable					\
+	GIC_BASER_SHAREABILITY(GICR_VPENDBASER, InnerShareable)
+
 #define GICR_VPENDBASER_nCnB	GIC_BASER_CACHEABILITY(GICR_VPENDBASER, INNER, nCnB)
 #define GICR_VPENDBASER_nC 	GIC_BASER_CACHEABILITY(GICR_VPENDBASER, INNER, nC)
 #define GICR_VPENDBASER_RaWt	GIC_BASER_CACHEABILITY(GICR_VPENDBASER, INNER, RaWt)

Re: [PATCH] irq/gic-its: gicv4: set VPENDING table as inner-shareable

[Heyi Guo]

On 2020/3/21 17:54, Marc Zyngier wrote:
> Hi Heyi,
>
> On 2020-02-24 02:22, Heyi Guo wrote:
>> Hi Marc,
>>
>> On 2019/12/2 2:04, Marc Zyngier wrote:
>>> On Sat, 30 Nov 2019 15:38:49 +0800
>>> Heyi Guo <guoheyi@huawei.com> wrote:
>>>
>>>> There is no special reason to set virtual LPI pending table as
>>>> non-shareable. If we choose to hard code the shareability without
>>>> probing, inner-shareable will be a better choice, for all the other
>>>> ITS/GICR tables prefer to be inner-shareable.
>>> One of the issues is that we have strictly no idea what the caches are
>>> Inner Shareable with (I've been asking for such clarification for years
>>> without getting anywhere). You can have as many disconnected inner
>>> shareable domains as you want!
>>>
>>> I suspect that in the grand scheme of things, the redistributors
>>> ought to be in the same inner shareable domain, and that with a bit of
>>> luck, the CPUs are there as well. Still, that's a massive guess.
>>>
>>>> What's more, on Hisilicon hip08 it will trigger some kind of bus
>>>> warning when mixing use of different shareabilities.
>>> Do you have more information about what the bus is complaining about?
>>> Is that because the CPUs have these pages mapped as inner shareable?
>>>
>>> I'll give it a go on D05 (HIP07) to find out what changes there.
>>
>> How's your go on D05? Did you see any issues?
>
> Sorry it took so long. I've given it a go on my D05, and didn't notice
> anything bad (or rather, nothing worse than usual, since GICv4 on this
> machine is pretty... funky).
>
> I've now take this into the 5.7 queue.


Thanks,

Heyi


>
> Thanks,
>
>          M.