[PATCH v4 3/3] coco: guest: arm64: Query host IPA-change alignment via RHI
Aneesh Kumar K.V
aneesh.kumar at kernel.org
Wed Apr 29 02:01:23 PDT 2026
Marc Zyngier <maz at kernel.org> writes:
> On Tue, 28 Apr 2026 13:49:46 +0100,
> Aneesh Kumar K.V <aneesh.kumar at kernel.org> wrote:
>>
>> Marc Zyngier <maz at kernel.org> writes:
>>
>> > On Mon, 27 Apr 2026 07:31:08 +0100,
>> > "Aneesh Kumar K.V (Arm)" <aneesh.kumar at kernel.org> wrote:
>> >>
>> >> Add the Realm Host Interface support needed to query host configuration
>> >> from a Realm guest. Define the RHI hostconf SMCs, add rsi_host_call(), and
>> >> use them during Realm initialization to retrieve the host IPA-change
>> >> alignment size.
>> >
>> > I don't understand what "IPA-change" means. What you are after is the
>> > host's sharing granule size.
>> >
>>
>> This is part of the RHI specification, and the call is named
>> RHI_HOSTCONF_GET_IPA_CHANGE_ALIGNMENT. The intent is to determine the
>> alignment requirements for changing IPA attributes (protected vs.
>> unprotected IPA
>
> This really is a terrible name. Why the 'change' part? It doesn't
> change, it is a constant.
>
> Oh well...
>
> [...]
>
>> >> +static inline unsigned long rsi_host_call(struct rsi_host_call *rhi_call)
>> >> +{
>> >> + phys_addr_t addr = virt_to_phys(rhi_call);
>> >> + struct arm_smccc_res res;
>> >> +
>> >> + arm_smccc_1_1_invoke(SMC_RSI_HOST_CALL, addr, &res);
>> >
>> > Errr... What guarantees that *rhi_call is *IPA contiguous*? This is
>> > incredibly fragile. You should at the very least check that this isn't
>> > vmalloc'd.
>> >
>>
>>
>> I didn’t quite follow that. We have other RSI calls (even RMI calls)
>> that do similar things, and the caller understands that the address
>> should be IPA-contiguous.
>
> Does it? Where is it documented? All you get is a pointer, so all
> bets are off.
>
>> Are you suggesting that all RSI calls should
>> add checks for this?. or are you suggesting to update the API to
>>
>> unsigned long rsi_host_call(unsigned long rhi_call_phys) ?
>
> I'm suggesting that this API is subtly broken because it makes random
> assumption about the physical contiguity of the VA space. It does so
> without any check, without any documentation.
>
> Simply changing the parameter to phys_addr_t could at the very least
> capture some of the requirements, but I'd like something in big bold
> letters.
>
>>
>> >> +
>> >> + return res.a0;
>> >> +}
>> >> +
>> >> #endif /* __ASM_RSI_CMDS_H */
>> >> diff --git a/arch/arm64/include/asm/rsi_smc.h b/arch/arm64/include/asm/rsi_smc.h
>> >> index e19253f96c94..9ee8b5c7612e 100644
>> >> --- a/arch/arm64/include/asm/rsi_smc.h
>> >> +++ b/arch/arm64/include/asm/rsi_smc.h
>> >> @@ -182,6 +182,13 @@ struct realm_config {
>> >> */
>> >> #define SMC_RSI_IPA_STATE_GET SMC_RSI_FID(0x198)
>> >>
>> >> +struct rsi_host_call {
>> >> + union {
>> >> + u16 imm;
>> >> + u64 padding0;
>> >> + };
>> >> + u64 gprs[31];
>> >> +} __aligned(0x100);
>> >> /*
>> >> * Make a Host call.
>> >> *
>> >> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
>> >> index fe627100d199..3e72dd9584ed 100644
>> >> --- a/arch/arm64/kernel/Makefile
>> >> +++ b/arch/arm64/kernel/Makefile
>> >> @@ -34,7 +34,7 @@ obj-y := debug-monitors.o entry.o irq.o fpsimd.o \
>> >> cpufeature.o alternative.o cacheinfo.o \
>> >> smp.o smp_spin_table.o topology.o smccc-call.o \
>> >> syscall.o proton-pack.o idle.o patching.o pi/ \
>> >> - rsi.o jump_label.o
>> >> + rsi.o jump_label.o rhi.o
>> >>
>> >> obj-$(CONFIG_COMPAT) += sys32.o signal32.o \
>> >> sys_compat.o
>> >> diff --git a/arch/arm64/kernel/rhi.c b/arch/arm64/kernel/rhi.c
>> >> new file mode 100644
>> >> index 000000000000..7cd6c5102464
>> >> --- /dev/null
>> >> +++ b/arch/arm64/kernel/rhi.c
>> >> @@ -0,0 +1,54 @@
>> >> +// SPDX-License-Identifier: GPL-2.0-only
>> >> +/*
>> >> + * Copyright (C) 2026 ARM Ltd.
>> >> + */
>> >> +
>> >> +#include <linux/mm.h>
>> >> +#include <asm/rsi.h>
>> >> +#include <asm/rhi.h>
>> >> +
>> >> +/* we need an aligned rhicall for rsi_host_call. slab is not yet ready */
>> >> +static struct rsi_host_call hyp_pagesize_rhicall;
>> >
>> > Why the "hyp_" prefix? This has absolutely nothing to with the
>> > hypervisor.
>> >
>>
>> Sure will update "hyp_" reference to host.
>>
>>
>> >> +unsigned long rhi_get_ipa_change_alignment(void)
>> >> +{
>> >> + long ret;
>> >> + unsigned long ipa_change_align;
>> >> +
>> >> + hyp_pagesize_rhicall.imm = 0;
>> >> + hyp_pagesize_rhicall.gprs[0] = RHI_HOSTCONF_VERSION;
>> >> + ret = rsi_host_call(lm_alias(&hyp_pagesize_rhicall));
>> >> + if (ret != RSI_SUCCESS)
>> >> + goto err_out;
>> >> +
>> >> + if (hyp_pagesize_rhicall.gprs[0] != RHI_HOSTCONF_VER_1_0)
>> >> + goto err_out;
>> >> +
>> >> + hyp_pagesize_rhicall.imm = 0;
>> >> + hyp_pagesize_rhicall.gprs[0] = RHI_HOSTCONF_FEATURES;
>> >> + ret = rsi_host_call(lm_alias(&hyp_pagesize_rhicall));
>> >> + if (ret != RSI_SUCCESS)
>> >> + goto err_out;
>> >> +
>> >> + if (!(hyp_pagesize_rhicall.gprs[0] & __RHI_HOSTCONF_GET_IPA_CHANGE_ALIGNMENT))
>> >> + goto err_out;
>> >> +
>> >> + hyp_pagesize_rhicall.imm = 0;
>> >> + hyp_pagesize_rhicall.gprs[0] = RHI_HOSTCONF_GET_IPA_CHANGE_ALIGNMENT;
>> >> + ret = rsi_host_call(lm_alias(&hyp_pagesize_rhicall));
>> >> + if (ret != RSI_SUCCESS)
>> >> + goto err_out;
>> >> +
>> >> + ipa_change_align = hyp_pagesize_rhicall.gprs[0];
>> >> + /* This error needs special handling in the caller */
>> >> + if (ipa_change_align & (SZ_4K - 1))
>> >> + return 0;
>> >> +
>> >> + return ipa_change_align;
>> >> +
>> >> +err_out:
>> >> + /*
>> >> + * For failure condition assume host is built with 4K page size
>> >> + * and hence ipa change alignment can be guest PAGE_SIZE.
>> >> + */
>> >> + return PAGE_SIZE;
>> >> +}
>> >
>> > Why can't this be part of rsi.c? This is an RSI call, and it should be
>> > part of the RSI initialisation.
>> >
>>
>> This is an RHI call as per the specification, hence it has been added to
>> rhi.c.
>
> News flash: this is the Linux kernel, not an ARM spec. We organise
> things based on the logical use, not on the TLA associated with it.
>
> And RHI is implemented in terms of RSI. In rsi.c it goes. We don't
> need this pointless proliferation of helper files that only result in
> equally pointless global symbols.
>
>>
>> >> diff --git a/arch/arm64/kernel/rsi.c b/arch/arm64/kernel/rsi.c
>> >> index 9e846ce4ef9c..ff735c04e236 100644
>> >> --- a/arch/arm64/kernel/rsi.c
>> >> +++ b/arch/arm64/kernel/rsi.c
>> >> @@ -14,8 +14,10 @@
>> >> #include <asm/mem_encrypt.h>
>> >> #include <asm/pgtable.h>
>> >> #include <asm/rsi.h>
>> >> +#include <asm/rhi.h>
>> >>
>> >> static struct realm_config config;
>> >> +static unsigned long ipa_change_alignment = PAGE_SIZE;
>> >>
>> >> unsigned long prot_ns_shared;
>> >> EXPORT_SYMBOL(prot_ns_shared);
>> >> @@ -139,6 +141,11 @@ static int realm_ioremap_hook(phys_addr_t phys, size_t size, pgprot_t *prot)
>> >> return 0;
>> >> }
>> >>
>> >> +unsigned long realm_get_hyp_pagesize(void)
>> >> +{
>> >> + return ipa_change_alignment;
>> >> +}
>> >
>> > Again, this has nothing to do with the hypervisor, but the host. And
>> > ipa_change_alignment is still a wording I can't wrap my small head
>> > around.
>> >
>> >> +
>> >> void __init arm64_rsi_init(void)
>> >> {
>> >> if (arm_smccc_1_1_get_conduit() != SMCCC_CONDUIT_SMC)
>> >> @@ -147,6 +154,12 @@ void __init arm64_rsi_init(void)
>> >> return;
>> >> if (WARN_ON(rsi_get_realm_config(&config)))
>> >> return;
>> >> +
>> >> + ipa_change_alignment = rhi_get_ipa_change_alignment();
>> >> + /* If we don't get a correct alignment response, don't enable realm */
>> >> + if (!ipa_change_alignment)
>> >> + return;
>> >
>> > But at the same time, you override a global value with an error, and
>> > then paper over it in mem_decrypt_granule_size()...
>> >
>>
>>
>> I believe I received similar feedback on my previous version as well,
>> which I didn’t quite follow.
>
> And you didn't think of asking? Sometimes I wonder what these patch
> reviews are for... Just to waste some more electrons, I guess :-/.
>
>>
>> rhi_get_ipa_change_alignment() only returns an error when the host
>> returns a size that is not 4K-aligned. Otherwise, it returns the
>> host-determined size, or defaults to guest PAGE_SIZE if the RHI call
>> itself is not supported.
>
> You encode the error as 0. You override ipa_change_alignment with 0.
>
> Then...
>
>> >> +size_t mem_decrypt_granule_size(void)
>> >> +{
>> >> + if (is_realm_world())
>> >> + return max(PAGE_SIZE, realm_get_hyp_pagesize());
>> >
>> > If you didn't mess with ipa_change_alignment above, you shouldn't need
>> > this max().
>> >
>>
>> size_t mem_decrypt_granule_size(void)
>> {
>> if (is_realm_world())
>> return max(PAGE_SIZE, realm_get_hyp_pagesize());
>> return PAGE_SIZE;
>> }
>>
>> That needs to use max(), because we should align to the guest PAGE_SIZE
>> if it is larger than the host-specified alignment value.
>
> ... you need to correct that back to PAGE_SIZE because you have stored
> something smaller than PAGE_SIZE.
>
> Isn't the problem really obvious? ipa_change_alignment can *NEVER* go
> down. It should never be allowed to reduce, because that's exactly
> the property you are trying to enforce.
>
Sure, I will update rhi_get_ipa_change_alignment() to always return the
max value.
-aneesh
More information about the linux-arm-kernel
mailing list