diff mbox series

[v1,2/4] xen: arm: make VMAP only support in MMU system

Message ID 20240802121443.1531693-3-ayan.kumar.halder@amd.com (mailing list archive)
State Superseded
Headers show
Series xen: arm: Split MMU code in preparation for MPU work (part 2) | expand

Commit Message

Ayan Kumar Halder Aug. 2, 2024, 12:14 p.m. UTC 
From: Penny Zheng <penny.zheng@arm.com>

VMAP is widely used in ALTERNATIVE feature, CPUERRATA feature, etc to
remap a range of memory with new memory attributes. Since this is
highly dependent on virtual address translation, we choose to fold VMAP
in MMU system.

In this patch, we introduce a new Kconfig CONFIG_HAS_VMAP, and make it
only support in MMU system on ARM architecture. And we make features
like ALTERNATIVE, CPUERRATA, etc, now depend on VMAP.

Split "https://patchwork.kernel.org/project/xen-devel/patch/20230626033443.2943270-16-Penny.Zheng@arm.com/"
into Arm specific

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Signed-off-by: Wei Chen <wei.chen@arm.com>
Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
 xen/arch/arm/Kconfig   | 6 +++++-
 xen/arch/arm/Makefile  | 2 +-
 xen/arch/arm/setup.c   | 4 ++++
 xen/arch/arm/smpboot.c | 2 ++
 xen/include/xen/vmap.h | 6 ++++++
 5 files changed, 18 insertions(+), 2 deletions(-)

Comments

Jan Beulich Aug. 2, 2024, 12:30 p.m. UTC | #1 
On 02.08.2024 14:14, Ayan Kumar Halder wrote:> --- a/xen/include/xen/vmap.h
> +++ b/xen/include/xen/vmap.h
> @@ -138,10 +138,16 @@ static inline void iounmap(void __iomem *va)
>  /* Pointer to 1 octet past the end of the VMAP_DEFAULT virtual area */
>  void *arch_vmap_virt_end(void);
>  
> +#ifdef CONFIG_MMU
>  /* Initialises the VMAP_DEFAULT virtual range */
>  static inline void vm_init(void)
>  {
>      vm_init_type(VMAP_DEFAULT, (void *)VMAP_VIRT_START, arch_vmap_virt_end());
>  }
> +#else
> +static inline void vm_init(void)
> +{
> +}
> +#endif

Imo in such a case the #ifdef wants to be put inside the existing function,
avoiding the need for a 2nd instance.

Jan
Julien Grall Aug. 2, 2024, 1:27 p.m. UTC | #2 
Hi,

On 02/08/2024 13:14, Ayan Kumar Halder wrote:
> From: Penny Zheng <penny.zheng@arm.com>
> 
> VMAP is widely used in ALTERNATIVE feature, CPUERRATA feature, etc to
> remap a range of memory with new memory attributes. Since this is
> highly dependent on virtual address translation, we choose to fold VMAP
> in MMU system.
> 
> In this patch, we introduce a new Kconfig CONFIG_HAS_VMAP, and make it
> only support in MMU system on ARM architecture. And we make features
> like ALTERNATIVE, CPUERRATA, etc, now depend on VMAP.

While I agree that alternative should depend on VMAP (for now), I feel 
this is incorrect for CPUERRATA. Very likely, you will need to deal with 
them on the MPU.

Before making any suggestion, would you be able to clarify how you 
envision to deal with errata? Will they be selected at built time or 
boot time?

Cheers,
Ayan Kumar Halder Aug. 5, 2024, 10:44 a.m. UTC | #3 
On 02/08/2024 14:27, Julien Grall wrote:
> Hi,
Hi Julien,
>
> On 02/08/2024 13:14, Ayan Kumar Halder wrote:
>> From: Penny Zheng <penny.zheng@arm.com>
>>
>> VMAP is widely used in ALTERNATIVE feature, CPUERRATA feature, etc to
>> remap a range of memory with new memory attributes. Since this is
>> highly dependent on virtual address translation, we choose to fold VMAP
>> in MMU system.
>>
>> In this patch, we introduce a new Kconfig CONFIG_HAS_VMAP, and make it
>> only support in MMU system on ARM architecture. And we make features
>> like ALTERNATIVE, CPUERRATA, etc, now depend on VMAP.
>
> While I agree that alternative should depend on VMAP (for now), I feel 
> this is incorrect for CPUERRATA. Very likely, you will need to deal 
> with them on the MPU.
>
> Before making any suggestion, would you be able to clarify how you 
> envision to deal with errata? Will they be selected at built time or 
> boot time?

TBH, I hadn't thought that through. I am thinking about selecting them 
at built time (like it is been done for Arm64 cpus).

However given that there are lesser number of MPU cpus (only R52 and 
R82) compared to  MMU ones, could there be a simpler approach.

I am open to any suggestions you have.

Also, can we disable the CPUERRATA on MPU until we add support for the 
first errata ?

- Ayan

>
> Cheers,
>
Julien Grall Aug. 5, 2024, 9:36 p.m. UTC | #4 
Hi Ayan,

On 05/08/2024 11:44, Ayan Kumar Halder wrote:
> 
> On 02/08/2024 14:27, Julien Grall wrote:
>> Hi,
> Hi Julien,
>>
>> On 02/08/2024 13:14, Ayan Kumar Halder wrote:
>>> From: Penny Zheng <penny.zheng@arm.com>
>>>
>>> VMAP is widely used in ALTERNATIVE feature, CPUERRATA feature, etc to
>>> remap a range of memory with new memory attributes. Since this is
>>> highly dependent on virtual address translation, we choose to fold VMAP
>>> in MMU system.
>>>
>>> In this patch, we introduce a new Kconfig CONFIG_HAS_VMAP, and make it
>>> only support in MMU system on ARM architecture. And we make features
>>> like ALTERNATIVE, CPUERRATA, etc, now depend on VMAP.
>>
>> While I agree that alternative should depend on VMAP (for now), I feel 
>> this is incorrect for CPUERRATA. Very likely, you will need to deal 
>> with them on the MPU.
>>
>> Before making any suggestion, would you be able to clarify how you 
>> envision to deal with errata? Will they be selected at built time or 
>> boot time?
> 
> TBH, I hadn't thought that through. I am thinking about selecting them 
> at built time (like it is been done for Arm64 cpus).

To clarify, on arm64, the decision to enable the bulk of mitigation is 
done at runtime. The Kconfig is just to remove it from the binary if you 
you are not targeting such HW. With that in mind...

> 
> However given that there are lesser number of MPU cpus (only R52 and 
> R82) compared to  MMU ones, could there be a simpler approach.

... do you mean always an errata enabled in the Kconfig would be always 
mitigate (IOW no runtime selection)?

> 
> I am open to any suggestions you have.
> 
> Also, can we disable the CPUERRATA on MPU until we add support for the 
> first errata ?

So I have looked at the code and it is rather unclear why you actually 
need to disable CPU Errata. Can you clarify what would happen if 
cpuerrata is always built *and* CONFIG_ARM64_HARDEN_BRANCH_PREDICATOR is 
gated by HAS_VMAP? I am assuming we will not need the branch predictor 
hardening on Cortex-R (at least this is the implication from [1]).

Now regarding alternative, at least for arm64 this is used outside of 
the errata. Not critical right now, but at some point you will want to 
re-enable alternatives.

Cheers,

[1] 
https://developer.arm.com/Arm%20Security%20Center/Speculative%20Processor%20Vulnerability
diff mbox series

Patch

diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 21d03d9f44..c8d417298c 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -12,7 +12,7 @@  config ARM_64
 config ARM
 	def_bool y
 	select FUNCTION_ALIGNMENT_4B
-	select HAS_ALTERNATIVE
+	select HAS_ALTERNATIVE if HAS_VMAP
 	select HAS_DEVICE_TREE
 	select HAS_PASSTHROUGH
 	select HAS_UBSAN
@@ -58,9 +58,13 @@  config PADDR_BITS
 	default 40 if ARM_PA_BITS_40
 	default 48 if ARM_64
 
+config HAS_VMAP
+	def_bool y
+
 config MMU
 	def_bool y
 	select HAS_PMAP
+	select HAS_VMAP
 
 source "arch/Kconfig"
 
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 45dc29ea53..6882814d38 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -11,7 +11,7 @@  obj-$(CONFIG_HAS_VPCI) += vpci.o
 
 obj-$(CONFIG_HAS_ALTERNATIVE) += alternative.o
 obj-y += bootfdt.init.o
-obj-y += cpuerrata.o
+obj-$(CONFIG_HAS_VMAP) += cpuerrata.o
 obj-y += cpufeature.o
 obj-y += decode.o
 obj-y += device.o
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 0c2fdaceaf..9d34ac7f64 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -804,11 +804,13 @@  void asmlinkage __init start_xen(unsigned long boot_phys_offset,
     nr_cpu_ids = smp_get_max_cpus();
     printk(XENLOG_INFO "SMP: Allowing %u CPUs\n", nr_cpu_ids);
 
+#ifdef CONFIG_HAS_VMAP
     /*
      * Some errata relies on SMCCC version which is detected by psci_init()
      * (called from smp_init_cpus()).
      */
     check_local_cpu_errata();
+#endif
 
     check_local_cpu_features();
 
@@ -879,8 +881,10 @@  void asmlinkage __init start_xen(unsigned long boot_phys_offset,
      * It needs to be called after do_initcalls to be able to use
      * stop_machine (tasklets initialized via an initcall).
      */
+#ifdef CONFIG_HAS_VMAP
     apply_alternatives_all();
     enable_errata_workarounds();
+#endif
     enable_cpu_features();
 
     /* Create initial domain 0. */
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index 04e363088d..999afc028e 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -406,7 +406,9 @@  void asmlinkage start_secondary(void)
 
     local_abort_enable();
 
+#ifdef CONFIG_HAS_VMAP
     check_local_cpu_errata();
+#endif
     check_local_cpu_features();
 
     printk(XENLOG_DEBUG "CPU %u booted.\n", smp_processor_id());
diff --git a/xen/include/xen/vmap.h b/xen/include/xen/vmap.h
index fdae37e950..84797acfc0 100644
--- a/xen/include/xen/vmap.h
+++ b/xen/include/xen/vmap.h
@@ -138,10 +138,16 @@  static inline void iounmap(void __iomem *va)
 /* Pointer to 1 octet past the end of the VMAP_DEFAULT virtual area */
 void *arch_vmap_virt_end(void);
 
+#ifdef CONFIG_MMU
 /* Initialises the VMAP_DEFAULT virtual range */
 static inline void vm_init(void)
 {
     vm_init_type(VMAP_DEFAULT, (void *)VMAP_VIRT_START, arch_vmap_virt_end());
 }
+#else
+static inline void vm_init(void)
+{
+}
+#endif
 
 #endif /* __XEN_VMAP_H__ */