linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] ARM64: errata: Add workaround for HIP10/HIP10C erratum 162200803
@ 2025-06-26 12:41 Zhou Wang
  2025-06-26 13:27 ` Marc Zyngier
  0 siblings, 1 reply; 9+ messages in thread
From: Zhou Wang @ 2025-06-26 12:41 UTC (permalink / raw)
  To: Marc Zyngier, Oliver Upton, Will Deacon, Catalin Marinas
  Cc: linux-arm-kernel, kvmarm, tangnianyao, wangwudi, Zhou Wang

For GICv4.0 of Hip10 and Hip10C, it has a SoC bug with vPE schedule:
when multiple vPEs are sending vpe schedule/deschedule commands
concurrently and repeatedly, some vPE schedule command may not be
scheduled, and it will cause the command timeout.

The hardware implementation is that there is one GIC hardware in one CPU die,
which handles all vPE schedule operations one by one in all CPUs of this die.
The bug is that if the number of queued vPE schedule operations is more
than a certain value, the last vPE schedule operation will be lost.

One possible way to solve this problem is to limit the number of vLPIs, so
the hardware could spend less time to scan virtual pending table when it
handles the vPE schedule operations, so the queued vPE schedule operations
will never be more than above certain value.

Given the number of CPUs of die, and imagine there is 100 vPE schedule
operations per second one CPU, it can be calculated that we can limit
the number of vLPI to 4096 for virtual machine to avoid the issue.

Signed-off-by: Zhou Wang <wangzhou1@hisilicon.com>
---
 Documentation/arch/arm64/silicon-errata.rst |  2 ++
 arch/arm64/Kconfig                          | 12 ++++++++++++
 arch/arm64/include/asm/cputype.h            |  4 ++++
 arch/arm64/kernel/cpu_errata.c              | 15 +++++++++++++++
 arch/arm64/kvm/vgic/vgic-mmio-v3.c          |  5 +++++
 arch/arm64/tools/cpucaps                    |  1 +
 include/linux/irqchip/arm-gic-v3.h          |  1 +
 7 files changed, 40 insertions(+)

diff --git a/Documentation/arch/arm64/silicon-errata.rst b/Documentation/arch/arm64/silicon-errata.rst
index b18ef4064bc0..e8baa04e3f8c 100644
--- a/Documentation/arch/arm64/silicon-errata.rst
+++ b/Documentation/arch/arm64/silicon-errata.rst
@@ -264,6 +264,8 @@ stable kernels.
 +----------------+-----------------+-----------------+-----------------------------+
 | Hisilicon      | Hip09           | #162100801      | HISILICON_ERRATUM_162100801 |
 +----------------+-----------------+-----------------+-----------------------------+
+| Hisilicon      | Hip{10,10C}     | #162200803      | HISILICON_ERRATUM_162200803 |
++----------------+-----------------+-----------------+-----------------------------+
 +----------------+-----------------+-----------------+-----------------------------+
 | Qualcomm Tech. | Kryo/Falkor v1  | E1003           | QCOM_FALKOR_ERRATUM_1003    |
 +----------------+-----------------+-----------------+-----------------------------+
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 55fc331af337..d3df92c0fde7 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1268,6 +1268,18 @@ config HISILICON_ERRATUM_162100801
 
 	  If unsure, say Y.
 
+config HISILICON_ERRATUM_162200803
+	bool "Hip{10, 10C} 162200803 erratum support"
+	default y
+	help
+	  For GICv4.0 of Hip10 and Hip10C, it has a soc bug with vPE schedule:
+	  when multiple vPEs are sending vpe schedule/deschedule commands
+	  concurrently and repeatly, some vPE schedule command may not be
+	  scheduled, and it will cause the command timeout. To avoid the
+	  issue, limit the number of vLPI to 4096 for virtual machine.
+
+	  If unsure, say Y.
+
 config QCOM_FALKOR_ERRATUM_1003
 	bool "Falkor E1003: Incorrect translation due to ASID change"
 	default y
diff --git a/arch/arm64/include/asm/cputype.h b/arch/arm64/include/asm/cputype.h
index 661735616787..c40328907433 100644
--- a/arch/arm64/include/asm/cputype.h
+++ b/arch/arm64/include/asm/cputype.h
@@ -134,6 +134,8 @@
 
 #define HISI_CPU_PART_TSV110		0xD01
 #define HISI_CPU_PART_HIP09			0xD02
+#define HISI_CPU_PART_HIP10		0xD03
+#define HISI_CPU_PART_HIP10C		0xD45
 #define HISI_CPU_PART_HIP12		0xD06
 
 #define APPLE_CPU_PART_M1_ICESTORM	0x022
@@ -223,6 +225,8 @@
 #define MIDR_FUJITSU_A64FX MIDR_CPU_MODEL(ARM_CPU_IMP_FUJITSU, FUJITSU_CPU_PART_A64FX)
 #define MIDR_HISI_TSV110 MIDR_CPU_MODEL(ARM_CPU_IMP_HISI, HISI_CPU_PART_TSV110)
 #define MIDR_HISI_HIP09 MIDR_CPU_MODEL(ARM_CPU_IMP_HISI, HISI_CPU_PART_HIP09)
+#define MIDR_HISI_HIP10 MIDR_CPU_MODEL(ARM_CPU_IMP_HISI, HISI_CPU_PART_HIP10)
+#define MIDR_HISI_HIP10C MIDR_CPU_MODEL(ARM_CPU_IMP_HISI, HISI_CPU_PART_HIP10C)
 #define MIDR_HISI_HIP12 MIDR_CPU_MODEL(ARM_CPU_IMP_HISI, HISI_CPU_PART_HIP12)
 #define MIDR_APPLE_M1_ICESTORM MIDR_CPU_MODEL(ARM_CPU_IMP_APPLE, APPLE_CPU_PART_M1_ICESTORM)
 #define MIDR_APPLE_M1_FIRESTORM MIDR_CPU_MODEL(ARM_CPU_IMP_APPLE, APPLE_CPU_PART_M1_FIRESTORM)
diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
index 59d723c9ab8f..693cf748396d 100644
--- a/arch/arm64/kernel/cpu_errata.c
+++ b/arch/arm64/kernel/cpu_errata.c
@@ -564,6 +564,14 @@ static const struct midr_range erratum_ac04_cpu_23_list[] = {
 };
 #endif
 
+#ifdef CONFIG_HISILICON_ERRATUM_162200803
+static const struct midr_range erratum_hisi_162200803[] = {
+	MIDR_ALL_VERSIONS(MIDR_HISI_HIP10),
+	MIDR_ALL_VERSIONS(MIDR_HISI_HIP10C),
+	{},
+};
+#endif
+
 const struct arm64_cpu_capabilities arm64_errata[] = {
 #ifdef CONFIG_ARM64_WORKAROUND_CLEAN_CACHE
 	{
@@ -905,6 +913,13 @@ const struct arm64_cpu_capabilities arm64_errata[] = {
 		.matches = has_impdef_pmuv3,
 		.cpu_enable = cpu_enable_impdef_pmuv3_traps,
 	},
+#ifdef CONFIG_HISILICON_ERRATUM_162200803
+	{
+		.desc = "HiSilicon erratum 162200803",
+		.capability = ARM64_WORKAROUND_HISI_162200803,
+		ERRATA_MIDR_RANGE_LIST(erratum_hisi_162200803),
+	},
+#endif
 	{
 	}
 };
diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
index ae4c0593d114..495a56e9dc4b 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
@@ -81,6 +81,11 @@ static unsigned long vgic_mmio_read_v3_misc(struct kvm_vcpu *vcpu,
 		if (vgic_has_its(vcpu->kvm)) {
 			value |= (INTERRUPT_ID_BITS_ITS - 1) << 19;
 			value |= GICD_TYPER_LPIS;
+			/* Limit the number of vlpis to 4096 */
+			if (cpus_have_final_cap(ARM64_WORKAROUND_HISI_162200803) &&
+			    kvm_vgic_global_state.has_gicv4 &&
+			    !kvm_vgic_global_state.has_gicv4_1)
+				value |= 11 << GICD_TYPER_NUM_LPIS_SHIFT;
 		} else {
 			value |= (INTERRUPT_ID_BITS_SPIS - 1) << 19;
 		}
diff --git a/arch/arm64/tools/cpucaps b/arch/arm64/tools/cpucaps
index 10effd4cff6b..fda817f830f9 100644
--- a/arch/arm64/tools/cpucaps
+++ b/arch/arm64/tools/cpucaps
@@ -106,6 +106,7 @@ WORKAROUND_CAVIUM_TX2_219_PRFM
 WORKAROUND_CAVIUM_TX2_219_TVM
 WORKAROUND_CLEAN_CACHE
 WORKAROUND_DEVICE_LOAD_ACQUIRE
+WORKAROUND_HISI_162200803
 WORKAROUND_NVIDIA_CARMEL_CNP
 WORKAROUND_PMUV3_IMPDEF_TRAPS
 WORKAROUND_QCOM_FALKOR_E1003
diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
index 70c0948f978e..3118728671e5 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -89,6 +89,7 @@
 #define GICD_TYPER_SPIS(typer)		((((typer) & 0x1f) + 1) * 32)
 #define GICD_TYPER_ESPIS(typer)						\
 	(((typer) & GICD_TYPER_ESPI) ? GICD_TYPER_SPIS((typer) >> 27) : 0)
+#define GICD_TYPER_NUM_LPIS_SHIFT	11
 
 #define GICD_TYPER2_nASSGIcap		(1U << 8)
 #define GICD_TYPER2_VIL			(1U << 7)
-- 
2.33.0



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] ARM64: errata: Add workaround for HIP10/HIP10C erratum 162200803
  2025-06-26 12:41 [PATCH] ARM64: errata: Add workaround for HIP10/HIP10C erratum 162200803 Zhou Wang
@ 2025-06-26 13:27 ` Marc Zyngier
  2025-06-27  6:36   ` Zhou Wang
  0 siblings, 1 reply; 9+ messages in thread
From: Marc Zyngier @ 2025-06-26 13:27 UTC (permalink / raw)
  To: Zhou Wang
  Cc: Oliver Upton, Will Deacon, Catalin Marinas, linux-arm-kernel,
	kvmarm, tangnianyao, wangwudi

On Thu, 26 Jun 2025 13:41:42 +0100,
Zhou Wang <wangzhou1@hisilicon.com> wrote:
> 
> For GICv4.0 of Hip10 and Hip10C, it has a SoC bug with vPE schedule:
> when multiple vPEs are sending vpe schedule/deschedule commands
> concurrently and repeatedly, some vPE schedule command may not be
> scheduled, and it will cause the command timeout.
> 
> The hardware implementation is that there is one GIC hardware in one CPU die,
> which handles all vPE schedule operations one by one in all CPUs of this die.
> The bug is that if the number of queued vPE schedule operations is more
> than a certain value, the last vPE schedule operation will be lost.
> 
> One possible way to solve this problem is to limit the number of vLPIs, so
> the hardware could spend less time to scan virtual pending table when it
> handles the vPE schedule operations, so the queued vPE schedule operations
> will never be more than above certain value.
> 
> Given the number of CPUs of die, and imagine there is 100 vPE schedule
> operations per second one CPU, it can be calculated that we can limit
> the number of vLPI to 4096 for virtual machine to avoid the issue.
> 
> Signed-off-by: Zhou Wang <wangzhou1@hisilicon.com>
> ---
>  Documentation/arch/arm64/silicon-errata.rst |  2 ++
>  arch/arm64/Kconfig                          | 12 ++++++++++++
>  arch/arm64/include/asm/cputype.h            |  4 ++++
>  arch/arm64/kernel/cpu_errata.c              | 15 +++++++++++++++
>  arch/arm64/kvm/vgic/vgic-mmio-v3.c          |  5 +++++
>  arch/arm64/tools/cpucaps                    |  1 +
>  include/linux/irqchip/arm-gic-v3.h          |  1 +
>  7 files changed, 40 insertions(+)
> 

[...]

> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> index ae4c0593d114..495a56e9dc4b 100644
> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> @@ -81,6 +81,11 @@ static unsigned long vgic_mmio_read_v3_misc(struct kvm_vcpu *vcpu,
>  		if (vgic_has_its(vcpu->kvm)) {
>  			value |= (INTERRUPT_ID_BITS_ITS - 1) << 19;
>  			value |= GICD_TYPER_LPIS;
> +			/* Limit the number of vlpis to 4096 */
> +			if (cpus_have_final_cap(ARM64_WORKAROUND_HISI_162200803) &&
> +			    kvm_vgic_global_state.has_gicv4 &&
> +			    !kvm_vgic_global_state.has_gicv4_1)
> +				value |= 11 << GICD_TYPER_NUM_LPIS_SHIFT;

This really doesn't solve your problem. Yes, the guest *may* honor
this limit. But KVM doesn't care and will happily allocate 2^16 vLPIs
if the guest asks -- there is no code enforcing this limit.

And even if we did. What would we do on a MAPTI command that tries to
map a vLPI outside of the allowed range? Do we need to tell the guest
it has screwed up?

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] ARM64: errata: Add workaround for HIP10/HIP10C erratum 162200803
  2025-06-26 13:27 ` Marc Zyngier
@ 2025-06-27  6:36   ` Zhou Wang
  2025-07-01  8:14     ` Marc Zyngier
  0 siblings, 1 reply; 9+ messages in thread
From: Zhou Wang @ 2025-06-27  6:36 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Oliver Upton, Will Deacon, Catalin Marinas, linux-arm-kernel,
	kvmarm, tangnianyao, wangwudi

On 2025/6/26 21:27, Marc Zyngier wrote:
> On Thu, 26 Jun 2025 13:41:42 +0100,
> Zhou Wang <wangzhou1@hisilicon.com> wrote:
>>
>> For GICv4.0 of Hip10 and Hip10C, it has a SoC bug with vPE schedule:
>> when multiple vPEs are sending vpe schedule/deschedule commands
>> concurrently and repeatedly, some vPE schedule command may not be
>> scheduled, and it will cause the command timeout.
>>
>> The hardware implementation is that there is one GIC hardware in one CPU die,
>> which handles all vPE schedule operations one by one in all CPUs of this die.
>> The bug is that if the number of queued vPE schedule operations is more
>> than a certain value, the last vPE schedule operation will be lost.
>>
>> One possible way to solve this problem is to limit the number of vLPIs, so
>> the hardware could spend less time to scan virtual pending table when it
>> handles the vPE schedule operations, so the queued vPE schedule operations
>> will never be more than above certain value.
>>
>> Given the number of CPUs of die, and imagine there is 100 vPE schedule
>> operations per second one CPU, it can be calculated that we can limit
>> the number of vLPI to 4096 for virtual machine to avoid the issue.
>>
>> Signed-off-by: Zhou Wang <wangzhou1@hisilicon.com>
>> ---
>>  Documentation/arch/arm64/silicon-errata.rst |  2 ++
>>  arch/arm64/Kconfig                          | 12 ++++++++++++
>>  arch/arm64/include/asm/cputype.h            |  4 ++++
>>  arch/arm64/kernel/cpu_errata.c              | 15 +++++++++++++++
>>  arch/arm64/kvm/vgic/vgic-mmio-v3.c          |  5 +++++
>>  arch/arm64/tools/cpucaps                    |  1 +
>>  include/linux/irqchip/arm-gic-v3.h          |  1 +
>>  7 files changed, 40 insertions(+)
>>
> 
> [...]
> 
>> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>> index ae4c0593d114..495a56e9dc4b 100644
>> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>> @@ -81,6 +81,11 @@ static unsigned long vgic_mmio_read_v3_misc(struct kvm_vcpu *vcpu,
>>  		if (vgic_has_its(vcpu->kvm)) {
>>  			value |= (INTERRUPT_ID_BITS_ITS - 1) << 19;
>>  			value |= GICD_TYPER_LPIS;
>> +			/* Limit the number of vlpis to 4096 */
>> +			if (cpus_have_final_cap(ARM64_WORKAROUND_HISI_162200803) &&
>> +			    kvm_vgic_global_state.has_gicv4 &&
>> +			    !kvm_vgic_global_state.has_gicv4_1)
>> +				value |= 11 << GICD_TYPER_NUM_LPIS_SHIFT;
> 
> This really doesn't solve your problem. Yes, the guest *may* honor
> this limit. But KVM doesn't care and will happily allocate 2^16 vLPIs
> if the guest asks -- there is no code enforcing this limit.

Hi Marc,

I am not sure if there is any other place guest can ask vLPI over
the limitation except for MAPTI/MAPT below?

> And even if we did. What would we do on a MAPTI command that tries to
> map a vLPI outside of the allowed range? Do we need to tell the guest
> it has screwed up?

Thanks for pointing this. Yes, we miss the lpi_nr checking in vgic_its_cmd_handle_mapi.
In fact, the fix of this errata introduces the usage of GICD.num_LPI,
so we need make related logic right as well.

I am not sure that if we could add related checking for lpi_nr in MAPTI/MAPI
as part of this errata fix, or we should add the basic support for
GICD.num_LPI before adding this errata?

Thanks,
Zhou

> 
> Thanks,
> 
> 	M.
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] ARM64: errata: Add workaround for HIP10/HIP10C erratum 162200803
  2025-06-27  6:36   ` Zhou Wang
@ 2025-07-01  8:14     ` Marc Zyngier
  2025-07-02  9:57       ` Zhou Wang
  0 siblings, 1 reply; 9+ messages in thread
From: Marc Zyngier @ 2025-07-01  8:14 UTC (permalink / raw)
  To: Zhou Wang
  Cc: Oliver Upton, Will Deacon, Catalin Marinas, linux-arm-kernel,
	kvmarm, tangnianyao, wangwudi

On Fri, 27 Jun 2025 07:36:31 +0100,
Zhou Wang <wangzhou1@hisilicon.com> wrote:
> 
> On 2025/6/26 21:27, Marc Zyngier wrote:
> > On Thu, 26 Jun 2025 13:41:42 +0100,
> > Zhou Wang <wangzhou1@hisilicon.com> wrote:
> >>
> >> For GICv4.0 of Hip10 and Hip10C, it has a SoC bug with vPE schedule:
> >> when multiple vPEs are sending vpe schedule/deschedule commands
> >> concurrently and repeatedly, some vPE schedule command may not be
> >> scheduled, and it will cause the command timeout.
> >>
> >> The hardware implementation is that there is one GIC hardware in one CPU die,
> >> which handles all vPE schedule operations one by one in all CPUs of this die.
> >> The bug is that if the number of queued vPE schedule operations is more
> >> than a certain value, the last vPE schedule operation will be lost.
> >>
> >> One possible way to solve this problem is to limit the number of vLPIs, so
> >> the hardware could spend less time to scan virtual pending table when it
> >> handles the vPE schedule operations, so the queued vPE schedule operations
> >> will never be more than above certain value.
> >>
> >> Given the number of CPUs of die, and imagine there is 100 vPE schedule
> >> operations per second one CPU, it can be calculated that we can limit
> >> the number of vLPI to 4096 for virtual machine to avoid the issue.
> >>
> >> Signed-off-by: Zhou Wang <wangzhou1@hisilicon.com>
> >> ---
> >>  Documentation/arch/arm64/silicon-errata.rst |  2 ++
> >>  arch/arm64/Kconfig                          | 12 ++++++++++++
> >>  arch/arm64/include/asm/cputype.h            |  4 ++++
> >>  arch/arm64/kernel/cpu_errata.c              | 15 +++++++++++++++
> >>  arch/arm64/kvm/vgic/vgic-mmio-v3.c          |  5 +++++
> >>  arch/arm64/tools/cpucaps                    |  1 +
> >>  include/linux/irqchip/arm-gic-v3.h          |  1 +
> >>  7 files changed, 40 insertions(+)
> >>
> > 
> > [...]
> > 
> >> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> >> index ae4c0593d114..495a56e9dc4b 100644
> >> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> >> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> >> @@ -81,6 +81,11 @@ static unsigned long vgic_mmio_read_v3_misc(struct kvm_vcpu *vcpu,
> >>  		if (vgic_has_its(vcpu->kvm)) {
> >>  			value |= (INTERRUPT_ID_BITS_ITS - 1) << 19;
> >>  			value |= GICD_TYPER_LPIS;
> >> +			/* Limit the number of vlpis to 4096 */
> >> +			if (cpus_have_final_cap(ARM64_WORKAROUND_HISI_162200803) &&
> >> +			    kvm_vgic_global_state.has_gicv4 &&
> >> +			    !kvm_vgic_global_state.has_gicv4_1)
> >> +				value |= 11 << GICD_TYPER_NUM_LPIS_SHIFT;
> > 
> > This really doesn't solve your problem. Yes, the guest *may* honor
> > this limit. But KVM doesn't care and will happily allocate 2^16 vLPIs
> > if the guest asks -- there is no code enforcing this limit.
> 
> Hi Marc,
> 
> I am not sure if there is any other place guest can ask vLPI over
> the limitation except for MAPTI/MAPT below?
>
> > And even if we did. What would we do on a MAPTI command that tries to
> > map a vLPI outside of the allowed range? Do we need to tell the guest
> > it has screwed up?
> 
> Thanks for pointing this. Yes, we miss the lpi_nr checking in vgic_its_cmd_handle_mapi.
> In fact, the fix of this errata introduces the usage of GICD.num_LPI,
> so we need make related logic right as well.

Exactly.

> 
> I am not sure that if we could add related checking for lpi_nr in MAPTI/MAPI
> as part of this errata fix, or we should add the basic support for
> GICD.num_LPI before adding this errata?

You definitely need to handle that before allowing such limit to be
enforced. Which also means allowing the limit to be saved/restored
from userspace in order to support migration.

I was really hoping to never have to support this thing (it really is
terrible), but if we have to introduce and honor it for correctness
reasons, then it has to be fully supported.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] ARM64: errata: Add workaround for HIP10/HIP10C erratum 162200803
  2025-07-01  8:14     ` Marc Zyngier
@ 2025-07-02  9:57       ` Zhou Wang
  2025-07-03 10:44         ` Marc Zyngier
  0 siblings, 1 reply; 9+ messages in thread
From: Zhou Wang @ 2025-07-02  9:57 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Oliver Upton, Will Deacon, Catalin Marinas, linux-arm-kernel,
	kvmarm, tangnianyao, wangwudi

On 2025/7/1 16:14, Marc Zyngier wrote:
> On Fri, 27 Jun 2025 07:36:31 +0100,
> Zhou Wang <wangzhou1@hisilicon.com> wrote:
>>
>> On 2025/6/26 21:27, Marc Zyngier wrote:
>>> On Thu, 26 Jun 2025 13:41:42 +0100,
>>> Zhou Wang <wangzhou1@hisilicon.com> wrote:
>>>>
>>>> For GICv4.0 of Hip10 and Hip10C, it has a SoC bug with vPE schedule:
>>>> when multiple vPEs are sending vpe schedule/deschedule commands
>>>> concurrently and repeatedly, some vPE schedule command may not be
>>>> scheduled, and it will cause the command timeout.
>>>>
>>>> The hardware implementation is that there is one GIC hardware in one CPU die,
>>>> which handles all vPE schedule operations one by one in all CPUs of this die.
>>>> The bug is that if the number of queued vPE schedule operations is more
>>>> than a certain value, the last vPE schedule operation will be lost.
>>>>
>>>> One possible way to solve this problem is to limit the number of vLPIs, so
>>>> the hardware could spend less time to scan virtual pending table when it
>>>> handles the vPE schedule operations, so the queued vPE schedule operations
>>>> will never be more than above certain value.
>>>>
>>>> Given the number of CPUs of die, and imagine there is 100 vPE schedule
>>>> operations per second one CPU, it can be calculated that we can limit
>>>> the number of vLPI to 4096 for virtual machine to avoid the issue.
>>>>
>>>> Signed-off-by: Zhou Wang <wangzhou1@hisilicon.com>
>>>> ---
>>>>  Documentation/arch/arm64/silicon-errata.rst |  2 ++
>>>>  arch/arm64/Kconfig                          | 12 ++++++++++++
>>>>  arch/arm64/include/asm/cputype.h            |  4 ++++
>>>>  arch/arm64/kernel/cpu_errata.c              | 15 +++++++++++++++
>>>>  arch/arm64/kvm/vgic/vgic-mmio-v3.c          |  5 +++++
>>>>  arch/arm64/tools/cpucaps                    |  1 +
>>>>  include/linux/irqchip/arm-gic-v3.h          |  1 +
>>>>  7 files changed, 40 insertions(+)
>>>>
>>>
>>> [...]
>>>
>>>> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>>>> index ae4c0593d114..495a56e9dc4b 100644
>>>> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>>>> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>>>> @@ -81,6 +81,11 @@ static unsigned long vgic_mmio_read_v3_misc(struct kvm_vcpu *vcpu,
>>>>  		if (vgic_has_its(vcpu->kvm)) {
>>>>  			value |= (INTERRUPT_ID_BITS_ITS - 1) << 19;
>>>>  			value |= GICD_TYPER_LPIS;
>>>> +			/* Limit the number of vlpis to 4096 */
>>>> +			if (cpus_have_final_cap(ARM64_WORKAROUND_HISI_162200803) &&
>>>> +			    kvm_vgic_global_state.has_gicv4 &&
>>>> +			    !kvm_vgic_global_state.has_gicv4_1)
>>>> +				value |= 11 << GICD_TYPER_NUM_LPIS_SHIFT;
>>>
>>> This really doesn't solve your problem. Yes, the guest *may* honor
>>> this limit. But KVM doesn't care and will happily allocate 2^16 vLPIs
>>> if the guest asks -- there is no code enforcing this limit.
>>
>> Hi Marc,
>>
>> I am not sure if there is any other place guest can ask vLPI over
>> the limitation except for MAPTI/MAPT below?
>>
>>> And even if we did. What would we do on a MAPTI command that tries to
>>> map a vLPI outside of the allowed range? Do we need to tell the guest
>>> it has screwed up?
>>
>> Thanks for pointing this. Yes, we miss the lpi_nr checking in vgic_its_cmd_handle_mapi.
>> In fact, the fix of this errata introduces the usage of GICD.num_LPI,
>> so we need make related logic right as well.
> 
> Exactly.
> 
>>
>> I am not sure that if we could add related checking for lpi_nr in MAPTI/MAPI
>> as part of this errata fix, or we should add the basic support for
>> GICD.num_LPI before adding this errata?
> 
> You definitely need to handle that before allowing such limit to be
> enforced. Which also means allowing the limit to be saved/restored
> from userspace in order to support migration.

Seems that in KVM we do not consider GICD_TYPER in migration. How about
making GICD_TYPER.num_LPIs as a default configuration, when KVM version is
same between source and destination during migration, the logic is still right.

Something like:

diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
index eb1205654ac8..2071b1445b22 100644
--- a/arch/arm64/kvm/vgic/vgic-init.c
+++ b/arch/arm64/kvm/vgic/vgic-init.c
@@ -385,6 +385,7 @@ int vgic_init(struct kvm *kvm)
 	/* freeze the number of spis */
 	if (!dist->nr_spis)
 		dist->nr_spis = VGIC_NR_IRQS_LEGACY - VGIC_NR_PRIVATE_IRQS;
+	dist->nr_lpis = 2 ^ (INTERRUPT_NUM_LPIS + 1);

 	ret = kvm_vgic_dist_init(kvm, dist->nr_spis);
 	if (ret)
@@ -433,6 +434,7 @@ static void kvm_vgic_dist_destroy(struct kvm *kvm)
 	kfree(dist->spis);
 	dist->spis = NULL;
 	dist->nr_spis = 0;
+	dist->nr_lpis = 0;
 	dist->vgic_dist_base = VGIC_ADDR_UNDEF;

 	if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) {
diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
index 534049c7c94b..c770eadc5188 100644
--- a/arch/arm64/kvm/vgic/vgic-its.c
+++ b/arch/arm64/kvm/vgic/vgic-its.c
@@ -1047,7 +1047,8 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
 	else
 		lpi_nr = event_id;
 	if (lpi_nr < GIC_LPI_OFFSET ||
-	    lpi_nr >= max_lpis_propbaser(kvm->arch.vgic.propbaser))
+	    lpi_nr >= max_lpis_propbaser(kvm->arch.vgic.propbaser) ||
+	    lpi_nr >= GIC_LPI_OFFSET + kvm->arch.vgic.nr_lpis)
 		return E_ITS_MAPTI_PHYSICALID_OOR;

 	/* If there is an existing mapping, behavior is UNPREDICTABLE. */
diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
index ae4c0593d114..224d0d88c823 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
@@ -81,6 +81,7 @@ static unsigned long vgic_mmio_read_v3_misc(struct kvm_vcpu *vcpu,
 		if (vgic_has_its(vcpu->kvm)) {
 			value |= (INTERRUPT_ID_BITS_ITS - 1) << 19;
 			value |= GICD_TYPER_LPIS;
+			value |= (ilog2(vgic->nr_lpis) - 1) << 11;
 		} else {
 			value |= (INTERRUPT_ID_BITS_SPIS - 1) << 19;
 		}
diff --git a/arch/arm64/kvm/vgic/vgic.h b/arch/arm64/kvm/vgic/vgic.h
index 4349084cb9a6..e11792dafcdf 100644
--- a/arch/arm64/kvm/vgic/vgic.h
+++ b/arch/arm64/kvm/vgic/vgic.h
@@ -16,6 +16,7 @@

 #define INTERRUPT_ID_BITS_SPIS	10
 #define INTERRUPT_ID_BITS_ITS	16
+#define INTERRUPT_NUM_LPIS	14
 #define VGIC_LPI_MAX_INTID	((1 << INTERRUPT_ID_BITS_ITS) - 1)
 #define VGIC_PRI_BITS		5

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 4a34f7f0a864..b637dc9460d9 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -296,6 +296,7 @@ struct vgic_dist {
 	 * else.
 	 */
 	struct its_vm		its_vm;
+	int			nr_lpis;
 };

However,migration between different KVMs will be broken :(
I am not sure that should we consider this case as well?

Thanks,
Zhou

> 
> I was really hoping to never have to support this thing (it really is
> terrible), but if we have to introduce and honor it for correctness
> reasons, then it has to be fully supported>
> Thanks,
> 
> 	M.
> 


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] ARM64: errata: Add workaround for HIP10/HIP10C erratum 162200803
  2025-07-02  9:57       ` Zhou Wang
@ 2025-07-03 10:44         ` Marc Zyngier
  2025-07-08 12:05           ` Zhou Wang
  0 siblings, 1 reply; 9+ messages in thread
From: Marc Zyngier @ 2025-07-03 10:44 UTC (permalink / raw)
  To: Zhou Wang
  Cc: Oliver Upton, Will Deacon, Catalin Marinas, linux-arm-kernel,
	kvmarm, tangnianyao, wangwudi

On Wed, 02 Jul 2025 10:57:13 +0100,
Zhou Wang <wangzhou1@hisilicon.com> wrote:
> 
> On 2025/7/1 16:14, Marc Zyngier wrote:
> > On Fri, 27 Jun 2025 07:36:31 +0100,
> > Zhou Wang <wangzhou1@hisilicon.com> wrote:
> >>
> >> On 2025/6/26 21:27, Marc Zyngier wrote:
> >>> On Thu, 26 Jun 2025 13:41:42 +0100,
> >>> Zhou Wang <wangzhou1@hisilicon.com> wrote:
> >>>>
> >>>> For GICv4.0 of Hip10 and Hip10C, it has a SoC bug with vPE schedule:
> >>>> when multiple vPEs are sending vpe schedule/deschedule commands
> >>>> concurrently and repeatedly, some vPE schedule command may not be
> >>>> scheduled, and it will cause the command timeout.
> >>>>
> >>>> The hardware implementation is that there is one GIC hardware in one CPU die,
> >>>> which handles all vPE schedule operations one by one in all CPUs of this die.
> >>>> The bug is that if the number of queued vPE schedule operations is more
> >>>> than a certain value, the last vPE schedule operation will be lost.
> >>>>
> >>>> One possible way to solve this problem is to limit the number of vLPIs, so
> >>>> the hardware could spend less time to scan virtual pending table when it
> >>>> handles the vPE schedule operations, so the queued vPE schedule operations
> >>>> will never be more than above certain value.
> >>>>
> >>>> Given the number of CPUs of die, and imagine there is 100 vPE schedule
> >>>> operations per second one CPU, it can be calculated that we can limit
> >>>> the number of vLPI to 4096 for virtual machine to avoid the issue.
> >>>>
> >>>> Signed-off-by: Zhou Wang <wangzhou1@hisilicon.com>
> >>>> ---
> >>>>  Documentation/arch/arm64/silicon-errata.rst |  2 ++
> >>>>  arch/arm64/Kconfig                          | 12 ++++++++++++
> >>>>  arch/arm64/include/asm/cputype.h            |  4 ++++
> >>>>  arch/arm64/kernel/cpu_errata.c              | 15 +++++++++++++++
> >>>>  arch/arm64/kvm/vgic/vgic-mmio-v3.c          |  5 +++++
> >>>>  arch/arm64/tools/cpucaps                    |  1 +
> >>>>  include/linux/irqchip/arm-gic-v3.h          |  1 +
> >>>>  7 files changed, 40 insertions(+)
> >>>>
> >>>
> >>> [...]
> >>>
> >>>> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> >>>> index ae4c0593d114..495a56e9dc4b 100644
> >>>> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> >>>> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> >>>> @@ -81,6 +81,11 @@ static unsigned long vgic_mmio_read_v3_misc(struct kvm_vcpu *vcpu,
> >>>>  		if (vgic_has_its(vcpu->kvm)) {
> >>>>  			value |= (INTERRUPT_ID_BITS_ITS - 1) << 19;
> >>>>  			value |= GICD_TYPER_LPIS;
> >>>> +			/* Limit the number of vlpis to 4096 */
> >>>> +			if (cpus_have_final_cap(ARM64_WORKAROUND_HISI_162200803) &&
> >>>> +			    kvm_vgic_global_state.has_gicv4 &&
> >>>> +			    !kvm_vgic_global_state.has_gicv4_1)
> >>>> +				value |= 11 << GICD_TYPER_NUM_LPIS_SHIFT;
> >>>
> >>> This really doesn't solve your problem. Yes, the guest *may* honor
> >>> this limit. But KVM doesn't care and will happily allocate 2^16 vLPIs
> >>> if the guest asks -- there is no code enforcing this limit.
> >>
> >> Hi Marc,
> >>
> >> I am not sure if there is any other place guest can ask vLPI over
> >> the limitation except for MAPTI/MAPT below?
> >>
> >>> And even if we did. What would we do on a MAPTI command that tries to
> >>> map a vLPI outside of the allowed range? Do we need to tell the guest
> >>> it has screwed up?
> >>
> >> Thanks for pointing this. Yes, we miss the lpi_nr checking in vgic_its_cmd_handle_mapi.
> >> In fact, the fix of this errata introduces the usage of GICD.num_LPI,
> >> so we need make related logic right as well.
> > 
> > Exactly.
> > 
> >>
> >> I am not sure that if we could add related checking for lpi_nr in MAPTI/MAPI
> >> as part of this errata fix, or we should add the basic support for
> >> GICD.num_LPI before adding this errata?
> > 
> > You definitely need to handle that before allowing such limit to be
> > enforced. Which also means allowing the limit to be saved/restored
> > from userspace in order to support migration.
> 
> Seems that in KVM we do not consider GICD_TYPER in migration.

What do you mean by that?

Today, we don't support anything being written to GICD_TYPER, just
like on a HW implementation. If you want it to be writable from
userspace (and I think you do), then it needs to be added.

> How about making GICD_TYPER.num_LPIs as a default configuration,
> when KVM version is same between source and destination during
> migration, the logic is still right.

The default configuration should be that GICD_TYPERR.num_LPIs is 0,
indicating that the hypervisor doesn't limit anything at all.

>
> Something like:
> 
> diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
> index eb1205654ac8..2071b1445b22 100644
> --- a/arch/arm64/kvm/vgic/vgic-init.c
> +++ b/arch/arm64/kvm/vgic/vgic-init.c
> @@ -385,6 +385,7 @@ int vgic_init(struct kvm *kvm)
>  	/* freeze the number of spis */
>  	if (!dist->nr_spis)
>  		dist->nr_spis = VGIC_NR_IRQS_LEGACY - VGIC_NR_PRIVATE_IRQS;
> +	dist->nr_lpis = 2 ^ (INTERRUPT_NUM_LPIS + 1);

No, this really should default to 0, and 0 being treated as "no limit
other than the architectural one", as per the architecture spec.

> 
>  	ret = kvm_vgic_dist_init(kvm, dist->nr_spis);
>  	if (ret)
> @@ -433,6 +434,7 @@ static void kvm_vgic_dist_destroy(struct kvm *kvm)
>  	kfree(dist->spis);
>  	dist->spis = NULL;
>  	dist->nr_spis = 0;
> +	dist->nr_lpis = 0;
>  	dist->vgic_dist_base = VGIC_ADDR_UNDEF;
> 
>  	if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) {
> diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> index 534049c7c94b..c770eadc5188 100644
> --- a/arch/arm64/kvm/vgic/vgic-its.c
> +++ b/arch/arm64/kvm/vgic/vgic-its.c
> @@ -1047,7 +1047,8 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
>  	else
>  		lpi_nr = event_id;
>  	if (lpi_nr < GIC_LPI_OFFSET ||
> -	    lpi_nr >= max_lpis_propbaser(kvm->arch.vgic.propbaser))
> +	    lpi_nr >= max_lpis_propbaser(kvm->arch.vgic.propbaser) ||
> +	    lpi_nr >= GIC_LPI_OFFSET + kvm->arch.vgic.nr_lpis)
>  		return E_ITS_MAPTI_PHYSICALID_OOR;
> 
>  	/* If there is an existing mapping, behavior is UNPREDICTABLE. */
> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> index ae4c0593d114..224d0d88c823 100644
> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> @@ -81,6 +81,7 @@ static unsigned long vgic_mmio_read_v3_misc(struct kvm_vcpu *vcpu,
>  		if (vgic_has_its(vcpu->kvm)) {
>  			value |= (INTERRUPT_ID_BITS_ITS - 1) << 19;
>  			value |= GICD_TYPER_LPIS;
> +			value |= (ilog2(vgic->nr_lpis) - 1) << 11;
>  		} else {
>  			value |= (INTERRUPT_ID_BITS_SPIS - 1) << 19;
>  		}
> diff --git a/arch/arm64/kvm/vgic/vgic.h b/arch/arm64/kvm/vgic/vgic.h
> index 4349084cb9a6..e11792dafcdf 100644
> --- a/arch/arm64/kvm/vgic/vgic.h
> +++ b/arch/arm64/kvm/vgic/vgic.h
> @@ -16,6 +16,7 @@
> 
>  #define INTERRUPT_ID_BITS_SPIS	10
>  #define INTERRUPT_ID_BITS_ITS	16
> +#define INTERRUPT_NUM_LPIS	14
>  #define VGIC_LPI_MAX_INTID	((1 << INTERRUPT_ID_BITS_ITS) - 1)
>  #define VGIC_PRI_BITS		5
> 
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 4a34f7f0a864..b637dc9460d9 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -296,6 +296,7 @@ struct vgic_dist {
>  	 * else.
>  	 */
>  	struct its_vm		its_vm;
> +	int			nr_lpis;
>  };
> 
> However,migration between different KVMs will be broken :(
> I am not sure that should we consider this case as well?

This isn't optional. You cannot break migration on existing systems,
and the only case that *must* break is to restore a VM that hasn't
seen this limitation on a HW that enforces it.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] ARM64: errata: Add workaround for HIP10/HIP10C erratum 162200803
  2025-07-03 10:44         ` Marc Zyngier
@ 2025-07-08 12:05           ` Zhou Wang
  2025-07-08 12:47             ` Marc Zyngier
  0 siblings, 1 reply; 9+ messages in thread
From: Zhou Wang @ 2025-07-08 12:05 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Oliver Upton, Will Deacon, Catalin Marinas, linux-arm-kernel,
	kvmarm, tangnianyao, wangwudi

On 2025/7/3 18:44, Marc Zyngier wrote:
> On Wed, 02 Jul 2025 10:57:13 +0100,
> Zhou Wang <wangzhou1@hisilicon.com> wrote:
>>
>> On 2025/7/1 16:14, Marc Zyngier wrote:
>>> On Fri, 27 Jun 2025 07:36:31 +0100,
>>> Zhou Wang <wangzhou1@hisilicon.com> wrote:
>>>>
>>>> On 2025/6/26 21:27, Marc Zyngier wrote:
>>>>> On Thu, 26 Jun 2025 13:41:42 +0100,
>>>>> Zhou Wang <wangzhou1@hisilicon.com> wrote:
>>>>>>
>>>>>> For GICv4.0 of Hip10 and Hip10C, it has a SoC bug with vPE schedule:
>>>>>> when multiple vPEs are sending vpe schedule/deschedule commands
>>>>>> concurrently and repeatedly, some vPE schedule command may not be
>>>>>> scheduled, and it will cause the command timeout.
>>>>>>
>>>>>> The hardware implementation is that there is one GIC hardware in one CPU die,
>>>>>> which handles all vPE schedule operations one by one in all CPUs of this die.
>>>>>> The bug is that if the number of queued vPE schedule operations is more
>>>>>> than a certain value, the last vPE schedule operation will be lost.
>>>>>>
>>>>>> One possible way to solve this problem is to limit the number of vLPIs, so
>>>>>> the hardware could spend less time to scan virtual pending table when it
>>>>>> handles the vPE schedule operations, so the queued vPE schedule operations
>>>>>> will never be more than above certain value.
>>>>>>
>>>>>> Given the number of CPUs of die, and imagine there is 100 vPE schedule
>>>>>> operations per second one CPU, it can be calculated that we can limit
>>>>>> the number of vLPI to 4096 for virtual machine to avoid the issue.
>>>>>>
>>>>>> Signed-off-by: Zhou Wang <wangzhou1@hisilicon.com>
>>>>>> ---
>>>>>>  Documentation/arch/arm64/silicon-errata.rst |  2 ++
>>>>>>  arch/arm64/Kconfig                          | 12 ++++++++++++
>>>>>>  arch/arm64/include/asm/cputype.h            |  4 ++++
>>>>>>  arch/arm64/kernel/cpu_errata.c              | 15 +++++++++++++++
>>>>>>  arch/arm64/kvm/vgic/vgic-mmio-v3.c          |  5 +++++
>>>>>>  arch/arm64/tools/cpucaps                    |  1 +
>>>>>>  include/linux/irqchip/arm-gic-v3.h          |  1 +
>>>>>>  7 files changed, 40 insertions(+)
>>>>>>
>>>>>
>>>>> [...]
>>>>>
>>>>>> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>>>>>> index ae4c0593d114..495a56e9dc4b 100644
>>>>>> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>>>>>> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>>>>>> @@ -81,6 +81,11 @@ static unsigned long vgic_mmio_read_v3_misc(struct kvm_vcpu *vcpu,
>>>>>>  		if (vgic_has_its(vcpu->kvm)) {
>>>>>>  			value |= (INTERRUPT_ID_BITS_ITS - 1) << 19;
>>>>>>  			value |= GICD_TYPER_LPIS;
>>>>>> +			/* Limit the number of vlpis to 4096 */
>>>>>> +			if (cpus_have_final_cap(ARM64_WORKAROUND_HISI_162200803) &&
>>>>>> +			    kvm_vgic_global_state.has_gicv4 &&
>>>>>> +			    !kvm_vgic_global_state.has_gicv4_1)
>>>>>> +				value |= 11 << GICD_TYPER_NUM_LPIS_SHIFT;
>>>>>
>>>>> This really doesn't solve your problem. Yes, the guest *may* honor
>>>>> this limit. But KVM doesn't care and will happily allocate 2^16 vLPIs
>>>>> if the guest asks -- there is no code enforcing this limit.
>>>>
>>>> Hi Marc,
>>>>
>>>> I am not sure if there is any other place guest can ask vLPI over
>>>> the limitation except for MAPTI/MAPT below?
>>>>
>>>>> And even if we did. What would we do on a MAPTI command that tries to
>>>>> map a vLPI outside of the allowed range? Do we need to tell the guest
>>>>> it has screwed up?
>>>>
>>>> Thanks for pointing this. Yes, we miss the lpi_nr checking in vgic_its_cmd_handle_mapi.
>>>> In fact, the fix of this errata introduces the usage of GICD.num_LPI,
>>>> so we need make related logic right as well.
>>>
>>> Exactly.
>>>
>>>>
>>>> I am not sure that if we could add related checking for lpi_nr in MAPTI/MAPI
>>>> as part of this errata fix, or we should add the basic support for
>>>> GICD.num_LPI before adding this errata?
>>>
>>> You definitely need to handle that before allowing such limit to be
>>> enforced. Which also means allowing the limit to be saved/restored
>>> from userspace in order to support migration.
>>
>> Seems that in KVM we do not consider GICD_TYPER in migration.
> 
> What do you mean by that?
> 
> Today, we don't support anything being written to GICD_TYPER, just
> like on a HW implementation. If you want it to be writable from
> userspace (and I think you do), then it needs to be added.

Many thanks, I got your idea.

> 
>> How about making GICD_TYPER.num_LPIs as a default configuration,
>> when KVM version is same between source and destination during
>> migration, the logic is still right.
> 
> The default configuration should be that GICD_TYPERR.num_LPIs is 0,
> indicating that the hypervisor doesn't limit anything at all.
> 
>>
>> Something like:
>>
>> diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
>> index eb1205654ac8..2071b1445b22 100644
>> --- a/arch/arm64/kvm/vgic/vgic-init.c
>> +++ b/arch/arm64/kvm/vgic/vgic-init.c
>> @@ -385,6 +385,7 @@ int vgic_init(struct kvm *kvm)
>>  	/* freeze the number of spis */
>>  	if (!dist->nr_spis)
>>  		dist->nr_spis = VGIC_NR_IRQS_LEGACY - VGIC_NR_PRIVATE_IRQS;
>> +	dist->nr_lpis = 2 ^ (INTERRUPT_NUM_LPIS + 1);
> 
> No, this really should default to 0, and 0 being treated as "no limit
> other than the architectural one", as per the architecture spec.
> 
>>
>>  	ret = kvm_vgic_dist_init(kvm, dist->nr_spis);
>>  	if (ret)
>> @@ -433,6 +434,7 @@ static void kvm_vgic_dist_destroy(struct kvm *kvm)
>>  	kfree(dist->spis);
>>  	dist->spis = NULL;
>>  	dist->nr_spis = 0;
>> +	dist->nr_lpis = 0;
>>  	dist->vgic_dist_base = VGIC_ADDR_UNDEF;
>>
>>  	if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) {
>> diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
>> index 534049c7c94b..c770eadc5188 100644
>> --- a/arch/arm64/kvm/vgic/vgic-its.c
>> +++ b/arch/arm64/kvm/vgic/vgic-its.c
>> @@ -1047,7 +1047,8 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
>>  	else
>>  		lpi_nr = event_id;
>>  	if (lpi_nr < GIC_LPI_OFFSET ||
>> -	    lpi_nr >= max_lpis_propbaser(kvm->arch.vgic.propbaser))
>> +	    lpi_nr >= max_lpis_propbaser(kvm->arch.vgic.propbaser) ||
>> +	    lpi_nr >= GIC_LPI_OFFSET + kvm->arch.vgic.nr_lpis)
>>  		return E_ITS_MAPTI_PHYSICALID_OOR;
>>
>>  	/* If there is an existing mapping, behavior is UNPREDICTABLE. */
>> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>> index ae4c0593d114..224d0d88c823 100644
>> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>> @@ -81,6 +81,7 @@ static unsigned long vgic_mmio_read_v3_misc(struct kvm_vcpu *vcpu,
>>  		if (vgic_has_its(vcpu->kvm)) {
>>  			value |= (INTERRUPT_ID_BITS_ITS - 1) << 19;
>>  			value |= GICD_TYPER_LPIS;
>> +			value |= (ilog2(vgic->nr_lpis) - 1) << 11;
>>  		} else {
>>  			value |= (INTERRUPT_ID_BITS_SPIS - 1) << 19;
>>  		}
>> diff --git a/arch/arm64/kvm/vgic/vgic.h b/arch/arm64/kvm/vgic/vgic.h
>> index 4349084cb9a6..e11792dafcdf 100644
>> --- a/arch/arm64/kvm/vgic/vgic.h
>> +++ b/arch/arm64/kvm/vgic/vgic.h
>> @@ -16,6 +16,7 @@
>>
>>  #define INTERRUPT_ID_BITS_SPIS	10
>>  #define INTERRUPT_ID_BITS_ITS	16
>> +#define INTERRUPT_NUM_LPIS	14
>>  #define VGIC_LPI_MAX_INTID	((1 << INTERRUPT_ID_BITS_ITS) - 1)
>>  #define VGIC_PRI_BITS		5
>>
>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>> index 4a34f7f0a864..b637dc9460d9 100644
>> --- a/include/kvm/arm_vgic.h
>> +++ b/include/kvm/arm_vgic.h
>> @@ -296,6 +296,7 @@ struct vgic_dist {
>>  	 * else.
>>  	 */
>>  	struct its_vm		its_vm;
>> +	int			nr_lpis;
>>  };
>>
>> However,migration between different KVMs will be broken :(
>> I am not sure that should we consider this case as well?
> 
> This isn't optional. You cannot break migration on existing systems,
> and the only case that *must* break is to restore a VM that hasn't
> seen this limitation on a HW that enforces it.

So the whole story is that we should let GICD_TYPE.num_LPIs as a
writable field, and set this field from VMM(e.g. QEMU). And it will
be failed if doing migration between VMs with different num_LPI
configurations. After above done, this errata could be added.

Not sure my thought above is right? I will try to prepare a num_LPIs
writable patch if the direction is right.

Thanks,
Zhou

> 
> Thanks,
> 
> 	M.
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] ARM64: errata: Add workaround for HIP10/HIP10C erratum 162200803
  2025-07-08 12:05           ` Zhou Wang
@ 2025-07-08 12:47             ` Marc Zyngier
  2025-07-09  2:08               ` Zhou Wang
  0 siblings, 1 reply; 9+ messages in thread
From: Marc Zyngier @ 2025-07-08 12:47 UTC (permalink / raw)
  To: Zhou Wang
  Cc: Oliver Upton, Will Deacon, Catalin Marinas, linux-arm-kernel,
	kvmarm, tangnianyao, wangwudi

On Tue, 08 Jul 2025 13:05:24 +0100,
Zhou Wang <wangzhou1@hisilicon.com> wrote:
> 
> On 2025/7/3 18:44, Marc Zyngier wrote:
> > On Wed, 02 Jul 2025 10:57:13 +0100,
> > Zhou Wang <wangzhou1@hisilicon.com> wrote:
> >>
> >> However,migration between different KVMs will be broken :(
> >> I am not sure that should we consider this case as well?
> > 
> > This isn't optional. You cannot break migration on existing systems,
> > and the only case that *must* break is to restore a VM that hasn't
> > seen this limitation on a HW that enforces it.
> 
> So the whole story is that we should let GICD_TYPE.num_LPIs as a
> writable field, and set this field from VMM(e.g. QEMU). And it will
> be failed if doing migration between VMs with different num_LPI
> configurations.

Note that on a non-broken system, setting num_LPIs to any value should
always work.

> After above done, this errata could be added.

Which would be the only case where num_LPIs wouldn't be writable, and
set to a fixed value.

Note that an alternative would be to not have a kernel erratum, but to
force num_LPIs from userspace when running on your platform. But that
implies that: you have your own, non-upstream VMM, and that there is
no opportunity for userspace to adversely impact the system as a whole.

> Not sure my thought above is right? I will try to prepare a num_LPIs
> writable patch if the direction is right.

I think you got the gist of it.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] ARM64: errata: Add workaround for HIP10/HIP10C erratum 162200803
  2025-07-08 12:47             ` Marc Zyngier
@ 2025-07-09  2:08               ` Zhou Wang
  0 siblings, 0 replies; 9+ messages in thread
From: Zhou Wang @ 2025-07-09  2:08 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Oliver Upton, Will Deacon, Catalin Marinas, linux-arm-kernel,
	kvmarm, tangnianyao, wangwudi

On 2025/7/8 20:47, Marc Zyngier wrote:
> On Tue, 08 Jul 2025 13:05:24 +0100,
> Zhou Wang <wangzhou1@hisilicon.com> wrote:
>>
>> On 2025/7/3 18:44, Marc Zyngier wrote:
>>> On Wed, 02 Jul 2025 10:57:13 +0100,
>>> Zhou Wang <wangzhou1@hisilicon.com> wrote:
>>>>
>>>> However,migration between different KVMs will be broken :(
>>>> I am not sure that should we consider this case as well?
>>>
>>> This isn't optional. You cannot break migration on existing systems,
>>> and the only case that *must* break is to restore a VM that hasn't
>>> seen this limitation on a HW that enforces it.
>>
>> So the whole story is that we should let GICD_TYPE.num_LPIs as a
>> writable field, and set this field from VMM(e.g. QEMU). And it will
>> be failed if doing migration between VMs with different num_LPI
>> configurations.
> 
> Note that on a non-broken system, setting num_LPIs to any value should
> always work.
> 
>> After above done, this errata could be added.
> 
> Which would be the only case where num_LPIs wouldn't be writable, and
> set to a fixed value.
> 
> Note that an alternative would be to not have a kernel erratum, but to
> force num_LPIs from userspace when running on your platform. But that
> implies that: you have your own, non-upstream VMM, and that there is
> no opportunity for userspace to adversely impact the system as a whole.

Got it. Seems it is better to have this erratum in kernel.

> 
>> Not sure my thought above is right? I will try to prepare a num_LPIs
>> writable patch if the direction is right.
> 
> I think you got the gist of it.

Thanks a lot for the whole discussion, I will prepare the new patch.

Best,
Zhou

> 
> Thanks,
> 
> 	M.
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2025-07-09  2:11 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-26 12:41 [PATCH] ARM64: errata: Add workaround for HIP10/HIP10C erratum 162200803 Zhou Wang
2025-06-26 13:27 ` Marc Zyngier
2025-06-27  6:36   ` Zhou Wang
2025-07-01  8:14     ` Marc Zyngier
2025-07-02  9:57       ` Zhou Wang
2025-07-03 10:44         ` Marc Zyngier
2025-07-08 12:05           ` Zhou Wang
2025-07-08 12:47             ` Marc Zyngier
2025-07-09  2:08               ` Zhou Wang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).