| Message ID | 20241004144307.66199-6-steven.price@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: Support for running as a guest in Arm CCA | expand |
On 10/5/24 12:43 AM, Steven Price wrote: > From: Suzuki K Poulose <suzuki.poulose@arm.com> > > Instead of marking every MMIO as shared, check if the given region is > "Protected" and apply the permissions accordingly. > > Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> > Signed-off-by: Steven Price <steven.price@arm.com> > --- > New patch for v5 > --- > arch/arm64/kernel/rsi.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > With the following potential issue addressed: Reviewed-by: Gavin Shan <gshan@redhat.com> > diff --git a/arch/arm64/kernel/rsi.c b/arch/arm64/kernel/rsi.c > index d7bba4cee627..f1add76f89ce 100644 > --- a/arch/arm64/kernel/rsi.c > +++ b/arch/arm64/kernel/rsi.c > @@ -6,6 +6,8 @@ > #include <linux/jump_label.h> > #include <linux/memblock.h> > #include <linux/psci.h> > + > +#include <asm/io.h> > #include <asm/rsi.h> > > struct realm_config config; > @@ -92,6 +94,16 @@ bool arm64_is_protected_mmio(phys_addr_t base, size_t size) > } > EXPORT_SYMBOL(arm64_is_protected_mmio); > > +static int realm_ioremap_hook(phys_addr_t phys, size_t size, pgprot_t *prot) > +{ > + if (arm64_is_protected_mmio(phys, size)) > + *prot = pgprot_encrypted(*prot); > + else > + *prot = pgprot_decrypted(*prot); > + > + return 0; > +} > + We probably need arm64_is_mmio_private() here, meaning arm64_is_protected_mmio() isn't sufficient to avoid invoking SMCCC call SMC_RSI_IPA_STATE_GET in a regular guest where realm capability isn't present. > void __init arm64_rsi_init(void) > { > if (arm_smccc_1_1_get_conduit() != SMCCC_CONDUIT_SMC) > @@ -102,6 +114,9 @@ void __init arm64_rsi_init(void) > return; > prot_ns_shared = BIT(config.ipa_bits - 1); > > + if (arm64_ioremap_prot_hook_register(realm_ioremap_hook)) > + return; > + > arm64_rsi_setup_memory(); > > static_branch_enable(&rsi_present); Thanks, Gavin
On Tue, Oct 08, 2024 at 10:31:06AM +1000, Gavin Shan wrote: > On 10/5/24 12:43 AM, Steven Price wrote: > > diff --git a/arch/arm64/kernel/rsi.c b/arch/arm64/kernel/rsi.c > > index d7bba4cee627..f1add76f89ce 100644 > > --- a/arch/arm64/kernel/rsi.c > > +++ b/arch/arm64/kernel/rsi.c > > @@ -6,6 +6,8 @@ > > #include <linux/jump_label.h> > > #include <linux/memblock.h> > > #include <linux/psci.h> > > + > > +#include <asm/io.h> > > #include <asm/rsi.h> > > struct realm_config config; > > @@ -92,6 +94,16 @@ bool arm64_is_protected_mmio(phys_addr_t base, size_t size) > > } > > EXPORT_SYMBOL(arm64_is_protected_mmio); > > +static int realm_ioremap_hook(phys_addr_t phys, size_t size, pgprot_t *prot) > > +{ > > + if (arm64_is_protected_mmio(phys, size)) > > + *prot = pgprot_encrypted(*prot); > > + else > > + *prot = pgprot_decrypted(*prot); > > + > > + return 0; > > +} > > + > > We probably need arm64_is_mmio_private() here, meaning arm64_is_protected_mmio() isn't > sufficient to avoid invoking SMCCC call SMC_RSI_IPA_STATE_GET in a regular guest where > realm capability isn't present. I think we get away with this since the hook won't be registered in a normal guest (done from arm64_rsi_init()). So the additional check in arm64_is_mmio_private() is unnecessary.
On Fri, Oct 04, 2024 at 03:43:00PM +0100, Steven Price wrote: > From: Suzuki K Poulose <suzuki.poulose@arm.com> > > Instead of marking every MMIO as shared, check if the given region is > "Protected" and apply the permissions accordingly. > > Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> > Signed-off-by: Steven Price <steven.price@arm.com> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
On 10/11/24 11:19 PM, Catalin Marinas wrote: > On Tue, Oct 08, 2024 at 10:31:06AM +1000, Gavin Shan wrote: >> On 10/5/24 12:43 AM, Steven Price wrote: >>> diff --git a/arch/arm64/kernel/rsi.c b/arch/arm64/kernel/rsi.c >>> index d7bba4cee627..f1add76f89ce 100644 >>> --- a/arch/arm64/kernel/rsi.c >>> +++ b/arch/arm64/kernel/rsi.c >>> @@ -6,6 +6,8 @@ >>> #include <linux/jump_label.h> >>> #include <linux/memblock.h> >>> #include <linux/psci.h> >>> + >>> +#include <asm/io.h> >>> #include <asm/rsi.h> >>> struct realm_config config; >>> @@ -92,6 +94,16 @@ bool arm64_is_protected_mmio(phys_addr_t base, size_t size) >>> } >>> EXPORT_SYMBOL(arm64_is_protected_mmio); >>> +static int realm_ioremap_hook(phys_addr_t phys, size_t size, pgprot_t *prot) >>> +{ >>> + if (arm64_is_protected_mmio(phys, size)) >>> + *prot = pgprot_encrypted(*prot); >>> + else >>> + *prot = pgprot_decrypted(*prot); >>> + >>> + return 0; >>> +} >>> + >> >> We probably need arm64_is_mmio_private() here, meaning arm64_is_protected_mmio() isn't >> sufficient to avoid invoking SMCCC call SMC_RSI_IPA_STATE_GET in a regular guest where >> realm capability isn't present. > > I think we get away with this since the hook won't be registered in a > normal guest (done from arm64_rsi_init()). So the additional check in > arm64_is_mmio_private() is unnecessary. > Indeed. I missed the point that the hook won't be registered for a normal guest. So we're good and safe. Thanks, Gavin
diff --git a/arch/arm64/kernel/rsi.c b/arch/arm64/kernel/rsi.c index d7bba4cee627..f1add76f89ce 100644 --- a/arch/arm64/kernel/rsi.c +++ b/arch/arm64/kernel/rsi.c @@ -6,6 +6,8 @@ #include <linux/jump_label.h> #include <linux/memblock.h> #include <linux/psci.h> + +#include <asm/io.h> #include <asm/rsi.h> struct realm_config config; @@ -92,6 +94,16 @@ bool arm64_is_protected_mmio(phys_addr_t base, size_t size) } EXPORT_SYMBOL(arm64_is_protected_mmio); +static int realm_ioremap_hook(phys_addr_t phys, size_t size, pgprot_t *prot) +{ + if (arm64_is_protected_mmio(phys, size)) + *prot = pgprot_encrypted(*prot); + else + *prot = pgprot_decrypted(*prot); + + return 0; +} + void __init arm64_rsi_init(void) { if (arm_smccc_1_1_get_conduit() != SMCCC_CONDUIT_SMC) @@ -102,6 +114,9 @@ void __init arm64_rsi_init(void) return; prot_ns_shared = BIT(config.ipa_bits - 1); + if (arm64_ioremap_prot_hook_register(realm_ioremap_hook)) + return; + arm64_rsi_setup_memory(); static_branch_enable(&rsi_present);