| Message ID | 20240819131924.372366-13-steven.price@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: Support for running as a guest in Arm CCA | expand |
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.
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
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
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 --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;