Message-ID: <0a96d2d9-e548-4362-ae23-856d1d44548b@arm.com>
Date: Fri, 17 Oct 2025 19:50:56 +0100
From: James Morse <james.morse@....com>
To: Jonathan Cameron <jonathan.cameron@...wei.com>,
 Lorenzo Pieralisi <lpieralisi@...nel.org>, Hanjun Guo <guohanjun@...wei.com>
Cc: linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-acpi@...r.kernel.org,
 D Scott Phillips OS <scott@...amperecomputing.com>,
 carl@...amperecomputing.com, lcherian@...vell.com,
 bobo.shaobowang@...wei.com, tan.shaopeng@...itsu.com,
 baolin.wang@...ux.alibaba.com, Jamie Iles <quic_jiles@...cinc.com>,
 Xin Hao <xhao@...ux.alibaba.com>, peternewman@...gle.com,
 dfustini@...libre.com, amitsinght@...vell.com,
 David Hildenbrand <david@...hat.com>, Dave Martin <dave.martin@....com>,
 Koba Ko <kobak@...dia.com>, Shanker Donthineni <sdonthineni@...dia.com>,
 fenghuay@...dia.com, baisheng.gao@...soc.com, Rob Herring <robh@...nel.org>,
 Rohit Mathew <rohit.mathew@....com>, Rafael Wysocki <rafael@...nel.org>,
 Len Brown <lenb@...nel.org>, Sudeep Holla <sudeep.holla@....com>,
 Catalin Marinas <catalin.marinas@....com>, Will Deacon <will@...nel.org>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Danilo Krummrich <dakr@...nel.org>
Subject: Re: [PATCH v2 06/29] ACPI / MPAM: Parse the MPAM table

Hi Jonathan,

On 26/09/2025 15:48, Jonathan Cameron wrote:
>>>> +	char uid[16];
>>>> +	u32 acpi_id;
>>>> +
>>>> +	if (acpi_disabled || !system_supports_mpam() || IS_ERR(table))
>>>> +		return 0;
>>>> +
>>>> +	if (table->revision < 1)
>>>> +		return 0;
>>>> +
>>>> +	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_debug("MSC entry overlaps end of ACPI table\n");
>>>> +			break;  
>>
>>> That this isn't considered an error is a bit subtle and made me wonder
>>> if there was a use of uninitialized pdev (there isn't because err == 0)  
>>
>> Its somewhat a philosophical arguement. I don't expect the kernel to have to validate
>> these tables, they're not provided by the user and there quickly becomes a point where
>> you have to trust them, and they have to be correct.

> Potential buffer overrun is to me always worth error out screaming, but I get your
> broader point.   Maybe just make it a pr_err()

Sure, done.


>> At the other extreme is the asusmption the table is line-noise and we should check
>> everything to avoid out of bounds accesses. Dave wanted the diagnostic messages on these.
>>
>> As this is called from an initcall, the best you get is an inexplicable print message.
>> (what should we say - update your firmware?)
> 
> Depends on whether you can lean hard on the firmware team. Much easier
> for me if I can tell them the board doesn't boot because they got it wrong.
> 
> That would have been safer if we had this upstream in advance of hardware, but indeed
> a little high risk today as who knows what borked tables are out there.
> 
> Personal preference though is to error out on things like this and handle the papering
> over at the top level.  Don't put extra effort into checking tables are invalid
> but if we happen to notice as part of code safety stuff like sizes then good to scream
> about it.
> 
>>
>>
>> Silently failing in this code is always safe as the driver has a count of the number of
>> MSC, and doesn't start accessing the hardware until its found them all.
>> (this is because to find the system wide minimum value - and its not worth starting if
>>  its not possible to finish).
>>
>>
>>> Why not return here?  
>>
>> Just because there was no other return in the loop, and I hate surprise returns.
>>
>> I'll change it if it avoids thinking about how that platform_device_put() gets skipped!
>>
>>
>>>   
>>>> +		}
>>>> +
>>>> +		/*
>>>> +		 * If any of the reserved fields are set, make no attempt to
>>>> +		 * parse the MSC structure. This MSC will still be counted,
>>>> +		 * meaning the MPAM driver can't probe against all MSC, and
>>>> +		 * will never be enabled. There is no way to enable it safely,
>>>> +		 * because we cannot determine safe system-wide partid and pmg
>>>> +		 * ranges in this situation.
>>>> +		 */  
>>
>>> This is decidedly paranoid. I'd normally expect the architecture to be based
>>> on assumption that is fine for old software to ignore new fields.  ACPI itself
>>> has fairly firm rules on this (though it goes wrong sometimes :)  
>>
>> Yeah - the MPAM table isn't properly structured as subtables. I don't see how they are
>> going to extend it if they need to.
>>
>> The paranoia is that anything set in these reserved fields probably indicates something
>> the driver needs to know about: a case in point is the way PCC was added.
>>
>> I'd much prefer we skip creation of MSC devices that have properties we don't understand.
>> acpi_mpam_count_msc() still counts them - which means the driver never finds all the MSC,
>> and never touches the hardware.
>>
>> MPAM isn't a critical feature, its better that it be disabled than make things worse.
>> (the same attitude holds with the response to the MPAM error interrupt - reset everything
>>  and pack up shop. This is bettern than accidentally combining important/unimportant
>>  tasks)
>>
>>
>>> I'm guessing there is something out there that made this necessary though so
>>> keep it if you actually need it.  
>>
>> It's a paranoid/violent reaction to the way PCC was added - without something like this,
>> that would have led to the OS trying to map the 0 page and poking around in it - never
>> likely to go well.
>>
>> Doing this does let them pull another PCC without stable kernels going wrong.
>> Ultimately I think they'll need to replace the table with one that is properly structured.
>> For now - this is working with what we have.

> Fair enough. I'm too lazy / behind with reviews to go scream via our channels about
> problems here.  Paranoia it is.  Maybe we'll end up backporting some 'fixes' that
> ignore nicely added fields with appropriate control bits to turn them on.
> So be it if that happens.

Yup - I'm expecting to at least backport "ignore this feature" patches when the table gets
changed. It's not how its supposed to work, but the missing subtable header wasn't caught
until it was too late.
MPAM isn't a critical feature, so it shouldn't matter too much if old kernels on new
hardware can't use it.


>>>> +		if (tbl_msc->reserved || tbl_msc->reserved1 || tbl_msc->reserved2) {
>>>> +			pr_err_once("Unrecognised MSC, MPAM not usable\n");
>>>> +			pr_debug("MSC.%u: reserved field set\n", tbl_msc->identifier);
>>>> +			continue;
>>>> +		}
>>>> +
>>>> +		if (!tbl_msc->mmio_size) {
>>>> +			pr_debug("MSC.%u: marked as disabled\n", tbl_msc->identifier);
>>>> +			continue;
>>>> +		}
>>>> +
>>>> +		if (decode_interface_type(tbl_msc, &iface)) {
>>>> +			pr_debug("MSC.%u: unknown interface type\n", tbl_msc->identifier);
>>>> +			continue;
>>>> +		}
>>>> +
>>>> +		next_res = 0;
>>>> +		next_prop = 0;
>>>> +		memset(res, 0, sizeof(res));
>>>> +		memset(props, 0, sizeof(props));
>>>> +
>>>> +		pdev = platform_device_alloc("mpam_msc", tbl_msc->identifier);  
>>>
>>> https://lore.kernel.org/all/20241009124120.1124-13-shiju.jose@huawei.com/
>>> was a proposal to add a DEFINE_FREE() to clean these up.  Might be worth a revisit.
>>> Then Greg was against the use it was put to and asking for an example of where
>>> it helped.  Maybe this is that example.
>>>
>>> If you do want to do that, I'd factor out a bunch of the stuff here as a helper
>>> so we can have the clean ownership pass of a return_ptr().  
>>> Similar to what Shiju did here (this is the usecase for platform device that
>>> Greg didn't like).
>>> https://lore.kernel.org/all/20241009124120.1124-14-shiju.jose@huawei.com/
>>>
>>> Even without that I think factoring some of this out and hence being able to
>>> do returns on errors and put the if (err) into the loop would be a nice
>>> improvement to readability.  
>>
>> If you think its more readable I'll structure it like that.
> 
> The refactor yes. I'd keep clear of the the DEFINE_FREE() unless you have
> some spare time ;)

spare what?



Thanks,

James

Powered by blists - more mailing lists