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

[Suzuki K Poulose <suzuki.poulose@arm.com>]

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
>