Re: [PATCH v4 3/3] coco: guest: arm64: Query host IPA-change alignment via RHI

[Aneesh Kumar K.V <aneesh.kumar@kernel.org>]

Marc Zyngier <maz@kernel.org> writes:

> On Tue, 28 Apr 2026 13:49:46 +0100,
> Aneesh Kumar K.V <aneesh.kumar@kernel.org> wrote:
>> 
>> Marc Zyngier <maz@kernel.org> writes:
>> 
>> > On Mon, 27 Apr 2026 07:31:08 +0100,
>> > "Aneesh Kumar K.V (Arm)" <aneesh.kumar@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