[PATCH] arm64: acpi: Support PCI devices numa_node property in ACPI mode

[PATCH] arm64: acpi: Support PCI devices numa_node property in ACPI mode

[Sergey Temerkhanov]

This commit modifies ARM64-specific ACPI PCI support functions to
properly set the numa_node property on host bridges and thus on PCI
devices. Such support requires _PXM objects to be set in DSDT/SSDT
tables describing PCI root bridges

Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
---
 arch/arm64/kernel/pci.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index 4f0e3ebfea4b..c45c7a26f984 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -81,7 +81,16 @@ int raw_pci_write(unsigned int domain, unsigned int bus,
 
 int pcibus_to_node(struct pci_bus *bus)
 {
-	return dev_to_node(&bus->dev);
+	struct pci_config_window *cfg = bus->sysdata;
+	struct acpi_device *adev = NULL;
+	struct device *dev;
+
+	if (!acpi_disabled)
+		adev = to_acpi_device(cfg->parent);
+
+	dev = adev ? &adev->dev : &bus->dev;
+
+	return dev_to_node(dev);
 }
 EXPORT_SYMBOL(pcibus_to_node);
 
@@ -186,6 +195,8 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
 	struct pci_bus *bus, *child;
 	struct acpi_pci_root_ops *root_ops;
 
+	set_dev_node(&root->device->dev, node);
+
 	ri = kzalloc_node(sizeof(*ri), GFP_KERNEL, node);
 	if (!ri)
 		return NULL;
-- 
2.12.1

[PATCH] arm64: acpi: Support PCI devices numa_node property in ACPI mode

[Lorenzo Pieralisi]

Hi Sergey,

On Thu, Apr 06, 2017 at 11:47:11AM +0000, Sergey Temerkhanov wrote:
> This commit modifies ARM64-specific ACPI PCI support functions to
> properly set the numa_node property on host bridges and thus on PCI
> devices. Such support requires _PXM objects to be set in DSDT/SSDT
> tables describing PCI root bridges
> 
> Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
> ---
>  arch/arm64/kernel/pci.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> index 4f0e3ebfea4b..c45c7a26f984 100644
> --- a/arch/arm64/kernel/pci.c
> +++ b/arch/arm64/kernel/pci.c
> @@ -81,7 +81,16 @@ int raw_pci_write(unsigned int domain, unsigned int bus,
>  
>  int pcibus_to_node(struct pci_bus *bus)
>  {
> -	return dev_to_node(&bus->dev);
> +	struct pci_config_window *cfg = bus->sysdata;

Well, this implicit cast is a guess (ie it might be a DT sysdata
and there you can't have a clue on what it really is).

> +	struct acpi_device *adev = NULL;
> +	struct device *dev;
> +
> +	if (!acpi_disabled)
> +		adev = to_acpi_device(cfg->parent);
> +
> +	dev = adev ? &adev->dev : &bus->dev;
> +
> +	return dev_to_node(dev);
>  }
>  EXPORT_SYMBOL(pcibus_to_node);
>  
> @@ -186,6 +195,8 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
>  	struct pci_bus *bus, *child;
>  	struct acpi_pci_root_ops *root_ops;
>  
> +	set_dev_node(&root->device->dev, node);

Ok I suspect this is one of those catch-22 issues with host bridge
creation/scan code (like domain nr), let me have a look I will send
you a patch next week when I figure out the call ordering required
to make this work, I want to make sure we can make this work with
DT and ACPI alike.

Thanks !
Lorenzo

> +
>  	ri = kzalloc_node(sizeof(*ri), GFP_KERNEL, node);
>  	if (!ri)
>  		return NULL;
> -- 
> 2.12.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] arm64: acpi: Support PCI devices numa_node property in ACPI mode

[Sergei Temerkhanov]

On Fri, Apr 7, 2017 at 10:18 AM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> Hi Sergey,
>
> On Thu, Apr 06, 2017 at 11:47:11AM +0000, Sergey Temerkhanov wrote:
>> This commit modifies ARM64-specific ACPI PCI support functions to
>> properly set the numa_node property on host bridges and thus on PCI
>> devices. Such support requires _PXM objects to be set in DSDT/SSDT
>> tables describing PCI root bridges
>>
>> Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
>> ---
>>  arch/arm64/kernel/pci.c | 13 ++++++++++++-
>>  1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
>> index 4f0e3ebfea4b..c45c7a26f984 100644
>> --- a/arch/arm64/kernel/pci.c
>> +++ b/arch/arm64/kernel/pci.c
>> @@ -81,7 +81,16 @@ int raw_pci_write(unsigned int domain, unsigned int bus,
>>
>>  int pcibus_to_node(struct pci_bus *bus)
>>  {
>> -     return dev_to_node(&bus->dev);
>> +     struct pci_config_window *cfg = bus->sysdata;
>
> Well, this implicit cast is a guess (ie it might be a DT sysdata
> and there you can't have a clue on what it really is).

Yes, but it's only accessed when acpi_disabled != 0. I just tried to
save few lines and brackets :-)

Regards,
Sergey

>
>> +     struct acpi_device *adev = NULL;
>> +     struct device *dev;
>> +
>> +     if (!acpi_disabled)
>> +             adev = to_acpi_device(cfg->parent);
>> +
>> +     dev = adev ? &adev->dev : &bus->dev;
>> +
>> +     return dev_to_node(dev);
>>  }
>>  EXPORT_SYMBOL(pcibus_to_node);
>>
>> @@ -186,6 +195,8 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
>>       struct pci_bus *bus, *child;
>>       struct acpi_pci_root_ops *root_ops;
>>
>> +     set_dev_node(&root->device->dev, node);
>
> Ok I suspect this is one of those catch-22 issues with host bridge
> creation/scan code (like domain nr), let me have a look I will send
> you a patch next week when I figure out the call ordering required
> to make this work, I want to make sure we can make this work with
> DT and ACPI alike.
>
> Thanks !
> Lorenzo
>
>> +
>>       ri = kzalloc_node(sizeof(*ri), GFP_KERNEL, node);
>>       if (!ri)
>>               return NULL;
>> --
>> 2.12.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
>> the body of a message to majordomo at vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] arm64: acpi: Support PCI devices numa_node property in ACPI mode

[Lorenzo Pieralisi]

On Sat, Apr 08, 2017 at 01:17:32PM -0400, Sinan Kaya wrote:
> On 12/31/1969 7:00 PM, linux-arm-kernel [mailto:linux-arm-kernel-bounces at lists.infradead.org] On Behalf Of Sergey Temerkhanov wrote:
> >  int pcibus_to_node(struct pci_bus *bus)  {
> > -	return dev_to_node(&bus->dev);
> > +	struct pci_config_window *cfg = bus->sysdata;
> > +	struct acpi_device *adev = NULL;
> > +	struct device *dev;
> > +
> > +	if (!acpi_disabled)
> > +		adev = to_acpi_device(cfg->parent);
> > +
> 
> I see a problem here that NUMA node information is read from the
> parent device.  PCI bus can have multiple levels of switches and
> bridges. The NUMA information is only present on the host bridge.
> 
> This code only works if the endpoint is directly connected to the root
> port. 

That's not what this code does. This code retrieves the struct device
backing the ACPI device representing the PNP0A08 host bridge and its
dev->numa_node value (that was set in pci_acpi_scan_root()).

I am not a big fan of this. I wonder if we could not make it DT/ACPI
agnostic by simply setting the numa_node in the pci_bus->dev field,
and propagate it downstream a PCI hierarcy (as we do with sysdata)
in pci_alloc_child_bus().

This way pcibus_to_node() would become straightforward (ie as it
is now - provided the above is doable):

dev_to_node(&bus->dev);

This is suspiciously similar to the domain number song and dance
except that the NUMA node now is in the struct pci_bus->dev->numa_node
instead of struct pci_bus->domain_nr.

Lorenzo

[PATCH] arm64: acpi: Support PCI devices numa_node property in ACPI mode

[Sinan Kaya]

Hi Lorenzo,

On 4/12/2017 1:04 PM, Lorenzo Pieralisi wrote:
> On Sat, Apr 08, 2017 at 01:17:32PM -0400, Sinan Kaya wrote:
>> On 12/31/1969 7:00 PM, linux-arm-kernel [mailto:linux-arm-kernel-bounces at lists.infradead.org] On Behalf Of Sergey Temerkhanov wrote:
>>>  int pcibus_to_node(struct pci_bus *bus)  {
>>> -	return dev_to_node(&bus->dev);
>>> +	struct pci_config_window *cfg = bus->sysdata;
>>> +	struct acpi_device *adev = NULL;
>>> +	struct device *dev;
>>> +
>>> +	if (!acpi_disabled)
>>> +		adev = to_acpi_device(cfg->parent);
>>> +
>>
>> I see a problem here that NUMA node information is read from the
>> parent device.  PCI bus can have multiple levels of switches and
>> bridges. The NUMA information is only present on the host bridge.
>>
>> This code only works if the endpoint is directly connected to the root
>> port. 
> 
> That's not what this code does. This code retrieves the struct device
> backing the ACPI device representing the PNP0A08 host bridge and its
> dev->numa_node value (that was set in pci_acpi_scan_root()).

pcibus_to_node() gets called from two places. The first place is 
pci_register_host_bridge() like you mentioned. 

The second place is pci_device_add(). pci_device_add() gets called for
every single device that is getting added to the system. 

> 
> I am not a big fan of this. I wonder if we could not make it DT/ACPI
> agnostic by simply setting the numa_node in the pci_bus->dev field,
> and propagate it downstream a PCI hierarcy (as we do with sysdata)
> in pci_alloc_child_bus().

Yeah, that's what I was going to ask next about how other arches solve
this. Common solution to DT/ACPI is preferable as the issue exists in
both places unless DT has some other mechanism hidden behind the scenes.

> 
> This way pcibus_to_node() would become straightforward (ie as it
> is now - provided the above is doable):
> 
> dev_to_node(&bus->dev);
> 
> This is suspiciously similar to the domain number song and dance
> except that the NUMA node now is in the struct pci_bus->dev->numa_node
> instead of struct pci_bus->domain_nr.

I think this looks like the _CCA dance we are doing.

> 
> Lorenzo
> 

Sinan

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH] arm64: acpi: Support PCI devices numa_node property in ACPI mode

[Sinan Kaya]

On 4/12/2017 1:27 PM, Sinan Kaya wrote:
> I am not a big fan of this. I wonder if we could not make it DT/ACPI
> agnostic by simply setting the numa_node in the pci_bus->dev field,
> and propagate it downstream a PCI hierarcy (as we do with sysdata)
> in pci_alloc_child_bus().

I didn't realize that we were pushing sysdata to the child buses. No
issues then. 

I was under the impression that sysdata only existed for the host bridge
itself and was not shared with the rest of the bus.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH] arm64: acpi: Support PCI devices numa_node property in ACPI mode

[Lorenzo Pieralisi]

Hi Sergei,

On Fri, Apr 07, 2017 at 11:31:12AM -0700, Sergei Temerkhanov wrote:
> On Fri, Apr 7, 2017 at 10:18 AM, Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> > Hi Sergey,
> >
> > On Thu, Apr 06, 2017 at 11:47:11AM +0000, Sergey Temerkhanov wrote:
> >> This commit modifies ARM64-specific ACPI PCI support functions to
> >> properly set the numa_node property on host bridges and thus on PCI
> >> devices. Such support requires _PXM objects to be set in DSDT/SSDT
> >> tables describing PCI root bridges
> >>
> >> Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
> >> ---
> >>  arch/arm64/kernel/pci.c | 13 ++++++++++++-
> >>  1 file changed, 12 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> >> index 4f0e3ebfea4b..c45c7a26f984 100644
> >> --- a/arch/arm64/kernel/pci.c
> >> +++ b/arch/arm64/kernel/pci.c
> >> @@ -81,7 +81,16 @@ int raw_pci_write(unsigned int domain, unsigned int bus,
> >>
> >>  int pcibus_to_node(struct pci_bus *bus)
> >>  {
> >> -     return dev_to_node(&bus->dev);
> >> +     struct pci_config_window *cfg = bus->sysdata;
> >
> > Well, this implicit cast is a guess (ie it might be a DT sysdata
> > and there you can't have a clue on what it really is).
> 
> Yes, but it's only accessed when acpi_disabled != 0. I just tried to
> save few lines and brackets :-)

I have put together a patch, I will drop it on linux-pci next week
so that we can all have a look and test it.

Thanks !
Lorenzo

> Regards,
> Sergey
> 
> >
> >> +     struct acpi_device *adev = NULL;
> >> +     struct device *dev;
> >> +
> >> +     if (!acpi_disabled)
> >> +             adev = to_acpi_device(cfg->parent);
> >> +
> >> +     dev = adev ? &adev->dev : &bus->dev;
> >> +
> >> +     return dev_to_node(dev);
> >>  }
> >>  EXPORT_SYMBOL(pcibus_to_node);
> >>
> >> @@ -186,6 +195,8 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
> >>       struct pci_bus *bus, *child;
> >>       struct acpi_pci_root_ops *root_ops;
> >>
> >> +     set_dev_node(&root->device->dev, node);
> >
> > Ok I suspect this is one of those catch-22 issues with host bridge
> > creation/scan code (like domain nr), let me have a look I will send
> > you a patch next week when I figure out the call ordering required
> > to make this work, I want to make sure we can make this work with
> > DT and ACPI alike.
> >
> > Thanks !
> > Lorenzo
> >
> >> +
> >>       ri = kzalloc_node(sizeof(*ri), GFP_KERNEL, node);
> >>       if (!ri)
> >>               return NULL;
> >> --
> >> 2.12.1
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> >> the body of a message to majordomo at vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html