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

Jonathan Cameron
2025-11-10T16:27:40Z

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