Re: [PATCH 09/33] ACPI / MPAM: Parse the MPAM table

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Sat, 8 Nov 2025 18:54:05 +1000
Gavin Shan <gshan@redhat.com> wrote:

> Hi Ben,
> 
> On 11/7/25 10:34 PM, Ben Horgan wrote:
> > From: James Morse <james.morse@arm.com>
> > 
> > Add code to parse the arm64 specific MPAM table, looking up the cache
> > level from the PPTT and feeding the end result into the MPAM driver.
> > 
> > This happens in two stages. Platform devices are created first for the
> > MSC devices. Once the driver probes it calls acpi_mpam_parse_resources()
> > to discover the RIS entries the MSC contains.
> > 
> > For now the MPAM hook mpam_ris_create() is stubbed out, but will update
> > the MPAM driver with optional discovered data about the RIS entries.
> > 
> > CC: Carl Worth <carl@os.amperecomputing.com>
> > Link: https://developer.arm.com/documentation/den0065/3-0bet/?lang=en
> > Reviewed-by: Lorenzo Pieralisi <lpieralisi@kernel.org>
> > Tested-by: Fenghua Yu <fenghuay@nvidia.com>
> > Tested-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
> > Tested-by: Peter Newman <peternewman@google.com>
> > Signed-off-by: James Morse <james.morse@arm.com>
> > Signed-off-by: Ben Horgan <ben.horgan@arm.com>
> > ---
> > Changes since v3:
> > return irq from acpi_mpam_register_irq (Jonathan)
> > err -> len rename (Jonathan)
> > Move table initialisation after checking (Jonathan)
> > Add sanity checking in acpi_mpam_count_msc() (Jonathan)
> > ---
> >   arch/arm64/Kconfig          |   1 +
> >   drivers/acpi/arm64/Kconfig  |   3 +
> >   drivers/acpi/arm64/Makefile |   1 +
> >   drivers/acpi/arm64/mpam.c   | 403 ++++++++++++++++++++++++++++++++++++
> >   drivers/acpi/tables.c       |   2 +-
> >   include/linux/arm_mpam.h    |  47 +++++
> >   6 files changed, 456 insertions(+), 1 deletion(-)
> >   create mode 100644 drivers/acpi/arm64/mpam.c
> >   create mode 100644 include/linux/arm_mpam.h
> >   
> 
> With the following minor comments addressed:
> 
> Reviewed-by: Gavin Shan <gshan@redhat.com>
Just picking out one comment where I think your suggestion
isn't quite the right one.

Jonathan

> > diff --git a/drivers/acpi/arm64/mpam.c b/drivers/acpi/arm64/mpam.c
> > new file mode 100644
> > index 000000000000..c199944862ed
> > --- /dev/null
> > +++ b/drivers/acpi/arm64/mpam.c

> > +static int __init acpi_mpam_parse(void)
> > +{
> > +	char *table_end, *table_offset;
> > +	struct acpi_mpam_msc_node *tbl_msc;
> > +	struct platform_device *pdev;
> > +
> > +	if (acpi_disabled || !system_supports_mpam())
> > +		return 0;
> > +
> > +	struct acpi_table_header *table __free(acpi_put_table) =
> > +		acpi_get_table_ret(ACPI_SIG_MPAM, 0);
> > +
> > +	if (IS_ERR(table))
> > +		return 0;
> > +
> > +	if (table->revision < 1)
> > +		return 0;
> > +  
> 
> It's correct to return zero on IS_ERR(table) with an error message, but
> a message printed by pr_debug() may be worthywhile on "if (table->revison < 1)".
> 
> > +	table_offset = (char *)(table + 1);
> > +	table_end = (char *)table + table->length;
> > +
> > +	while (table_offset < table_end) {
> > +		tbl_msc = (struct acpi_mpam_msc_node *)table_offset;
> > +		table_offset += tbl_msc->length;
> > +
> > +		if (table_offset > table_end) {
> > +			pr_err("MSC entry overlaps end of ACPI table\n");
> > +			return -EINVAL;
> > +		}
> > +  
> 
> Would be:
> 
> 		if (table_offset + sizeof(*tbl_msc) > table_end)

I'm not seeing this one.  table_offset has already been moved on by
tbl_msc->length which should be bigger than sizeof(*tbl_msc).

Could add a check before reading tbl_msc->length that there is enough
there to do so. That to me would make sense (like the other case you point
at later).

Jonathan