[PATCH v3 0/4] xen: arm: Split MMU code in preparation for MPU work (part 2)

[PATCH v3 0/4] xen: arm: Split MMU code in preparation for MPU work (part 2)

[Ayan Kumar Halder]

Hi,

In https://patchew.org/Xen/20231116145032.1651305-1-Henry.Wang@arm.com/, Henry has
reorganized some of the code between the MMU specific and generic files.

In this patch serie, we address the remaining code reorg so that MMU specific
code is cleanly separated and we have added stubs wherever necessary to avoid
introducing if-def.

Ayan Kumar Halder (4):
  xen: arm: Add a new helper update_boot_mapping()
  xen: make VMAP only support in MMU system
  xen: arm: Move the functions of domain_page to MMU specific
  xen: arm: Enclose access to EL2 MMU specific registers under
    CONFIG_MMU

 xen/arch/arm/Kconfig                 |  4 +++-
 xen/arch/arm/Makefile                |  1 -
 xen/arch/arm/arm64/mmu/mm.c          |  7 ++++++-
 xen/arch/arm/arm64/smpboot.c         |  6 +++---
 xen/arch/arm/include/asm/arm64/mm.h  |  2 +-
 xen/arch/arm/mmu/Makefile            |  1 +
 xen/arch/arm/{ => mmu}/domain_page.c |  0
 xen/arch/arm/setup.c                 |  2 ++
 xen/arch/arm/traps.c                 | 10 ++++++++++
 xen/arch/x86/Kconfig                 |  2 ++
 xen/common/Kconfig                   |  3 +++
 xen/include/xen/vmap.h               |  2 ++
 12 files changed, 33 insertions(+), 7 deletions(-)
 rename xen/arch/arm/{ => mmu}/domain_page.c (100%)

-- 
2.25.1

[PATCH v3 1/4] xen: arm: Add a new helper update_boot_mapping()

[Ayan Kumar Halder]

update_boot_mapping() invokes update_identity_mapping() for the MMU specific
code.
Later when the MPU code is added, update_boot_mapping() would invoke the
equivalent.

The common code now invokes update_boot_mapping() instead of
update_identity_mapping(). So, that there is clear abstraction between the
common and MMU/MPU specific logic.

This is in continuation to commit
f661a20aa880: "Extract MMU-specific MM code".

update_identity_mapping() is now marked as static as it is called within
xen/arch/arm/arm64/mmu/mm.c only. Also, updated the prototype to
update_boot_mapping() which is now invoked from other files.

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
Changes from :-

v1 - 1. Introduced update_boot_mapping() which invokes
update_identity_mapping() in MMU specific code.

v2 - 1. Make update_identity_mapping() static and update the prototype.

 xen/arch/arm/arm64/mmu/mm.c         | 7 ++++++-
 xen/arch/arm/arm64/smpboot.c        | 6 +++---
 xen/arch/arm/include/asm/arm64/mm.h | 2 +-
 3 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/arm64/mmu/mm.c b/xen/arch/arm/arm64/mmu/mm.c
index 293acb67e0..1afbbeda5a 100644
--- a/xen/arch/arm/arm64/mmu/mm.c
+++ b/xen/arch/arm/arm64/mmu/mm.c
@@ -111,7 +111,7 @@ void __init arch_setup_page_tables(void)
     prepare_runtime_identity_mapping();
 }
 
-void update_identity_mapping(bool enable)
+static void update_identity_mapping(bool enable)
 {
     paddr_t id_addr = virt_to_maddr(_start);
     int rc;
@@ -125,6 +125,11 @@ void update_identity_mapping(bool enable)
     BUG_ON(rc);
 }
 
+void update_boot_mapping(bool enable)
+{
+    update_identity_mapping(enable);
+}
+
 extern void switch_ttbr_id(uint64_t ttbr);
 
 typedef void (switch_ttbr_fn)(uint64_t ttbr);
diff --git a/xen/arch/arm/arm64/smpboot.c b/xen/arch/arm/arm64/smpboot.c
index a225fae64d..789f352ab6 100644
--- a/xen/arch/arm/arm64/smpboot.c
+++ b/xen/arch/arm/arm64/smpboot.c
@@ -112,18 +112,18 @@ int arch_cpu_up(int cpu)
     if ( !smp_enable_ops[cpu].prepare_cpu )
         return -ENODEV;
 
-    update_identity_mapping(true);
+    update_boot_mapping(true);
 
     rc = smp_enable_ops[cpu].prepare_cpu(cpu);
     if ( rc )
-        update_identity_mapping(false);
+        update_boot_mapping(false);
 
     return rc;
 }
 
 void arch_cpu_up_finish(void)
 {
-    update_identity_mapping(false);
+    update_boot_mapping(false);
 }
 
 /*
diff --git a/xen/arch/arm/include/asm/arm64/mm.h b/xen/arch/arm/include/asm/arm64/mm.h
index e0bd23a6ed..ac8d1f5c78 100644
--- a/xen/arch/arm/include/asm/arm64/mm.h
+++ b/xen/arch/arm/include/asm/arm64/mm.h
@@ -21,7 +21,7 @@ void arch_setup_page_tables(void);
  * Note that nested call (e.g. enable=true, enable=true) is not
  * supported.
  */
-void update_identity_mapping(bool enable);
+void update_boot_mapping(bool enable);
 
 #endif /* __ARM_ARM64_MM_H__ */
 
-- 
2.25.1

[PATCH v3 2/4] xen: make VMAP only support in MMU system

[Ayan Kumar Halder]

From: Penny Zheng <penny.zheng@arm.com>

Introduced CONFIG_VMAP which is selected by the architectures that use
MMU. vm_init() does not do anything if CONFIG_VMAP is not enabled.

VMAP is widely used in ALTERNATIVE feature 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 ALTERNATIVE now
depends on VMAP.

HARDEN_BRANCH_PREDICTOR is now gated on HAS_VMAP as speculative
attacks are not possible on non MMU based systems (ie Cortex-R52, R82).
See https://developer.arm.com/Arm%20Security%20Center/Speculative%20Processor%20Vulnerability.

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>
---
Changes from :-

v1 - 1. HARDEN_BRANCH_PREDICTOR is now gated on HAS_VMAP.
2. cpuerrata.c is not gated on HAS_VMAP.

v2 - 1. Introduced CONFIG_VMAP in common/Kconfig.
2. Architectures using MMU select this config.
3. vm_init() now uses CONFIG_VMAP.

 xen/arch/arm/Kconfig   | 4 +++-
 xen/arch/arm/setup.c   | 2 ++
 xen/arch/x86/Kconfig   | 2 ++
 xen/common/Kconfig     | 3 +++
 xen/include/xen/vmap.h | 2 ++
 5 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 21d03d9f44..e30a7da186 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
@@ -61,6 +61,7 @@ config PADDR_BITS
 config MMU
 	def_bool y
 	select HAS_PMAP
+	select HAS_VMAP
 
 source "arch/Kconfig"
 
@@ -171,6 +172,7 @@ config ARM_SSBD
 
 config HARDEN_BRANCH_PREDICTOR
 	bool "Harden the branch predictor against aliasing attacks" if EXPERT
+	depends on HAS_VMAP
 	default y
 	help
 	  Speculation attacks against some high-performance processors rely on
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index cb2c0a16b8..7f686d2cca 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -447,7 +447,9 @@ 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_ALTERNATIVE
     apply_alternatives_all();
+#endif
     enable_errata_workarounds();
     enable_cpu_features();
 
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 7ef5c8bc48..32be057978 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -1,6 +1,7 @@
 config X86_64
 	def_bool y
 	select 64BIT
+	select HAS_VMAP
 
 config X86
 	def_bool y
@@ -31,6 +32,7 @@ config X86
 	select HAS_UBSAN
 	select HAS_VPCI if HVM
 	select NEEDS_LIBELF
+	select HAS_VMAP
 
 config ARCH_DEFCONFIG
 	string
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 565ceda741..188918ec5c 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -77,6 +77,9 @@ config HAS_PIRQ
 config HAS_PMAP
 	bool
 
+config HAS_VMAP
+	bool
+
 config HAS_SCHED_GRANULARITY
 	bool
 
diff --git a/xen/include/xen/vmap.h b/xen/include/xen/vmap.h
index fdae37e950..c1dd7ac22f 100644
--- a/xen/include/xen/vmap.h
+++ b/xen/include/xen/vmap.h
@@ -141,7 +141,9 @@ void *arch_vmap_virt_end(void);
 /* Initialises the VMAP_DEFAULT virtual range */
 static inline void vm_init(void)
 {
+#ifdef CONFIG_HAS_VMAP
     vm_init_type(VMAP_DEFAULT, (void *)VMAP_VIRT_START, arch_vmap_virt_end());
+#endif
 }
 
 #endif /* __XEN_VMAP_H__ */
-- 
2.25.1

[PATCH v3 3/4] xen: arm: Move the functions of domain_page to MMU specific

[Ayan Kumar Halder]

Moved init_domheap_mappings(), map_domain_page_global(),
unmap_domain_page_global(), map_domain_page(), unmap_domain_page(),
domain_page_map_to_mfn() to MMU specific folder.

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
Changes from :-

v1 - Moved domain_page.c to mmu/domain_page.c.

v2 - Updated arm/Makefile.

 xen/arch/arm/Makefile                | 1 -
 xen/arch/arm/mmu/Makefile            | 1 +
 xen/arch/arm/{ => mmu}/domain_page.c | 0
 3 files changed, 1 insertion(+), 1 deletion(-)
 rename xen/arch/arm/{ => mmu}/domain_page.c (100%)

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index da9c979dc4..7792bff597 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -18,7 +18,6 @@ obj-$(CONFIG_IOREQ_SERVER) += dm.o
 obj-$(CONFIG_DOM0LESS_BOOT) += dom0less-build.init.o
 obj-y += domain.o
 obj-y += domain_build.init.o
-obj-$(CONFIG_ARCH_MAP_DOMAIN_PAGE) += domain_page.o
 obj-y += domctl.o
 obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
 obj-y += efi/
diff --git a/xen/arch/arm/mmu/Makefile b/xen/arch/arm/mmu/Makefile
index 67475fcd80..2cb44b857d 100644
--- a/xen/arch/arm/mmu/Makefile
+++ b/xen/arch/arm/mmu/Makefile
@@ -2,3 +2,4 @@ obj-y += p2m.o
 obj-y += pt.o
 obj-y += setup.o
 obj-y += smpboot.o
+obj-$(CONFIG_ARCH_MAP_DOMAIN_PAGE) += domain_page.o
diff --git a/xen/arch/arm/domain_page.c b/xen/arch/arm/mmu/domain_page.c
similarity index 100%
rename from xen/arch/arm/domain_page.c
rename to xen/arch/arm/mmu/domain_page.c
-- 
2.25.1

[PATCH v3 4/4] xen: arm: Enclose access to EL2 MMU specific registers under CONFIG_MMU

[Ayan Kumar Halder]

All the EL2 MMU specific registers are enclosed within CONFIG_MMU.

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
Changes from :

v1 -
1. 'vttbr_el2' field is enclosed with ifdef.
2. No movement of code.

v2 -
1. Enclosed 'vttbr_el2' access in show_registers() and vcpu_show_registers().

 xen/arch/arm/traps.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index aac6c599f8..737f4d65e3 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -720,8 +720,10 @@ struct reg_ctxt {
     uint32_t ifsr32_el2;
 #endif
 
+#ifdef CONFIG_MMU
     /* Hypervisor-side state */
     uint64_t vttbr_el2;
+#endif
 };
 
 static const char *mode_string(register_t cpsr)
@@ -919,12 +921,16 @@ static void _show_registers(const struct cpu_user_regs *regs,
 #endif
     }
     printk("  VTCR_EL2: %"PRIregister"\n", READ_SYSREG(VTCR_EL2));
+#ifdef CONFIG_MMU
     printk(" VTTBR_EL2: %016"PRIx64"\n", ctxt->vttbr_el2);
+#endif
     printk("\n");
 
     printk(" SCTLR_EL2: %"PRIregister"\n", READ_SYSREG(SCTLR_EL2));
     printk("   HCR_EL2: %"PRIregister"\n", READ_SYSREG(HCR_EL2));
+#ifdef CONFIG_MMU
     printk(" TTBR0_EL2: %016"PRIx64"\n", READ_SYSREG64(TTBR0_EL2));
+#endif
     printk("\n");
     printk("   ESR_EL2: %"PRIregister"\n", regs->hsr);
     printk(" HPFAR_EL2: %"PRIregister"\n", READ_SYSREG(HPFAR_EL2));
@@ -956,7 +962,9 @@ void show_registers(const struct cpu_user_regs *regs)
     if ( guest_mode(regs) && is_32bit_domain(current->domain) )
         ctxt.ifsr32_el2 = READ_SYSREG(IFSR32_EL2);
 #endif
+#ifdef CONFIG_MMU
     ctxt.vttbr_el2 = READ_SYSREG64(VTTBR_EL2);
+#endif
 
     _show_registers(regs, &ctxt, guest_mode(regs), current);
 }
@@ -979,7 +987,9 @@ void vcpu_show_registers(const struct vcpu *v)
     ctxt.ifsr32_el2 = v->arch.ifsr;
 #endif
 
+#ifdef CONFIG_MMU
     ctxt.vttbr_el2 = v->domain->arch.p2m.vttbr;
+#endif
 
     _show_registers(&v->arch.cpu_info->guest_cpu_user_regs, &ctxt, 1, v);
 }
-- 
2.25.1

Re: [PATCH v3 2/4] xen: make VMAP only support in MMU system

[Jan Beulich]

On 13.08.2024 19:13, Ayan Kumar Halder wrote:
> From: Penny Zheng <penny.zheng@arm.com>
> 
> Introduced CONFIG_VMAP which is selected by the architectures that use
> MMU. vm_init() does not do anything if CONFIG_VMAP is not enabled.
> 
> VMAP is widely used in ALTERNATIVE feature 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 ALTERNATIVE now
> depends on VMAP.
> 
> HARDEN_BRANCH_PREDICTOR is now gated on HAS_VMAP as speculative
> attacks are not possible on non MMU based systems (ie Cortex-R52, R82).
> See https://developer.arm.com/Arm%20Security%20Center/Speculative%20Processor%20Vulnerability.

While I'm not an Arm expert and hence I'm likely missing aspects, I question
the one (Spectre-BHB) vulnerability there to be sufficient to draw a
conclusion towards the usefulness of branch hardening. I would advise
against encoding such a connection in the Kconfig dependencies.

> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -1,6 +1,7 @@
>  config X86_64
>  	def_bool y
>  	select 64BIT
> +	select HAS_VMAP
>  
>  config X86
>  	def_bool y
> @@ -31,6 +32,7 @@ config X86
>  	select HAS_UBSAN
>  	select HAS_VPCI if HVM
>  	select NEEDS_LIBELF
> +	select HAS_VMAP

Why in two places? Also please respect alphabetic sorting here (if this
hunk is kept, which may be the more consistent approach) ...

> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -77,6 +77,9 @@ config HAS_PIRQ
>  config HAS_PMAP
>  	bool
>  
> +config HAS_VMAP
> +	bool
> +
>  config HAS_SCHED_GRANULARITY
>  	bool

... and here.

Jan

Re: [PATCH v3 2/4] xen: make VMAP only support in MMU system

[Ayan Kumar Halder]

Hi Jan,

On 14/08/2024 07:37, Jan Beulich wrote:
> On 13.08.2024 19:13, Ayan Kumar Halder wrote:
>> From: Penny Zheng <penny.zheng@arm.com>
>>
>> Introduced CONFIG_VMAP which is selected by the architectures that use
>> MMU. vm_init() does not do anything if CONFIG_VMAP is not enabled.
>>
>> VMAP is widely used in ALTERNATIVE feature 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 ALTERNATIVE now
>> depends on VMAP.
>>
>> HARDEN_BRANCH_PREDICTOR is now gated on HAS_VMAP as speculative
>> attacks are not possible on non MMU based systems (ie Cortex-R52, R82).
>> See https://developer.arm.com/Arm%20Security%20Center/Speculative%20Processor%20Vulnerability.
> While I'm not an Arm expert and hence I'm likely missing aspects, I question
> the one (Spectre-BHB) vulnerability there to be sufficient to draw a
> conclusion towards the usefulness of branch hardening. I would advise
> against encoding such a connection in the Kconfig dependencies.

AFAIU, to address 'Spectre' like vulnerabilities 'branch hardening' was 
added.

See https://lore.kernel.org/all/E1fNadD-0000fz-9r@rmk-PC.armlinux.org.uk/

And from 
https://lists.linaro.org/archives/list/linux-stable-mirror@lists.linaro.org/message/F4MGL4WT2R7T54NLRDGKFRQNSXF3DZGD/

Spectre is valid on MMU based systems.

Thus, I would make 'branch hardenining' valid on MMU based systems only.

Let me know your thoughts.

>
>> --- a/xen/arch/x86/Kconfig
>> +++ b/xen/arch/x86/Kconfig
>> @@ -1,6 +1,7 @@
>>   config X86_64
>>   	def_bool y
>>   	select 64BIT
>> +	select HAS_VMAP
>>   
>>   config X86
>>   	def_bool y
>> @@ -31,6 +32,7 @@ config X86
>>   	select HAS_UBSAN
>>   	select HAS_VPCI if HVM
>>   	select NEEDS_LIBELF
>> +	select HAS_VMAP
> Why in two places? Also please respect alphabetic sorting here (if this
> hunk is kept, which may be the more consistent approach) ...

My mistake. I initially thought user could select one of the two. 
However, "vm_init()" is invoked only from xen/arch/x86/setup.c.

Thus, keeping 'HAS_VMAP' under 'config X86' is dufficient.

I agree that HAS_VMAP needs to go before HAS_VPCI.

>
>> --- a/xen/common/Kconfig
>> +++ b/xen/common/Kconfig
>> @@ -77,6 +77,9 @@ config HAS_PIRQ
>>   config HAS_PMAP
>>   	bool
>>   
>> +config HAS_VMAP
>> +	bool
>> +
>>   config HAS_SCHED_GRANULARITY
>>   	bool
> ... and here.

Yes, it needs to go after HAS_UBSAN.

- Ayan

>
> Jan

Re: [PATCH v3 2/4] xen: make VMAP only support in MMU system

[Jan Beulich]

On 14.08.2024 12:55, Ayan Kumar Halder wrote:
> Hi Jan,
> 
> On 14/08/2024 07:37, Jan Beulich wrote:
>> On 13.08.2024 19:13, Ayan Kumar Halder wrote:
>>> From: Penny Zheng <penny.zheng@arm.com>
>>>
>>> Introduced CONFIG_VMAP which is selected by the architectures that use
>>> MMU. vm_init() does not do anything if CONFIG_VMAP is not enabled.
>>>
>>> VMAP is widely used in ALTERNATIVE feature 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 ALTERNATIVE now
>>> depends on VMAP.
>>>
>>> HARDEN_BRANCH_PREDICTOR is now gated on HAS_VMAP as speculative
>>> attacks are not possible on non MMU based systems (ie Cortex-R52, R82).
>>> See https://developer.arm.com/Arm%20Security%20Center/Speculative%20Processor%20Vulnerability.
>> While I'm not an Arm expert and hence I'm likely missing aspects, I question
>> the one (Spectre-BHB) vulnerability there to be sufficient to draw a
>> conclusion towards the usefulness of branch hardening. I would advise
>> against encoding such a connection in the Kconfig dependencies.
> 
> AFAIU, to address 'Spectre' like vulnerabilities 'branch hardening' was 
> added.
> 
> See https://lore.kernel.org/all/E1fNadD-0000fz-9r@rmk-PC.armlinux.org.uk/
> 
> And from 
> https://lists.linaro.org/archives/list/linux-stable-mirror@lists.linaro.org/message/F4MGL4WT2R7T54NLRDGKFRQNSXF3DZGD/
> 
> Spectre is valid on MMU based systems.

Since then various other issues / flavors were found. I've been focusing
on the x86 side of things, but I'd be very surprised if some didn't
affect other architectures as well. Plus branch hardening can be a pre-
cautionary measure, too, I think.

Jan

Re: [PATCH v3 2/4] xen: make VMAP only support in MMU system

[Ayan Kumar Halder]

Hi Jan,

On 14/08/2024 12:35, Jan Beulich wrote:
> On 14.08.2024 12:55, Ayan Kumar Halder wrote:
>> Hi Jan,
>>
>> On 14/08/2024 07:37, Jan Beulich wrote:
>>> On 13.08.2024 19:13, Ayan Kumar Halder wrote:
>>>> From: Penny Zheng <penny.zheng@arm.com>
>>>>
>>>> Introduced CONFIG_VMAP which is selected by the architectures that use
>>>> MMU. vm_init() does not do anything if CONFIG_VMAP is not enabled.
>>>>
>>>> VMAP is widely used in ALTERNATIVE feature 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 ALTERNATIVE now
>>>> depends on VMAP.
>>>>
>>>> HARDEN_BRANCH_PREDICTOR is now gated on HAS_VMAP as speculative
>>>> attacks are not possible on non MMU based systems (ie Cortex-R52, R82).
>>>> See https://developer.arm.com/Arm%20Security%20Center/Speculative%20Processor%20Vulnerability.
>>> While I'm not an Arm expert and hence I'm likely missing aspects, I question
>>> the one (Spectre-BHB) vulnerability there to be sufficient to draw a
>>> conclusion towards the usefulness of branch hardening. I would advise
>>> against encoding such a connection in the Kconfig dependencies.
>> AFAIU, to address 'Spectre' like vulnerabilities 'branch hardening' was
>> added.
>>
>> See https://lore.kernel.org/all/E1fNadD-0000fz-9r@rmk-PC.armlinux.org.uk/
>>
>> And from
>> https://lists.linaro.org/archives/list/linux-stable-mirror@lists.linaro.org/message/F4MGL4WT2R7T54NLRDGKFRQNSXF3DZGD/
>>
>> Spectre is valid on MMU based systems.
> Since then various other issues / flavors were found. I've been focusing
> on the x86 side of things, but I'd be very surprised if some didn't
> affect other architectures as well.

We are talking about Arm here as "HARDEN_BRANCH_PREDICTOR" is specific 
to Arm.

https://developer.arm.com/Arm%20Security%20Center/Speculative%20Processor%20Vulnerability 
covers all the flavours and it does not include Cortex-R82 or R52.

It says the following :-

"Cortex-R cores typically use a closed software stack. In those 
environments, applications or processes are strictly controlled, and 
therefore not exploitable"

> Plus branch hardening can be a pre-
> cautionary measure, too, I think.

The first two Arm non MMU cores that we wish to support in the 
forthcoming series is Cortex-R82 and R52.

As seen in https://developer.arm.com/documentation/ka005109/latest/, it 
explicitly states the following about R82

The Cortex-R82 implements the faulting feature (FEAT_FPAC) but is not 
vulnerable. The Cortex-R82 behaves differently than vulnerable A-class 
CPUs when speculatively executing past an instruction that authenticates 
PAC, and that behavior does not allow the attack software to create the 
"oracle".

We can re-enable branch hardening if we know there is a valid non MMU 
Arm core which is vulnerable.

Let me know if you are ok with the rationale.

- Ayan

>
> Jan

Re: [PATCH v3 1/4] xen: arm: Add a new helper update_boot_mapping()

[Michal Orzel]

Hi Ayan,

On 13/08/2024 19:13, Ayan Kumar Halder wrote:
> update_boot_mapping() invokes update_identity_mapping() for the MMU specific
> code.
> Later when the MPU code is added, update_boot_mapping() would invoke the
> equivalent.
> 
> The common code now invokes update_boot_mapping() instead of
> update_identity_mapping(). So, that there is clear abstraction between the
> common and MMU/MPU specific logic.
> 
> This is in continuation to commit
> f661a20aa880: "Extract MMU-specific MM code".
> 
> update_identity_mapping() is now marked as static as it is called within
> xen/arch/arm/arm64/mmu/mm.c only. Also, updated the prototype to
> update_boot_mapping() which is now invoked from other files.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
> Changes from :-
> 
> v1 - 1. Introduced update_boot_mapping() which invokes
> update_identity_mapping() in MMU specific code.
> 
> v2 - 1. Make update_identity_mapping() static and update the prototype.
> 
>  xen/arch/arm/arm64/mmu/mm.c         | 7 ++++++-
>  xen/arch/arm/arm64/smpboot.c        | 6 +++---
>  xen/arch/arm/include/asm/arm64/mm.h | 2 +-
>  3 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/mmu/mm.c b/xen/arch/arm/arm64/mmu/mm.c
> index 293acb67e0..1afbbeda5a 100644
> --- a/xen/arch/arm/arm64/mmu/mm.c
> +++ b/xen/arch/arm/arm64/mmu/mm.c
> @@ -111,7 +111,7 @@ void __init arch_setup_page_tables(void)
>      prepare_runtime_identity_mapping();
>  }
>  
> -void update_identity_mapping(bool enable)
> +static void update_identity_mapping(bool enable)
>  {
>      paddr_t id_addr = virt_to_maddr(_start);
>      int rc;
> @@ -125,6 +125,11 @@ void update_identity_mapping(bool enable)
>      BUG_ON(rc);
>  }
>  
> +void update_boot_mapping(bool enable)
> +{
> +    update_identity_mapping(enable);
> +}
> +
>  extern void switch_ttbr_id(uint64_t ttbr);
>  
>  typedef void (switch_ttbr_fn)(uint64_t ttbr);
> diff --git a/xen/arch/arm/arm64/smpboot.c b/xen/arch/arm/arm64/smpboot.c
> index a225fae64d..789f352ab6 100644
> --- a/xen/arch/arm/arm64/smpboot.c
> +++ b/xen/arch/arm/arm64/smpboot.c
> @@ -112,18 +112,18 @@ int arch_cpu_up(int cpu)
>      if ( !smp_enable_ops[cpu].prepare_cpu )
>          return -ENODEV;
>  
> -    update_identity_mapping(true);
> +    update_boot_mapping(true);
>  
>      rc = smp_enable_ops[cpu].prepare_cpu(cpu);
>      if ( rc )
> -        update_identity_mapping(false);
> +        update_boot_mapping(false);
>  
>      return rc;
>  }
>  
>  void arch_cpu_up_finish(void)
>  {
> -    update_identity_mapping(false);
> +    update_boot_mapping(false);
>  }
>  
>  /*
> diff --git a/xen/arch/arm/include/asm/arm64/mm.h b/xen/arch/arm/include/asm/arm64/mm.h
> index e0bd23a6ed..ac8d1f5c78 100644
> --- a/xen/arch/arm/include/asm/arm64/mm.h
> +++ b/xen/arch/arm/include/asm/arm64/mm.h
> @@ -21,7 +21,7 @@ void arch_setup_page_tables(void);
>   * Note that nested call (e.g. enable=true, enable=true) is not
>   * supported.
>   */
> -void update_identity_mapping(bool enable);
> +void update_boot_mapping(bool enable);
The whole point of adding a wrapper was to avoid ambiguity \wrt identity mapping and MPU.
You changed the name of a function but left the comment which is pretty MMU specific. I think
it should be moved to update_identity_mapping.

Apart from that:
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal

Re: [PATCH v3 3/4] xen: arm: Move the functions of domain_page to MMU specific

[Michal Orzel]

Hi Ayan,

On 13/08/2024 19:13, Ayan Kumar Halder wrote:
> Moved init_domheap_mappings(), map_domain_page_global(),
> unmap_domain_page_global(), map_domain_page(), unmap_domain_page(),
> domain_page_map_to_mfn() to MMU specific folder.
Both the commit title and msg seem incorrect (you took them from v1, instead of v2).

With that changed (i.e. title and msg taken from v2):
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal

Re: [PATCH v3 2/4] xen: make VMAP only support in MMU system

[Jan Beulich]

On 14.08.2024 14:33, Ayan Kumar Halder wrote:
> On 14/08/2024 12:35, Jan Beulich wrote:
>> On 14.08.2024 12:55, Ayan Kumar Halder wrote:
>>> On 14/08/2024 07:37, Jan Beulich wrote:
>>>> On 13.08.2024 19:13, Ayan Kumar Halder wrote:
>>>>> From: Penny Zheng <penny.zheng@arm.com>
>>>>>
>>>>> Introduced CONFIG_VMAP which is selected by the architectures that use
>>>>> MMU. vm_init() does not do anything if CONFIG_VMAP is not enabled.
>>>>>
>>>>> VMAP is widely used in ALTERNATIVE feature 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 ALTERNATIVE now
>>>>> depends on VMAP.
>>>>>
>>>>> HARDEN_BRANCH_PREDICTOR is now gated on HAS_VMAP as speculative
>>>>> attacks are not possible on non MMU based systems (ie Cortex-R52, R82).
>>>>> See https://developer.arm.com/Arm%20Security%20Center/Speculative%20Processor%20Vulnerability.
>>>> While I'm not an Arm expert and hence I'm likely missing aspects, I question
>>>> the one (Spectre-BHB) vulnerability there to be sufficient to draw a
>>>> conclusion towards the usefulness of branch hardening. I would advise
>>>> against encoding such a connection in the Kconfig dependencies.
>>> AFAIU, to address 'Spectre' like vulnerabilities 'branch hardening' was
>>> added.
>>>
>>> See https://lore.kernel.org/all/E1fNadD-0000fz-9r@rmk-PC.armlinux.org.uk/
>>>
>>> And from
>>> https://lists.linaro.org/archives/list/linux-stable-mirror@lists.linaro.org/message/F4MGL4WT2R7T54NLRDGKFRQNSXF3DZGD/
>>>
>>> Spectre is valid on MMU based systems.
>> Since then various other issues / flavors were found. I've been focusing
>> on the x86 side of things, but I'd be very surprised if some didn't
>> affect other architectures as well.
> 
> We are talking about Arm here as "HARDEN_BRANCH_PREDICTOR" is specific 
> to Arm.
> 
> https://developer.arm.com/Arm%20Security%20Center/Speculative%20Processor%20Vulnerability 
> covers all the flavours and it does not include Cortex-R82 or R52.
> 
> It says the following :-
> 
> "Cortex-R cores typically use a closed software stack. In those 
> environments, applications or processes are strictly controlled, and 
> therefore not exploitable"
> 
>> Plus branch hardening can be a pre-
>> cautionary measure, too, I think.
> 
> The first two Arm non MMU cores that we wish to support in the 
> forthcoming series is Cortex-R82 and R52.
> 
> As seen in https://developer.arm.com/documentation/ka005109/latest/, it 
> explicitly states the following about R82
> 
> The Cortex-R82 implements the faulting feature (FEAT_FPAC) but is not 
> vulnerable. The Cortex-R82 behaves differently than vulnerable A-class 
> CPUs when speculatively executing past an instruction that authenticates 
> PAC, and that behavior does not allow the attack software to create the 
> "oracle".
> 
> We can re-enable branch hardening if we know there is a valid non MMU 
> Arm core which is vulnerable.
> 
> Let me know if you are ok with the rationale.

I'm okay with any rationale. As indicated, I'm likely missing (large) parts
of the picture here. The connection between the two Kconfig settings merely
seemed questionable to me, hence the comment.

Jan

Re: [PATCH v3 4/4] xen: arm: Enclose access to EL2 MMU specific registers under CONFIG_MMU

[Michal Orzel]

Hi Ayan,

On 13/08/2024 19:13, Ayan Kumar Halder wrote:
> All the EL2 MMU specific registers are enclosed within CONFIG_MMU.
"Also, protect vttbr_el2 field of struct reg_ctxt and accesses to it."

> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>

Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal

Re: [PATCH v3 2/4] xen: make VMAP only support in MMU system

[Michal Orzel]

Hi Ayan,

On 14/08/2024 14:33, Ayan Kumar Halder wrote:
> Hi Jan,
> 
> On 14/08/2024 12:35, Jan Beulich wrote:
>> On 14.08.2024 12:55, Ayan Kumar Halder wrote:
>>> Hi Jan,
>>>
>>> On 14/08/2024 07:37, Jan Beulich wrote:
>>>> On 13.08.2024 19:13, Ayan Kumar Halder wrote:
>>>>> From: Penny Zheng <penny.zheng@arm.com>
>>>>>
>>>>> Introduced CONFIG_VMAP which is selected by the architectures that use
>>>>> MMU. vm_init() does not do anything if CONFIG_VMAP is not enabled.
>>>>>
>>>>> VMAP is widely used in ALTERNATIVE feature 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 ALTERNATIVE now
>>>>> depends on VMAP.
>>>>>
>>>>> HARDEN_BRANCH_PREDICTOR is now gated on HAS_VMAP as speculative
>>>>> attacks are not possible on non MMU based systems (ie Cortex-R52, R82).
>>>>> See https://developer.arm.com/Arm%20Security%20Center/Speculative%20Processor%20Vulnerability.
>>>> While I'm not an Arm expert and hence I'm likely missing aspects, I question
>>>> the one (Spectre-BHB) vulnerability there to be sufficient to draw a
>>>> conclusion towards the usefulness of branch hardening. I would advise
>>>> against encoding such a connection in the Kconfig dependencies.
>>> AFAIU, to address 'Spectre' like vulnerabilities 'branch hardening' was
>>> added.
>>>
>>> See https://lore.kernel.org/all/E1fNadD-0000fz-9r@rmk-PC.armlinux.org.uk/
>>>
>>> And from
>>> https://lists.linaro.org/archives/list/linux-stable-mirror@lists.linaro.org/message/F4MGL4WT2R7T54NLRDGKFRQNSXF3DZGD/
>>>
>>> Spectre is valid on MMU based systems.
>> Since then various other issues / flavors were found. I've been focusing
>> on the x86 side of things, but I'd be very surprised if some didn't
>> affect other architectures as well.
> 
> We are talking about Arm here as "HARDEN_BRANCH_PREDICTOR" is specific 
> to Arm.
> 
> https://developer.arm.com/Arm%20Security%20Center/Speculative%20Processor%20Vulnerability 
> covers all the flavours and it does not include Cortex-R82 or R52.
> 
> It says the following :-
> 
> "Cortex-R cores typically use a closed software stack. In those 
> environments, applications or processes are strictly controlled, and 
> therefore not exploitable"
> 
>> Plus branch hardening can be a pre-
>> cautionary measure, too, I think.
> 
> The first two Arm non MMU cores that we wish to support in the 
> forthcoming series is Cortex-R82 and R52.
> 
> As seen in https://developer.arm.com/documentation/ka005109/latest/, it 
> explicitly states the following about R82
> 
> The Cortex-R82 implements the faulting feature (FEAT_FPAC) but is not 
> vulnerable. The Cortex-R82 behaves differently than vulnerable A-class 
> CPUs when speculatively executing past an instruction that authenticates 
> PAC, and that behavior does not allow the attack software to create the 
> "oracle".
> 
> We can re-enable branch hardening if we know there is a valid non MMU 
> Arm core which is vulnerable.
> 
> Let me know if you are ok with the rationale.
I'm ok with your rationale.

I have one question for this patch. Why can't we use CONFIG_HAS_VMAP to conditionally compile vmap.c, like:
obj-$(CONFIG_HAS_VMAP) += vmap.o
and get rid of VMAP_VIRT_START guard on an entire file?
With this config in place, it seems strange to use VMAP_VIRT_START as a guard.

~Michal

Re: [PATCH v3 2/4] xen: make VMAP only support in MMU system

[Ayan Kumar Halder]

On 16/08/2024 10:28, Michal Orzel wrote:
> Hi Ayan,
Hi Michal,
>
> On 14/08/2024 14:33, Ayan Kumar Halder wrote:
>> Hi Jan,
>>
>> On 14/08/2024 12:35, Jan Beulich wrote:
>>> On 14.08.2024 12:55, Ayan Kumar Halder wrote:
>>>> Hi Jan,
>>>>
>>>> On 14/08/2024 07:37, Jan Beulich wrote:
>>>>> On 13.08.2024 19:13, Ayan Kumar Halder wrote:
>>>>>> From: Penny Zheng <penny.zheng@arm.com>
>>>>>>
>>>>>> Introduced CONFIG_VMAP which is selected by the architectures that use
>>>>>> MMU. vm_init() does not do anything if CONFIG_VMAP is not enabled.
>>>>>>
>>>>>> VMAP is widely used in ALTERNATIVE feature 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 ALTERNATIVE now
>>>>>> depends on VMAP.
>>>>>>
>>>>>> HARDEN_BRANCH_PREDICTOR is now gated on HAS_VMAP as speculative
>>>>>> attacks are not possible on non MMU based systems (ie Cortex-R52, R82).
>>>>>> See https://developer.arm.com/Arm%20Security%20Center/Speculative%20Processor%20Vulnerability.
>>>>> While I'm not an Arm expert and hence I'm likely missing aspects, I question
>>>>> the one (Spectre-BHB) vulnerability there to be sufficient to draw a
>>>>> conclusion towards the usefulness of branch hardening. I would advise
>>>>> against encoding such a connection in the Kconfig dependencies.
>>>> AFAIU, to address 'Spectre' like vulnerabilities 'branch hardening' was
>>>> added.
>>>>
>>>> See https://lore.kernel.org/all/E1fNadD-0000fz-9r@rmk-PC.armlinux.org.uk/
>>>>
>>>> And from
>>>> https://lists.linaro.org/archives/list/linux-stable-mirror@lists.linaro.org/message/F4MGL4WT2R7T54NLRDGKFRQNSXF3DZGD/
>>>>
>>>> Spectre is valid on MMU based systems.
>>> Since then various other issues / flavors were found. I've been focusing
>>> on the x86 side of things, but I'd be very surprised if some didn't
>>> affect other architectures as well.
>> We are talking about Arm here as "HARDEN_BRANCH_PREDICTOR" is specific
>> to Arm.
>>
>> https://developer.arm.com/Arm%20Security%20Center/Speculative%20Processor%20Vulnerability
>> covers all the flavours and it does not include Cortex-R82 or R52.
>>
>> It says the following :-
>>
>> "Cortex-R cores typically use a closed software stack. In those
>> environments, applications or processes are strictly controlled, and
>> therefore not exploitable"
>>
>>> Plus branch hardening can be a pre-
>>> cautionary measure, too, I think.
>> The first two Arm non MMU cores that we wish to support in the
>> forthcoming series is Cortex-R82 and R52.
>>
>> As seen in https://developer.arm.com/documentation/ka005109/latest/, it
>> explicitly states the following about R82
>>
>> The Cortex-R82 implements the faulting feature (FEAT_FPAC) but is not
>> vulnerable. The Cortex-R82 behaves differently than vulnerable A-class
>> CPUs when speculatively executing past an instruction that authenticates
>> PAC, and that behavior does not allow the attack software to create the
>> "oracle".
>>
>> We can re-enable branch hardening if we know there is a valid non MMU
>> Arm core which is vulnerable.
>>
>> Let me know if you are ok with the rationale.
> I'm ok with your rationale.
>
> I have one question for this patch. Why can't we use CONFIG_HAS_VMAP to conditionally compile vmap.c, like:
> obj-$(CONFIG_HAS_VMAP) += vmap.o
> and get rid of VMAP_VIRT_START guard on an entire file?
> With this config in place, it seems strange to use VMAP_VIRT_START as a guard.

I am fine with your suggestion.

However, this implies HAS_VMAP needs to be selected by "config PPC" as 
well (as it defines VMAP_VIRT_START in xen/arch/ppc/include/asm/config.h ).

This is fine as I could see PPC uses MMU as well.

- Ayan

Re: [PATCH v3 2/4] xen: make VMAP only support in MMU system

[Julien Grall]

Hi Ayan,

On 14/08/2024 13:33, Ayan Kumar Halder wrote:
> Hi Jan,
> 
> On 14/08/2024 12:35, Jan Beulich wrote:
>> On 14.08.2024 12:55, Ayan Kumar Halder wrote:
>>> Hi Jan,
>>>
>>> On 14/08/2024 07:37, Jan Beulich wrote:
>>>> On 13.08.2024 19:13, Ayan Kumar Halder wrote:
>>>>> From: Penny Zheng <penny.zheng@arm.com>
>>>>>
>>>>> Introduced CONFIG_VMAP which is selected by the architectures that use
>>>>> MMU. vm_init() does not do anything if CONFIG_VMAP is not enabled.
>>>>>
>>>>> VMAP is widely used in ALTERNATIVE feature 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 ALTERNATIVE now
>>>>> depends on VMAP.
>>>>>
>>>>> HARDEN_BRANCH_PREDICTOR is now gated on HAS_VMAP as speculative
>>>>> attacks are not possible on non MMU based systems (ie Cortex-R52, 
>>>>> R82).
>>>>> See https://developer.arm.com/Arm%20Security%20Center/ 
>>>>> Speculative%20Processor%20Vulnerability.
>>>> While I'm not an Arm expert and hence I'm likely missing aspects, I 
>>>> question
>>>> the one (Spectre-BHB) vulnerability there to be sufficient to draw a
>>>> conclusion towards the usefulness of branch hardening. I would advise
>>>> against encoding such a connection in the Kconfig dependencies.
>>> AFAIU, to address 'Spectre' like vulnerabilities 'branch hardening' was
>>> added.
>>>
>>> See https://lore.kernel.org/all/E1fNadD-0000fz-9r@rmk- 
>>> PC.armlinux.org.uk/
>>>
>>> And from
>>> https://lists.linaro.org/archives/list/linux-stable- 
>>> mirror@lists.linaro.org/message/F4MGL4WT2R7T54NLRDGKFRQNSXF3DZGD/
>>>
>>> Spectre is valid on MMU based systems.
>> Since then various other issues / flavors were found. I've been focusing
>> on the x86 side of things, but I'd be very surprised if some didn't
>> affect other architectures as well.
> 
> We are talking about Arm here as "HARDEN_BRANCH_PREDICTOR" is specific 
> to Arm.
> 
> https://developer.arm.com/Arm%20Security%20Center/ 
> Speculative%20Processor%20Vulnerability covers all the flavours and it 
> does not include Cortex-R82 or R52.
> 
> It says the following :-
> 
> "Cortex-R cores typically use a closed software stack. In those 
> environments, applications or processes are strictly controlled, and 
> therefore not exploitable"
> 
>> Plus branch hardening can be a pre-
>> cautionary measure, too, I think.
> 
> The first two Arm non MMU cores that we wish to support in the 
> forthcoming series is Cortex-R82 and R52.
> 
> As seen in https://developer.arm.com/documentation/ka005109/latest/, it 
> explicitly states the following about R82
> 
> The Cortex-R82 implements the faulting feature (FEAT_FPAC) but is not 
> vulnerable. The Cortex-R82 behaves differently than vulnerable A-class 
> CPUs when speculatively executing past an instruction that authenticates 
> PAC, and that behavior does not allow the attack software to create the 
> "oracle".

I am confused. This is describing why R82 is not affected by PACMAN. But 
the Kconfig HARDEN_BRANCH_PREDICTOR is not for that (Xen doesn't yet use 
Pointer Authentification Codes). The Kconfig was introduced with XSA-254 
which predates PACMAN by nearly 4 years.

> 
> We can re-enable branch hardening if we know there is a valid non MMU 
> Arm core which is vulnerable.

Re-quoting what you wrote earlier:

"Cortex-R cores typically use a closed software stack. In those
environments, applications or processes are strictly controlled, and
therefore not exploitable"

It is quite subtle. This wording doesn't imply the cores are not 
vulnerable. It says that if they are, then it would be difficult to 
exploit because the software should be tightly controlled.

Now this would be really up to the user to decide whether they want to 
be extra cautious/futureproof or not.

As we are at the beginning of the MPU support, then I don't think we 
need to resolve issue right now. And it would be fine to gate it. But 
maybe ARCH_VMAP was an incorrect suggestion. It might be better to gate 
with the !MMU (IIRC this would imply MPU).

But before the feature can be marked as security support. We will need 
to agree on how the hypervisor is intended to be used on ARMv8-R. Maybe 
it would need a caveat "only trusted software can be run" which means we 
don't have to worry about speculation on Cortex-R. Although, it would be 
nice to have some defense in-depth :).

Cheers,

-- 
Julien Grall

Re: [PATCH v3 2/4] xen: make VMAP only support in MMU system

[Ayan Kumar Halder]

On 16/08/2024 17:40, Julien Grall wrote:
> Hi Ayan,
Hi Julien,
>
> On 14/08/2024 13:33, Ayan Kumar Halder wrote:
>> Hi Jan,
>>
>> On 14/08/2024 12:35, Jan Beulich wrote:
>>> On 14.08.2024 12:55, Ayan Kumar Halder wrote:
>>>> Hi Jan,
>>>>
>>>> On 14/08/2024 07:37, Jan Beulich wrote:
>>>>> On 13.08.2024 19:13, Ayan Kumar Halder wrote:
>>>>>> From: Penny Zheng <penny.zheng@arm.com>
>>>>>>
>>>>>> Introduced CONFIG_VMAP which is selected by the architectures 
>>>>>> that use
>>>>>> MMU. vm_init() does not do anything if CONFIG_VMAP is not enabled.
>>>>>>
>>>>>> VMAP is widely used in ALTERNATIVE feature 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 ALTERNATIVE now
>>>>>> depends on VMAP.
>>>>>>
>>>>>> HARDEN_BRANCH_PREDICTOR is now gated on HAS_VMAP as speculative
>>>>>> attacks are not possible on non MMU based systems (ie Cortex-R52, 
>>>>>> R82).
>>>>>> See https://developer.arm.com/Arm%20Security%20Center/ 
>>>>>> Speculative%20Processor%20Vulnerability.
>>>>> While I'm not an Arm expert and hence I'm likely missing aspects, 
>>>>> I question
>>>>> the one (Spectre-BHB) vulnerability there to be sufficient to draw a
>>>>> conclusion towards the usefulness of branch hardening. I would advise
>>>>> against encoding such a connection in the Kconfig dependencies.
>>>> AFAIU, to address 'Spectre' like vulnerabilities 'branch hardening' 
>>>> was
>>>> added.
>>>>
>>>> See https://lore.kernel.org/all/E1fNadD-0000fz-9r@rmk- 
>>>> PC.armlinux.org.uk/
>>>>
>>>> And from
>>>> https://lists.linaro.org/archives/list/linux-stable- 
>>>> mirror@lists.linaro.org/message/F4MGL4WT2R7T54NLRDGKFRQNSXF3DZGD/
>>>>
>>>> Spectre is valid on MMU based systems.
>>> Since then various other issues / flavors were found. I've been 
>>> focusing
>>> on the x86 side of things, but I'd be very surprised if some didn't
>>> affect other architectures as well.
>>
>> We are talking about Arm here as "HARDEN_BRANCH_PREDICTOR" is 
>> specific to Arm.
>>
>> https://developer.arm.com/Arm%20Security%20Center/ 
>> Speculative%20Processor%20Vulnerability covers all the flavours and 
>> it does not include Cortex-R82 or R52.
>>
>> It says the following :-
>>
>> "Cortex-R cores typically use a closed software stack. In those 
>> environments, applications or processes are strictly controlled, and 
>> therefore not exploitable"
>>
>>> Plus branch hardening can be a pre-
>>> cautionary measure, too, I think.
>>
>> The first two Arm non MMU cores that we wish to support in the 
>> forthcoming series is Cortex-R82 and R52.
>>
>> As seen in https://developer.arm.com/documentation/ka005109/latest/, 
>> it explicitly states the following about R82
>>
>> The Cortex-R82 implements the faulting feature (FEAT_FPAC) but is not 
>> vulnerable. The Cortex-R82 behaves differently than vulnerable 
>> A-class CPUs when speculatively executing past an instruction that 
>> authenticates PAC, and that behavior does not allow the attack 
>> software to create the "oracle".
>
> I am confused. This is describing why R82 is not affected by PACMAN. 
> But the Kconfig HARDEN_BRANCH_PREDICTOR is not for that (Xen doesn't 
> yet use Pointer Authentification Codes). The Kconfig was introduced 
> with XSA-254 which predates PACMAN by nearly 4 years.
Yes, you are correct. I was somehow linking PACMAN to branch prediction 
attacks. So, by mistake I assumed that HARDEN_BRANCH_PREDICTOR is used 
for protection against these attacks.
>
>>
>> We can re-enable branch hardening if we know there is a valid non MMU 
>> Arm core which is vulnerable.
>
> Re-quoting what you wrote earlier:
>
> "Cortex-R cores typically use a closed software stack. In those
> environments, applications or processes are strictly controlled, and
> therefore not exploitable"
>
> It is quite subtle. This wording doesn't imply the cores are not 
> vulnerable. It says that if they are, then it would be difficult to 
> exploit because the software should be tightly controlled.
>
> Now this would be really up to the user to decide whether they want to 
> be extra cautious/futureproof or not.
>
> As we are at the beginning of the MPU support, then I don't think we 
> need to resolve issue right now. And it would be fine to gate it. But 
> maybe ARCH_VMAP was an incorrect suggestion. It might be better to 
> gate with the !MMU (IIRC this would imply MPU).

I am ok with this. This has the benefit that the change can be contained 
within arch/arm if we do the following :-

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index cb2c0a16b8..26f7406278 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -329,7 +329,9 @@ void asmlinkage __init start_xen(unsigned long 
boot_phys_offset,

      setup_mm();

+#ifdef CONFIG_MMU
      vm_init();
+#endif

      /* Parse the ACPI tables for possible boot-time configuration */
      acpi_boot_table_init();

Are we ok with this ?

The definition of vm_init() is in xen/include/xen/vmap.h. If I enclose 
it using any CONFIG_XXX (like I have done in the current patch), then I 
need to introduce it in common/Kconfig and define it for x86 and PPC. I 
would prefer to contain the change within arch/arm only if possible.

>
> But before the feature can be marked as security support. We will need 
> to agree on how the hypervisor is intended to be used on ARMv8-R. 
> Maybe it would need a caveat "only trusted software can be run" which 
> means we don't have to worry about speculation on Cortex-R. Although, 
> it would be nice to have some defense in-depth :).

Yes, the support for ARMv8-R will be 'experimental' for some time. I am 
planning to add the minimal support required to just boot one or more DomUs.

- Ayan

Re: [PATCH v3 2/4] xen: make VMAP only support in MMU system

[Julien Grall]

Hi Ayan,

On 19/08/2024 10:45, Ayan Kumar Halder wrote:
> I am ok with this. This has the benefit that the change can be contained 
> within arch/arm if we do the following :-
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index cb2c0a16b8..26f7406278 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -329,7 +329,9 @@ void asmlinkage __init start_xen(unsigned long 
> boot_phys_offset,
> 
>       setup_mm();
> 
> +#ifdef CONFIG_MMU
>       vm_init();
> +#endif
> 
>       /* Parse the ACPI tables for possible boot-time configuration */
>       acpi_boot_table_init();
> 
> Are we ok with this ?
> 
> The definition of vm_init() is in xen/include/xen/vmap.h. If I enclose 
> it using any CONFIG_XXX (like I have done in the current patch), then I 
> need to introduce it in common/Kconfig and define it for x86 and PPC. I 
> would prefer to contain the change within arch/arm only if possible.

Just to clarify, are you suggesting to just protect the call vm_init(). 
In other word, common/vmap.c would still be included in the final binary 
for the MPU?

If yes, then I think it would be a bit odd... Someone could still call 
vmap() and this would not break until runtime.

So I don't see how we could get away from modifying the common code.

Cheers,

-- 
Julien Grall

Re: [PATCH v3 2/4] xen: make VMAP only support in MMU system

[Julien Grall]

On 19/08/2024 10:55, Julien Grall wrote:
> Hi Ayan,
> 
> On 19/08/2024 10:45, Ayan Kumar Halder wrote:
>> I am ok with this. This has the benefit that the change can be 
>> contained within arch/arm if we do the following :-
>>
>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>> index cb2c0a16b8..26f7406278 100644
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -329,7 +329,9 @@ void asmlinkage __init start_xen(unsigned long 
>> boot_phys_offset,
>>
>>       setup_mm();
>>
>> +#ifdef CONFIG_MMU
>>       vm_init();
>> +#endif
>>
>>       /* Parse the ACPI tables for possible boot-time configuration */
>>       acpi_boot_table_init();
>>
>> Are we ok with this ?
>>
>> The definition of vm_init() is in xen/include/xen/vmap.h. If I enclose 
>> it using any CONFIG_XXX (like I have done in the current patch), then 
>> I need to introduce it in common/Kconfig and define it for x86 and 
>> PPC. I would prefer to contain the change within arch/arm only if 
>> possible.
> 
> Just to clarify, are you suggesting to just protect the call vm_init(). 
> In other word, common/vmap.c would still be included in the final binary 
> for the MPU?
> 
> If yes, then I think it would be a bit odd... Someone could still call 
> vmap() and this would not break until runtime.
> 
> So I don't see how we could get away from modifying the common code.

Readying my previous reply again. I think the confusion comes from:

 > But maybe ARCH_VMAP was an incorrect suggestion. It might be better 
to gate with the !MMU (IIRC this would imply MPU).

This was specifically referring to the branch predictor Kconfig. This 
was not a suggestion to avoid introducing ARCH_VMAP.

Cheers,

-- 
Julien Grall

Re: [PATCH v3 2/4] xen: make VMAP only support in MMU system

[Jan Beulich]

Guys,

On 19.08.2024 11:45, Ayan Kumar Halder wrote:
> On 16/08/2024 17:40, Julien Grall wrote:
>> On 14/08/2024 13:33, Ayan Kumar Halder wrote:

mind me asking why I continue to be on the To: list of this communication
between the two of you?

Jan

Re: [PATCH v3 2/4] xen: make VMAP only support in MMU system

[Julien Grall]

On 19/08/2024 12:39, Jan Beulich wrote:
> Guys,
> 
> On 19.08.2024 11:45, Ayan Kumar Halder wrote:
>> On 16/08/2024 17:40, Julien Grall wrote:
>>> On 14/08/2024 13:33, Ayan Kumar Halder wrote:
> 
> mind me asking why I continue to be on the To: list of this communication
> between the two of you?

You were involved in the review and AFAICT will still touch the common 
code. I am happy to remove you from the conversation if you are not 
interested :).

Cheers,

-- 
Julien Grall

Re: [PATCH v3 2/4] xen: make VMAP only support in MMU system

[Jan Beulich]

On 19.08.2024 14:12, Julien Grall wrote:
> On 19/08/2024 12:39, Jan Beulich wrote:
>> Guys,
>>
>> On 19.08.2024 11:45, Ayan Kumar Halder wrote:
>>> On 16/08/2024 17:40, Julien Grall wrote:
>>>> On 14/08/2024 13:33, Ayan Kumar Halder wrote:
>>
>> mind me asking why I continue to be on the To: list of this communication
>> between the two of you?
> 
> You were involved in the review and AFAICT will still touch the common 
> code. I am happy to remove you from the conversation if you are not 
> interested :).

That wasn't my request though. It's fine to keep me Cc-ed.

Jan

Re: [PATCH v3 2/4] xen: make VMAP only support in MMU system

[Julien Grall]

Hi Jan,

On 19/08/2024 13:24, Jan Beulich wrote:
> On 19.08.2024 14:12, Julien Grall wrote:
>> On 19/08/2024 12:39, Jan Beulich wrote:
>>> Guys,
>>>
>>> On 19.08.2024 11:45, Ayan Kumar Halder wrote:
>>>> On 16/08/2024 17:40, Julien Grall wrote:
>>>>> On 14/08/2024 13:33, Ayan Kumar Halder wrote:
>>>
>>> mind me asking why I continue to be on the To: list of this communication
>>> between the two of you?
>>
>> You were involved in the review and AFAICT will still touch the common
>> code. I am happy to remove you from the conversation if you are not
>> interested :).
> 
> That wasn't my request though. It's fine to keep me Cc-ed.

TBH, in this context, the difference between CC and TO is very blur.

For me, CC is for people that may be interested but not involved in the 
conversation. In this case, Ayan was suggesting whether we should use 
ARCH_VMAP or not.

Given you reviewed the rest, I feel you have your say on this decision. 
Hence why the TO makes sense. But I also understand why people may view 
it differently...

Personally, I don't make any filter on "cc" vs "to" because this is a 
matter of opinion whether you think someone should be actively involved 
(or not) in the discussion. So it is best if I look at everything even 
if this is just having a glancing look at on the content.

If you have some clear and unambiguous rules on when you want to be in 
the CC or TO, then I would be happy to follow them.

Cheers,

-- 
Julien Grall

Re: [PATCH v3 1/4] xen: arm: Add a new helper update_boot_mapping()

[Ayan Kumar Halder]

On 14/08/2024 13:50, Michal Orzel wrote:
> Hi Ayan,
Hi Michal,
>
> On 13/08/2024 19:13, Ayan Kumar Halder wrote:
>> update_boot_mapping() invokes update_identity_mapping() for the MMU specific
>> code.
>> Later when the MPU code is added, update_boot_mapping() would invoke the
>> equivalent.
>>
>> The common code now invokes update_boot_mapping() instead of
>> update_identity_mapping(). So, that there is clear abstraction between the
>> common and MMU/MPU specific logic.
>>
>> This is in continuation to commit
>> f661a20aa880: "Extract MMU-specific MM code".
>>
>> update_identity_mapping() is now marked as static as it is called within
>> xen/arch/arm/arm64/mmu/mm.c only. Also, updated the prototype to
>> update_boot_mapping() which is now invoked from other files.
>>
>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>> ---
>> Changes from :-
>>
>> v1 - 1. Introduced update_boot_mapping() which invokes
>> update_identity_mapping() in MMU specific code.
>>
>> v2 - 1. Make update_identity_mapping() static and update the prototype.
>>
>>   xen/arch/arm/arm64/mmu/mm.c         | 7 ++++++-
>>   xen/arch/arm/arm64/smpboot.c        | 6 +++---
>>   xen/arch/arm/include/asm/arm64/mm.h | 2 +-
>>   3 files changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm64/mmu/mm.c b/xen/arch/arm/arm64/mmu/mm.c
>> index 293acb67e0..1afbbeda5a 100644
>> --- a/xen/arch/arm/arm64/mmu/mm.c
>> +++ b/xen/arch/arm/arm64/mmu/mm.c
>> @@ -111,7 +111,7 @@ void __init arch_setup_page_tables(void)
>>       prepare_runtime_identity_mapping();
>>   }
>>   
>> -void update_identity_mapping(bool enable)
>> +static void update_identity_mapping(bool enable)
>>   {
>>       paddr_t id_addr = virt_to_maddr(_start);
>>       int rc;
>> @@ -125,6 +125,11 @@ void update_identity_mapping(bool enable)
>>       BUG_ON(rc);
>>   }
>>   
>> +void update_boot_mapping(bool enable)
>> +{
>> +    update_identity_mapping(enable);
>> +}
>> +
>>   extern void switch_ttbr_id(uint64_t ttbr);
>>   
>>   typedef void (switch_ttbr_fn)(uint64_t ttbr);
>> diff --git a/xen/arch/arm/arm64/smpboot.c b/xen/arch/arm/arm64/smpboot.c
>> index a225fae64d..789f352ab6 100644
>> --- a/xen/arch/arm/arm64/smpboot.c
>> +++ b/xen/arch/arm/arm64/smpboot.c
>> @@ -112,18 +112,18 @@ int arch_cpu_up(int cpu)
>>       if ( !smp_enable_ops[cpu].prepare_cpu )
>>           return -ENODEV;
>>   
>> -    update_identity_mapping(true);
>> +    update_boot_mapping(true);
>>   
>>       rc = smp_enable_ops[cpu].prepare_cpu(cpu);
>>       if ( rc )
>> -        update_identity_mapping(false);
>> +        update_boot_mapping(false);
>>   
>>       return rc;
>>   }
>>   
>>   void arch_cpu_up_finish(void)
>>   {
>> -    update_identity_mapping(false);
>> +    update_boot_mapping(false);
>>   }
>>   
>>   /*
>> diff --git a/xen/arch/arm/include/asm/arm64/mm.h b/xen/arch/arm/include/asm/arm64/mm.h
>> index e0bd23a6ed..ac8d1f5c78 100644
>> --- a/xen/arch/arm/include/asm/arm64/mm.h
>> +++ b/xen/arch/arm/include/asm/arm64/mm.h
>> @@ -21,7 +21,7 @@ void arch_setup_page_tables(void);
>>    * Note that nested call (e.g. enable=true, enable=true) is not
>>    * supported.
>>    */
>> -void update_identity_mapping(bool enable);
>> +void update_boot_mapping(bool enable);
> The whole point of adding a wrapper was to avoid ambiguity \wrt identity mapping and MPU.
> You changed the name of a function but left the comment which is pretty MMU specific. I think
> it should be moved to update_identity_mapping.

Yes, you are correct. I will move the comment before

static void update_identity_mapping(bool enable)

{...}

>
> Apart from that:
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>

I will keep your R-b

- Ayan

Re: [PATCH v3 2/4] xen: make VMAP only support in MMU system

[Ayan Kumar Halder]

Hi Julien/Jan,

On 19/08/2024 10:58, Julien Grall wrote:
>
>
> On 19/08/2024 10:55, Julien Grall wrote:
>> Hi Ayan,
>>
>> On 19/08/2024 10:45, Ayan Kumar Halder wrote:
>>> I am ok with this. This has the benefit that the change can be 
>>> contained within arch/arm if we do the following :-
>>>
>>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>>> index cb2c0a16b8..26f7406278 100644
>>> --- a/xen/arch/arm/setup.c
>>> +++ b/xen/arch/arm/setup.c
>>> @@ -329,7 +329,9 @@ void asmlinkage __init start_xen(unsigned long 
>>> boot_phys_offset,
>>>
>>>       setup_mm();
>>>
>>> +#ifdef CONFIG_MMU
>>>       vm_init();
>>> +#endif
>>>
>>>       /* Parse the ACPI tables for possible boot-time configuration */
>>>       acpi_boot_table_init();
>>>
>>> Are we ok with this ?
>>>
>>> The definition of vm_init() is in xen/include/xen/vmap.h. If I 
>>> enclose it using any CONFIG_XXX (like I have done in the current 
>>> patch), then I need to introduce it in common/Kconfig and define it 
>>> for x86 and PPC. I would prefer to contain the change within 
>>> arch/arm only if possible.
>>
>> Just to clarify, are you suggesting to just protect the call 
>> vm_init(). In other word, common/vmap.c would still be included in 
>> the final binary for the MPU?
>>
>> If yes, then I think it would be a bit odd... Someone could still 
>> call vmap() and this would not break until runtime.
>>
>> So I don't see how we could get away from modifying the common code.
>
> Readying my previous reply again. I think the confusion comes from:
>
> > But maybe ARCH_VMAP was an incorrect suggestion. It might be better 
> to gate with the !MMU (IIRC this would imply MPU).
>
> This was specifically referring to the branch predictor Kconfig. This 
> was not a suggestion to avoid introducing ARCH_VMAP.

Thanks for clarifying this. Yes, I misunderstood your previous comment. 
So I will do :-

1. HARDEN_BRANCH_PREDICTOR will depend on MMU.

2. ARCH_VMAP will be selected by PPC and RISCV. The reason is below.

3. xen/common/vmap.c will be conditionally compiled on ARCH_VMAP and the 
"#ifdef VMAP_VIRT_START .. endif" will be from removed within the file. 
As VMAP_VIRT_START is defined by RISCV and PPC, thus #2 is needed.

Julien, Jan :- Please let me know if you are ok with #3. This was in 
response to Michal's comment. While his suggestion makes sense, I am not 
sure if extending the changes to other architectures is the correct 
approach. Or do you prefer keeping xen/common/vmap.c unchanged.

Kind regards,

Ayan


>
> Cheers,
>

Re: [PATCH v3 2/4] xen: make VMAP only support in MMU system

[Jan Beulich]

On 20.08.2024 13:48, Ayan Kumar Halder wrote:
> So I will do :-
> 
> 1. HARDEN_BRANCH_PREDICTOR will depend on MMU.
> 
> 2. ARCH_VMAP will be selected by PPC and RISCV. The reason is below.
> 
> 3. xen/common/vmap.c will be conditionally compiled on ARCH_VMAP and the 
> "#ifdef VMAP_VIRT_START .. endif" will be from removed within the file. 
> As VMAP_VIRT_START is defined by RISCV and PPC, thus #2 is needed.
> 
> Julien, Jan :- Please let me know if you are ok with #3. This was in 
> response to Michal's comment. While his suggestion makes sense, I am not 
> sure if extending the changes to other architectures is the correct 
> approach. Or do you prefer keeping xen/common/vmap.c unchanged.

No, the VMAP_VIRT_START thing was, from all I recall, merely to cover
for the lack of a way to exclude building of the file from Makefile.
After all this pre-dates the introduction of kconfig in Xen.

And yes, suitably covering PPC and RISC-V is The Right Thing To Do (tm);
their maintainers can shout if they disagree.

Jan