Re: [PATCH 1/2] x86/amd_node: Remove smn_exclusive

On 2026-08-13 19:58, Borislav Petkov wrote:
> On Thu, Aug 13, 2026 at 09:40:33AM -0400, Jason Andryuk wrote:
>> On 2026-08-12 15:28, Borislav Petkov wrote:
>>> On Mon, Aug 10, 2026 at 10:07:47AM -0400, Yazen Ghannam wrote:
>>>> 'smn_exclusive' wasn't originally intended to be used for bounds
>>>> checking the array. But the original use is no longer needed, so it can
>>>> be removed.
>>>
>>> Why is it no longer needed? Apparently I didn't get that memo yet...
>>>
>>> 83518453074d ("x86/amd_node: Add SMN offsets to exclusive region access") is
>>> talking about the potential concurrent access. Why is that ok now all of
>>> a sudden?
>> 83518453074d set smn_exclusive after successfully calling
>> pci_request_config_region_exclusive().  smn_exclusive gated access in
>> __amd_smn_rw().
>>
>> Things moved around, and now pci_request_config_region_exclusive() is called
>> earlier.  At the end of amd_smn_init(), either amd_roots == NULL and
>> smn_exclusive == false or amd_roots != NULL and smn_exclusive == true.
>> smn_exclusive is redundant, and amd_roots can just be used directly.
> 
> So you want to use amd_roots to *also* *denote* whether exclusive SMN access
> is enabled. Then you need to at least put a comment in __amd_smn_rw() where
> you're testing it, what exactly that test means.

I'm not sure adding a comment is helpful.  smn_exclusive lost its 
usefulness between its introduction and now.

> Then, your patch has a Fixes: tag and CC:stable which means, it needs to go to
> all kernels but then I don't know what you're fixing.

In my original commit message I stated this removes a potential NULL deref:
"""
amd_smn_read/write() are exported functions around __amd_smn_rw(), so
they are always available even if amd_smn_init() fails.  smn_exclusive
would prevent access __amd_smn_rw(), but it is placed too late.  If
amd_smn_init() failed, amd_roots is NULL and __amd_smn_rw() will fault
over it.  Replace smn_exclusive with directly checking amd_roots to
avoid the NULL pointer deref - writeerence.
"""

I will add something about how smn_exclusive became redundant.

> Looking at it more, you're basically fixing one more aspect of running the
> kernel as a Xen guest and it not having AMD roots exported.

If amd_smn_init() fails for any reason, __amd_smn_rw() will fault.  It 
was seen with Xen, but it is not a property of running on Xen.

> But what you really wanna do, IMNSVHO, you want to make your patch 2 be patch
> 1 and it can go to stable.
> 
> Your patch 1 then becomes a simplification/cleanup patch which goes as patch
> 2 and doesn't need to be Cc:stable but it can be queued later, during the
> normal cycle.
Re-ordering is fine.  With the divide by zero fix, then amd_smn_init() 
succeeds in my test with Xen.  So the other patch isn't needed in that 
case, but it looked like a worthwhile fix.  In the cover letter, I noted 
accesses from amd_pmf_probe(), amd_pmc_probe(), and 
acp_acp70_audio_probe() to __amd_smn_rw() on my test system.  These are 
called even if amd_smn_init() fails.

Regards,
Jason