Re: [PATCH v7 00/11] arm64: Support for running as a guest in Arm CCA

Re: [PATCH v7 00/11] arm64: Support for running as a guest in Arm CCA

[Catalin Marinas]

On Thu, 17 Oct 2024 14:14:23 +0100, Steven Price wrote:
> This series adds support for running Linux in a protected VM under the
> Arm Confidential Compute Architecture (CCA). This is a minor update
> following the feedback from the v6 posting[1]. Thanks for the feedback!
> 
> Individual patches have a change log. The biggest changes are in patch
> 10 where Gavin gave some great feedback to tidy things up a bit.
> 
> [...]

Applied to arm64 (for-next/guest-cca), thanks!

Note that this branch cannot be tested in isolation as it doesn't have
the irqchip CCA changes. I pulled tip irq/core into the arm64
for-kernelci. Please give the latter branch a go (or linux-next when the
patches turn up).

[01/11] arm64: rsi: Add RSI definitions
        https://git.kernel.org/arm64/c/b880a80011f5
[02/11] arm64: Detect if in a realm and set RIPAS RAM
        https://git.kernel.org/arm64/c/c077711f718b
[03/11] arm64: realm: Query IPA size from the RMM
        https://git.kernel.org/arm64/c/399306954996
[04/11] arm64: rsi: Add support for checking whether an MMIO is protected
        https://git.kernel.org/arm64/c/371589437616
[05/11] arm64: rsi: Map unprotected MMIO as decrypted
        https://git.kernel.org/arm64/c/3c6c70613956
[06/11] efi: arm64: Map Device with Prot Shared
        https://git.kernel.org/arm64/c/491db21d8256
[07/11] arm64: Enforce bounce buffers for realm DMA
        https://git.kernel.org/arm64/c/fbf979a01375
[08/11] arm64: mm: Avoid TLBI when marking pages as valid
        https://git.kernel.org/arm64/c/0e9cb5995b25
[09/11] arm64: Enable memory encrypt for Realms
        https://git.kernel.org/arm64/c/42be24a4178f
[10/11] virt: arm-cca-guest: TSM_REPORT support for realms
        https://git.kernel.org/arm64/c/7999edc484ca
[11/11] arm64: Document Arm Confidential Compute
        https://git.kernel.org/arm64/c/972d755f0195

-- 
Catalin

Re: [PATCH v7 10/11] virt: arm-cca-guest: TSM_REPORT support for realms

[Dan Williams]

Steven Price wrote:
> From: Sami Mujawar <sami.mujawar@arm.com>
> 
> Introduce an arm-cca-guest driver that registers with
> the configfs-tsm module to provide user interfaces for
> retrieving an attestation token.
> 
> When a new report is requested the arm-cca-guest driver
> invokes the appropriate RSI interfaces to query an
> attestation token.
> 
> The steps to retrieve an attestation token are as follows:
>   1. Mount the configfs filesystem if not already mounted
>      mount -t configfs none /sys/kernel/config
>   2. Generate an attestation token
>      report=/sys/kernel/config/tsm/report/report0
>      mkdir $report
>      dd if=/dev/urandom bs=64 count=1 > $report/inblob
>      hexdump -C $report/outblob
>      rmdir $report
> 
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> Signed-off-by: Steven Price <steven.price@arm.com>
> ---
> Changes since v6:
>  * Avoid get_cpu() and instead make the init attestation call using
>    smp_call_function_single(). Improve comments to explain the logic.
>  * Minor code reorgnisation and comment cleanup following Gavin's review
>    (thanks!)
> ---
>  drivers/virt/coco/Kconfig                     |   2 +
>  drivers/virt/coco/Makefile                    |   1 +
>  drivers/virt/coco/arm-cca-guest/Kconfig       |  11 +
>  drivers/virt/coco/arm-cca-guest/Makefile      |   2 +
>  .../virt/coco/arm-cca-guest/arm-cca-guest.c   | 224 ++++++++++++++++++
>  5 files changed, 240 insertions(+)
>  create mode 100644 drivers/virt/coco/arm-cca-guest/Kconfig
>  create mode 100644 drivers/virt/coco/arm-cca-guest/Makefile
>  create mode 100644 drivers/virt/coco/arm-cca-guest/arm-cca-guest.c
[..]
> diff --git a/drivers/virt/coco/arm-cca-guest/Kconfig b/drivers/virt/coco/arm-cca-guest/Kconfig
> new file mode 100644
> index 000000000000..9dd27c3ee215
> --- /dev/null
> +++ b/drivers/virt/coco/arm-cca-guest/Kconfig
> @@ -0,0 +1,11 @@
> +config ARM_CCA_GUEST
> +	tristate "Arm CCA Guest driver"
> +	depends on ARM64
> +	default m

I am working on some updates to the TSM_REPORTS interface, rebased them
to test the changes with this driver, and discovered that this driver is
enabled by default.

Just a reminder to please do not mark new drivers as "default m" [1]. In
this case it is difficult to imagine that every arm64 kernel on the
planet needs this functionality enabled by default. In general, someone
should be able to run olddefconfig with a new kernel and not be exposed
to brand new drivers that they have not considered previously.

[1]: http://lore.kernel.org/CA+55aFzxL6-Xp=-mnBwMisZsuKhRZ6zRDJoAmH8W5LDHU2oJuw@mail.gmail.com/

Re: [PATCH v7 10/11] virt: arm-cca-guest: TSM_REPORT support for realms

[Catalin Marinas]

On Wed, Dec 04, 2024 at 01:16:05PM -0800, Dan Williams wrote:
> Steven Price wrote:
> > diff --git a/drivers/virt/coco/arm-cca-guest/Kconfig b/drivers/virt/coco/arm-cca-guest/Kconfig
> > new file mode 100644
> > index 000000000000..9dd27c3ee215
> > --- /dev/null
> > +++ b/drivers/virt/coco/arm-cca-guest/Kconfig
> > @@ -0,0 +1,11 @@
> > +config ARM_CCA_GUEST
> > +	tristate "Arm CCA Guest driver"
> > +	depends on ARM64
> > +	default m
> 
> I am working on some updates to the TSM_REPORTS interface, rebased them
> to test the changes with this driver, and discovered that this driver is
> enabled by default.
> 
> Just a reminder to please do not mark new drivers as "default m" [1]. In
> this case it is difficult to imagine that every arm64 kernel on the
> planet needs this functionality enabled by default. In general, someone
> should be able to run olddefconfig with a new kernel and not be exposed
> to brand new drivers that they have not considered previously.
> 
> [1]: http://lore.kernel.org/CA+55aFzxL6-Xp=-mnBwMisZsuKhRZ6zRDJoAmH8W5LDHU2oJuw@mail.gmail.com/

Fair point, the pKVM driver is also default off. At least with the arm64
defconfig, VIRT_DRIVERS is default off, so this wouldn't be built. But
an olddefconfig will indeed enable it (this reminds me to add the coco
drivers to my test configs).

-- 
Catalin

Re: [PATCH v7 09/11] arm64: Enable memory encrypt for Realms

[Steven Price]

On 17/10/2024 14:14, Steven Price wrote:
> From: Suzuki K Poulose <suzuki.poulose@arm.com>
> 
> Use the memory encryption APIs to trigger a RSI call to request a
> transition between protected memory and shared memory (or vice versa)
> and updating the kernel's linear map of modified pages to flip the top
> bit of the IPA. This requires that block mappings are not used in the
> direct map for realm guests.
> 
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
> Reviewed-by: Gavin Shan <gshan@redhat.com>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> Co-developed-by: Steven Price <steven.price@arm.com>
> Signed-off-by: Steven Price <steven.price@arm.com>
> ---
[...]
> diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c
> index 547a9e0b46c2..6ae6ae806454 100644
> --- a/arch/arm64/mm/pageattr.c
> +++ b/arch/arm64/mm/pageattr.c
> @@ -5,10 +5,12 @@
>  #include <linux/kernel.h>
>  #include <linux/mm.h>
>  #include <linux/module.h>
> +#include <linux/mem_encrypt.h>
>  #include <linux/sched.h>
>  #include <linux/vmalloc.h>
>  
>  #include <asm/cacheflush.h>
> +#include <asm/pgtable-prot.h>
>  #include <asm/set_memory.h>
>  #include <asm/tlbflush.h>
>  #include <asm/kfence.h>
> @@ -23,14 +25,16 @@ bool rodata_full __ro_after_init = IS_ENABLED(CONFIG_RODATA_FULL_DEFAULT_ENABLED
>  bool can_set_direct_map(void)
>  {
>  	/*
> -	 * rodata_full and DEBUG_PAGEALLOC require linear map to be
> -	 * mapped at page granularity, so that it is possible to
> +	 * rodata_full, DEBUG_PAGEALLOC and a Realm guest all require linear
> +	 * map to be mapped at page granularity, so that it is possible to
>  	 * protect/unprotect single pages.
>  	 *
>  	 * KFENCE pool requires page-granular mapping if initialized late.
> +	 *
> +	 * Realms need to make pages shared/protected at page granularity.
>  	 */
>  	return rodata_full || debug_pagealloc_enabled() ||
> -	       arm64_kfence_can_set_direct_map();
> +		arm64_kfence_can_set_direct_map() || is_realm_world();
>  }

Aneesh pointed out that this call to is_realm_world() is now too early 
since the decision to delay the RSI detection. The upshot is that a 
realm guest which doesn't have page granularity forced for other reasons 
will fail to share pages with the host.

At the moment I can think of a couple of options:

(1) Make rodata_full a requirement for realm guests. 
    CONFIG_RODATA_FULL_DEFAULT_ENABLED is already "default y" so this 
    isn't a big ask.

(2) Revisit the idea of detecting when running as a realm guest early. 
    This has the advantage of also "fixing" earlycon (no need to 
    manually specify the shared-alias of an unprotected UART).

I'm currently leaning towards (1) because it's the default anyway. But 
if we're going to need to fix earlycon (or indeed find other similar 
issues) then (2) would obviously make sense.

Any thoughts on the best option here.

Untested patch for (1) below. Although updating the docs would be
probably be a good idea too ;)

Thanks,
Steve

----8<---
diff --git a/arch/arm64/kernel/rsi.c b/arch/arm64/kernel/rsi.c
index ce4778141ec7..48a6ef0f401c 100644
--- a/arch/arm64/kernel/rsi.c
+++ b/arch/arm64/kernel/rsi.c
@@ -126,6 +126,10 @@ void __init arm64_rsi_init(void)
 		return;
 	if (!rsi_version_matches())
 		return;
+	if (!can_set_direct_map()) {
+		pr_err("rodata_full disabled, unable to run as a realm guest. Please enable CONFIG_RODATA_FULL_DEFAULT_ENABLED\n");
+		return;
+	}
 	if (WARN_ON(rsi_get_realm_config(&config)))
 		return;
 	prot_ns_shared = BIT(config.ipa_bits - 1);
diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c
index 39fd1f7ff02a..f8fd8a3816fb 100644
--- a/arch/arm64/mm/pageattr.c
+++ b/arch/arm64/mm/pageattr.c
@@ -25,16 +25,14 @@ bool rodata_full __ro_after_init = IS_ENABLED(CONFIG_RODATA_FULL_DEFAULT_ENABLED
 bool can_set_direct_map(void)
 {
 	/*
-	 * rodata_full, DEBUG_PAGEALLOC and a Realm guest all require linear
-	 * map to be mapped at page granularity, so that it is possible to
+	 * rodata_full, DEBUG_PAGEALLOC require linear map to be
+	 * mapped at page granularity, so that it is possible to
 	 * protect/unprotect single pages.
 	 *
 	 * KFENCE pool requires page-granular mapping if initialized late.
-	 *
-	 * Realms need to make pages shared/protected at page granularity.
 	 */
 	return rodata_full || debug_pagealloc_enabled() ||
-		arm64_kfence_can_set_direct_map() || is_realm_world();
+		arm64_kfence_can_set_direct_map();
 }
 
 static int change_page_range(pte_t *ptep, unsigned long addr, void *data)

Re: [PATCH v7 09/11] arm64: Enable memory encrypt for Realms

[Catalin Marinas]

On Wed, Feb 19, 2025 at 02:30:28PM +0000, Steven Price wrote:
> On 17/10/2024 14:14, Steven Price wrote:
> > From: Suzuki K Poulose <suzuki.poulose@arm.com>
> > 
> > Use the memory encryption APIs to trigger a RSI call to request a
> > transition between protected memory and shared memory (or vice versa)
> > and updating the kernel's linear map of modified pages to flip the top
> > bit of the IPA. This requires that block mappings are not used in the
> > direct map for realm guests.
> > 
> > Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
> > Reviewed-by: Gavin Shan <gshan@redhat.com>
> > Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> > Co-developed-by: Steven Price <steven.price@arm.com>
> > Signed-off-by: Steven Price <steven.price@arm.com>
> > ---
> [...]
> > diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c
> > index 547a9e0b46c2..6ae6ae806454 100644
> > --- a/arch/arm64/mm/pageattr.c
> > +++ b/arch/arm64/mm/pageattr.c
> > @@ -5,10 +5,12 @@
> >  #include <linux/kernel.h>
> >  #include <linux/mm.h>
> >  #include <linux/module.h>
> > +#include <linux/mem_encrypt.h>
> >  #include <linux/sched.h>
> >  #include <linux/vmalloc.h>
> >  
> >  #include <asm/cacheflush.h>
> > +#include <asm/pgtable-prot.h>
> >  #include <asm/set_memory.h>
> >  #include <asm/tlbflush.h>
> >  #include <asm/kfence.h>
> > @@ -23,14 +25,16 @@ bool rodata_full __ro_after_init = IS_ENABLED(CONFIG_RODATA_FULL_DEFAULT_ENABLED
> >  bool can_set_direct_map(void)
> >  {
> >  	/*
> > -	 * rodata_full and DEBUG_PAGEALLOC require linear map to be
> > -	 * mapped at page granularity, so that it is possible to
> > +	 * rodata_full, DEBUG_PAGEALLOC and a Realm guest all require linear
> > +	 * map to be mapped at page granularity, so that it is possible to
> >  	 * protect/unprotect single pages.
> >  	 *
> >  	 * KFENCE pool requires page-granular mapping if initialized late.
> > +	 *
> > +	 * Realms need to make pages shared/protected at page granularity.
> >  	 */
> >  	return rodata_full || debug_pagealloc_enabled() ||
> > -	       arm64_kfence_can_set_direct_map();
> > +		arm64_kfence_can_set_direct_map() || is_realm_world();
> >  }
> 
> Aneesh pointed out that this call to is_realm_world() is now too early 
> since the decision to delay the RSI detection. The upshot is that a 
> realm guest which doesn't have page granularity forced for other reasons 
> will fail to share pages with the host.
> 
> At the moment I can think of a couple of options:
> 
> (1) Make rodata_full a requirement for realm guests. 
>     CONFIG_RODATA_FULL_DEFAULT_ENABLED is already "default y" so this 
>     isn't a big ask.
> 
> (2) Revisit the idea of detecting when running as a realm guest early. 
>     This has the advantage of also "fixing" earlycon (no need to 
>     manually specify the shared-alias of an unprotected UART).
> 
> I'm currently leaning towards (1) because it's the default anyway. But 
> if we're going to need to fix earlycon (or indeed find other similar 
> issues) then (2) would obviously make sense.

I'd go with (1) since the end result is the same even if we implemented
(2) - i.e. we still avoid block mappings in realms.

> diff --git a/arch/arm64/kernel/rsi.c b/arch/arm64/kernel/rsi.c
> index ce4778141ec7..48a6ef0f401c 100644
> --- a/arch/arm64/kernel/rsi.c
> +++ b/arch/arm64/kernel/rsi.c
> @@ -126,6 +126,10 @@ void __init arm64_rsi_init(void)
>  		return;
>  	if (!rsi_version_matches())
>  		return;
> +	if (!can_set_direct_map()) {
> +		pr_err("rodata_full disabled, unable to run as a realm guest. Please enable CONFIG_RODATA_FULL_DEFAULT_ENABLED\n");

It's a bit strange to complain about rodata since, in principle, it
doesn't have anything to do with realms. Its only side-effect is that we
avoid block kernel mappings. Maybe "cannot set the kernel direct map,
consider rodata=full" or something like that.

-- 
Catalin

Re: [PATCH v7 09/11] arm64: Enable memory encrypt for Realms

[Will Deacon]

On Wed, Feb 26, 2025 at 07:03:01PM +0000, Catalin Marinas wrote:
> On Wed, Feb 19, 2025 at 02:30:28PM +0000, Steven Price wrote:
> > > @@ -23,14 +25,16 @@ bool rodata_full __ro_after_init = IS_ENABLED(CONFIG_RODATA_FULL_DEFAULT_ENABLED
> > >  bool can_set_direct_map(void)
> > >  {
> > >  	/*
> > > -	 * rodata_full and DEBUG_PAGEALLOC require linear map to be
> > > -	 * mapped at page granularity, so that it is possible to
> > > +	 * rodata_full, DEBUG_PAGEALLOC and a Realm guest all require linear
> > > +	 * map to be mapped at page granularity, so that it is possible to
> > >  	 * protect/unprotect single pages.
> > >  	 *
> > >  	 * KFENCE pool requires page-granular mapping if initialized late.
> > > +	 *
> > > +	 * Realms need to make pages shared/protected at page granularity.
> > >  	 */
> > >  	return rodata_full || debug_pagealloc_enabled() ||
> > > -	       arm64_kfence_can_set_direct_map();
> > > +		arm64_kfence_can_set_direct_map() || is_realm_world();
> > >  }
> > 
> > Aneesh pointed out that this call to is_realm_world() is now too early 
> > since the decision to delay the RSI detection. The upshot is that a 
> > realm guest which doesn't have page granularity forced for other reasons 
> > will fail to share pages with the host.
> > 
> > At the moment I can think of a couple of options:
> > 
> > (1) Make rodata_full a requirement for realm guests. 
> >     CONFIG_RODATA_FULL_DEFAULT_ENABLED is already "default y" so this 
> >     isn't a big ask.
> > 
> > (2) Revisit the idea of detecting when running as a realm guest early. 
> >     This has the advantage of also "fixing" earlycon (no need to 
> >     manually specify the shared-alias of an unprotected UART).
> > 
> > I'm currently leaning towards (1) because it's the default anyway. But 
> > if we're going to need to fix earlycon (or indeed find other similar 
> > issues) then (2) would obviously make sense.
> 
> I'd go with (1) since the end result is the same even if we implemented
> (2) - i.e. we still avoid block mappings in realms.

Is it, though? The config option is about the default behaviour but there's
still an "rodata=" option on the command-line.

Will

Re: [PATCH v7 09/11] arm64: Enable memory encrypt for Realms

[Steven Price]

On 27/02/2025 00:23, Will Deacon wrote:
> On Wed, Feb 26, 2025 at 07:03:01PM +0000, Catalin Marinas wrote:
>> On Wed, Feb 19, 2025 at 02:30:28PM +0000, Steven Price wrote:
>>>> @@ -23,14 +25,16 @@ bool rodata_full __ro_after_init = IS_ENABLED(CONFIG_RODATA_FULL_DEFAULT_ENABLED
>>>>  bool can_set_direct_map(void)
>>>>  {
>>>>  	/*
>>>> -	 * rodata_full and DEBUG_PAGEALLOC require linear map to be
>>>> -	 * mapped at page granularity, so that it is possible to
>>>> +	 * rodata_full, DEBUG_PAGEALLOC and a Realm guest all require linear
>>>> +	 * map to be mapped at page granularity, so that it is possible to
>>>>  	 * protect/unprotect single pages.
>>>>  	 *
>>>>  	 * KFENCE pool requires page-granular mapping if initialized late.
>>>> +	 *
>>>> +	 * Realms need to make pages shared/protected at page granularity.
>>>>  	 */
>>>>  	return rodata_full || debug_pagealloc_enabled() ||
>>>> -	       arm64_kfence_can_set_direct_map();
>>>> +		arm64_kfence_can_set_direct_map() || is_realm_world();
>>>>  }
>>>
>>> Aneesh pointed out that this call to is_realm_world() is now too early 
>>> since the decision to delay the RSI detection. The upshot is that a 
>>> realm guest which doesn't have page granularity forced for other reasons 
>>> will fail to share pages with the host.
>>>
>>> At the moment I can think of a couple of options:
>>>
>>> (1) Make rodata_full a requirement for realm guests. 
>>>     CONFIG_RODATA_FULL_DEFAULT_ENABLED is already "default y" so this 
>>>     isn't a big ask.
>>>
>>> (2) Revisit the idea of detecting when running as a realm guest early. 
>>>     This has the advantage of also "fixing" earlycon (no need to 
>>>     manually specify the shared-alias of an unprotected UART).
>>>
>>> I'm currently leaning towards (1) because it's the default anyway. But 
>>> if we're going to need to fix earlycon (or indeed find other similar 
>>> issues) then (2) would obviously make sense.
>>
>> I'd go with (1) since the end result is the same even if we implemented
>> (2) - i.e. we still avoid block mappings in realms.
> 
> Is it, though? The config option is about the default behaviour but there's
> still an "rodata=" option on the command-line.

I think the question comes down to is there any value in having page
mappings and not setting the read-only permissions? I.e.
rodata_full=false but we're still avoiding block mappings.

(1) as I've currently proposed doesn't allow that combination - if you
disable rodata_full you also break realms (assuming
DEBUG_PAGEALLOC/kfence don't otherwise force can_set_direct_map().

(2) forces page mappings if there's an RMM present, but does allow
disabling the read-only permissions with "rodata=".

So I guess there's also another option:

(3) Provide another compile/command line flag which forces page mapping
which is different from rodata_full. That would then allow realms
without affecting the permissions.

or indeed:

(4) Change can_set_direct_map() to always return true! ;)

Thanks,
Steve

Re: [PATCH v7 09/11] arm64: Enable memory encrypt for Realms

[Catalin Marinas]

On Thu, Feb 27, 2025 at 12:23:31AM +0000, Will Deacon wrote:
> On Wed, Feb 26, 2025 at 07:03:01PM +0000, Catalin Marinas wrote:
> > On Wed, Feb 19, 2025 at 02:30:28PM +0000, Steven Price wrote:
> > > > @@ -23,14 +25,16 @@ bool rodata_full __ro_after_init = IS_ENABLED(CONFIG_RODATA_FULL_DEFAULT_ENABLED
> > > >  bool can_set_direct_map(void)
> > > >  {
> > > >  	/*
> > > > -	 * rodata_full and DEBUG_PAGEALLOC require linear map to be
> > > > -	 * mapped at page granularity, so that it is possible to
> > > > +	 * rodata_full, DEBUG_PAGEALLOC and a Realm guest all require linear
> > > > +	 * map to be mapped at page granularity, so that it is possible to
> > > >  	 * protect/unprotect single pages.
> > > >  	 *
> > > >  	 * KFENCE pool requires page-granular mapping if initialized late.
> > > > +	 *
> > > > +	 * Realms need to make pages shared/protected at page granularity.
> > > >  	 */
> > > >  	return rodata_full || debug_pagealloc_enabled() ||
> > > > -	       arm64_kfence_can_set_direct_map();
> > > > +		arm64_kfence_can_set_direct_map() || is_realm_world();
> > > >  }
> > > 
> > > Aneesh pointed out that this call to is_realm_world() is now too early 
> > > since the decision to delay the RSI detection. The upshot is that a 
> > > realm guest which doesn't have page granularity forced for other reasons 
> > > will fail to share pages with the host.
> > > 
> > > At the moment I can think of a couple of options:
> > > 
> > > (1) Make rodata_full a requirement for realm guests. 
> > >     CONFIG_RODATA_FULL_DEFAULT_ENABLED is already "default y" so this 
> > >     isn't a big ask.
> > > 
> > > (2) Revisit the idea of detecting when running as a realm guest early. 
> > >     This has the advantage of also "fixing" earlycon (no need to 
> > >     manually specify the shared-alias of an unprotected UART).
> > > 
> > > I'm currently leaning towards (1) because it's the default anyway. But 
> > > if we're going to need to fix earlycon (or indeed find other similar 
> > > issues) then (2) would obviously make sense.
> > 
> > I'd go with (1) since the end result is the same even if we implemented
> > (2) - i.e. we still avoid block mappings in realms.
> 
> Is it, though? The config option is about the default behaviour but there's
> still an "rodata=" option on the command-line.

Yeah, that's why I suggested the pr_err() to only state that it cannot
set the direct map and consider rodata=full rather than a config option.
We already force CONFIG_STRICT_KERNEL_RWX.

But we can also revisit the decision not to probe the RSI early.

-- 
Catalin

Re: [PATCH v7 09/11] arm64: Enable memory encrypt for Realms

[Will Deacon]

On Thu, Feb 27, 2025 at 10:55:00AM +0000, Catalin Marinas wrote:
> On Thu, Feb 27, 2025 at 12:23:31AM +0000, Will Deacon wrote:
> > On Wed, Feb 26, 2025 at 07:03:01PM +0000, Catalin Marinas wrote:
> > > On Wed, Feb 19, 2025 at 02:30:28PM +0000, Steven Price wrote:
> > > > > @@ -23,14 +25,16 @@ bool rodata_full __ro_after_init = IS_ENABLED(CONFIG_RODATA_FULL_DEFAULT_ENABLED
> > > > >  bool can_set_direct_map(void)
> > > > >  {
> > > > >  	/*
> > > > > -	 * rodata_full and DEBUG_PAGEALLOC require linear map to be
> > > > > -	 * mapped at page granularity, so that it is possible to
> > > > > +	 * rodata_full, DEBUG_PAGEALLOC and a Realm guest all require linear
> > > > > +	 * map to be mapped at page granularity, so that it is possible to
> > > > >  	 * protect/unprotect single pages.
> > > > >  	 *
> > > > >  	 * KFENCE pool requires page-granular mapping if initialized late.
> > > > > +	 *
> > > > > +	 * Realms need to make pages shared/protected at page granularity.
> > > > >  	 */
> > > > >  	return rodata_full || debug_pagealloc_enabled() ||
> > > > > -	       arm64_kfence_can_set_direct_map();
> > > > > +		arm64_kfence_can_set_direct_map() || is_realm_world();
> > > > >  }
> > > > 
> > > > Aneesh pointed out that this call to is_realm_world() is now too early 
> > > > since the decision to delay the RSI detection. The upshot is that a 
> > > > realm guest which doesn't have page granularity forced for other reasons 
> > > > will fail to share pages with the host.
> > > > 
> > > > At the moment I can think of a couple of options:
> > > > 
> > > > (1) Make rodata_full a requirement for realm guests. 
> > > >     CONFIG_RODATA_FULL_DEFAULT_ENABLED is already "default y" so this 
> > > >     isn't a big ask.
> > > > 
> > > > (2) Revisit the idea of detecting when running as a realm guest early. 
> > > >     This has the advantage of also "fixing" earlycon (no need to 
> > > >     manually specify the shared-alias of an unprotected UART).
> > > > 
> > > > I'm currently leaning towards (1) because it's the default anyway. But 
> > > > if we're going to need to fix earlycon (or indeed find other similar 
> > > > issues) then (2) would obviously make sense.
> > > 
> > > I'd go with (1) since the end result is the same even if we implemented
> > > (2) - i.e. we still avoid block mappings in realms.
> > 
> > Is it, though? The config option is about the default behaviour but there's
> > still an "rodata=" option on the command-line.
> 
> Yeah, that's why I suggested the pr_err() to only state that it cannot
> set the direct map and consider rodata=full rather than a config option.
> We already force CONFIG_STRICT_KERNEL_RWX.

rodata=full has absolutely nothing to do with realms, though. It just
happens to result in the linear map being created at page granularity
and I don't think we should expose that implementation detail like this.

> But we can also revisit the decision not to probe the RSI early.

Alternatively, could we predicate realm support on BBM level-3 w/o TLB
conflicts? Then we could crack the blocks in the linear map.

Will

Re: [PATCH v7 09/11] arm64: Enable memory encrypt for Realms

[Catalin Marinas]

On Thu, Feb 27, 2025 at 05:22:55PM +0000, Will Deacon wrote:
> On Thu, Feb 27, 2025 at 10:55:00AM +0000, Catalin Marinas wrote:
> > On Thu, Feb 27, 2025 at 12:23:31AM +0000, Will Deacon wrote:
> > > On Wed, Feb 26, 2025 at 07:03:01PM +0000, Catalin Marinas wrote:
> > > > On Wed, Feb 19, 2025 at 02:30:28PM +0000, Steven Price wrote:
> > > > > > @@ -23,14 +25,16 @@ bool rodata_full __ro_after_init = IS_ENABLED(CONFIG_RODATA_FULL_DEFAULT_ENABLED
> > > > > >  bool can_set_direct_map(void)
> > > > > >  {
> > > > > >  	/*
> > > > > > -	 * rodata_full and DEBUG_PAGEALLOC require linear map to be
> > > > > > -	 * mapped at page granularity, so that it is possible to
> > > > > > +	 * rodata_full, DEBUG_PAGEALLOC and a Realm guest all require linear
> > > > > > +	 * map to be mapped at page granularity, so that it is possible to
> > > > > >  	 * protect/unprotect single pages.
> > > > > >  	 *
> > > > > >  	 * KFENCE pool requires page-granular mapping if initialized late.
> > > > > > +	 *
> > > > > > +	 * Realms need to make pages shared/protected at page granularity.
> > > > > >  	 */
> > > > > >  	return rodata_full || debug_pagealloc_enabled() ||
> > > > > > -	       arm64_kfence_can_set_direct_map();
> > > > > > +		arm64_kfence_can_set_direct_map() || is_realm_world();
> > > > > >  }
> > > > > 
> > > > > Aneesh pointed out that this call to is_realm_world() is now too early 
> > > > > since the decision to delay the RSI detection. The upshot is that a 
> > > > > realm guest which doesn't have page granularity forced for other reasons 
> > > > > will fail to share pages with the host.
> > > > > 
> > > > > At the moment I can think of a couple of options:
> > > > > 
> > > > > (1) Make rodata_full a requirement for realm guests. 
> > > > >     CONFIG_RODATA_FULL_DEFAULT_ENABLED is already "default y" so this 
> > > > >     isn't a big ask.
> > > > > 
> > > > > (2) Revisit the idea of detecting when running as a realm guest early. 
> > > > >     This has the advantage of also "fixing" earlycon (no need to 
> > > > >     manually specify the shared-alias of an unprotected UART).
> > > > > 
> > > > > I'm currently leaning towards (1) because it's the default anyway. But 
> > > > > if we're going to need to fix earlycon (or indeed find other similar 
> > > > > issues) then (2) would obviously make sense.
> > > > 
> > > > I'd go with (1) since the end result is the same even if we implemented
> > > > (2) - i.e. we still avoid block mappings in realms.
> > > 
> > > Is it, though? The config option is about the default behaviour but there's
> > > still an "rodata=" option on the command-line.
> > 
> > Yeah, that's why I suggested the pr_err() to only state that it cannot
> > set the direct map and consider rodata=full rather than a config option.
> > We already force CONFIG_STRICT_KERNEL_RWX.
> 
> rodata=full has absolutely nothing to do with realms, though.

I fully agree, that's what I said a couple of emails earlier (towards
the end, not quoted above).

> It just
> happens to result in the linear map being created at page granularity
> and I don't think we should expose that implementation detail like this.

I wasn't keen on adding a new realms=on or whatever command line option,
so I suggested the lazy but confusing rodata=full.

> > But we can also revisit the decision not to probe the RSI early.
> 
> Alternatively, could we predicate realm support on BBM level-3 w/o TLB
> conflicts? Then we could crack the blocks in the linear map.

Long term, I agree that's a better option. It needs wiring up though,
with some care to handle page table allocation failures at run-time. I
think most callers already handle the return code from set_memory_*().

-- 
Catalin