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

[Marc Zyngier <maz@kernel.org>]

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.

	M.

-- 
Without deviation from the norm, progress is not possible.