Re: [PATCH v17 3/7] firmware: arm_rmm: Configure the RMM with the host's page size

[Gavin Shan]

Hi Kohei,

On 9/8/26 6:00 PM, Kohei Enju wrote:
> Hi Gavin,
>
> On 09/08 17:04, Gavin Shan wrote:
> > Hi Suzuki,
> >
> > On 9/7/26 7:59 PM, Suzuki K Poulose wrote:
> > > From: Steven Price <steven.price@xxxxxxx>
> > >
> > > RMM v2.0 brings the ability to set the RMM's granule size. Check the
> > > feature registers and configure the RMM so that it matches the host's
> > > page size. This means that operations can be done with a granularity
> > > equal to PAGE_SIZE.
> > >
> > > Signed-off-by: Steven Price <steven.price@xxxxxxx>
> > > Signed-off-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> > > ---
> > >    Changes since v15:
> > >     * Actually check the feature register for the host's page-size support.
> > >    Changes since v14:
> > >     * Move the implementation into drivers/firmware/arm_rmm.
> > >    Changes since v13:
> > >     * Moved out of KVM.
> > > ---
> > >    drivers/firmware/arm_rmm/rmi.c | 58 ++++++++++++++++++++++++++++++++++
> > >    include/linux/arm-rmi-cmds.h   | 17 ++++++++++
> > >    2 files changed, 75 insertions(+)
> > >
> > > diff --git a/drivers/firmware/arm_rmm/rmi.c b/drivers/firmware/arm_rmm/rmi.c
> > > index 008a783407b4e..76f91c145e1fd 100644
> > > --- a/drivers/firmware/arm_rmm/rmi.c
> > > +++ b/drivers/firmware/arm_rmm/rmi.c
> > > @@ -77,6 +77,60 @@ static int rmi_read_features(void)
> > >    	return 0;
> > >    }
> > > +static int rmi_configure(void)
> > > +{
> > > +	unsigned long granule_feature;
> > > +	unsigned long granule_size;
> > > +	int ret = 0;
> > > +	struct rmm_config *config;
> > > +
> > > +	switch (PAGE_SIZE) {
> > > +	case SZ_4K:
> > > +		granule_size = RMI_GRANULE_SIZE_4KB;
> > > +		granule_feature = RMI_FEATURE_REGISTER_1_RMI_GRAN_SZ_4KB;
> > > +		break;
> > > +	case SZ_16K:
> > > +		granule_size = RMI_GRANULE_SIZE_16KB;
> > > +		granule_feature = RMI_FEATURE_REGISTER_1_RMI_GRAN_SZ_16KB;
> > > +		break;
> > > +	case SZ_64K:
> > > +		granule_size = RMI_GRANULE_SIZE_64KB;
> > > +		granule_feature = RMI_FEATURE_REGISTER_1_RMI_GRAN_SZ_64KB;
> > > +		break;
> > > +	default:
> > > +		BUILD_BUG();
> > > +	}
> > > +
> > > +	if (!(rmi_feat_reg(1) & granule_feature)) {
> > > +		pr_err("RMM does not support %luKB granules\n",
> > > +		       PAGE_SIZE >> 10);
> > > +		return -ENXIO;
> > > +	}
> > > +
> > > +	config = (struct rmm_config *)get_zeroed_page(GFP_KERNEL);
> > > +	if (!config)
> > > +		return -ENOMEM;
> >
> > An error message is needed here.
> >
> > 	if (!config) {
> > 		pr_err("Unable to alloc RMM config memory\n");
> > 		return -ENOMEM;
> > 	}
>
> I largely agree with your suggestions, and they look reasonable to me.
>
> However, wouldn't this be redundant? Since __GFP_NOWARN is not set, the
> page allocator would normally emit an allocation failure warning with a
> stack trace anyway.
>
> I don't have a strong preference, but as per "14) Allocating memory" in
> the coding style, I believe it would be considered unnecessary.

You're correct that the error message is most likely redundant here and we
needn't it. However, it's notable the allocation failure warning and stack
trace isn't 100% raised when looking at mm/page_alloc.c::warn_alloc(). At least,
it depends on !__ratelimit(&nopage_rs) even it's less likely to happen. Similarly,
there are conditions to raise a warning and stack track in case of the injected
allocation error in lib/fault-inject.c::fail_dump().

Thanks,
Gavin

> Thanks,
> Kohei
>
> > > +
> > > +	config->rmi_granule_size = granule_size;
> > > +
> > > +	/*
> > > +	 * For now we set the tracking_region_size to 0 which is the only option
> > > +	 * for 4KB PAGE_SIZE (1GB for 4KB PAGE_SIZE, 32MB/512MB for 16KB/64KB).
> > > +	 * TODO: Support other tracking sizes via Kconfig option for other
> > > +	 * PAGE_SIZES
> > > +	 */
> > > +	config->tracking_region_size = 0;
> > > +
> > > +	ret = rmi_rmm_config_set(virt_to_phys(config));
> > > +	if (ret) {
> > > +		pr_err("RMM config set failed\n");
> > > +		ret = -EINVAL;
> > > +	}
> >
> > The error code from rmi_rmm_config_set() is indicative sometimes. Also, -ENXIO
> > would be more appropriate than -EINVAL?
> >
> > 	if (ret) {
> > 		pr_err("RMM config set failed (%d)\n", ret);
> > 		ret = -ENXIO;
> > 	}
> >
> > > +
> > > +	free_page((unsigned long)config);
> > > +	return ret;
> > > +}
> > > +
> > >    static int __init arm64_init_rmi(void)
> > >    {
> > >    	int ret;
> > > @@ -90,6 +144,10 @@ static int __init arm64_init_rmi(void)
> > >    	if (ret)
> > >    		return ret;
> > > +	ret = rmi_configure();
> > > +	if (ret)
> > > +		return ret;
> > > +
> > >    	return 0;
> > >    }
> > > diff --git a/include/linux/arm-rmi-cmds.h b/include/linux/arm-rmi-cmds.h
> > > index 2fb1f7f86d71e..9aa27697e2377 100644
> > > --- a/include/linux/arm-rmi-cmds.h
> > > +++ b/include/linux/arm-rmi-cmds.h
> > > @@ -12,6 +12,23 @@
> > >    unsigned long rmi_feat_reg(unsigned long id);
> > > +/**
> > > + * rmi_rmm_config_set() - Configure the RMM
> > > + * @cfg_ptr: PA of a struct rmm_config
> > > + *
> > > + * Sets configuration options on the RMM.
> > > + *
> > > + * Return: RMI return code
> > > + */
> > > +static inline int rmi_rmm_config_set(unsigned long cfg_ptr)
> > > +{
> > > +	struct arm_smccc_res res;
> > > +
> > > +	arm_smccc_1_1_invoke(SMC_RMI_RMM_CONFIG_SET, cfg_ptr, &res);
> > > +
> > > +	return res.a0;
> > > +}
> > > +
> >
> > rmi_rmm_config_set() is used for once by rmi.c::rmi_configure(), I would not expose
> > rmi_rmm_config_set() by combining the logic to rmi.c::rmi_configure().
> >
> > >    /**
> > >     * rmi_features() - Read feature register
> > >     * @index: Feature register index
> >
> > Thanks,
> > Gavin