diff mbox series

[v5,12/19] efi: arm64: Map Device with Prot Shared

Message ID 20240819131924.372366-13-steven.price@arm.com (mailing list archive)
State New, archived
Headers show
Series arm64: Support for running as a guest in Arm CCA | expand

Commit Message

Steven Price Aug. 19, 2024, 1:19 p.m. UTC
From: Suzuki K Poulose <suzuki.poulose@arm.com>

Device mappings need to be emualted by the VMM so must be mapped shared
with the host.

Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
Signed-off-by: Steven Price <steven.price@arm.com>
---
Changes since v4:
 * Reworked to use arm64_is_iomem_private() to decide whether the memory
   needs to be decrypted or not.
---
 arch/arm64/kernel/efi.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Comments

Catalin Marinas Aug. 26, 2024, 10:13 a.m. UTC | #1
On Mon, Aug 19, 2024 at 02:19:17PM +0100, Steven Price wrote:
> From: Suzuki K Poulose <suzuki.poulose@arm.com>
> 
> Device mappings need to be emualted by the VMM so must be mapped shared
> with the host.
> 
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> Signed-off-by: Steven Price <steven.price@arm.com>
> ---
> Changes since v4:
>  * Reworked to use arm64_is_iomem_private() to decide whether the memory
>    needs to be decrypted or not.
> ---
>  arch/arm64/kernel/efi.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> index 712718aed5dd..95f8e8bf07f8 100644
> --- a/arch/arm64/kernel/efi.c
> +++ b/arch/arm64/kernel/efi.c
> @@ -34,8 +34,16 @@ static __init pteval_t create_mapping_protection(efi_memory_desc_t *md)
>  	u64 attr = md->attribute;
>  	u32 type = md->type;
>  
> -	if (type == EFI_MEMORY_MAPPED_IO)
> -		return PROT_DEVICE_nGnRE;
> +	if (type == EFI_MEMORY_MAPPED_IO) {
> +		pgprot_t prot = __pgprot(PROT_DEVICE_nGnRE);
> +
> +		if (arm64_is_iomem_private(md->phys_addr,
> +					   md->num_pages << EFI_PAGE_SHIFT))
> +			prot = pgprot_encrypted(prot);
> +		else
> +			prot = pgprot_decrypted(prot);
> +		return pgprot_val(prot);

Nit: This pattern appears in the previous patch as well. Maybe add a
pgprot_maybe_decrypted().

The patch looks fine other than the need for an early initialisation if
we find any workaround. In the pKVM case, IIUC this would need to call
into the hypervisor as well but that can be handled by the bootloader.
For CCA, our problem is setting the top bit of the IPA.

What's the x86 approach here? The EFI is a bigger problem than the
earlycon.
Matias Ezequiel Vara Larsen Sept. 9, 2024, 1:55 p.m. UTC | #2
On Mon, Aug 19, 2024 at 02:19:17PM +0100, Steven Price wrote:
> From: Suzuki K Poulose <suzuki.poulose@arm.com>
> 
> Device mappings need to be emualted by the VMM so must be mapped shared
> with the host.
> 
Typo. s/emualted/emulated 

Matias
Gavin Shan Sept. 10, 2024, 4:15 a.m. UTC | #3
On 8/19/24 11:19 PM, Steven Price wrote:
> From: Suzuki K Poulose <suzuki.poulose@arm.com>
> 
> Device mappings need to be emualted by the VMM so must be mapped shared
> with the host.
> 
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> Signed-off-by: Steven Price <steven.price@arm.com>
> ---
> Changes since v4:
>   * Reworked to use arm64_is_iomem_private() to decide whether the memory
>     needs to be decrypted or not.
> ---
>   arch/arm64/kernel/efi.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> index 712718aed5dd..95f8e8bf07f8 100644
> --- a/arch/arm64/kernel/efi.c
> +++ b/arch/arm64/kernel/efi.c
> @@ -34,8 +34,16 @@ static __init pteval_t create_mapping_protection(efi_memory_desc_t *md)
>   	u64 attr = md->attribute;
>   	u32 type = md->type;
>   
> -	if (type == EFI_MEMORY_MAPPED_IO)
> -		return PROT_DEVICE_nGnRE;
> +	if (type == EFI_MEMORY_MAPPED_IO) {
> +		pgprot_t prot = __pgprot(PROT_DEVICE_nGnRE);
> +
> +		if (arm64_is_iomem_private(md->phys_addr,
> +					   md->num_pages << EFI_PAGE_SHIFT))
> +			prot = pgprot_encrypted(prot);
> +		else
> +			prot = pgprot_decrypted(prot);
> +		return pgprot_val(prot);
> +	}
>   

Question: the second parameter (@size) passed to arm64_is_iomem_private() covers the
whole region. In [PATCH v5 10/19] arm64: Override set_fixmap_io, the size has been
truncated to the granule size (4KB). They look inconsistent and I don't understand
the reason.

Thanks,
Gavin
Suzuki K Poulose Sept. 10, 2024, 9:15 a.m. UTC | #4
On 10/09/2024 05:15, Gavin Shan wrote:
> On 8/19/24 11:19 PM, Steven Price wrote:
>> From: Suzuki K Poulose <suzuki.poulose@arm.com>
>>
>> Device mappings need to be emualted by the VMM so must be mapped shared
>> with the host.
>>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> Signed-off-by: Steven Price <steven.price@arm.com>
>> ---
>> Changes since v4:
>>   * Reworked to use arm64_is_iomem_private() to decide whether the memory
>>     needs to be decrypted or not.
>> ---
>>   arch/arm64/kernel/efi.c | 12 ++++++++++--
>>   1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
>> index 712718aed5dd..95f8e8bf07f8 100644
>> --- a/arch/arm64/kernel/efi.c
>> +++ b/arch/arm64/kernel/efi.c
>> @@ -34,8 +34,16 @@ static __init pteval_t 
>> create_mapping_protection(efi_memory_desc_t *md)
>>       u64 attr = md->attribute;
>>       u32 type = md->type;
>> -    if (type == EFI_MEMORY_MAPPED_IO)
>> -        return PROT_DEVICE_nGnRE;
>> +    if (type == EFI_MEMORY_MAPPED_IO) {
>> +        pgprot_t prot = __pgprot(PROT_DEVICE_nGnRE);
>> +
>> +        if (arm64_is_iomem_private(md->phys_addr,
>> +                       md->num_pages << EFI_PAGE_SHIFT))
>> +            prot = pgprot_encrypted(prot);
>> +        else
>> +            prot = pgprot_decrypted(prot);
>> +        return pgprot_val(prot);
>> +    }
> 
> Question: the second parameter (@size) passed to 
> arm64_is_iomem_private() covers the
> whole region. In [PATCH v5 10/19] arm64: Override set_fixmap_io, the 
> size has been
> truncated to the granule size (4KB). They look inconsistent and I don't 
> understand
> the reason.

Agreed, and the comment in patches 09/19 and 10/19 kind of vaguely 
explains this. For set_fixmap_io, we are trying to map a PAGE_SIZE, 
always. And when we want to check the "Is the range Private ?" the
answer could be different based on the PAGE_SIZE used by the kernel.
This is due to the fact that RMM always tracks the RIPAS for a 4K
granule and not the OS page size. So, if the kernel uses a 64K page
size, the RMM cannot confirm that the region is entirely RIPAS_DEV
if only the 4K granule is indeed marked as RIPAS_DEV. However, given
the same driver works for a 4K page size kernel, we can safely assume
that the driver doesn't access it beyond the 4K range  with FIXMAP.

e.g:

Addr  = 0x100000        +0x2000              0x110000
RIPAS = [ EMPTY | EMPTY | DEV | EMPTY | ...] [EMPTY | ...]

So, if we were to check the RIPAS of 0x102000, we have to restrict the
check to 4K (for the FIXMAP). Elswhere, we get the exact size of the
requested map region and can use that to check the state.

I agree that we should have a comment explaining this in the appropriate
patch.

Kind regards
Suzuki


> 
> Thanks,
> Gavin
>
diff mbox series

Patch

diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
index 712718aed5dd..95f8e8bf07f8 100644
--- a/arch/arm64/kernel/efi.c
+++ b/arch/arm64/kernel/efi.c
@@ -34,8 +34,16 @@  static __init pteval_t create_mapping_protection(efi_memory_desc_t *md)
 	u64 attr = md->attribute;
 	u32 type = md->type;
 
-	if (type == EFI_MEMORY_MAPPED_IO)
-		return PROT_DEVICE_nGnRE;
+	if (type == EFI_MEMORY_MAPPED_IO) {
+		pgprot_t prot = __pgprot(PROT_DEVICE_nGnRE);
+
+		if (arm64_is_iomem_private(md->phys_addr,
+					   md->num_pages << EFI_PAGE_SHIFT))
+			prot = pgprot_encrypted(prot);
+		else
+			prot = pgprot_decrypted(prot);
+		return pgprot_val(prot);
+	}
 
 	if (region_is_misaligned(md)) {
 		static bool __initdata code_is_misaligned;