[PATCH 0/2] x86/amd_node: Fixes for virtualized systems

[PATCH 0/2] x86/amd_node: Fixes for virtualized systems

[Jason Andryuk]

On my test system, I see SMN accesses from:
amd_pmf_probe() - read
amd_pmc_probe() - read
acp_acp70_audio_probe() - write

Jason Andryuk (2):
  x86/amd_node: Remove smn_exclusive
  x86/amd_node: Avoid divide by zero on virtualized systems

 arch/x86/kernel/amd_node.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

-- 
2.55.0

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

[Jason Andryuk]

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 dereference.

Fixes: 77466b798d59 ("x86/amd_node: Remove dependency on AMD_NB")
Cc: stable@vger.kernel.org
Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
---
Fixes is the introduction of amd_roots
---
 arch/x86/kernel/amd_node.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/amd_node.c b/arch/x86/kernel/amd_node.c
index 0be01725a2a4..ea553267e5fa 100644
--- a/arch/x86/kernel/amd_node.c
+++ b/arch/x86/kernel/amd_node.c
@@ -38,7 +38,6 @@ static struct pci_dev **amd_roots;
 
 /* Protect the PCI config register pairs used for SMN. */
 static DEFINE_MUTEX(smn_mutex);
-static bool smn_exclusive;
 
 #define SMN_INDEX_OFFSET	0x60
 #define SMN_DATA_OFFSET		0x64
@@ -91,11 +90,11 @@ static int __amd_smn_rw(u8 i_off, u8 d_off, u16 node, u32 address, u32 *value, b
 	if (node >= amd_num_nodes())
 		return err;
 
-	root = amd_roots[node];
-	if (!root)
+	if (!amd_roots)
 		return err;
 
-	if (!smn_exclusive)
+	root = amd_roots[node];
+	if (!root)
 		return err;
 
 	guard(mutex)(&smn_mutex);
@@ -308,8 +307,6 @@ static int __init amd_smn_init(void)
 		debugfs_create_file("value",	0600, debugfs_dir, NULL, &smn_value_fops);
 	}
 
-	smn_exclusive = true;
-
 	return 0;
 }
 
-- 
2.55.0

[PATCH 2/2] x86/amd_node: Avoid divide by zero on virtualized systems

[Jason Andryuk]

On a virtualized system, the number of nodes does not have a
relationship to the number of roots.  A Xen PVH dom0 can calculate
roots_per_node as 0, which crashes with a divide by zero in:

    if (count++ % roots_per_node)

On a virtualized system, default the value to 1.  The issue is seen with
Xen, but it could affect other systems.

Fixes: 0a4b61d9c2e4 ("x86/amd_node: Fix AMD root device caching")
Cc: stable@vger.kernel.org
Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
---
X86_FEATURE_XENPV is only for PV, but this is observed with a PVH dom0.
---
 arch/x86/kernel/amd_node.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/x86/kernel/amd_node.c b/arch/x86/kernel/amd_node.c
index ea553267e5fa..c5025e5291b6 100644
--- a/arch/x86/kernel/amd_node.c
+++ b/arch/x86/kernel/amd_node.c
@@ -286,6 +286,11 @@ static int __init amd_smn_init(void)
 		return -ENOMEM;
 
 	roots_per_node = num_roots / num_nodes;
+	if (roots_per_node == 0) {
+		if (!cpu_feature_enabled(X86_FEATURE_HYPERVISOR))
+			pr_err(FW_BUG "Error detecting roots per node.\n");
+		roots_per_node = 1;
+	}
 
 	count = 0;
 	node = 0;
-- 
2.55.0

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

[Yazen Ghannam]

On Thu, Aug 06, 2026 at 12:01:56PM -0400, Jason Andryuk wrote:
> 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 dereference.
> 
> Fixes: 77466b798d59 ("x86/amd_node: Remove dependency on AMD_NB")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
> ---
> Fixes is the introduction of amd_roots
> ---
>  arch/x86/kernel/amd_node.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kernel/amd_node.c b/arch/x86/kernel/amd_node.c
> index 0be01725a2a4..ea553267e5fa 100644
> --- a/arch/x86/kernel/amd_node.c
> +++ b/arch/x86/kernel/amd_node.c
> @@ -38,7 +38,6 @@ static struct pci_dev **amd_roots;
>  
>  /* Protect the PCI config register pairs used for SMN. */
>  static DEFINE_MUTEX(smn_mutex);
> -static bool smn_exclusive;
>  
>  #define SMN_INDEX_OFFSET	0x60
>  #define SMN_DATA_OFFSET		0x64
> @@ -91,11 +90,11 @@ static int __amd_smn_rw(u8 i_off, u8 d_off, u16 node, u32 address, u32 *value, b
>  	if (node >= amd_num_nodes())
>  		return err;
>  
> -	root = amd_roots[node];
> -	if (!root)
> +	if (!amd_roots)
>  		return err;
>  
> -	if (!smn_exclusive)
> +	root = amd_roots[node];
> +	if (!root)
>  		return err;
>  
>  	guard(mutex)(&smn_mutex);
> @@ -308,8 +307,6 @@ static int __init amd_smn_init(void)
>  		debugfs_create_file("value",	0600, debugfs_dir, NULL, &smn_value_fops);
>  	}
>  
> -	smn_exclusive = true;
> -
>  	return 0;
>  }
>  

'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.

Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com>

Thanks,
Yazen

Re: [PATCH 2/2] x86/amd_node: Avoid divide by zero on virtualized systems

[Yazen Ghannam]

On Thu, Aug 06, 2026 at 12:01:57PM -0400, Jason Andryuk wrote:
> On a virtualized system, the number of nodes does not have a
> relationship to the number of roots.  A Xen PVH dom0 can calculate
> roots_per_node as 0, which crashes with a divide by zero in:
> 
>     if (count++ % roots_per_node)
> 
> On a virtualized system, default the value to 1.  The issue is seen with
> Xen, but it could affect other systems.
> 
> Fixes: 0a4b61d9c2e4 ("x86/amd_node: Fix AMD root device caching")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>

I agree with the idea with some minor feedback below.

> ---
> X86_FEATURE_XENPV is only for PV, but this is observed with a PVH dom0.
> ---
>  arch/x86/kernel/amd_node.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/x86/kernel/amd_node.c b/arch/x86/kernel/amd_node.c
> index ea553267e5fa..c5025e5291b6 100644
> --- a/arch/x86/kernel/amd_node.c
> +++ b/arch/x86/kernel/amd_node.c
> @@ -286,6 +286,11 @@ static int __init amd_smn_init(void)
>  		return -ENOMEM;
>  
>  	roots_per_node = num_roots / num_nodes;
> +	if (roots_per_node == 0) {

Can be '!roots_per_node'.

> +		if (!cpu_feature_enabled(X86_FEATURE_HYPERVISOR))
> +			pr_err(FW_BUG "Error detecting roots per node.\n");
> +		roots_per_node = 1;
> +	}

I don't think this is necessarily a Firmware bug.

Likewise, it can be a 'warning' rather than 'error'. I interpret 'error'
as something known to be incorrect. This was my position before on this
topic. But the various contrary reports changed my mind (even if they
come from virtualization).

Furthermore, I think 'warning' is more appropriate. It says "We found
something unexpected. We're letting you (the user) know about it. And
we'll mitigate it to avoid an error."

Anyways, I just wanted to write out some thoughts since this topic has
come up a few times.

Possible rework for the above change:

	if (!roots_per_node && !cpu_feature_enabled(X86_FEATURE_HYPERVISOR)
		pr_warn("Expected at least 1 root per AMD node.\n");

	roots_per_node = max(roots_per_node, 1);

In any case, we should just go with a simple fix for the virt cases.

I think we could even do away with caching the 'root' devices. But
that'll be another rework/cleanup.

Thanks,
Yazen

Re: [PATCH 2/2] x86/amd_node: Avoid divide by zero on virtualized systems

[Jason Andryuk]

On 2026-08-10 10:47, Yazen Ghannam wrote:
> On Thu, Aug 06, 2026 at 12:01:57PM -0400, Jason Andryuk wrote:
>> On a virtualized system, the number of nodes does not have a
>> relationship to the number of roots.  A Xen PVH dom0 can calculate
>> roots_per_node as 0, which crashes with a divide by zero in:
>>
>>      if (count++ % roots_per_node)
>>
>> On a virtualized system, default the value to 1.  The issue is seen with
>> Xen, but it could affect other systems.
>>
>> Fixes: 0a4b61d9c2e4 ("x86/amd_node: Fix AMD root device caching")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
> 
> I agree with the idea with some minor feedback below.
> 
>> ---
>> X86_FEATURE_XENPV is only for PV, but this is observed with a PVH dom0.
>> ---
>>   arch/x86/kernel/amd_node.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/arch/x86/kernel/amd_node.c b/arch/x86/kernel/amd_node.c
>> index ea553267e5fa..c5025e5291b6 100644
>> --- a/arch/x86/kernel/amd_node.c
>> +++ b/arch/x86/kernel/amd_node.c
>> @@ -286,6 +286,11 @@ static int __init amd_smn_init(void)
>>   		return -ENOMEM;
>>   
>>   	roots_per_node = num_roots / num_nodes;
>> +	if (roots_per_node == 0) {
> 
> Can be '!roots_per_node'.

Sure.
>> +		if (!cpu_feature_enabled(X86_FEATURE_HYPERVISOR))
>> +			pr_err(FW_BUG "Error detecting roots per node.\n");
>> +		roots_per_node = 1;
>> +	}
> 
> I don't think this is necessarily a Firmware bug.
> 
> Likewise, it can be a 'warning' rather than 'error'. I interpret 'error'
> as something known to be incorrect. This was my position before on this
> topic. But the various contrary reports changed my mind (even if they
> come from virtualization).
> 
> Furthermore, I think 'warning' is more appropriate. It says "We found
> something unexpected. We're letting you (the user) know about it. And
> we'll mitigate it to avoid an error."

Makes sense to me.  Boris suggested error in 
https://lore.kernel.org/lkml/20260624192530.GGajwvKuhxbnHe0s42@fat_crate.local/

(I probably should have used Suggested-by.)

> Anyways, I just wanted to write out some thoughts since this topic has
> come up a few times.
> 
> Possible rework for the above change:
> 
> 	if (!roots_per_node && !cpu_feature_enabled(X86_FEATURE_HYPERVISOR)
> 		pr_warn("Expected at least 1 root per AMD node.\n");
> 
> 	roots_per_node = max(roots_per_node, 1);
> 
> In any case, we should just go with a simple fix for the virt cases.

I'm okay with this.  I'll wait to see if anyone chimes in further.  If 
not, I'll go with your approach and Suggested-by.

Thanks,
Jason

Re: [PATCH 2/2] x86/amd_node: Avoid divide by zero on virtualized systems

[David Laight]

On Mon, 10 Aug 2026 10:47:06 -0400
Yazen Ghannam <yazen.ghannam@amd.com> wrote:

> On Thu, Aug 06, 2026 at 12:01:57PM -0400, Jason Andryuk wrote:
> > On a virtualized system, the number of nodes does not have a
> > relationship to the number of roots.  A Xen PVH dom0 can calculate
> > roots_per_node as 0, which crashes with a divide by zero in:
> > 
> >     if (count++ % roots_per_node)
> > 
> > On a virtualized system, default the value to 1.  The issue is seen with
> > Xen, but it could affect other systems.
> > 
> > Fixes: 0a4b61d9c2e4 ("x86/amd_node: Fix AMD root device caching")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>  
> 
> I agree with the idea with some minor feedback below.
> 
> > ---
> > X86_FEATURE_XENPV is only for PV, but this is observed with a PVH dom0.
> > ---
> >  arch/x86/kernel/amd_node.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/arch/x86/kernel/amd_node.c b/arch/x86/kernel/amd_node.c
> > index ea553267e5fa..c5025e5291b6 100644
> > --- a/arch/x86/kernel/amd_node.c
> > +++ b/arch/x86/kernel/amd_node.c
> > @@ -286,6 +286,11 @@ static int __init amd_smn_init(void)
> >  		return -ENOMEM;
> >  
> >  	roots_per_node = num_roots / num_nodes;
> > +	if (roots_per_node == 0) {  
> 
> Can be '!roots_per_node'.
> 
> > +		if (!cpu_feature_enabled(X86_FEATURE_HYPERVISOR))
> > +			pr_err(FW_BUG "Error detecting roots per node.\n");
> > +		roots_per_node = 1;
> > +	}  
> 
> I don't think this is necessarily a Firmware bug.
> 
> Likewise, it can be a 'warning' rather than 'error'. I interpret 'error'
> as something known to be incorrect. This was my position before on this
> topic. But the various contrary reports changed my mind (even if they
> come from virtualization).
> 
> Furthermore, I think 'warning' is more appropriate. It says "We found
> something unexpected. We're letting you (the user) know about it. And
> we'll mitigate it to avoid an error."
> 
> Anyways, I just wanted to write out some thoughts since this topic has
> come up a few times.
> 
> Possible rework for the above change:
> 
> 	if (!roots_per_node && !cpu_feature_enabled(X86_FEATURE_HYPERVISOR)
> 		pr_warn("Expected at least 1 root per AMD node.\n");
> 
> 	roots_per_node = max(roots_per_node, 1);

I'd not add the max(), just;
	if (!roots_per_node) {
		roots_per_node = 1;
		if (cpu_feature_enabled(X86_FEATURE_HYPERVISOR)
			pr_warn("Expected at least 1 root per AMD node.\n");
	}

David

> 
> In any case, we should just go with a simple fix for the virt cases.
> 
> I think we could even do away with caching the 'root' devices. But
> that'll be another rework/cleanup.
> 
> Thanks,
> Yazen
> 

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

[Mario Limonciello]

On 8/10/26 09:07, Yazen Ghannam wrote:
> On Thu, Aug 06, 2026 at 12:01:56PM -0400, Jason Andryuk wrote:
>> 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 dereference.
>>
>> Fixes: 77466b798d59 ("x86/amd_node: Remove dependency on AMD_NB")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
>> ---
>> Fixes is the introduction of amd_roots
>> ---
>>   arch/x86/kernel/amd_node.c | 9 +++------
>>   1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/kernel/amd_node.c b/arch/x86/kernel/amd_node.c
>> index 0be01725a2a4..ea553267e5fa 100644
>> --- a/arch/x86/kernel/amd_node.c
>> +++ b/arch/x86/kernel/amd_node.c
>> @@ -38,7 +38,6 @@ static struct pci_dev **amd_roots;
>>   
>>   /* Protect the PCI config register pairs used for SMN. */
>>   static DEFINE_MUTEX(smn_mutex);
>> -static bool smn_exclusive;
>>   
>>   #define SMN_INDEX_OFFSET	0x60
>>   #define SMN_DATA_OFFSET		0x64
>> @@ -91,11 +90,11 @@ static int __amd_smn_rw(u8 i_off, u8 d_off, u16 node, u32 address, u32 *value, b
>>   	if (node >= amd_num_nodes())
>>   		return err;
>>   
>> -	root = amd_roots[node];
>> -	if (!root)
>> +	if (!amd_roots)
>>   		return err;
>>   
>> -	if (!smn_exclusive)
>> +	root = amd_roots[node];
>> +	if (!root)
>>   		return err;
>>   
>>   	guard(mutex)(&smn_mutex);
>> @@ -308,8 +307,6 @@ static int __init amd_smn_init(void)
>>   		debugfs_create_file("value",	0600, debugfs_dir, NULL, &smn_value_fops);
>>   	}
>>   
>> -	smn_exclusive = true;
>> -
>>   	return 0;
>>   }
>>   
> 
> '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.
> 
> Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com>
> 
> Thanks,
> Yazen

Reviewed-by: Mario Limonciello (AMD) <superm1@kernel.org>

Re: [PATCH 2/2] x86/amd_node: Avoid divide by zero on virtualized systems

[Jason Andryuk]

On 2026-08-11 04:21, David Laight wrote:
> On Mon, 10 Aug 2026 10:47:06 -0400
> Yazen Ghannam <yazen.ghannam@amd.com> wrote:
> 
>> On Thu, Aug 06, 2026 at 12:01:57PM -0400, Jason Andryuk wrote:
>>> On a virtualized system, the number of nodes does not have a
>>> relationship to the number of roots.  A Xen PVH dom0 can calculate
>>> roots_per_node as 0, which crashes with a divide by zero in:
>>>
>>>      if (count++ % roots_per_node)
>>>
>>> On a virtualized system, default the value to 1.  The issue is seen with
>>> Xen, but it could affect other systems.
>>>
>>> Fixes: 0a4b61d9c2e4 ("x86/amd_node: Fix AMD root device caching")
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
>>
>> I agree with the idea with some minor feedback below.
>>
>>> ---
>>> X86_FEATURE_XENPV is only for PV, but this is observed with a PVH dom0.
>>> ---
>>>   arch/x86/kernel/amd_node.c | 5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/arch/x86/kernel/amd_node.c b/arch/x86/kernel/amd_node.c
>>> index ea553267e5fa..c5025e5291b6 100644
>>> --- a/arch/x86/kernel/amd_node.c
>>> +++ b/arch/x86/kernel/amd_node.c
>>> @@ -286,6 +286,11 @@ static int __init amd_smn_init(void)
>>>   		return -ENOMEM;
>>>   
>>>   	roots_per_node = num_roots / num_nodes;
>>> +	if (roots_per_node == 0) {
>>
>> Can be '!roots_per_node'.
>>
>>> +		if (!cpu_feature_enabled(X86_FEATURE_HYPERVISOR))
>>> +			pr_err(FW_BUG "Error detecting roots per node.\n");
>>> +		roots_per_node = 1;
>>> +	}
>>
>> I don't think this is necessarily a Firmware bug.
>>
>> Likewise, it can be a 'warning' rather than 'error'. I interpret 'error'
>> as something known to be incorrect. This was my position before on this
>> topic. But the various contrary reports changed my mind (even if they
>> come from virtualization).
>>
>> Furthermore, I think 'warning' is more appropriate. It says "We found
>> something unexpected. We're letting you (the user) know about it. And
>> we'll mitigate it to avoid an error."
>>
>> Anyways, I just wanted to write out some thoughts since this topic has
>> come up a few times.
>>
>> Possible rework for the above change:
>>
>> 	if (!roots_per_node && !cpu_feature_enabled(X86_FEATURE_HYPERVISOR)
>> 		pr_warn("Expected at least 1 root per AMD node.\n");
>>
>> 	roots_per_node = max(roots_per_node, 1);
> 
> I'd not add the max(), just;
> 	if (!roots_per_node) {
> 		roots_per_node = 1;
> 		if (cpu_feature_enabled(X86_FEATURE_HYPERVISOR)
> 			pr_warn("Expected at least 1 root per AMD node.\n");
> 	}

I prefer this because it puts the special handing inside the check. 
David, you dropped ! from X86_FEATURE_HYPERVISOR - did you mean that? 
The intent was to only warn if real hardware doesn't match the expectation.

Really, I just want to get this fixed.

Thanks,
Jason

Re: [PATCH 2/2] x86/amd_node: Avoid divide by zero on virtualized systems

[David Laight]

On Tue, 11 Aug 2026 17:23:47 -0400
Jason Andryuk <jason.andryuk@amd.com> wrote:

> On 2026-08-11 04:21, David Laight wrote:
> > On Mon, 10 Aug 2026 10:47:06 -0400
> > Yazen Ghannam <yazen.ghannam@amd.com> wrote:
> >   
> >> On Thu, Aug 06, 2026 at 12:01:57PM -0400, Jason Andryuk wrote:  
> >>> On a virtualized system, the number of nodes does not have a
> >>> relationship to the number of roots.  A Xen PVH dom0 can calculate
> >>> roots_per_node as 0, which crashes with a divide by zero in:
> >>>
> >>>      if (count++ % roots_per_node)
> >>>
> >>> On a virtualized system, default the value to 1.  The issue is seen with
> >>> Xen, but it could affect other systems.
> >>>
> >>> Fixes: 0a4b61d9c2e4 ("x86/amd_node: Fix AMD root device caching")
> >>> Cc: stable@vger.kernel.org
> >>> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>  
> >>
> >> I agree with the idea with some minor feedback below.
> >>  
> >>> ---
> >>> X86_FEATURE_XENPV is only for PV, but this is observed with a PVH dom0.
> >>> ---
> >>>   arch/x86/kernel/amd_node.c | 5 +++++
> >>>   1 file changed, 5 insertions(+)
> >>>
> >>> diff --git a/arch/x86/kernel/amd_node.c b/arch/x86/kernel/amd_node.c
> >>> index ea553267e5fa..c5025e5291b6 100644
> >>> --- a/arch/x86/kernel/amd_node.c
> >>> +++ b/arch/x86/kernel/amd_node.c
> >>> @@ -286,6 +286,11 @@ static int __init amd_smn_init(void)
> >>>   		return -ENOMEM;
> >>>   
> >>>   	roots_per_node = num_roots / num_nodes;
> >>> +	if (roots_per_node == 0) {  
> >>
> >> Can be '!roots_per_node'.
> >>  
> >>> +		if (!cpu_feature_enabled(X86_FEATURE_HYPERVISOR))
> >>> +			pr_err(FW_BUG "Error detecting roots per node.\n");
> >>> +		roots_per_node = 1;
> >>> +	}  
> >>
> >> I don't think this is necessarily a Firmware bug.
> >>
> >> Likewise, it can be a 'warning' rather than 'error'. I interpret 'error'
> >> as something known to be incorrect. This was my position before on this
> >> topic. But the various contrary reports changed my mind (even if they
> >> come from virtualization).
> >>
> >> Furthermore, I think 'warning' is more appropriate. It says "We found
> >> something unexpected. We're letting you (the user) know about it. And
> >> we'll mitigate it to avoid an error."
> >>
> >> Anyways, I just wanted to write out some thoughts since this topic has
> >> come up a few times.
> >>
> >> Possible rework for the above change:
> >>
> >> 	if (!roots_per_node && !cpu_feature_enabled(X86_FEATURE_HYPERVISOR)
> >> 		pr_warn("Expected at least 1 root per AMD node.\n");
> >>
> >> 	roots_per_node = max(roots_per_node, 1);  
> > 
> > I'd not add the max(), just;
> > 	if (!roots_per_node) {
> > 		roots_per_node = 1;
> > 		if (cpu_feature_enabled(X86_FEATURE_HYPERVISOR)
> > 			pr_warn("Expected at least 1 root per AMD node.\n");
> > 	}  
> 
> I prefer this because it puts the special handing inside the check. 
> David, you dropped ! from X86_FEATURE_HYPERVISOR - did you mean that? 

No, C&P typo in the email client.

David

> The intent was to only warn if real hardware doesn't match the expectation.
> 
> Really, I just want to get this fixed.
> 
> Thanks,
> Jason

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

[Borislav Petkov]

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?

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 2/2] x86/amd_node: Avoid divide by zero on virtualized systems

[Borislav Petkov]

On Mon, Aug 10, 2026 at 10:47:06AM -0400, Yazen Ghannam wrote:
> I don't think this is necessarily a Firmware bug.

What else can it be if we're running on baremetal?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

[Jason Andryuk]

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.

Regards,
Jason

Re: [PATCH 2/2] x86/amd_node: Avoid divide by zero on virtualized systems

[Yazen Ghannam]

On Wed, Aug 12, 2026 at 12:31:17PM -0700, Borislav Petkov wrote:
> On Mon, Aug 10, 2026 at 10:47:06AM -0400, Yazen Ghannam wrote:
> > I don't think this is necessarily a Firmware bug.
> 
> What else can it be if we're running on baremetal?
> 

Could be the hardware convention changed. Unlikely, but could happen.

Thanks,
Yazen

Re: [PATCH 2/2] x86/amd_node: Avoid divide by zero on virtualized systems

[Borislav Petkov]

On August 13, 2026 3:56:21 PM UTC, Yazen Ghannam <yazen.ghannam@amd.com> wrote:
>On Wed, Aug 12, 2026 at 12:31:17PM -0700, Borislav Petkov wrote:
>> On Mon, Aug 10, 2026 at 10:47:06AM -0400, Yazen Ghannam wrote:
>> > I don't think this is necessarily a Firmware bug.
>> 
>> What else can it be if we're running on baremetal?
>> 
>
>Could be the hardware convention changed. Unlikely, but could happen.
>
>Thanks,
>Yazen

That's ok, we'll address it when we cross that bridge...
-- 
Small device. Typos and formatting crap

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

[Borislav Petkov]

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.

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.

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.

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.

Right?

Anything I'm missing?

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

[Jason Andryuk]

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