[PATCH 00/10] GICv3 support for kexec/kdump on EFI systems

[PATCH 00/10] GICv3 support for kexec/kdump on EFI systems

[Marc Zyngier]

The GICv3 architecture has the remarkable feature that once LPI tables
have been assigned to redistributors and that LPI delivery is enabled,
there is no guarantee that LPIs can be turned off (and most
implementations do not allow it), nor can it be reprogrammed to use
other tables.

This is a bit of a problem for kexec, where the secondary kernel
completely looses track of the previous allocations. If the secondary
kernel doesn't allocate the tables exactly the same way, no LPIs will
be delivered by the GIC (which continues to use the old tables), and
memory previously allocated for the pending tables will be slowly
corrupted, one bit at a time.

The workaround for this is based on a series[1] by Ard Biesheuvel,
which adds the required infrastructure for memory reservations to be
passed from one kernel to another using an EFI table.

This infrastructure is then used to register the allocation of GIC
tables with EFI, and allow the GIC driver to safely reuse the existing
programming if it detects that the tables have been correctly
registered. On non-EFI systems, there is not much we can do.

This has been tested on a TX2 system both as a host and a guest. I'd
welcome additional testing of different HW. For convenience, I've
stashed a branch containing the whole thing at [2].

[1] https://marc.info/?l=linux-efi&m=153754757208163&w=2
[2] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/gicv3-kdump

Marc Zyngier (10):
  irqchip/gic-v3-its: Change initialization ordering for LPIs
  irqchip/gic-v3-its: Consolidate LPI_PENDBASE_SZ usage
  irqchip/gic-v3-its: Split property table clearing from allocation
  irqchip/gic-v3-its: Move pending table allocation to init time
  irqchip/gic-v3-its: Keep track of property table's PA and VA
  irqchip/gic-v3-its: Allow use of pre-programmed LPI tables
  irqchip/gic-v3-its: Use pre-programmed redistributor tables with kdump
    kernels
  irqchip/gic-v3-its: Check that all RDs have the same property table
  irqchip/gic-v3-its: Register LPI tables with EFI config table
  irqchip/gic-v3-its: Allow use of LPI tables in reserved memory

 drivers/irqchip/irq-gic-v3-its.c   | 249 ++++++++++++++++++++++-------
 drivers/irqchip/irq-gic-v3.c       |  20 ++-
 include/linux/irqchip/arm-gic-v3.h |   4 +-
 3 files changed, 208 insertions(+), 65 deletions(-)

-- 
2.18.0

[PATCH 01/10] irqchip/gic-v3-its: Change initialization ordering for LPIs

[Marc Zyngier]

We currently initialize the LPIs (and the ITS) fairly early, even
before the SMP support and the CPU interface. This is a bit odd
(as LPIs are not exactly crutial for the early boot process),
and is going to cause issues when reorganizing the probing code.

Let's move this initialization later.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/irqchip/irq-gic-v3.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index d5912f1ec884..6232f98ef81b 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -653,7 +653,9 @@ early_param("irqchip.gicv3_nolpi", gicv3_nolpi_cfg);
 
 static int gic_dist_supports_lpis(void)
 {
-	return !!(readl_relaxed(gic_data.dist_base + GICD_TYPER) & GICD_TYPER_LPIS) && !gicv3_nolpi;
+	return (IS_ENABLED(CONFIG_ARM_GIC_V3_ITS) &&
+		!!(readl_relaxed(gic_data.dist_base + GICD_TYPER) & GICD_TYPER_LPIS) &&
+		!gicv3_nolpi);
 }
 
 static void gic_cpu_init(void)
@@ -673,10 +675,6 @@ static void gic_cpu_init(void)
 
 	gic_cpu_config(rbase, gic_redist_wait_for_rwp);
 
-	/* Give LPIs a spin */
-	if (IS_ENABLED(CONFIG_ARM_GIC_V3_ITS) && gic_dist_supports_lpis())
-		its_cpu_init();
-
 	/* initialise system registers */
 	gic_cpu_sys_reg_init();
 }
@@ -689,6 +687,10 @@ static void gic_cpu_init(void)
 static int gic_starting_cpu(unsigned int cpu)
 {
 	gic_cpu_init();
+
+	if (gic_dist_supports_lpis())
+		its_cpu_init();
+
 	return 0;
 }
 
@@ -1127,14 +1129,16 @@ static int __init gic_init_bases(void __iomem *dist_base,
 
 	gic_update_vlpi_properties();
 
-	if (IS_ENABLED(CONFIG_ARM_GIC_V3_ITS) && gic_dist_supports_lpis())
-		its_init(handle, &gic_data.rdists, gic_data.domain);
-
 	gic_smp_init();
 	gic_dist_init();
 	gic_cpu_init();
 	gic_cpu_pm_init();
 
+	if (gic_dist_supports_lpis()) {
+		its_init(handle, &gic_data.rdists, gic_data.domain);
+		its_cpu_init();
+	}
+
 	return 0;
 
 out_free:
-- 
2.18.0

Re: [PATCH 01/10] irqchip/gic-v3-its: Change initialization ordering for LPIs

[Julien Thierry]

On 21/09/18 20:59, Marc Zyngier wrote:
> We currently initialize the LPIs (and the ITS) fairly early, even
> before the SMP support and the CPU interface. This is a bit odd
> (as LPIs are not exactly crutial for the early boot process),
> and is going to cause issues when reorganizing the probing code.
> 
> Let's move this initialization later.
> 

Reviewed-by: Julien Thierry <julien.thierry@arm.com>

> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>   drivers/irqchip/irq-gic-v3.c | 20 ++++++++++++--------
>   1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index d5912f1ec884..6232f98ef81b 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -653,7 +653,9 @@ early_param("irqchip.gicv3_nolpi", gicv3_nolpi_cfg);
>   
>   static int gic_dist_supports_lpis(void)
>   {
> -	return !!(readl_relaxed(gic_data.dist_base + GICD_TYPER) & GICD_TYPER_LPIS) && !gicv3_nolpi;
> +	return (IS_ENABLED(CONFIG_ARM_GIC_V3_ITS) &&
> +		!!(readl_relaxed(gic_data.dist_base + GICD_TYPER) & GICD_TYPER_LPIS) &&
> +		!gicv3_nolpi);
>   }
>   
>   static void gic_cpu_init(void)
> @@ -673,10 +675,6 @@ static void gic_cpu_init(void)
>   
>   	gic_cpu_config(rbase, gic_redist_wait_for_rwp);
>   
> -	/* Give LPIs a spin */
> -	if (IS_ENABLED(CONFIG_ARM_GIC_V3_ITS) && gic_dist_supports_lpis())
> -		its_cpu_init();
> -
>   	/* initialise system registers */
>   	gic_cpu_sys_reg_init();
>   }
> @@ -689,6 +687,10 @@ static void gic_cpu_init(void)
>   static int gic_starting_cpu(unsigned int cpu)
>   {
>   	gic_cpu_init();
> +
> +	if (gic_dist_supports_lpis())
> +		its_cpu_init();
> +
>   	return 0;
>   }
>   
> @@ -1127,14 +1129,16 @@ static int __init gic_init_bases(void __iomem *dist_base,
>   
>   	gic_update_vlpi_properties();
>   
> -	if (IS_ENABLED(CONFIG_ARM_GIC_V3_ITS) && gic_dist_supports_lpis())
> -		its_init(handle, &gic_data.rdists, gic_data.domain);
> -
>   	gic_smp_init();
>   	gic_dist_init();
>   	gic_cpu_init();
>   	gic_cpu_pm_init();
>   
> +	if (gic_dist_supports_lpis()) {
> +		its_init(handle, &gic_data.rdists, gic_data.domain);
> +		its_cpu_init();
> +	}
> +
>   	return 0;
>   
>   out_free:
> 

-- 
Julien Thierry

[PATCH 02/10] irqchip/gic-v3-its: Consolidate LPI_PENDBASE_SZ usage

[Marc Zyngier]

LPI_PENDING_SZ is always used in conjunction with a max(). Let's
factor this in the definition of the macro, and simplify the rest
of the code.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/irqchip/irq-gic-v3-its.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index c2df341ff6fa..ed6aab11e019 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -62,7 +62,7 @@ static u32 lpi_id_bits;
  */
 #define LPI_NRBITS		lpi_id_bits
 #define LPI_PROPBASE_SZ		ALIGN(BIT(LPI_NRBITS), SZ_64K)
-#define LPI_PENDBASE_SZ		ALIGN(BIT(LPI_NRBITS) / 8, SZ_64K)
+#define LPI_PENDBASE_SZ		max_t(u32, SZ_64K, ALIGN(BIT(LPI_NRBITS) / 8, SZ_64K))
 
 #define LPI_PROP_DEFAULT_PRIO	0xa0
 
@@ -1924,12 +1924,9 @@ static int its_alloc_collections(struct its_node *its)
 static struct page *its_allocate_pending_table(gfp_t gfp_flags)
 {
 	struct page *pend_page;
-	/*
-	 * The pending pages have to be at least 64kB aligned,
-	 * hence the 'max(LPI_PENDBASE_SZ, SZ_64K)' below.
-	 */
+
 	pend_page = alloc_pages(gfp_flags | __GFP_ZERO,
-				get_order(max_t(u32, LPI_PENDBASE_SZ, SZ_64K)));
+				get_order(LPI_PENDBASE_SZ));
 	if (!pend_page)
 		return NULL;
 
@@ -1941,8 +1938,7 @@ static struct page *its_allocate_pending_table(gfp_t gfp_flags)
 
 static void its_free_pending_table(struct page *pt)
 {
-	free_pages((unsigned long)page_address(pt),
-		   get_order(max_t(u32, LPI_PENDBASE_SZ, SZ_64K)));
+	free_pages((unsigned long)page_address(pt), get_order(LPI_PENDBASE_SZ));
 }
 
 static void its_cpu_init_lpis(void)
-- 
2.18.0

Re: [PATCH 02/10] irqchip/gic-v3-its: Consolidate LPI_PENDBASE_SZ usage

[Suzuki K Poulose]

Hi Marc,

On 21/09/18 20:59, Marc Zyngier wrote:
> LPI_PENDING_SZ is always used in conjunction with a max(). Let's
> factor this in the definition of the macro, and simplify the rest
> of the code.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>   drivers/irqchip/irq-gic-v3-its.c | 12 ++++--------
>   1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index c2df341ff6fa..ed6aab11e019 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -62,7 +62,7 @@ static u32 lpi_id_bits;
>    */
>   #define LPI_NRBITS		lpi_id_bits
>   #define LPI_PROPBASE_SZ		ALIGN(BIT(LPI_NRBITS), SZ_64K)
> -#define LPI_PENDBASE_SZ		ALIGN(BIT(LPI_NRBITS) / 8, SZ_64K)
> +#define LPI_PENDBASE_SZ		max_t(u32, SZ_64K, ALIGN(BIT(LPI_NRBITS) / 8, SZ_64K))

minor nit: The ALIGN() already aligns the given value up to the required
alignment. So, if the LPI_NRBITS is guaranteed to be non-zero,
we could simply drop the max_t().

Rest looks good to me.

Suzuki

>   
>   #define LPI_PROP_DEFAULT_PRIO	0xa0
>   
> @@ -1924,12 +1924,9 @@ static int its_alloc_collections(struct its_node *its)
>   static struct page *its_allocate_pending_table(gfp_t gfp_flags)
>   {
>   	struct page *pend_page;
> -	/*
> -	 * The pending pages have to be at least 64kB aligned,
> -	 * hence the 'max(LPI_PENDBASE_SZ, SZ_64K)' below.
> -	 */
> +
>   	pend_page = alloc_pages(gfp_flags | __GFP_ZERO,
> -				get_order(max_t(u32, LPI_PENDBASE_SZ, SZ_64K)));
> +				get_order(LPI_PENDBASE_SZ));
>   	if (!pend_page)
>   		return NULL;
>   
> @@ -1941,8 +1938,7 @@ static struct page *its_allocate_pending_table(gfp_t gfp_flags)
>   
>   static void its_free_pending_table(struct page *pt)
>   {
> -	free_pages((unsigned long)page_address(pt),
> -		   get_order(max_t(u32, LPI_PENDBASE_SZ, SZ_64K)));
> +	free_pages((unsigned long)page_address(pt), get_order(LPI_PENDBASE_SZ));
>   }
>   
>   static void its_cpu_init_lpis(void)
> 

Re: [PATCH 02/10] irqchip/gic-v3-its: Consolidate LPI_PENDBASE_SZ usage

[Julien Thierry]

Hi,

On 24/09/18 11:33, Suzuki K Poulose wrote:
> Hi Marc,
> 
> On 21/09/18 20:59, Marc Zyngier wrote:
>> LPI_PENDING_SZ is always used in conjunction with a max(). Let's
>> factor this in the definition of the macro, and simplify the rest
>> of the code.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>   drivers/irqchip/irq-gic-v3-its.c | 12 ++++--------
>>   1 file changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c 
>> b/drivers/irqchip/irq-gic-v3-its.c
>> index c2df341ff6fa..ed6aab11e019 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -62,7 +62,7 @@ static u32 lpi_id_bits;
>>    */
>>   #define LPI_NRBITS        lpi_id_bits
>>   #define LPI_PROPBASE_SZ        ALIGN(BIT(LPI_NRBITS), SZ_64K)
>> -#define LPI_PENDBASE_SZ        ALIGN(BIT(LPI_NRBITS) / 8, SZ_64K)
>> +#define LPI_PENDBASE_SZ        max_t(u32, SZ_64K, 
>> ALIGN(BIT(LPI_NRBITS) / 8, SZ_64K))
> 
> minor nit: The ALIGN() already aligns the given value up to the required
> alignment. So, if the LPI_NRBITS is guaranteed to be non-zero,
> we could simply drop the max_t().
> 

Hmmm, Doesn't ALIGN only aligns down? So if "BIT(LPI_NR_BITS) / 8 < 
SZ_64K" (i.e. LPI_NRBITS < 20) The ALIGN(..., SZ_64K) would give 0.

Thanks,

> Rest looks good to me.
> 
> Suzuki
> 
>>   #define LPI_PROP_DEFAULT_PRIO    0xa0
>> @@ -1924,12 +1924,9 @@ static int its_alloc_collections(struct 
>> its_node *its)
>>   static struct page *its_allocate_pending_table(gfp_t gfp_flags)
>>   {
>>       struct page *pend_page;
>> -    /*
>> -     * The pending pages have to be at least 64kB aligned,
>> -     * hence the 'max(LPI_PENDBASE_SZ, SZ_64K)' below.
>> -     */
>> +
>>       pend_page = alloc_pages(gfp_flags | __GFP_ZERO,
>> -                get_order(max_t(u32, LPI_PENDBASE_SZ, SZ_64K)));
>> +                get_order(LPI_PENDBASE_SZ));
>>       if (!pend_page)
>>           return NULL;
>> @@ -1941,8 +1938,7 @@ static struct page 
>> *its_allocate_pending_table(gfp_t gfp_flags)
>>   static void its_free_pending_table(struct page *pt)
>>   {
>> -    free_pages((unsigned long)page_address(pt),
>> -           get_order(max_t(u32, LPI_PENDBASE_SZ, SZ_64K)));
>> +    free_pages((unsigned long)page_address(pt), 
>> get_order(LPI_PENDBASE_SZ));
>>   }
>>   static void its_cpu_init_lpis(void)
>>

-- 
Julien Thierry

Re: [PATCH 02/10] irqchip/gic-v3-its: Consolidate LPI_PENDBASE_SZ usage

[Suzuki K Poulose]

On 24/09/18 11:50, Julien Thierry wrote:
> Hi,
> 
> On 24/09/18 11:33, Suzuki K Poulose wrote:
>> Hi Marc,
>>
>> On 21/09/18 20:59, Marc Zyngier wrote:
>>> LPI_PENDING_SZ is always used in conjunction with a max(). Let's
>>> factor this in the definition of the macro, and simplify the rest
>>> of the code.
>>>
>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>> ---
>>>   drivers/irqchip/irq-gic-v3-its.c | 12 ++++--------
>>>   1 file changed, 4 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/irqchip/irq-gic-v3-its.c 
>>> b/drivers/irqchip/irq-gic-v3-its.c
>>> index c2df341ff6fa..ed6aab11e019 100644
>>> --- a/drivers/irqchip/irq-gic-v3-its.c
>>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>>> @@ -62,7 +62,7 @@ static u32 lpi_id_bits;
>>>    */
>>>   #define LPI_NRBITS        lpi_id_bits
>>>   #define LPI_PROPBASE_SZ        ALIGN(BIT(LPI_NRBITS), SZ_64K)
>>> -#define LPI_PENDBASE_SZ        ALIGN(BIT(LPI_NRBITS) / 8, SZ_64K)
>>> +#define LPI_PENDBASE_SZ        max_t(u32, SZ_64K, 
>>> ALIGN(BIT(LPI_NRBITS) / 8, SZ_64K))
>>
>> minor nit: The ALIGN() already aligns the given value up to the required
>> alignment. So, if the LPI_NRBITS is guaranteed to be non-zero,
>> we could simply drop the max_t().
>>
> 
> Hmmm, Doesn't ALIGN only aligns down? So if "BIT(LPI_NR_BITS) / 8 < 
> SZ_64K" (i.e. LPI_NRBITS < 20) The ALIGN(..., SZ_64K) would give 0.

Isn't it the ALIGN_DOWN(), which aligns it down ? Following the kernel
definitions :
linux/kernel.h -> uapi/linux/kernel.h
ALIGN(x,a) => 	__ALIGN_KERNEL(x, a)
		\ => __ALIGN_KERNEL_MASK(x, (a -1)
		\ => (((x + (a - 1)) & ~ (a - 1))

Cheers
Suzuki


> 
> Thanks,
> 
>> Rest looks good to me.
>>
>> Suzuki
>>
>>>   #define LPI_PROP_DEFAULT_PRIO    0xa0
>>> @@ -1924,12 +1924,9 @@ static int its_alloc_collections(struct 
>>> its_node *its)
>>>   static struct page *its_allocate_pending_table(gfp_t gfp_flags)
>>>   {
>>>       struct page *pend_page;
>>> -    /*
>>> -     * The pending pages have to be at least 64kB aligned,
>>> -     * hence the 'max(LPI_PENDBASE_SZ, SZ_64K)' below.
>>> -     */
>>> +
>>>       pend_page = alloc_pages(gfp_flags | __GFP_ZERO,
>>> -                get_order(max_t(u32, LPI_PENDBASE_SZ, SZ_64K)));
>>> +                get_order(LPI_PENDBASE_SZ));
>>>       if (!pend_page)
>>>           return NULL;
>>> @@ -1941,8 +1938,7 @@ static struct page 
>>> *its_allocate_pending_table(gfp_t gfp_flags)
>>>   static void its_free_pending_table(struct page *pt)
>>>   {
>>> -    free_pages((unsigned long)page_address(pt),
>>> -           get_order(max_t(u32, LPI_PENDBASE_SZ, SZ_64K)));
>>> +    free_pages((unsigned long)page_address(pt), 
>>> get_order(LPI_PENDBASE_SZ));
>>>   }
>>>   static void its_cpu_init_lpis(void)
>>>
> 

Re: [PATCH 02/10] irqchip/gic-v3-its: Consolidate LPI_PENDBASE_SZ usage

[Julien Thierry]

On 24/09/18 11:54, Suzuki K Poulose wrote:
> 
> 
> On 24/09/18 11:50, Julien Thierry wrote:
>> Hi,
>>
>> On 24/09/18 11:33, Suzuki K Poulose wrote:
>>> Hi Marc,
>>>
>>> On 21/09/18 20:59, Marc Zyngier wrote:
>>>> LPI_PENDING_SZ is always used in conjunction with a max(). Let's
>>>> factor this in the definition of the macro, and simplify the rest
>>>> of the code.
>>>>
>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>>> ---
>>>>   drivers/irqchip/irq-gic-v3-its.c | 12 ++++--------
>>>>   1 file changed, 4 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/irqchip/irq-gic-v3-its.c 
>>>> b/drivers/irqchip/irq-gic-v3-its.c
>>>> index c2df341ff6fa..ed6aab11e019 100644
>>>> --- a/drivers/irqchip/irq-gic-v3-its.c
>>>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>>>> @@ -62,7 +62,7 @@ static u32 lpi_id_bits;
>>>>    */
>>>>   #define LPI_NRBITS        lpi_id_bits
>>>>   #define LPI_PROPBASE_SZ        ALIGN(BIT(LPI_NRBITS), SZ_64K)
>>>> -#define LPI_PENDBASE_SZ        ALIGN(BIT(LPI_NRBITS) / 8, SZ_64K)
>>>> +#define LPI_PENDBASE_SZ        max_t(u32, SZ_64K, 
>>>> ALIGN(BIT(LPI_NRBITS) / 8, SZ_64K))
>>>
>>> minor nit: The ALIGN() already aligns the given value up to the required
>>> alignment. So, if the LPI_NRBITS is guaranteed to be non-zero,
>>> we could simply drop the max_t().
>>>
>>
>> Hmmm, Doesn't ALIGN only aligns down? So if "BIT(LPI_NR_BITS) / 8 < 
>> SZ_64K" (i.e. LPI_NRBITS < 20) The ALIGN(..., SZ_64K) would give 0.
> 
> Isn't it the ALIGN_DOWN(), which aligns it down ? Following the kernel
> definitions :
> linux/kernel.h -> uapi/linux/kernel.h
> ALIGN(x,a) =>     __ALIGN_KERNEL(x, a)
>          \ => __ALIGN_KERNEL_MASK(x, (a -1)
>          \ => (((x + (a - 1)) & ~ (a - 1))

Oh, yes you're right, made the wrong assumption.
Your suggestion makes sense. Sorry for the noise.

Thanks,

> 
> Cheers
> Suzuki
> 
> 
>>
>> Thanks,
>>
>>> Rest looks good to me.
>>>
>>> Suzuki
>>>
>>>>   #define LPI_PROP_DEFAULT_PRIO    0xa0
>>>> @@ -1924,12 +1924,9 @@ static int its_alloc_collections(struct 
>>>> its_node *its)
>>>>   static struct page *its_allocate_pending_table(gfp_t gfp_flags)
>>>>   {
>>>>       struct page *pend_page;
>>>> -    /*
>>>> -     * The pending pages have to be at least 64kB aligned,
>>>> -     * hence the 'max(LPI_PENDBASE_SZ, SZ_64K)' below.
>>>> -     */
>>>> +
>>>>       pend_page = alloc_pages(gfp_flags | __GFP_ZERO,
>>>> -                get_order(max_t(u32, LPI_PENDBASE_SZ, SZ_64K)));
>>>> +                get_order(LPI_PENDBASE_SZ));
>>>>       if (!pend_page)
>>>>           return NULL;
>>>> @@ -1941,8 +1938,7 @@ static struct page 
>>>> *its_allocate_pending_table(gfp_t gfp_flags)
>>>>   static void its_free_pending_table(struct page *pt)
>>>>   {
>>>> -    free_pages((unsigned long)page_address(pt),
>>>> -           get_order(max_t(u32, LPI_PENDBASE_SZ, SZ_64K)));
>>>> +    free_pages((unsigned long)page_address(pt), 
>>>> get_order(LPI_PENDBASE_SZ));
>>>>   }
>>>>   static void its_cpu_init_lpis(void)
>>>>
>>

-- 
Julien Thierry

Re: [PATCH 02/10] irqchip/gic-v3-its: Consolidate LPI_PENDBASE_SZ usage

[Marc Zyngier]

On 24/09/18 11:33, Suzuki K Poulose wrote:
> Hi Marc,
> 
> On 21/09/18 20:59, Marc Zyngier wrote:
>> LPI_PENDING_SZ is always used in conjunction with a max(). Let's
>> factor this in the definition of the macro, and simplify the rest
>> of the code.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>    drivers/irqchip/irq-gic-v3-its.c | 12 ++++--------
>>    1 file changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>> index c2df341ff6fa..ed6aab11e019 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -62,7 +62,7 @@ static u32 lpi_id_bits;
>>     */
>>    #define LPI_NRBITS		lpi_id_bits
>>    #define LPI_PROPBASE_SZ		ALIGN(BIT(LPI_NRBITS), SZ_64K)
>> -#define LPI_PENDBASE_SZ		ALIGN(BIT(LPI_NRBITS) / 8, SZ_64K)
>> +#define LPI_PENDBASE_SZ		max_t(u32, SZ_64K, ALIGN(BIT(LPI_NRBITS) / 8, SZ_64K))
> 
> minor nit: The ALIGN() already aligns the given value up to the required
> alignment. So, if the LPI_NRBITS is guaranteed to be non-zero,
> we could simply drop the max_t().

Yeah, that's a brain fart on my part. I've fixed up.

Thanks,

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

[PATCH 03/10] irqchip/gic-v3-its: Split property table clearing from allocation

[Marc Zyngier]

As we're going to reuse some pre-allocated memory for the property
table, split out the zeroing of that table into a separate function
for later use.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/irqchip/irq-gic-v3-its.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index ed6aab11e019..7ef6baea2d78 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1597,6 +1597,15 @@ static void its_lpi_free(unsigned long *bitmap, u32 base, u32 nr_ids)
 	kfree(bitmap);
 }
 
+static void gic_reset_prop_table(void *va)
+{
+	/* Priority 0xa0, Group-1, disabled */
+	memset(va, LPI_PROP_DEFAULT_PRIO | LPI_PROP_GROUP1, LPI_PROPBASE_SZ);
+
+	/* Make sure the GIC will observe the written configuration */
+	gic_flush_dcache_to_poc(va, LPI_PROPBASE_SZ);
+}
+
 static struct page *its_allocate_prop_table(gfp_t gfp_flags)
 {
 	struct page *prop_page;
@@ -1605,13 +1614,7 @@ static struct page *its_allocate_prop_table(gfp_t gfp_flags)
 	if (!prop_page)
 		return NULL;
 
-	/* Priority 0xa0, Group-1, disabled */
-	memset(page_address(prop_page),
-	       LPI_PROP_DEFAULT_PRIO | LPI_PROP_GROUP1,
-	       LPI_PROPBASE_SZ);
-
-	/* Make sure the GIC will observe the written configuration */
-	gic_flush_dcache_to_poc(page_address(prop_page), LPI_PROPBASE_SZ);
+	gic_reset_prop_table(page_address(prop_page));
 
 	return prop_page;
 }
-- 
2.18.0

[PATCH 04/10] irqchip/gic-v3-its: Move pending table allocation to init time

[Marc Zyngier]

Pending tables for the redistributors are currently allocated
one at a time as each CPU boots. This is causing some grief
for Linux/RT (allocation from within a CPU hotplug notifier is
frown upon).

Let's more this allocation to take place at init time, when we
only have a single CPU. It means we're allocating memory for CPUs
that are not online yet, but most system will boot all of their
CPUs anyway, so that's not completely wasted.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/irqchip/irq-gic-v3-its.c   | 80 +++++++++++++++++++-----------
 include/linux/irqchip/arm-gic-v3.h |  1 +
 2 files changed, 53 insertions(+), 28 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 7ef6baea2d78..462bba422189 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -173,6 +173,7 @@ static DEFINE_RAW_SPINLOCK(vmovp_lock);
 static DEFINE_IDA(its_vpeid_ida);
 
 #define gic_data_rdist()		(raw_cpu_ptr(gic_rdists->rdist))
+#define gic_data_rdist_cpu(cpu)		(per_cpu_ptr(gic_rdists->rdist, cpu))
 #define gic_data_rdist_rd_base()	(gic_data_rdist()->rd_base)
 #define gic_data_rdist_vlpi_base()	(gic_data_rdist_rd_base() + SZ_128K)
 
@@ -1625,7 +1626,7 @@ static void its_free_prop_table(struct page *prop_page)
 		   get_order(LPI_PROPBASE_SZ));
 }
 
-static int __init its_alloc_lpi_tables(void)
+static int __init its_alloc_lpi_prop_table(void)
 {
 	phys_addr_t paddr;
 
@@ -1944,30 +1945,47 @@ static void its_free_pending_table(struct page *pt)
 	free_pages((unsigned long)page_address(pt), get_order(LPI_PENDBASE_SZ));
 }
 
-static void its_cpu_init_lpis(void)
+static int __init allocate_lpi_tables(void)
 {
-	void __iomem *rbase = gic_data_rdist_rd_base();
-	struct page *pend_page;
-	u64 val, tmp;
+	int err, cpu;
 
-	/* If we didn't allocate the pending table yet, do it now */
-	pend_page = gic_data_rdist()->pend_page;
-	if (!pend_page) {
-		phys_addr_t paddr;
+	err = its_alloc_lpi_prop_table();
+	if (err)
+		return err;
+
+	/*
+	 * We allocate all the pending tables anyway, as we may have a
+	 * mix of RDs that have had LPIs enabled, and some that
+	 * don't. We'll free the unused ones as each CPU comes online.
+	 */
+	for_each_possible_cpu(cpu) {
+		struct page *pend_page;
 
 		pend_page = its_allocate_pending_table(GFP_NOWAIT);
 		if (!pend_page) {
-			pr_err("Failed to allocate PENDBASE for CPU%d\n",
-			       smp_processor_id());
-			return;
+			pr_err("Failed to allocate PENDBASE for CPU%d\n", cpu);
+			return -ENOMEM;
 		}
 
-		paddr = page_to_phys(pend_page);
-		pr_info("CPU%d: using LPI pending table @%pa\n",
-			smp_processor_id(), &paddr);
-		gic_data_rdist()->pend_page = pend_page;
+		gic_data_rdist_cpu(cpu)->pend_page = pend_page;
 	}
 
+	return 0;
+}
+
+static void its_cpu_init_lpis(void)
+{
+	void __iomem *rbase = gic_data_rdist_rd_base();
+	struct page *pend_page;
+	phys_addr_t paddr;
+	u64 val, tmp;
+
+	if (gic_data_rdist()->lpi_enabled)
+		return;
+
+	pend_page = gic_data_rdist()->pend_page;
+	paddr = page_to_phys(pend_page);
+
 	/* set PROPBASE */
 	val = (page_to_phys(gic_rdists->prop_page) |
 	       GICR_PROPBASER_InnerShareable |
@@ -2019,6 +2037,10 @@ static void its_cpu_init_lpis(void)
 
 	/* Make sure the GIC has seen the above */
 	dsb(sy);
+	gic_data_rdist()->lpi_enabled = true;
+	pr_info("GICv3: CPU%d: using LPI pending table @%pa\n",
+		smp_processor_id(),
+		&paddr);
 }
 
 static void its_cpu_init_collection(struct its_node *its)
@@ -3497,16 +3519,6 @@ static int redist_disable_lpis(void)
 	u64 timeout = USEC_PER_SEC;
 	u64 val;
 
-	/*
-	 * If coming via a CPU hotplug event, we don't need to disable
-	 * LPIs before trying to re-enable them. They are already
-	 * configured and all is well in the world. Detect this case
-	 * by checking the allocation of the pending table for the
-	 * current CPU.
-	 */
-	if (gic_data_rdist()->pend_page)
-		return 0;
-
 	if (!gic_rdists_supports_plpis()) {
 		pr_info("CPU%d: LPIs not supported\n", smp_processor_id());
 		return -ENXIO;
@@ -3516,7 +3528,18 @@ static int redist_disable_lpis(void)
 	if (!(val & GICR_CTLR_ENABLE_LPIS))
 		return 0;
 
-	pr_warn("CPU%d: Booted with LPIs enabled, memory probably corrupted\n",
+	/*
+	 * If coming via a CPU hotplug event, we don't need to disable
+	 * LPIs before trying to re-enable them. They are already
+	 * configured and all is well in the world.
+	 */
+	if (gic_data_rdist()->lpi_enabled)
+		return 0;
+
+	/*
+	 * From that point on, we only try to do some damage control.
+	 */
+	pr_warn("GICv3: CPU%d: Booted with LPIs enabled, memory probably corrupted\n",
 		smp_processor_id());
 	add_taint(TAINT_CRAP, LOCKDEP_STILL_OK);
 
@@ -3772,7 +3795,8 @@ int __init its_init(struct fwnode_handle *handle, struct rdists *rdists,
 	}
 
 	gic_rdists = rdists;
-	err = its_alloc_lpi_tables();
+
+	err = allocate_lpi_tables();
 	if (err)
 		return err;
 
diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
index 8bdbb5f29494..266093e845bb 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -585,6 +585,7 @@ struct rdists {
 		void __iomem	*rd_base;
 		struct page	*pend_page;
 		phys_addr_t	phys_base;
+		bool		lpi_enabled;
 	} __percpu		*rdist;
 	struct page		*prop_page;
 	u64			flags;
-- 
2.18.0

Re: [PATCH 04/10] irqchip/gic-v3-its: Move pending table allocation to init time

[Julien Thierry]

Hi Marc,

On 21/09/18 20:59, Marc Zyngier wrote:
> Pending tables for the redistributors are currently allocated
> one at a time as each CPU boots. This is causing some grief
> for Linux/RT (allocation from within a CPU hotplug notifier is
> frown upon).
> 
> Let's more this allocation to take place at init time, when we
> only have a single CPU. It means we're allocating memory for CPUs
> that are not online yet, but most system will boot all of their
> CPUs anyway, so that's not completely wasted.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>   drivers/irqchip/irq-gic-v3-its.c   | 80 +++++++++++++++++++-----------
>   include/linux/irqchip/arm-gic-v3.h |  1 +
>   2 files changed, 53 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 7ef6baea2d78..462bba422189 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -173,6 +173,7 @@ static DEFINE_RAW_SPINLOCK(vmovp_lock);
>   static DEFINE_IDA(its_vpeid_ida);
>   
>   #define gic_data_rdist()		(raw_cpu_ptr(gic_rdists->rdist))
> +#define gic_data_rdist_cpu(cpu)		(per_cpu_ptr(gic_rdists->rdist, cpu))
>   #define gic_data_rdist_rd_base()	(gic_data_rdist()->rd_base)
>   #define gic_data_rdist_vlpi_base()	(gic_data_rdist_rd_base() + SZ_128K)
>   
> @@ -1625,7 +1626,7 @@ static void its_free_prop_table(struct page *prop_page)
>   		   get_order(LPI_PROPBASE_SZ));
>   }
>   
> -static int __init its_alloc_lpi_tables(void)
> +static int __init its_alloc_lpi_prop_table(void)

A bit of a nit, but there is already a function called 
"its_allocate_prop_table" which I find very easy to confuse with this one.

And patch 3 factored the initialization out of its_allocate_prop_table. 
So I was wondering whether it would not actually be better to open-code 
it here and get rid of that function. Otherwise I'd suggest having more 
distinct names.

Otherwise the patch looks good.

Thanks,

>   {
>   	phys_addr_t paddr;
>   
> @@ -1944,30 +1945,47 @@ static void its_free_pending_table(struct page *pt)
>   	free_pages((unsigned long)page_address(pt), get_order(LPI_PENDBASE_SZ));
>   }
>   
> -static void its_cpu_init_lpis(void)
> +static int __init allocate_lpi_tables(void)
>   {
> -	void __iomem *rbase = gic_data_rdist_rd_base();
> -	struct page *pend_page;
> -	u64 val, tmp;
> +	int err, cpu;
>   
> -	/* If we didn't allocate the pending table yet, do it now */
> -	pend_page = gic_data_rdist()->pend_page;
> -	if (!pend_page) {
> -		phys_addr_t paddr;
> +	err = its_alloc_lpi_prop_table();
> +	if (err)
> +		return err;
> +
> +	/*
> +	 * We allocate all the pending tables anyway, as we may have a
> +	 * mix of RDs that have had LPIs enabled, and some that
> +	 * don't. We'll free the unused ones as each CPU comes online.
> +	 */
> +	for_each_possible_cpu(cpu) {
> +		struct page *pend_page;
>   
>   		pend_page = its_allocate_pending_table(GFP_NOWAIT);
>   		if (!pend_page) {
> -			pr_err("Failed to allocate PENDBASE for CPU%d\n",
> -			       smp_processor_id());
> -			return;
> +			pr_err("Failed to allocate PENDBASE for CPU%d\n", cpu);
> +			return -ENOMEM;
>   		}
>   
> -		paddr = page_to_phys(pend_page);
> -		pr_info("CPU%d: using LPI pending table @%pa\n",
> -			smp_processor_id(), &paddr);
> -		gic_data_rdist()->pend_page = pend_page;
> +		gic_data_rdist_cpu(cpu)->pend_page = pend_page;
>   	}
>   
> +	return 0;
> +}
> +
> +static void its_cpu_init_lpis(void)
> +{
> +	void __iomem *rbase = gic_data_rdist_rd_base();
> +	struct page *pend_page;
> +	phys_addr_t paddr;
> +	u64 val, tmp;
> +
> +	if (gic_data_rdist()->lpi_enabled)
> +		return;
> +
> +	pend_page = gic_data_rdist()->pend_page;
> +	paddr = page_to_phys(pend_page);
> +
>   	/* set PROPBASE */
>   	val = (page_to_phys(gic_rdists->prop_page) |
>   	       GICR_PROPBASER_InnerShareable |
> @@ -2019,6 +2037,10 @@ static void its_cpu_init_lpis(void)
>   
>   	/* Make sure the GIC has seen the above */
>   	dsb(sy);
> +	gic_data_rdist()->lpi_enabled = true;
> +	pr_info("GICv3: CPU%d: using LPI pending table @%pa\n",
> +		smp_processor_id(),
> +		&paddr);
>   }
>   
>   static void its_cpu_init_collection(struct its_node *its)
> @@ -3497,16 +3519,6 @@ static int redist_disable_lpis(void)
>   	u64 timeout = USEC_PER_SEC;
>   	u64 val;
>   
> -	/*
> -	 * If coming via a CPU hotplug event, we don't need to disable
> -	 * LPIs before trying to re-enable them. They are already
> -	 * configured and all is well in the world. Detect this case
> -	 * by checking the allocation of the pending table for the
> -	 * current CPU.
> -	 */
> -	if (gic_data_rdist()->pend_page)
> -		return 0;
> -
>   	if (!gic_rdists_supports_plpis()) {
>   		pr_info("CPU%d: LPIs not supported\n", smp_processor_id());
>   		return -ENXIO;
> @@ -3516,7 +3528,18 @@ static int redist_disable_lpis(void)
>   	if (!(val & GICR_CTLR_ENABLE_LPIS))
>   		return 0;
>   
> -	pr_warn("CPU%d: Booted with LPIs enabled, memory probably corrupted\n",
> +	/*
> +	 * If coming via a CPU hotplug event, we don't need to disable
> +	 * LPIs before trying to re-enable them. They are already
> +	 * configured and all is well in the world.
> +	 */
> +	if (gic_data_rdist()->lpi_enabled)
> +		return 0;
> +
> +	/*
> +	 * From that point on, we only try to do some damage control.
> +	 */
> +	pr_warn("GICv3: CPU%d: Booted with LPIs enabled, memory probably corrupted\n",
>   		smp_processor_id());
>   	add_taint(TAINT_CRAP, LOCKDEP_STILL_OK);
>   
> @@ -3772,7 +3795,8 @@ int __init its_init(struct fwnode_handle *handle, struct rdists *rdists,
>   	}
>   
>   	gic_rdists = rdists;
> -	err = its_alloc_lpi_tables();
> +
> +	err = allocate_lpi_tables();
>   	if (err)
>   		return err;
>   
> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
> index 8bdbb5f29494..266093e845bb 100644
> --- a/include/linux/irqchip/arm-gic-v3.h
> +++ b/include/linux/irqchip/arm-gic-v3.h
> @@ -585,6 +585,7 @@ struct rdists {
>   		void __iomem	*rd_base;
>   		struct page	*pend_page;
>   		phys_addr_t	phys_base;
> +		bool		lpi_enabled;
>   	} __percpu		*rdist;
>   	struct page		*prop_page;
>   	u64			flags;
> 

-- 
Julien Thierry

Re: [PATCH 04/10] irqchip/gic-v3-its: Move pending table allocation to init time

[Marc Zyngier]

Hi Julien,

On 24/09/18 12:58, Julien Thierry wrote:
> Hi Marc,
> 
> On 21/09/18 20:59, Marc Zyngier wrote:
>> Pending tables for the redistributors are currently allocated
>> one at a time as each CPU boots. This is causing some grief
>> for Linux/RT (allocation from within a CPU hotplug notifier is
>> frown upon).
>>
>> Let's more this allocation to take place at init time, when we
>> only have a single CPU. It means we're allocating memory for CPUs
>> that are not online yet, but most system will boot all of their
>> CPUs anyway, so that's not completely wasted.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>    drivers/irqchip/irq-gic-v3-its.c   | 80 +++++++++++++++++++-----------
>>    include/linux/irqchip/arm-gic-v3.h |  1 +
>>    2 files changed, 53 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>> index 7ef6baea2d78..462bba422189 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -173,6 +173,7 @@ static DEFINE_RAW_SPINLOCK(vmovp_lock);
>>    static DEFINE_IDA(its_vpeid_ida);
>>    
>>    #define gic_data_rdist()		(raw_cpu_ptr(gic_rdists->rdist))
>> +#define gic_data_rdist_cpu(cpu)		(per_cpu_ptr(gic_rdists->rdist, cpu))
>>    #define gic_data_rdist_rd_base()	(gic_data_rdist()->rd_base)
>>    #define gic_data_rdist_vlpi_base()	(gic_data_rdist_rd_base() + SZ_128K)
>>    
>> @@ -1625,7 +1626,7 @@ static void its_free_prop_table(struct page *prop_page)
>>    		   get_order(LPI_PROPBASE_SZ));
>>    }
>>    
>> -static int __init its_alloc_lpi_tables(void)
>> +static int __init its_alloc_lpi_prop_table(void)
> 
> A bit of a nit, but there is already a function called
> "its_allocate_prop_table" which I find very easy to confuse with this one.
> 
> And patch 3 factored the initialization out of its_allocate_prop_table.
> So I was wondering whether it would not actually be better to open-code
> it here and get rid of that function. Otherwise I'd suggest having more
> distinct names.

its_allocate_prop_table is also used by the VLPI code to allocate guest 
property tables, so I'd rather not open-code it.

How about renaming this function to its_setup_lpi_prop_table?

Thanks,

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

[PATCH 05/10] irqchip/gic-v3-its: Keep track of property table's PA and VA

[Marc Zyngier]

We're currently only tracking the page allocated to contain the
property table by its struct page. In the future, it is going to
be convenient to track both PA and VA for that page instead. Let's
do that.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/irqchip/irq-gic-v3-its.c   | 23 +++++++++++++----------
 include/linux/irqchip/arm-gic-v3.h |  3 ++-
 2 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 462bba422189..c2f1138034fd 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1029,7 +1029,7 @@ static inline u32 its_get_event_id(struct irq_data *d)
 static void lpi_write_config(struct irq_data *d, u8 clr, u8 set)
 {
 	irq_hw_number_t hwirq;
-	struct page *prop_page;
+	void *va;
 	u8 *cfg;
 
 	if (irqd_is_forwarded_to_vcpu(d)) {
@@ -1037,7 +1037,7 @@ static void lpi_write_config(struct irq_data *d, u8 clr, u8 set)
 		u32 event = its_get_event_id(d);
 		struct its_vlpi_map *map;
 
-		prop_page = its_dev->event_map.vm->vprop_page;
+		va = page_address(its_dev->event_map.vm->vprop_page);
 		map = &its_dev->event_map.vlpi_maps[event];
 		hwirq = map->vintid;
 
@@ -1045,11 +1045,11 @@ static void lpi_write_config(struct irq_data *d, u8 clr, u8 set)
 		map->properties &= ~clr;
 		map->properties |= set | LPI_PROP_GROUP1;
 	} else {
-		prop_page = gic_rdists->prop_page;
+		va = gic_rdists->prop_table_va;
 		hwirq = d->hwirq;
 	}
 
-	cfg = page_address(prop_page) + hwirq - 8192;
+	cfg = va + hwirq - 8192;
 	*cfg &= ~clr;
 	*cfg |= set | LPI_PROP_GROUP1;
 
@@ -1628,18 +1628,21 @@ static void its_free_prop_table(struct page *prop_page)
 
 static int __init its_alloc_lpi_prop_table(void)
 {
-	phys_addr_t paddr;
+	struct page *page;
 
 	lpi_id_bits = min_t(u32, GICD_TYPER_ID_BITS(gic_rdists->gicd_typer),
 				ITS_MAX_LPI_NRBITS);
-	gic_rdists->prop_page = its_allocate_prop_table(GFP_NOWAIT);
-	if (!gic_rdists->prop_page) {
+	page = its_allocate_prop_table(GFP_NOWAIT);
+	if (!page) {
 		pr_err("Failed to allocate PROPBASE\n");
 		return -ENOMEM;
 	}
 
-	paddr = page_to_phys(gic_rdists->prop_page);
-	pr_info("GIC: using LPI property table @%pa\n", &paddr);
+	gic_rdists->prop_table_pa = page_to_phys(page);
+	gic_rdists->prop_table_va = page_address(page);
+
+	pr_info("GICv3: using LPI property table @%pa\n",
+		&gic_rdists->prop_table_pa);
 
 	return its_lpi_init(lpi_id_bits);
 }
@@ -1987,7 +1990,7 @@ static void its_cpu_init_lpis(void)
 	paddr = page_to_phys(pend_page);
 
 	/* set PROPBASE */
-	val = (page_to_phys(gic_rdists->prop_page) |
+	val = (gic_rdists->prop_table_pa |
 	       GICR_PROPBASER_InnerShareable |
 	       GICR_PROPBASER_RaWaWb |
 	       ((LPI_NRBITS - 1) & GICR_PROPBASER_IDBITS_MASK));
diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
index 266093e845bb..c2a7b863fc2e 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -587,7 +587,8 @@ struct rdists {
 		phys_addr_t	phys_base;
 		bool		lpi_enabled;
 	} __percpu		*rdist;
-	struct page		*prop_page;
+	phys_addr_t		prop_table_pa;
+	void			*prop_table_va;
 	u64			flags;
 	u32			gicd_typer;
 	bool			has_vlpis;
-- 
2.18.0

[PATCH 06/10] irqchip/gic-v3-its: Allow use of pre-programmed LPI tables

[Marc Zyngier]

In order to cope with kexec and GICv3, let's try and spot when
we're booting with LPIs already enabled, and the tables already
programmed into the redistributors.

This code is currently guarded by a predicate that is always false,
meaning this is not functionnal just yet.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/irqchip/irq-gic-v3-its.c | 73 ++++++++++++++++++++++++++------
 1 file changed, 61 insertions(+), 12 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index c2f1138034fd..e29ce9f2ac8a 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -52,6 +52,7 @@
 #define ITS_FLAGS_SAVE_SUSPEND_STATE		(1ULL << 3)
 
 #define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING	(1 << 0)
+#define RDIST_FLAGS_RD_TABLES_PREALLOCATED	(1 << 1)
 
 static u32 lpi_id_bits;
 
@@ -1628,18 +1629,32 @@ static void its_free_prop_table(struct page *prop_page)
 
 static int __init its_alloc_lpi_prop_table(void)
 {
-	struct page *page;
+	if (gic_rdists->flags & RDIST_FLAGS_RD_TABLES_PREALLOCATED) {
+		u64 val;
 
-	lpi_id_bits = min_t(u32, GICD_TYPER_ID_BITS(gic_rdists->gicd_typer),
-				ITS_MAX_LPI_NRBITS);
-	page = its_allocate_prop_table(GFP_NOWAIT);
-	if (!page) {
-		pr_err("Failed to allocate PROPBASE\n");
-		return -ENOMEM;
-	}
+		val = gicr_read_propbaser(gic_data_rdist_rd_base() + GICR_PROPBASER);
+		lpi_id_bits = (val & GICR_PROPBASER_IDBITS_MASK) + 1;
 
-	gic_rdists->prop_table_pa = page_to_phys(page);
-	gic_rdists->prop_table_va = page_address(page);
+		gic_rdists->prop_table_pa = val & GENMASK_ULL(51, 12);
+		gic_rdists->prop_table_va = memremap(gic_rdists->prop_table_pa,
+						     LPI_PROPBASE_SZ,
+						     MEMREMAP_WB);
+		gic_reset_prop_table(gic_rdists->prop_table_va);
+	} else {
+		struct page *page;
+
+		lpi_id_bits = min_t(u32,
+				    GICD_TYPER_ID_BITS(gic_rdists->gicd_typer),
+				    ITS_MAX_LPI_NRBITS);
+		page = its_allocate_prop_table(GFP_NOWAIT);
+		if (!page) {
+			pr_err("Failed to allocate PROPBASE\n");
+			return -ENOMEM;
+		}
+
+		gic_rdists->prop_table_pa = page_to_phys(page);
+		gic_rdists->prop_table_va = page_address(page);
+	}
 
 	pr_info("GICv3: using LPI property table @%pa\n",
 		&gic_rdists->prop_table_pa);
@@ -1948,10 +1963,27 @@ static void its_free_pending_table(struct page *pt)
 	free_pages((unsigned long)page_address(pt), get_order(LPI_PENDBASE_SZ));
 }
 
+static bool enabled_lpis_allowed(void)
+{
+	return false;
+}
+
 static int __init allocate_lpi_tables(void)
 {
+	u64 val;
 	int err, cpu;
 
+	/*
+	 * If LPIs are enabled while we run this from the boot CPU,
+	 * flag the RD tables as pre-allocated if the stars do align.
+	 */
+	val = readl_relaxed(gic_data_rdist_rd_base() + GICR_CTLR);
+	if ((val & GICR_CTLR_ENABLE_LPIS) && enabled_lpis_allowed()) {
+		gic_rdists->flags |= (RDIST_FLAGS_RD_TABLES_PREALLOCATED |
+				      RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING);
+		pr_info("GICv3: Using preallocated redistributor tables\n");
+	}
+
 	err = its_alloc_lpi_prop_table();
 	if (err)
 		return err;
@@ -1986,6 +2018,18 @@ static void its_cpu_init_lpis(void)
 	if (gic_data_rdist()->lpi_enabled)
 		return;
 
+	val = readl_relaxed(rbase + GICR_CTLR);
+	if ((gic_rdists->flags & RDIST_FLAGS_RD_TABLES_PREALLOCATED) &&
+	    (val & GICR_CTLR_ENABLE_LPIS)) {
+		paddr = gicr_read_pendbaser(rbase + GICR_PENDBASER);
+		paddr &= GENMASK_ULL(51, 16);
+
+		its_free_pending_table(gic_data_rdist()->pend_page);
+		gic_data_rdist()->pend_page = NULL;
+
+		goto out;
+	}
+
 	pend_page = gic_data_rdist()->pend_page;
 	paddr = page_to_phys(pend_page);
 
@@ -2040,9 +2084,11 @@ static void its_cpu_init_lpis(void)
 
 	/* Make sure the GIC has seen the above */
 	dsb(sy);
+out:
 	gic_data_rdist()->lpi_enabled = true;
-	pr_info("GICv3: CPU%d: using LPI pending table @%pa\n",
+	pr_info("GICv3: CPU%d: using %s LPI pending table @%pa\n",
 		smp_processor_id(),
+		gic_data_rdist()->pend_page ? "allocated" : "reserved",
 		&paddr);
 }
 
@@ -3535,8 +3581,11 @@ static int redist_disable_lpis(void)
 	 * If coming via a CPU hotplug event, we don't need to disable
 	 * LPIs before trying to re-enable them. They are already
 	 * configured and all is well in the world.
+	 *
+	 * If running with preallocated tables, there is nothing to do.
 	 */
-	if (gic_data_rdist()->lpi_enabled)
+	if (gic_data_rdist()->lpi_enabled ||
+	    (gic_rdists->flags & RDIST_FLAGS_RD_TABLES_PREALLOCATED))
 		return 0;
 
 	/*
-- 
2.18.0

Re: [PATCH 06/10] irqchip/gic-v3-its: Allow use of pre-programmed LPI tables

[Julien Thierry]

On 21/09/18 20:59, Marc Zyngier wrote:
> In order to cope with kexec and GICv3, let's try and spot when
> we're booting with LPIs already enabled, and the tables already
> programmed into the redistributors.
> 
> This code is currently guarded by a predicate that is always false,
> meaning this is not functionnal just yet.
> 

Reviewed-by: Julien Thierry <julien.thierry@arm.com>

> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>   drivers/irqchip/irq-gic-v3-its.c | 73 ++++++++++++++++++++++++++------
>   1 file changed, 61 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index c2f1138034fd..e29ce9f2ac8a 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -52,6 +52,7 @@
>   #define ITS_FLAGS_SAVE_SUSPEND_STATE		(1ULL << 3)
>   
>   #define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING	(1 << 0)
> +#define RDIST_FLAGS_RD_TABLES_PREALLOCATED	(1 << 1)
>   
>   static u32 lpi_id_bits;
>   
> @@ -1628,18 +1629,32 @@ static void its_free_prop_table(struct page *prop_page)
>   
>   static int __init its_alloc_lpi_prop_table(void)
>   {
> -	struct page *page;
> +	if (gic_rdists->flags & RDIST_FLAGS_RD_TABLES_PREALLOCATED) {
> +		u64 val;
>   
> -	lpi_id_bits = min_t(u32, GICD_TYPER_ID_BITS(gic_rdists->gicd_typer),
> -				ITS_MAX_LPI_NRBITS);
> -	page = its_allocate_prop_table(GFP_NOWAIT);
> -	if (!page) {
> -		pr_err("Failed to allocate PROPBASE\n");
> -		return -ENOMEM;
> -	}
> +		val = gicr_read_propbaser(gic_data_rdist_rd_base() + GICR_PROPBASER);
> +		lpi_id_bits = (val & GICR_PROPBASER_IDBITS_MASK) + 1;
>   
> -	gic_rdists->prop_table_pa = page_to_phys(page);
> -	gic_rdists->prop_table_va = page_address(page);
> +		gic_rdists->prop_table_pa = val & GENMASK_ULL(51, 12);
> +		gic_rdists->prop_table_va = memremap(gic_rdists->prop_table_pa,
> +						     LPI_PROPBASE_SZ,
> +						     MEMREMAP_WB);
> +		gic_reset_prop_table(gic_rdists->prop_table_va);
> +	} else {
> +		struct page *page;
> +
> +		lpi_id_bits = min_t(u32,
> +				    GICD_TYPER_ID_BITS(gic_rdists->gicd_typer),
> +				    ITS_MAX_LPI_NRBITS);
> +		page = its_allocate_prop_table(GFP_NOWAIT);
> +		if (!page) {
> +			pr_err("Failed to allocate PROPBASE\n");
> +			return -ENOMEM;
> +		}
> +
> +		gic_rdists->prop_table_pa = page_to_phys(page);
> +		gic_rdists->prop_table_va = page_address(page);
> +	}
>   
>   	pr_info("GICv3: using LPI property table @%pa\n",
>   		&gic_rdists->prop_table_pa);
> @@ -1948,10 +1963,27 @@ static void its_free_pending_table(struct page *pt)
>   	free_pages((unsigned long)page_address(pt), get_order(LPI_PENDBASE_SZ));
>   }
>   
> +static bool enabled_lpis_allowed(void)
> +{
> +	return false;
> +}
> +
>   static int __init allocate_lpi_tables(void)
>   {
> +	u64 val;
>   	int err, cpu;
>   
> +	/*
> +	 * If LPIs are enabled while we run this from the boot CPU,
> +	 * flag the RD tables as pre-allocated if the stars do align.
> +	 */
> +	val = readl_relaxed(gic_data_rdist_rd_base() + GICR_CTLR);
> +	if ((val & GICR_CTLR_ENABLE_LPIS) && enabled_lpis_allowed()) {
> +		gic_rdists->flags |= (RDIST_FLAGS_RD_TABLES_PREALLOCATED |
> +				      RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING);
> +		pr_info("GICv3: Using preallocated redistributor tables\n");
> +	}
> +
>   	err = its_alloc_lpi_prop_table();
>   	if (err)
>   		return err;
> @@ -1986,6 +2018,18 @@ static void its_cpu_init_lpis(void)
>   	if (gic_data_rdist()->lpi_enabled)
>   		return;
>   
> +	val = readl_relaxed(rbase + GICR_CTLR);
> +	if ((gic_rdists->flags & RDIST_FLAGS_RD_TABLES_PREALLOCATED) &&
> +	    (val & GICR_CTLR_ENABLE_LPIS)) {
> +		paddr = gicr_read_pendbaser(rbase + GICR_PENDBASER);
> +		paddr &= GENMASK_ULL(51, 16);
> +
> +		its_free_pending_table(gic_data_rdist()->pend_page);
> +		gic_data_rdist()->pend_page = NULL;
> +
> +		goto out;
> +	}
> +
>   	pend_page = gic_data_rdist()->pend_page;
>   	paddr = page_to_phys(pend_page);
>   
> @@ -2040,9 +2084,11 @@ static void its_cpu_init_lpis(void)
>   
>   	/* Make sure the GIC has seen the above */
>   	dsb(sy);
> +out:
>   	gic_data_rdist()->lpi_enabled = true;
> -	pr_info("GICv3: CPU%d: using LPI pending table @%pa\n",
> +	pr_info("GICv3: CPU%d: using %s LPI pending table @%pa\n",
>   		smp_processor_id(),
> +		gic_data_rdist()->pend_page ? "allocated" : "reserved",
>   		&paddr);
>   }
>   
> @@ -3535,8 +3581,11 @@ static int redist_disable_lpis(void)
>   	 * If coming via a CPU hotplug event, we don't need to disable
>   	 * LPIs before trying to re-enable them. They are already
>   	 * configured and all is well in the world.
> +	 *
> +	 * If running with preallocated tables, there is nothing to do.
>   	 */
> -	if (gic_data_rdist()->lpi_enabled)
> +	if (gic_data_rdist()->lpi_enabled ||
> +	    (gic_rdists->flags & RDIST_FLAGS_RD_TABLES_PREALLOCATED))
>   		return 0;
>   
>   	/*
> 

-- 
Julien Thierry

[PATCH 07/10] irqchip/gic-v3-its: Use pre-programmed redistributor tables with kdump kernels

[Marc Zyngier]

If using a kdump kernel, and that we cannot disable LPIs to install
our own tables, let's switch to using the already allocated tables.

This means that we'll change some of the initial kernel's memory,
but at least we'll be able to have LPIs in this secondary kernel.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/irqchip/irq-gic-v3-its.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index e29ce9f2ac8a..0b3e76cdde26 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -19,6 +19,7 @@
 #include <linux/acpi_iort.h>
 #include <linux/bitmap.h>
 #include <linux/cpu.h>
+#include <linux/crash_dump.h>
 #include <linux/delay.h>
 #include <linux/dma-iommu.h>
 #include <linux/interrupt.h>
@@ -1963,8 +1964,15 @@ static void its_free_pending_table(struct page *pt)
 	free_pages((unsigned long)page_address(pt), get_order(LPI_PENDBASE_SZ));
 }
 
+/*
+ * Booting with kdump and LPIs enabled is generally fine.
+ */
 static bool enabled_lpis_allowed(void)
 {
+	/* Allow a kdump kernel */
+	if (is_kdump_kernel())
+		return true;
+
 	return false;
 }
 
-- 
2.18.0

[PATCH 08/10] irqchip/gic-v3-its: Check that all RDs have the same property table

[Marc Zyngier]

If booting with LPIs enabled, all the redistributors must have the
exact same property table. No ifs, no buts.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/irqchip/irq-gic-v3-its.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 0b3e76cdde26..4d9604dd6fb1 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -2029,6 +2029,15 @@ static void its_cpu_init_lpis(void)
 	val = readl_relaxed(rbase + GICR_CTLR);
 	if ((gic_rdists->flags & RDIST_FLAGS_RD_TABLES_PREALLOCATED) &&
 	    (val & GICR_CTLR_ENABLE_LPIS)) {
+		/*
+		 * Check that we get the same property table on all
+		 * RDs. If we don't, this is hopeless.
+		 */
+		paddr = gicr_read_propbaser(rbase + GICR_PROPBASER);
+		paddr &= GENMASK_ULL(51, 12);
+		if (WARN_ON(gic_rdists->prop_table_pa != paddr))
+			add_taint(TAINT_CRAP, LOCKDEP_STILL_OK);
+
 		paddr = gicr_read_pendbaser(rbase + GICR_PENDBASER);
 		paddr &= GENMASK_ULL(51, 16);
 
-- 
2.18.0

[PATCH 09/10] irqchip/gic-v3-its: Register LPI tables with EFI config table

[Marc Zyngier]

Upon enabling a redistributor, let's register the allocated tables
with the EFI table that tracks the memory reservations.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/irqchip/irq-gic-v3-its.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 4d9604dd6fb1..4912dc57bf07 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -22,6 +22,7 @@
 #include <linux/crash_dump.h>
 #include <linux/delay.h>
 #include <linux/dma-iommu.h>
+#include <linux/efi.h>
 #include <linux/interrupt.h>
 #include <linux/irqdomain.h>
 #include <linux/list.h>
@@ -1628,6 +1629,14 @@ static void its_free_prop_table(struct page *prop_page)
 		   get_order(LPI_PROPBASE_SZ));
 }
 
+static int gic_reserve_range(phys_addr_t addr, unsigned long size)
+{
+	if (efi_enabled(EFI_CONFIG_TABLES))
+		return efi_mem_reserve_persistent(addr, size);
+
+	return 0;
+}
+
 static int __init its_alloc_lpi_prop_table(void)
 {
 	if (gic_rdists->flags & RDIST_FLAGS_RD_TABLES_PREALLOCATED) {
@@ -1655,6 +1664,8 @@ static int __init its_alloc_lpi_prop_table(void)
 
 		gic_rdists->prop_table_pa = page_to_phys(page);
 		gic_rdists->prop_table_va = page_address(page);
+		WARN_ON(gic_reserve_range(gic_rdists->prop_table_pa,
+					  LPI_PROPBASE_SZ));
 	}
 
 	pr_info("GICv3: using LPI property table @%pa\n",
@@ -2049,6 +2060,7 @@ static void its_cpu_init_lpis(void)
 
 	pend_page = gic_data_rdist()->pend_page;
 	paddr = page_to_phys(pend_page);
+	WARN_ON(gic_reserve_range(paddr, LPI_PENDBASE_SZ));
 
 	/* set PROPBASE */
 	val = (gic_rdists->prop_table_pa |
-- 
2.18.0

[PATCH 10/10] irqchip/gic-v3-its: Allow use of LPI tables in reserved memory

[Marc Zyngier]

If the LPI tables have been reserved with the EFI reservation
mechanism, we assume that these tables are safe to use even
when we find the redistributors to have LPIs enabled at
boot time, meaning that kexec can now work with GICv3.

You're welcome.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/irqchip/irq-gic-v3-its.c | 43 ++++++++++++++++++++++++++++----
 1 file changed, 38 insertions(+), 5 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 4912dc57bf07..0235b69160bc 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -28,6 +28,7 @@
 #include <linux/list.h>
 #include <linux/list_sort.h>
 #include <linux/log2.h>
+#include <linux/memblock.h>
 #include <linux/mm.h>
 #include <linux/msi.h>
 #include <linux/of.h>
@@ -1629,6 +1630,33 @@ static void its_free_prop_table(struct page *prop_page)
 		   get_order(LPI_PROPBASE_SZ));
 }
 
+static bool gic_check_reserved_range(phys_addr_t addr, unsigned long size)
+{
+	phys_addr_t start, end, addr_end;
+	u64 i;
+
+	/*
+	 * We don't bother checking for a kdump kernel as by
+	 * construction, the LPI tables are out of this kernel's
+	 * memory map.
+	 */
+	if (is_kdump_kernel())
+		return true;
+
+	addr_end = addr + size - 1;
+
+	for_each_reserved_mem_region(i, &start, &end) {
+		if (addr >= start && addr_end <= end)
+			return true;
+	}
+
+	/* Not found, not a good sign... */
+	pr_warn("GICv3: Expected reserved range [%pa:%pa], not found\n",
+		&addr, &addr_end);
+	add_taint(TAINT_CRAP, LOCKDEP_STILL_OK);
+	return false;
+}
+
 static int gic_reserve_range(phys_addr_t addr, unsigned long size)
 {
 	if (efi_enabled(EFI_CONFIG_TABLES))
@@ -1976,15 +2004,19 @@ static void its_free_pending_table(struct page *pt)
 }
 
 /*
- * Booting with kdump and LPIs enabled is generally fine.
+ * Booting with kdump and LPIs enabled is generally fine. Any other
+ * case is wrong in the absence of firmware/EFI support.
  */
 static bool enabled_lpis_allowed(void)
 {
-	/* Allow a kdump kernel */
-	if (is_kdump_kernel())
-		return true;
+	phys_addr_t addr;
+	u64 val;
 
-	return false;
+	/* Check whether the property table is in a reserved region */
+	val = gicr_read_propbaser(gic_data_rdist_rd_base() + GICR_PROPBASER);
+	addr = val & GENMASK_ULL(51, 12);
+
+	return gic_check_reserved_range(addr, LPI_PROPBASE_SZ);
 }
 
 static int __init allocate_lpi_tables(void)
@@ -2052,6 +2084,7 @@ static void its_cpu_init_lpis(void)
 		paddr = gicr_read_pendbaser(rbase + GICR_PENDBASER);
 		paddr &= GENMASK_ULL(51, 16);
 
+		WARN_ON(!gic_check_reserved_range(paddr, LPI_PENDBASE_SZ));
 		its_free_pending_table(gic_data_rdist()->pend_page);
 		gic_data_rdist()->pend_page = NULL;
 
-- 
2.18.0

Re: [PATCH 00/10] GICv3 support for kexec/kdump on EFI systems

[Jeremy Linton]

Hi,

On 09/21/2018 02:59 PM, Marc Zyngier wrote:
> The GICv3 architecture has the remarkable feature that once LPI tables
> have been assigned to redistributors and that LPI delivery is enabled,
> there is no guarantee that LPIs can be turned off (and most
> implementations do not allow it), nor can it be reprogrammed to use
> other tables.
> 
> This is a bit of a problem for kexec, where the secondary kernel
> completely looses track of the previous allocations. If the secondary
> kernel doesn't allocate the tables exactly the same way, no LPIs will
> be delivered by the GIC (which continues to use the old tables), and
> memory previously allocated for the pending tables will be slowly
> corrupted, one bit at a time.
> 
> The workaround for this is based on a series[1] by Ard Biesheuvel,
> which adds the required infrastructure for memory reservations to be
> passed from one kernel to another using an EFI table.
> 
> This infrastructure is then used to register the allocation of GIC
> tables with EFI, and allow the GIC driver to safely reuse the existing
> programming if it detects that the tables have been correctly
> registered. On non-EFI systems, there is not much we can do.
> 
> This has been tested on a TX2 system both as a host and a guest. I'd
> welcome additional testing of different HW. For convenience, I've
> stashed a branch containing the whole thing at [2].

When combined with Ard's patch set, this fixes kdump on a QC machine.

Tested-by: Jeremy Linton <jeremy.linton@arm.com>

Thanks,

> 
> [1] https://marc.info/?l=linux-efi&m=153754757208163&w=2
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/gicv3-kdump
> 
> Marc Zyngier (10):
>    irqchip/gic-v3-its: Change initialization ordering for LPIs
>    irqchip/gic-v3-its: Consolidate LPI_PENDBASE_SZ usage
>    irqchip/gic-v3-its: Split property table clearing from allocation
>    irqchip/gic-v3-its: Move pending table allocation to init time
>    irqchip/gic-v3-its: Keep track of property table's PA and VA
>    irqchip/gic-v3-its: Allow use of pre-programmed LPI tables
>    irqchip/gic-v3-its: Use pre-programmed redistributor tables with kdump
>      kernels
>    irqchip/gic-v3-its: Check that all RDs have the same property table
>    irqchip/gic-v3-its: Register LPI tables with EFI config table
>    irqchip/gic-v3-its: Allow use of LPI tables in reserved memory
> 
>   drivers/irqchip/irq-gic-v3-its.c   | 249 ++++++++++++++++++++++-------
>   drivers/irqchip/irq-gic-v3.c       |  20 ++-
>   include/linux/irqchip/arm-gic-v3.h |   4 +-
>   3 files changed, 208 insertions(+), 65 deletions(-)
> 

Re: [PATCH 00/10] GICv3 support for kexec/kdump on EFI systems

[Bhupesh Sharma]

Hi Marc,

On 09/22/2018 01:29 AM, Marc Zyngier wrote:
> The GICv3 architecture has the remarkable feature that once LPI tables
> have been assigned to redistributors and that LPI delivery is enabled,
> there is no guarantee that LPIs can be turned off (and most
> implementations do not allow it), nor can it be reprogrammed to use
> other tables.
> 
> This is a bit of a problem for kexec, where the secondary kernel
> completely looses track of the previous allocations. If the secondary
> kernel doesn't allocate the tables exactly the same way, no LPIs will
> be delivered by the GIC (which continues to use the old tables), and
> memory previously allocated for the pending tables will be slowly
> corrupted, one bit at a time.
> 
> The workaround for this is based on a series[1] by Ard Biesheuvel,
> which adds the required infrastructure for memory reservations to be
> passed from one kernel to another using an EFI table.
> 
> This infrastructure is then used to register the allocation of GIC
> tables with EFI, and allow the GIC driver to safely reuse the existing
> programming if it detects that the tables have been correctly
> registered. On non-EFI systems, there is not much we can do.
> 
> This has been tested on a TX2 system both as a host and a guest. I'd
> welcome additional testing of different HW. For convenience, I've
> stashed a branch containing the whole thing at [2].
> 
> [1] https://marc.info/?l=linux-efi&m=153754757208163&w=2
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/gicv3-kdump
> 
> Marc Zyngier (10):
>    irqchip/gic-v3-its: Change initialization ordering for LPIs
>    irqchip/gic-v3-its: Consolidate LPI_PENDBASE_SZ usage
>    irqchip/gic-v3-its: Split property table clearing from allocation
>    irqchip/gic-v3-its: Move pending table allocation to init time
>    irqchip/gic-v3-its: Keep track of property table's PA and VA
>    irqchip/gic-v3-its: Allow use of pre-programmed LPI tables
>    irqchip/gic-v3-its: Use pre-programmed redistributor tables with kdump
>      kernels
>    irqchip/gic-v3-its: Check that all RDs have the same property table
>    irqchip/gic-v3-its: Register LPI tables with EFI config table
>    irqchip/gic-v3-its: Allow use of LPI tables in reserved memory
> 
>   drivers/irqchip/irq-gic-v3-its.c   | 249 ++++++++++++++++++++++-------
>   drivers/irqchip/irq-gic-v3.c       |  20 ++-
>   include/linux/irqchip/arm-gic-v3.h |   4 +-
>   3 files changed, 208 insertions(+), 65 deletions(-)

Thanks for the patchset. I can confirm that with Ard's patchset in [1] 
and this patchset applied on 'efi/next' branch, I see that the "Booted 
with LPIs enabled, memory probably corrupted" issue that I was seeing on 
gigabyte boards in kdump kernel is fixed. Here are some logs:

without patchset applied:
=========================

[    0.000000] NR_IRQS: 64, nr_irqs: 64, preallocated irqs: 0
[    0.000000] GICv3: GIC: Using split EOI/Deactivate mode
[    0.000000] GICv3: Distributor has no Range Selector support
[    0.000000] GICv3: no VLPI support, direct LPI support
[    0.000000] ITS [mem 0x801000020000-0x80100021ffff]
[    0.000000] ITS@0x0000801000020000: allocated 2097152 Devices 
@c1000000 (flat, esz 8, psz 64K, shr 1)
[    0.000000] GIC: using LPI property table @0x00000000c03b0000
[    0.000000] GICv3: CPU0: found redistributor a region 
0:0x0000801080140000
[    0.000000] CPU0: Booted with LPIs enabled, memory probably corrupted
[    0.000000] CPU0: Failed to disable LPIs
<..snip..>
[  198.702976] dracut-initqueue[298]: Warning: dracut-initqueue timeout 
- starting timeout scripts
[  199.332238] dracut-initqueue[298]: Warning: dracut-initqueue timeout 
- starting timeout scripts
[  199.922944] dracut-initqueue[298]: Warning: dracut-initqueue timeout 
- starting timeout scripts
[  200.512239] dracut-initqueue[298]: Warning: dracut-initqueue timeout 
- starting timeout scripts
<..snip..>


with patchset applied:
======================
[    0.000000] NR_IRQS: 64, nr_irqs: 64, preallocated irqs: 0
[    0.000000] GICv3: GIC: Using split EOI/Deactivate mode
[    0.000000] GICv3: Distributor has no Range Selector support
[    0.000000] GICv3: no VLPI support, direct LPI support
[    0.000000] GICv3: CPU0: found redistributor 109 region 
0:0x0000801080320000
[    0.000000] ITS [mem 0x801000020000-0x80100021ffff]
[    0.000000] ITS@0x0000801000020000: allocated 2097152 Devices 
@c1000000 (flat, esz 8, psz 64K, shr 1)
[    0.000000] GICv3: Using preallocated redistributor tables
[    0.000000] GICv3: using LPI property table @0x0000000fc0420000
[    0.000000] GICv3: CPU0: using reserved LPI pending table 
@0x0000000fc05c0000


So, please feel to add:
Tested-by: Bhupesh Sharma <bhsharma@redhat.com>

Thanks,
Bhupesh

RE: [PATCH 00/10] GICv3 support for kexec/kdump on EFI systems

[Zhang, Lei]

Hi Marc

> -----Original Message-----
> From: linux-arm-kernel
> [mailto:linux-arm-kernel-bounces@lists.infradead.org] On Behalf Of Marc
> Zyngier
> Sent: Saturday, September 22, 2018 5:00 AM
> To: linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> Cc: Jeffrey Hugo; Thomas Gleixner; Jason Cooper; Jeremy Linton; Ard
> Biesheuvel
> Subject: [PATCH 00/10] GICv3 support for kexec/kdump on EFI systems
> 
> The GICv3 architecture has the remarkable feature that once LPI tables
> have been assigned to redistributors and that LPI delivery is enabled,
> there is no guarantee that LPIs can be turned off (and most
> implementations do not allow it), nor can it be reprogrammed to use
> other tables.
> 
> This is a bit of a problem for kexec, where the secondary kernel
> completely looses track of the previous allocations. If the secondary
> kernel doesn't allocate the tables exactly the same way, no LPIs will
> be delivered by the GIC (which continues to use the old tables), and
> memory previously allocated for the pending tables will be slowly
> corrupted, one bit at a time.
> 
> The workaround for this is based on a series[1] by Ard Biesheuvel,
> which adds the required infrastructure for memory reservations to be
> passed from one kernel to another using an EFI table.
> 
> This infrastructure is then used to register the allocation of GIC
> tables with EFI, and allow the GIC driver to safely reuse the existing
> programming if it detects that the tables have been correctly
> registered. On non-EFI systems, there is not much we can do.
> 
> This has been tested on a TX2 system both as a host and a guest. I'd
> welcome additional testing of different HW. For convenience, I've
> stashed a branch containing the whole thing at [2].

We have done the test on our chip A64FX that When a write changes 
EnableLPI bit from 0 to 1, this bit becomes RES1. 
The result is that the kexec operation successfully works on our chip,
 and PCI based on LPI also works after kexec.

For detail:
We did "kexec -e" command, and the message, "Using preallocated 
redistributor tables", was shown.
After kexec, we can use our ssd normally.

Test environment
CPU: A64FX

Kernel version: v4.19 rc4 base
https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/gicv3-kdump
8bc67da irqchip/gic-v3-its: Allow use of LPI tables in reserved memory

kexec version:kexec-tools-2.0.14-17.2.el7.aarch64

Tested-by: Lei Zhang <zhang.lei@jp.fujitsu.com>

Thanks a lot.

Best Regards,
Lei,Zhang
FUJITSU LIMITED.

Re: [PATCH 00/10] GICv3 support for kexec/kdump on EFI systems

[Richard Ruigrok]

Hi Marc

On 9/21/2018 1:59 PM, Marc Zyngier wrote:
> The GICv3 architecture has the remarkable feature that once LPI tables
> have been assigned to redistributors and that LPI delivery is enabled,
> there is no guarantee that LPIs can be turned off (and most
> implementations do not allow it), nor can it be reprogrammed to use
> other tables.
>
> This is a bit of a problem for kexec, where the secondary kernel
> completely looses track of the previous allocations. If the secondary
> kernel doesn't allocate the tables exactly the same way, no LPIs will
> be delivered by the GIC (which continues to use the old tables), and
> memory previously allocated for the pending tables will be slowly
> corrupted, one bit at a time.
>
> The workaround for this is based on a series[1] by Ard Biesheuvel,
> which adds the required infrastructure for memory reservations to be
> passed from one kernel to another using an EFI table.
>
> This infrastructure is then used to register the allocation of GIC
> tables with EFI, and allow the GIC driver to safely reuse the existing
> programming if it detects that the tables have been correctly
> registered. On non-EFI systems, there is not much we can do.
>
> This has been tested on a TX2 system both as a host and a guest. I'd
> welcome additional testing of different HW. For convenience, I've
> stashed a branch containing the whole thing at [2].
I tested [2] from the 4.19-rc4 set which included this series and [1].
Tested kexec on Centriq system with ITS support (46 core).  On-board was a MLX CX5 NIC, verified MSIs are active in /proc/interrupts.
Prior to this we used a workaround from Shanker to reuse the same tables in the kexec'ed kernel.
Let me know if further testing is needed, and thanks for adding this support.
> [1] https://marc.info/?l=linux-efi&m=153754757208163&w=2
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/gicv3-kdump
>
> Marc Zyngier (10):
>   irqchip/gic-v3-its: Change initialization ordering for LPIs
>   irqchip/gic-v3-its: Consolidate LPI_PENDBASE_SZ usage
>   irqchip/gic-v3-its: Split property table clearing from allocation
>   irqchip/gic-v3-its: Move pending table allocation to init time
>   irqchip/gic-v3-its: Keep track of property table's PA and VA
>   irqchip/gic-v3-its: Allow use of pre-programmed LPI tables
>   irqchip/gic-v3-its: Use pre-programmed redistributor tables with kdump
>     kernels
>   irqchip/gic-v3-its: Check that all RDs have the same property table
>   irqchip/gic-v3-its: Register LPI tables with EFI config table
>   irqchip/gic-v3-its: Allow use of LPI tables in reserved memory
>
>  drivers/irqchip/irq-gic-v3-its.c   | 249 ++++++++++++++++++++++-------
>  drivers/irqchip/irq-gic-v3.c       |  20 ++-
>  include/linux/irqchip/arm-gic-v3.h |   4 +-
>  3 files changed, 208 insertions(+), 65 deletions(-)
>

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 00/10] GICv3 support for kexec/kdump on EFI systems

[Marc Zyngier]

Hi Richard,

On 27/09/18 22:10, Richard Ruigrok wrote:
> Hi Marc
> 
> On 9/21/2018 1:59 PM, Marc Zyngier wrote:
>> The GICv3 architecture has the remarkable feature that once LPI tables
>> have been assigned to redistributors and that LPI delivery is enabled,
>> there is no guarantee that LPIs can be turned off (and most
>> implementations do not allow it), nor can it be reprogrammed to use
>> other tables.
>>
>> This is a bit of a problem for kexec, where the secondary kernel
>> completely looses track of the previous allocations. If the secondary
>> kernel doesn't allocate the tables exactly the same way, no LPIs will
>> be delivered by the GIC (which continues to use the old tables), and
>> memory previously allocated for the pending tables will be slowly
>> corrupted, one bit at a time.
>>
>> The workaround for this is based on a series[1] by Ard Biesheuvel,
>> which adds the required infrastructure for memory reservations to be
>> passed from one kernel to another using an EFI table.
>>
>> This infrastructure is then used to register the allocation of GIC
>> tables with EFI, and allow the GIC driver to safely reuse the existing
>> programming if it detects that the tables have been correctly
>> registered. On non-EFI systems, there is not much we can do.
>>
>> This has been tested on a TX2 system both as a host and a guest. I'd
>> welcome additional testing of different HW. For convenience, I've
>> stashed a branch containing the whole thing at [2].
> I tested [2] from the 4.19-rc4 set which included this series and [1].
> Tested kexec on Centriq system with ITS support (46 core).  On-board was a MLX CX5 NIC, verified MSIs are active in /proc/interrupts.
> Prior to this we used a workaround from Shanker to reuse the same tables in the kexec'ed kernel.

Yes, I remember seeing this workaround. Hopefully we're in a better 
place now that we can guarantee that the tables are not reused.

> Let me know if further testing is needed, and thanks for adding this support.

Good to know, thanks for having tested it. I've now put this code into 
-next for some more soaking. Hopefully nothing horrible will happen ;-)

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

Re: [PATCH 00/10] GICv3 support for kexec/kdump on EFI systems

[Marc Zyngier]

Hi Xulin,

On 01/02/2019 06:11, Sun Ted wrote:
> Hi Marc,
> 
> Marc Zyngier <marc.zyngier@arm.com <mailto:marc.zyngier@arm.com>> 于2018
> 年9月22日周六 上午4:03写道：
> 
>     The GICv3 architecture has the remarkable feature that once LPI tables
>     have been assigned to redistributors and that LPI delivery is enabled,
>     there is no guarantee that LPIs can be turned off (and most
>     implementations do not allow it), nor can it be reprogrammed to use
>     other tables.
> 
>     This is a bit of a problem for kexec, where the secondary kernel
>     completely looses track of the previous allocations. If the secondary
>     kernel doesn't allocate the tables exactly the same way, no LPIs will
>     be delivered by the GIC (which continues to use the old tables), and
>     memory previously allocated for the pending tables will be slowly
>     corrupted, one bit at a time.
> 
>     The workaround for this is based on a series[1] by Ard Biesheuvel,
>     which adds the required infrastructure for memory reservations to be
>     passed from one kernel to another using an EFI table.
> 
>     This infrastructure is then used to register the allocation of GIC
>     tables with EFI, and allow the GIC driver to safely reuse the existing
>     programming if it detects that the tables have been correctly
>     registered. On non-EFI systems, there is not much we can do.
> 
> 
> Sorry to turn this question out again.
> For others non-EFI systems, just as your said till now we did do
> anything, right?

We didn't do anything, because there is nothing we can do.

> I did see the kexec booting failure since re-setting
> the GICR_CTLR.EnableLPI from "1" to "0" unsuccessful.
> 
> The  below commit adds the judgement for disabling LPIs, and returned
> error. Caused "kexec" booting failure.

And I fully expected this. When I said "we don't do anything", I meant
"we don't do anything to make LPIs work".

> 
> 6eb486b66a (irqchip/gic-v3: Ensure GICR_CTLR.EnableLPI=0 is observed
> before enabling).
> 
> <snip patch>
>  int its_cpu_init(void)
>  {
>         if (!list_empty(&its_nodes)) {
> -               if (!gic_rdists_supports_plpis()) {
> -                       pr_info("CPU%d: LPIs not supported\n",
> smp_processor_id());
> -                       return -ENXIO;
> -               }
> +               int ret;
> +
> +               ret = redist_disable_lpis();
> +               if (ret)
> +                       return ret;
> +

And I maintain that this is the right thing to do. If LPIs are on and
the memory has not been reserved, it is then likely that this memory is
now being used by the kernel for something else. The system is thus
going to see single-bit corruption in some random places.

At that stage, your system is horribly unsafe, and I will not let it
boot under these conditions. If it was working before, that's because
you were lucky, and I place no faith in luck.

Now you have two alternatives:

- You switch to an EFI-based firmware. These days, even u-boot has an
EFI implementation. COnsider doing that if you can.

- If there is no EFI implementation for your SoC, you can pass the
"irqchip.gicv3_nolpi=1" option to the first kernel. This will keep LPI
disabled, and you'll be able to kexec a secondary kernel (and get
working LPIs there). This is what I do on my Chromebook.

Thanks,

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

Re: [PATCH 00/10] GICv3 support for kexec/kdump on EFI systems

[Xulin Sun]

On 02/01/2019 05:15 PM, Marc Zyngier wrote:
> Hi Xulin,
>
> On 01/02/2019 06:11, Sun Ted wrote:
>> Hi Marc,
>>
>> Marc Zyngier <marc.zyngier@arm.com <mailto:marc.zyngier@arm.com>> 于2018
>> 年9月22日周六 上午4:03写道：
>>
>>      The GICv3 architecture has the remarkable feature that once LPI tables
>>      have been assigned to redistributors and that LPI delivery is enabled,
>>      there is no guarantee that LPIs can be turned off (and most
>>      implementations do not allow it), nor can it be reprogrammed to use
>>      other tables.
>>
>>      This is a bit of a problem for kexec, where the secondary kernel
>>      completely looses track of the previous allocations. If the secondary
>>      kernel doesn't allocate the tables exactly the same way, no LPIs will
>>      be delivered by the GIC (which continues to use the old tables), and
>>      memory previously allocated for the pending tables will be slowly
>>      corrupted, one bit at a time.
>>
>>      The workaround for this is based on a series[1] by Ard Biesheuvel,
>>      which adds the required infrastructure for memory reservations to be
>>      passed from one kernel to another using an EFI table.
>>
>>      This infrastructure is then used to register the allocation of GIC
>>      tables with EFI, and allow the GIC driver to safely reuse the existing
>>      programming if it detects that the tables have been correctly
>>      registered. On non-EFI systems, there is not much we can do.
>>
>>
>> Sorry to turn this question out again.
>> For others non-EFI systems, just as your said till now we did do
>> anything, right?
> We didn't do anything, because there is nothing we can do.
>
>> I did see the kexec booting failure since re-setting
>> the GICR_CTLR.EnableLPI from "1" to "0" unsuccessful.
>>
>> The  below commit adds the judgement for disabling LPIs, and returned
>> error. Caused "kexec" booting failure.
> And I fully expected this. When I said "we don't do anything", I meant
> "we don't do anything to make LPIs work".
>
>> 6eb486b66a (irqchip/gic-v3: Ensure GICR_CTLR.EnableLPI=0 is observed
>> before enabling).
>>
>> <snip patch>
>>   int its_cpu_init(void)
>>   {
>>          if (!list_empty(&its_nodes)) {
>> -               if (!gic_rdists_supports_plpis()) {
>> -                       pr_info("CPU%d: LPIs not supported\n",
>> smp_processor_id());
>> -                       return -ENXIO;
>> -               }
>> +               int ret;
>> +
>> +               ret = redist_disable_lpis();
>> +               if (ret)
>> +                       return ret;
>> +
> And I maintain that this is the right thing to do. If LPIs are on and
> the memory has not been reserved, it is then likely that this memory is
> now being used by the kernel for something else. The system is thus
> going to see single-bit corruption in some random places.
>
> At that stage, your system is horribly unsafe, and I will not let it
> boot under these conditions. If it was working before, that's because
> you were lucky, and I place no faith in luck.
>
> Now you have two alternatives:
>
> - You switch to an EFI-based firmware. These days, even u-boot has an
> EFI implementation. COnsider doing that if you can.
>
> - If there is no EFI implementation for your SoC, you can pass the
> "irqchip.gicv3_nolpi=1" option to the first kernel. This will keep LPI
> disabled, and you'll be able to kexec a secondary kernel (and get
> working LPIs there). This is what I do on my Chromebook.

Hi Marc,

Thanks for detailed explanation.

Thanks
Xulin
>
> Thanks,
>
> 	M.