Re: [PATCH v2 1/8] powerpc/xive: Use cpu_to_node() instead of ibm,chip-id property

Re: [PATCH v2 1/8] powerpc/xive: Use cpu_to_node() instead of ibm,chip-id property

[Cédric Le Goater]

On 3/9/21 6:08 PM, Daniel Henrique Barboza wrote:
> 
> 
> On 3/9/21 12:33 PM, Cédric Le Goater wrote:
>> On 3/8/21 6:13 PM, Greg Kurz wrote:
>>> On Wed, 3 Mar 2021 18:48:50 +0100
>>> Cédric Le Goater <clg@kaod.org> wrote:
>>>
>>>> The 'chip_id' field of the XIVE CPU structure is used to choose a
>>>> target for a source located on the same chip when possible. This field
>>>> is assigned on the PowerNV platform using the "ibm,chip-id" property
>>>> on pSeries under KVM when NUMA nodes are defined but it is undefined
>>>
>>> This sentence seems to have a syntax problem... like it is missing an
>>> 'and' before 'on pSeries'.
>>
>> ah yes, or simply a comma.
>>
>>>> under PowerVM. The XIVE source structure has a similar field
>>>> 'src_chip' which is only assigned on the PowerNV platform.
>>>>
>>>> cpu_to_node() returns a compatible value on all platforms, 0 being the
>>>> default node. It will also give us the opportunity to set the affinity
>>>> of a source on pSeries when we can localize them.
>>>>
>>>
>>> IIUC this relies on the fact that the NUMA node id is == to chip id
>>> on PowerNV, i.e. xc->chip_id which is passed to OPAL remain stable
>>> with this change.
>>
>> Linux sets the NUMA node in numa_setup_cpu(). On pseries, the hcall
>> H_HOME_NODE_ASSOCIATIVITY returns the node id if I am correct (Daniel
>> in Cc:)
> 
> That's correct. H_HOME_NODE_ASSOCIATIVITY returns not only the node_id, but
> a list with the ibm,associativity domains of the CPU that "proc-no" (processor
> identifier) is mapped to inside QEMU.
> 
> node_id in this case, considering that we're working with a reference-points
> of size 4, is the 4th element of the returned list. The last element is
> "procno" itself.
> 
> 
>>
>> On PowerNV, Linux uses "ibm,associativity" property of the CPU to find
>> the node id. This value is built from the chip id in OPAL, so the
>> value returned by cpu_to_node(cpu) and the value of the "ibm,chip-id"
>> property are unlikely to be different.
>>
>> cpu_to_node(cpu) is used in many places to allocate the structures
>> locally to the owning node. XIVE is not an exception (see below in the
>> same patch), it is better to be consistent and get the same information
>> (node id) using the same routine.
>>
>>
>> In Linux, "ibm,chip-id" is only used in low level PowerNV drivers :
>> LPC, XSCOM, RNG, VAS, NX. XIVE should be in that list also but skiboot
>> unifies the controllers of the system to only expose one the OS. This
>> is problematic and should be changed but it's another topic.
>>
>>
>>> On the other hand, you have the pSeries case under PowerVM that
>>> doesn't xc->chip_id, which isn't passed to any hcall AFAICT.
>>
>> yes "ibm,chip-id" is an OPAL concept unfortunately and it has no meaning
>> under PAPR. xc->chip_id on pseries (PowerVM) will contains an invalid
>> chip id.
>>
>> QEMU/KVM exposes "ibm,chip-id" but it's not used. (its value is not
>> always correct btw)
> 
> 
> If you have a way to reliably reproduce this, let me know and I'll fix it
> up in QEMU.

with :

   -smp 4,cores=1,maxcpus=8 -object memory-backend-ram,id=ram-node0,size=2G -numa node,nodeid=0,cpus=0-1,cpus=4-5,memdev=ram-node0 -object memory-backend-ram,id=ram-node1,size=2G -numa node,nodeid=1,cpus=2-3,cpus=6-7,memdev=ram-node1

# dmesg | grep numa
[    0.013106] numa: Node 0 CPUs: 0-1
[    0.013136] numa: Node 1 CPUs: 2-3

# dtc -I fs /proc/device-tree/cpus/ -f | grep ibm,chip-id
		ibm,chip-id = <0x01>;
		ibm,chip-id = <0x02>;
		ibm,chip-id = <0x00>;
		ibm,chip-id = <0x03>;

with :

  -smp 4,cores=4,maxcpus=8,threads=1 -object memory-backend-ram,id=ram-node0,size=2G -numa node,nodeid=0,cpus=0-1,cpus=4-5,memdev=ram-node0 -object memory-backend-ram,id=ram-node1,size=2G -numa node,nodeid=1,cpus=2-3,cpus=6-7,memdev=ram-node1

# dmesg | grep numa
[    0.013106] numa: Node 0 CPUs: 0-1
[    0.013136] numa: Node 1 CPUs: 2-3

# dtc -I fs /proc/device-tree/cpus/ -f | grep ibm,chip-id
		ibm,chip-id = <0x00>;
		ibm,chip-id = <0x00>;
		ibm,chip-id = <0x00>;
		ibm,chip-id = <0x00>;

I think we should simply remove "ibm,chip-id" since it's not used and
not in the PAPR spec.

Thanks,

C.

 

> 
> Thanks,
> 
> 
> DHB
> 
> 
>>
>>> It looks like the chip id is only used for localization purpose in
>>> this case, right ?
>>
>> Yes and PAPR sources are not localized. So it's not used. MSI sources
>> could be if we rewrote the MSI driver.
>>
>>> In this case, what about doing this change for pSeries only,
>>> somewhere in spapr.c ?
>>
>> The IPI code is common to all platforms and all have the same issue.
>> I rather not.
>>
>> Thanks,
>>
>> C.
>>  
>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>> ---
>>>>   arch/powerpc/sysdev/xive/common.c | 7 +------
>>>>   1 file changed, 1 insertion(+), 6 deletions(-)
>>>>
>>>> diff --git a/arch/powerpc/sysdev/xive/common.c b/arch/powerpc/sysdev/xive/common.c
>>>> index 595310e056f4..b8e456da28aa 100644
>>>> --- a/arch/powerpc/sysdev/xive/common.c
>>>> +++ b/arch/powerpc/sysdev/xive/common.c
>>>> @@ -1335,16 +1335,11 @@ static int xive_prepare_cpu(unsigned int cpu)
>>>>         xc = per_cpu(xive_cpu, cpu);
>>>>       if (!xc) {
>>>> -        struct device_node *np;
>>>> -
>>>>           xc = kzalloc_node(sizeof(struct xive_cpu),
>>>>                     GFP_KERNEL, cpu_to_node(cpu));
>>>>           if (!xc)
>>>>               return -ENOMEM;
>>>> -        np = of_get_cpu_node(cpu, NULL);
>>>> -        if (np)
>>>> -            xc->chip_id = of_get_ibm_chip_id(np);
>>>> -        of_node_put(np);
>>>> +        xc->chip_id = cpu_to_node(cpu);
>>>>           xc->hw_ipi = XIVE_BAD_IRQ;
>>>>             per_cpu(xive_cpu, cpu) = xc;
>>>
>>

Re: [PATCH v2 1/8] powerpc/xive: Use cpu_to_node() instead of ibm,chip-id property

[David Gibson]

On Tue, 9 Mar 2021 18:26:35 +0100
Cédric Le Goater <clg@kaod.org> wrote:

> On 3/9/21 6:08 PM, Daniel Henrique Barboza wrote:
> > 
> > 
> > On 3/9/21 12:33 PM, Cédric Le Goater wrote:  
> >> On 3/8/21 6:13 PM, Greg Kurz wrote:  
> >>> On Wed, 3 Mar 2021 18:48:50 +0100
> >>> Cédric Le Goater <clg@kaod.org> wrote:
> >>>  
> >>>> The 'chip_id' field of the XIVE CPU structure is used to choose a
> >>>> target for a source located on the same chip when possible. This field
> >>>> is assigned on the PowerNV platform using the "ibm,chip-id" property
> >>>> on pSeries under KVM when NUMA nodes are defined but it is undefined  
> >>>
> >>> This sentence seems to have a syntax problem... like it is missing an
> >>> 'and' before 'on pSeries'.  
> >>
> >> ah yes, or simply a comma.
> >>  
> >>>> under PowerVM. The XIVE source structure has a similar field
> >>>> 'src_chip' which is only assigned on the PowerNV platform.
> >>>>
> >>>> cpu_to_node() returns a compatible value on all platforms, 0 being the
> >>>> default node. It will also give us the opportunity to set the affinity
> >>>> of a source on pSeries when we can localize them.
> >>>>  
> >>>
> >>> IIUC this relies on the fact that the NUMA node id is == to chip id
> >>> on PowerNV, i.e. xc->chip_id which is passed to OPAL remain stable
> >>> with this change.  
> >>
> >> Linux sets the NUMA node in numa_setup_cpu(). On pseries, the hcall
> >> H_HOME_NODE_ASSOCIATIVITY returns the node id if I am correct (Daniel
> >> in Cc:)  
>  [...]  
> >>
> >> On PowerNV, Linux uses "ibm,associativity" property of the CPU to find
> >> the node id. This value is built from the chip id in OPAL, so the
> >> value returned by cpu_to_node(cpu) and the value of the "ibm,chip-id"
> >> property are unlikely to be different.
> >>
> >> cpu_to_node(cpu) is used in many places to allocate the structures
> >> locally to the owning node. XIVE is not an exception (see below in the
> >> same patch), it is better to be consistent and get the same information
> >> (node id) using the same routine.
> >>
> >>
> >> In Linux, "ibm,chip-id" is only used in low level PowerNV drivers :
> >> LPC, XSCOM, RNG, VAS, NX. XIVE should be in that list also but skiboot
> >> unifies the controllers of the system to only expose one the OS. This
> >> is problematic and should be changed but it's another topic.
> >>
> >>  
> >>> On the other hand, you have the pSeries case under PowerVM that
> >>> doesn't xc->chip_id, which isn't passed to any hcall AFAICT.  
> >>
> >> yes "ibm,chip-id" is an OPAL concept unfortunately and it has no meaning
> >> under PAPR. xc->chip_id on pseries (PowerVM) will contains an invalid
> >> chip id.
> >>
> >> QEMU/KVM exposes "ibm,chip-id" but it's not used. (its value is not
> >> always correct btw)  
> > 
> > 
> > If you have a way to reliably reproduce this, let me know and I'll fix it
> > up in QEMU.  
> 
> with :
> 
>    -smp 4,cores=1,maxcpus=8 -object memory-backend-ram,id=ram-node0,size=2G -numa node,nodeid=0,cpus=0-1,cpus=4-5,memdev=ram-node0 -object memory-backend-ram,id=ram-node1,size=2G -numa node,nodeid=1,cpus=2-3,cpus=6-7,memdev=ram-node1
> 
> # dmesg | grep numa
> [    0.013106] numa: Node 0 CPUs: 0-1
> [    0.013136] numa: Node 1 CPUs: 2-3
> 
> # dtc -I fs /proc/device-tree/cpus/ -f | grep ibm,chip-id
> 		ibm,chip-id = <0x01>;
> 		ibm,chip-id = <0x02>;
> 		ibm,chip-id = <0x00>;
> 		ibm,chip-id = <0x03>;
> 
> with :
> 
>   -smp 4,cores=4,maxcpus=8,threads=1 -object memory-backend-ram,id=ram-node0,size=2G -numa node,nodeid=0,cpus=0-1,cpus=4-5,memdev=ram-node0 -object memory-backend-ram,id=ram-node1,size=2G -numa node,nodeid=1,cpus=2-3,cpus=6-7,memdev=ram-node1
> 
> # dmesg | grep numa
> [    0.013106] numa: Node 0 CPUs: 0-1
> [    0.013136] numa: Node 1 CPUs: 2-3
> 
> # dtc -I fs /proc/device-tree/cpus/ -f | grep ibm,chip-id
> 		ibm,chip-id = <0x00>;
> 		ibm,chip-id = <0x00>;
> 		ibm,chip-id = <0x00>;
> 		ibm,chip-id = <0x00>;
> 
> I think we should simply remove "ibm,chip-id" since it's not used and
> not in the PAPR spec.

As I mentioned to Daniel on our call this morning, oddly it *does*
appear to be used in the RHEL kernel, even though that's 4.18 based.
This patch seems to have caused a minor regression; not in the
identification of NUMA nodes, but in the number of sockets shown be
lscpu, etc.  See https://bugzilla.redhat.com/show_bug.cgi?id=1934421
for more information.

Since the value was used by some PAPR kernels - even if they shouldn't
have - I think we should only remove this for newer machine types.  We
also need to check what we're not supplying that the guest kernel is
showing a different number of sockets than specified on the qemu
command line.

> 
> Thanks,
> 
> C.
> 
>  
> 
>  [...]  
>  [...]  
>  [...]  
>  [...]  
>  [...]  
>  [...]  
>  [...]  
>  [...]  
>  [...]  
> 


-- 
David Gibson <dgibson@redhat.com>
Principal Software Engineer, Virtualization, Red Hat

Re: [PATCH v2 1/8] powerpc/xive: Use cpu_to_node() instead of ibm,chip-id property

[Cédric Le Goater]

On 3/12/21 2:55 AM, David Gibson wrote:
> On Tue, 9 Mar 2021 18:26:35 +0100
> Cédric Le Goater <clg@kaod.org> wrote:
> 
>> On 3/9/21 6:08 PM, Daniel Henrique Barboza wrote:
>>>
>>>
>>> On 3/9/21 12:33 PM, Cédric Le Goater wrote:  
>>>> On 3/8/21 6:13 PM, Greg Kurz wrote:  
>>>>> On Wed, 3 Mar 2021 18:48:50 +0100
>>>>> Cédric Le Goater <clg@kaod.org> wrote:
>>>>>  
>>>>>> The 'chip_id' field of the XIVE CPU structure is used to choose a
>>>>>> target for a source located on the same chip when possible. This field
>>>>>> is assigned on the PowerNV platform using the "ibm,chip-id" property
>>>>>> on pSeries under KVM when NUMA nodes are defined but it is undefined  
>>>>>
>>>>> This sentence seems to have a syntax problem... like it is missing an
>>>>> 'and' before 'on pSeries'.  
>>>>
>>>> ah yes, or simply a comma.
>>>>  
>>>>>> under PowerVM. The XIVE source structure has a similar field
>>>>>> 'src_chip' which is only assigned on the PowerNV platform.
>>>>>>
>>>>>> cpu_to_node() returns a compatible value on all platforms, 0 being the
>>>>>> default node. It will also give us the opportunity to set the affinity
>>>>>> of a source on pSeries when we can localize them.
>>>>>>  
>>>>>
>>>>> IIUC this relies on the fact that the NUMA node id is == to chip id
>>>>> on PowerNV, i.e. xc->chip_id which is passed to OPAL remain stable
>>>>> with this change.  
>>>>
>>>> Linux sets the NUMA node in numa_setup_cpu(). On pseries, the hcall
>>>> H_HOME_NODE_ASSOCIATIVITY returns the node id if I am correct (Daniel
>>>> in Cc:)  
>>  [...]  
>>>>
>>>> On PowerNV, Linux uses "ibm,associativity" property of the CPU to find
>>>> the node id. This value is built from the chip id in OPAL, so the
>>>> value returned by cpu_to_node(cpu) and the value of the "ibm,chip-id"
>>>> property are unlikely to be different.
>>>>
>>>> cpu_to_node(cpu) is used in many places to allocate the structures
>>>> locally to the owning node. XIVE is not an exception (see below in the
>>>> same patch), it is better to be consistent and get the same information
>>>> (node id) using the same routine.
>>>>
>>>>
>>>> In Linux, "ibm,chip-id" is only used in low level PowerNV drivers :
>>>> LPC, XSCOM, RNG, VAS, NX. XIVE should be in that list also but skiboot
>>>> unifies the controllers of the system to only expose one the OS. This
>>>> is problematic and should be changed but it's another topic.
>>>>
>>>>  
>>>>> On the other hand, you have the pSeries case under PowerVM that
>>>>> doesn't xc->chip_id, which isn't passed to any hcall AFAICT.  
>>>>
>>>> yes "ibm,chip-id" is an OPAL concept unfortunately and it has no meaning
>>>> under PAPR. xc->chip_id on pseries (PowerVM) will contains an invalid
>>>> chip id.
>>>>
>>>> QEMU/KVM exposes "ibm,chip-id" but it's not used. (its value is not
>>>> always correct btw)  
>>>
>>>
>>> If you have a way to reliably reproduce this, let me know and I'll fix it
>>> up in QEMU.  
>>
>> with :
>>
>>    -smp 4,cores=1,maxcpus=8 -object memory-backend-ram,id=ram-node0,size=2G -numa node,nodeid=0,cpus=0-1,cpus=4-5,memdev=ram-node0 -object memory-backend-ram,id=ram-node1,size=2G -numa node,nodeid=1,cpus=2-3,cpus=6-7,memdev=ram-node1
>>
>> # dmesg | grep numa
>> [    0.013106] numa: Node 0 CPUs: 0-1
>> [    0.013136] numa: Node 1 CPUs: 2-3
>>
>> # dtc -I fs /proc/device-tree/cpus/ -f | grep ibm,chip-id
>> 		ibm,chip-id = <0x01>;
>> 		ibm,chip-id = <0x02>;
>> 		ibm,chip-id = <0x00>;
>> 		ibm,chip-id = <0x03>;
>>
>> with :
>>
>>   -smp 4,cores=4,maxcpus=8,threads=1 -object memory-backend-ram,id=ram-node0,size=2G -numa node,nodeid=0,cpus=0-1,cpus=4-5,memdev=ram-node0 -object memory-backend-ram,id=ram-node1,size=2G -numa node,nodeid=1,cpus=2-3,cpus=6-7,memdev=ram-node1
>>
>> # dmesg | grep numa
>> [    0.013106] numa: Node 0 CPUs: 0-1
>> [    0.013136] numa: Node 1 CPUs: 2-3
>>
>> # dtc -I fs /proc/device-tree/cpus/ -f | grep ibm,chip-id
>> 		ibm,chip-id = <0x00>;
>> 		ibm,chip-id = <0x00>;
>> 		ibm,chip-id = <0x00>;
>> 		ibm,chip-id = <0x00>;
>>
>> I think we should simply remove "ibm,chip-id" since it's not used and
>> not in the PAPR spec.
> 
> As I mentioned to Daniel on our call this morning, oddly it *does*
> appear to be used in the RHEL kernel, even though that's 4.18 based.
> This patch seems to have caused a minor regression; not in the
> identification of NUMA nodes, but in the number of sockets shown be
> lscpu, etc.  See https://bugzilla.redhat.com/show_bug.cgi?id=1934421
> for more information.

Yes. The property "ibm,chip-id" is wrongly calculated in QEMU. If we 
remove it, we get with 4.18.0-295.el8.ppc64le or 5.12.0-rc2 :

   [root@localhost ~]# lscpu 
   Architecture:        ppc64le
   Byte Order:          Little Endian
   CPU(s):              128
   On-line CPU(s) list: 0-127
   Thread(s) per core:  4
   Core(s) per socket:  16
   Socket(s):           2
   NUMA node(s):        2
   Model:               2.2 (pvr 004e 1202)
   Model name:          POWER9 (architected), altivec supported
   Hypervisor vendor:   KVM
   Virtualization type: para
   L1d cache:           32K
   L1i cache:           32K
   NUMA node0 CPU(s):   0-63
   NUMA node1 CPU(s):   64-127

   [root@localhost ~]# grep . /sys/devices/system/cpu/*/topology/physical_package_id
   /sys/devices/system/cpu/cpu0/topology/physical_package_id:-1
   /sys/devices/system/cpu/cpu100/topology/physical_package_id:-1
   /sys/devices/system/cpu/cpu101/topology/physical_package_id:-1
   /sys/devices/system/cpu/cpu102/topology/physical_package_id:-1
   /sys/devices/system/cpu/cpu103/topology/physical_package_id:-1
   ....

"ibm,chip-id" is still being used on some occasion on pSeries machines.
This is wrong :/ The problem is :

  #define topology_physical_package_id(cpu)      (cpu_to_chip_id(cpu))

We should be using cpu_to_node(). 

C.

> 
> Since the value was used by some PAPR kernels - even if they shouldn't
> have - I think we should only remove this for newer machine types.  We
> also need to check what we're not supplying that the guest kernel is
> showing a different number of sockets than specified on the qemu
> command line.
> 
>>
>> Thanks,
>>
>> C.
>>
>>  
>>
>>  [...]  
>>  [...]  
>>  [...]  
>>  [...]  
>>  [...]  
>>  [...]  
>>  [...]  
>>  [...]  
>>  [...]  
>>
> 
> 

Re: [PATCH v2 1/8] powerpc/xive: Use cpu_to_node() instead of ibm,chip-id property

[Daniel Henrique Barboza]

On 3/12/21 6:53 AM, Cédric Le Goater wrote:
> On 3/12/21 2:55 AM, David Gibson wrote:
>> On Tue, 9 Mar 2021 18:26:35 +0100
>> Cédric Le Goater <clg@kaod.org> wrote:
>>
>>> On 3/9/21 6:08 PM, Daniel Henrique Barboza wrote:
>>>>
>>>>
>>>> On 3/9/21 12:33 PM, Cédric Le Goater wrote:
>>>>> On 3/8/21 6:13 PM, Greg Kurz wrote:
>>>>>> On Wed, 3 Mar 2021 18:48:50 +0100
>>>>>> Cédric Le Goater <clg@kaod.org> wrote:
>>>>>>   
>>>>>>> The 'chip_id' field of the XIVE CPU structure is used to choose a
>>>>>>> target for a source located on the same chip when possible. This field
>>>>>>> is assigned on the PowerNV platform using the "ibm,chip-id" property
>>>>>>> on pSeries under KVM when NUMA nodes are defined but it is undefined
>>>>>>
>>>>>> This sentence seems to have a syntax problem... like it is missing an
>>>>>> 'and' before 'on pSeries'.
>>>>>
>>>>> ah yes, or simply a comma.
>>>>>   
>>>>>>> under PowerVM. The XIVE source structure has a similar field
>>>>>>> 'src_chip' which is only assigned on the PowerNV platform.
>>>>>>>
>>>>>>> cpu_to_node() returns a compatible value on all platforms, 0 being the
>>>>>>> default node. It will also give us the opportunity to set the affinity
>>>>>>> of a source on pSeries when we can localize them.
>>>>>>>   
>>>>>>
>>>>>> IIUC this relies on the fact that the NUMA node id is == to chip id
>>>>>> on PowerNV, i.e. xc->chip_id which is passed to OPAL remain stable
>>>>>> with this change.
>>>>>
>>>>> Linux sets the NUMA node in numa_setup_cpu(). On pseries, the hcall
>>>>> H_HOME_NODE_ASSOCIATIVITY returns the node id if I am correct (Daniel
>>>>> in Cc:)
>>>   [...]
>>>>>
>>>>> On PowerNV, Linux uses "ibm,associativity" property of the CPU to find
>>>>> the node id. This value is built from the chip id in OPAL, so the
>>>>> value returned by cpu_to_node(cpu) and the value of the "ibm,chip-id"
>>>>> property are unlikely to be different.
>>>>>
>>>>> cpu_to_node(cpu) is used in many places to allocate the structures
>>>>> locally to the owning node. XIVE is not an exception (see below in the
>>>>> same patch), it is better to be consistent and get the same information
>>>>> (node id) using the same routine.
>>>>>
>>>>>
>>>>> In Linux, "ibm,chip-id" is only used in low level PowerNV drivers :
>>>>> LPC, XSCOM, RNG, VAS, NX. XIVE should be in that list also but skiboot
>>>>> unifies the controllers of the system to only expose one the OS. This
>>>>> is problematic and should be changed but it's another topic.
>>>>>
>>>>>   
>>>>>> On the other hand, you have the pSeries case under PowerVM that
>>>>>> doesn't xc->chip_id, which isn't passed to any hcall AFAICT.
>>>>>
>>>>> yes "ibm,chip-id" is an OPAL concept unfortunately and it has no meaning
>>>>> under PAPR. xc->chip_id on pseries (PowerVM) will contains an invalid
>>>>> chip id.
>>>>>
>>>>> QEMU/KVM exposes "ibm,chip-id" but it's not used. (its value is not
>>>>> always correct btw)
>>>>
>>>>
>>>> If you have a way to reliably reproduce this, let me know and I'll fix it
>>>> up in QEMU.
>>>
>>> with :
>>>
>>>     -smp 4,cores=1,maxcpus=8 -object memory-backend-ram,id=ram-node0,size=2G -numa node,nodeid=0,cpus=0-1,cpus=4-5,memdev=ram-node0 -object memory-backend-ram,id=ram-node1,size=2G -numa node,nodeid=1,cpus=2-3,cpus=6-7,memdev=ram-node1
>>>
>>> # dmesg | grep numa
>>> [    0.013106] numa: Node 0 CPUs: 0-1
>>> [    0.013136] numa: Node 1 CPUs: 2-3
>>>
>>> # dtc -I fs /proc/device-tree/cpus/ -f | grep ibm,chip-id
>>> 		ibm,chip-id = <0x01>;
>>> 		ibm,chip-id = <0x02>;
>>> 		ibm,chip-id = <0x00>;
>>> 		ibm,chip-id = <0x03>;
>>>
>>> with :
>>>
>>>    -smp 4,cores=4,maxcpus=8,threads=1 -object memory-backend-ram,id=ram-node0,size=2G -numa node,nodeid=0,cpus=0-1,cpus=4-5,memdev=ram-node0 -object memory-backend-ram,id=ram-node1,size=2G -numa node,nodeid=1,cpus=2-3,cpus=6-7,memdev=ram-node1
>>>
>>> # dmesg | grep numa
>>> [    0.013106] numa: Node 0 CPUs: 0-1
>>> [    0.013136] numa: Node 1 CPUs: 2-3
>>>
>>> # dtc -I fs /proc/device-tree/cpus/ -f | grep ibm,chip-id
>>> 		ibm,chip-id = <0x00>;
>>> 		ibm,chip-id = <0x00>;
>>> 		ibm,chip-id = <0x00>;
>>> 		ibm,chip-id = <0x00>;
>>>
>>> I think we should simply remove "ibm,chip-id" since it's not used and
>>> not in the PAPR spec.
>>
>> As I mentioned to Daniel on our call this morning, oddly it *does*
>> appear to be used in the RHEL kernel, even though that's 4.18 based.
>> This patch seems to have caused a minor regression; not in the
>> identification of NUMA nodes, but in the number of sockets shown be
>> lscpu, etc.  See https://bugzilla.redhat.com/show_bug.cgi?id=1934421
>> for more information.
> 
> Yes. The property "ibm,chip-id" is wrongly calculated in QEMU. If we
> remove it, we get with 4.18.0-295.el8.ppc64le or 5.12.0-rc2 :
> 
>     [root@localhost ~]# lscpu
>     Architecture:        ppc64le
>     Byte Order:          Little Endian
>     CPU(s):              128
>     On-line CPU(s) list: 0-127
>     Thread(s) per core:  4
>     Core(s) per socket:  16
>     Socket(s):           2
>     NUMA node(s):        2
>     Model:               2.2 (pvr 004e 1202)
>     Model name:          POWER9 (architected), altivec supported
>     Hypervisor vendor:   KVM
>     Virtualization type: para
>     L1d cache:           32K
>     L1i cache:           32K
>     NUMA node0 CPU(s):   0-63
>     NUMA node1 CPU(s):   64-127
> 
>     [root@localhost ~]# grep . /sys/devices/system/cpu/*/topology/physical_package_id
>     /sys/devices/system/cpu/cpu0/topology/physical_package_id:-1
>     /sys/devices/system/cpu/cpu100/topology/physical_package_id:-1
>     /sys/devices/system/cpu/cpu101/topology/physical_package_id:-1
>     /sys/devices/system/cpu/cpu102/topology/physical_package_id:-1
>     /sys/devices/system/cpu/cpu103/topology/physical_package_id:-1
>     ....
> 
> "ibm,chip-id" is still being used on some occasion on pSeries machines.
> This is wrong :/ The problem is :
> 
>    #define topology_physical_package_id(cpu)      (cpu_to_chip_id(cpu))
> 
> We should be using cpu_to_node().


IIUC the "real fix" then is this change you mentioned above, together with
this xive patch as well, to stop using ibm,chip-id for good in the pserie
  kernel. With these changes QEMU can remove 'ibm,chip-id' from the pseries
machine without impact. Is this correct?

If that's the case, then I believe it's ok to go forward with the QEMU side
change (just for 6.0.0 and newer machines). Or should I wait for the kernel
changes to be merged upstream first?


Thanks,


DHB


> 
> C.
> 
>>
>> Since the value was used by some PAPR kernels - even if they shouldn't
>> have - I think we should only remove this for newer machine types.  We
>> also need to check what we're not supplying that the guest kernel is
>> showing a different number of sockets than specified on the qemu
>> command line.
>>
>>>
>>> Thanks,
>>>
>>> C.
>>>
>>>   
>>>
>>>   [...]
>>>   [...]
>>>   [...]
>>>   [...]
>>>   [...]
>>>   [...]
>>>   [...]
>>>   [...]
>>>   [...]
>>>
>>
>>
> 

Re: [PATCH v2 1/8] powerpc/xive: Use cpu_to_node() instead of ibm,chip-id property

[Greg Kurz]

On Fri, 12 Mar 2021 09:18:39 -0300
Daniel Henrique Barboza <danielhb@linux.ibm.com> wrote:

> 
> 
> On 3/12/21 6:53 AM, Cédric Le Goater wrote:
> > On 3/12/21 2:55 AM, David Gibson wrote:
> >> On Tue, 9 Mar 2021 18:26:35 +0100
> >> Cédric Le Goater <clg@kaod.org> wrote:
> >>
> >>> On 3/9/21 6:08 PM, Daniel Henrique Barboza wrote:
> >>>>
> >>>>
> >>>> On 3/9/21 12:33 PM, Cédric Le Goater wrote:
> >>>>> On 3/8/21 6:13 PM, Greg Kurz wrote:
> >>>>>> On Wed, 3 Mar 2021 18:48:50 +0100
> >>>>>> Cédric Le Goater <clg@kaod.org> wrote:
> >>>>>>   
> >>>>>>> The 'chip_id' field of the XIVE CPU structure is used to choose a
> >>>>>>> target for a source located on the same chip when possible. This field
> >>>>>>> is assigned on the PowerNV platform using the "ibm,chip-id" property
> >>>>>>> on pSeries under KVM when NUMA nodes are defined but it is undefined
> >>>>>>
> >>>>>> This sentence seems to have a syntax problem... like it is missing an
> >>>>>> 'and' before 'on pSeries'.
> >>>>>
> >>>>> ah yes, or simply a comma.
> >>>>>   
> >>>>>>> under PowerVM. The XIVE source structure has a similar field
> >>>>>>> 'src_chip' which is only assigned on the PowerNV platform.
> >>>>>>>
> >>>>>>> cpu_to_node() returns a compatible value on all platforms, 0 being the
> >>>>>>> default node. It will also give us the opportunity to set the affinity
> >>>>>>> of a source on pSeries when we can localize them.
> >>>>>>>   
> >>>>>>
> >>>>>> IIUC this relies on the fact that the NUMA node id is == to chip id
> >>>>>> on PowerNV, i.e. xc->chip_id which is passed to OPAL remain stable
> >>>>>> with this change.
> >>>>>
> >>>>> Linux sets the NUMA node in numa_setup_cpu(). On pseries, the hcall
> >>>>> H_HOME_NODE_ASSOCIATIVITY returns the node id if I am correct (Daniel
> >>>>> in Cc:)
> >>>   [...]
> >>>>>
> >>>>> On PowerNV, Linux uses "ibm,associativity" property of the CPU to find
> >>>>> the node id. This value is built from the chip id in OPAL, so the
> >>>>> value returned by cpu_to_node(cpu) and the value of the "ibm,chip-id"
> >>>>> property are unlikely to be different.
> >>>>>
> >>>>> cpu_to_node(cpu) is used in many places to allocate the structures
> >>>>> locally to the owning node. XIVE is not an exception (see below in the
> >>>>> same patch), it is better to be consistent and get the same information
> >>>>> (node id) using the same routine.
> >>>>>
> >>>>>
> >>>>> In Linux, "ibm,chip-id" is only used in low level PowerNV drivers :
> >>>>> LPC, XSCOM, RNG, VAS, NX. XIVE should be in that list also but skiboot
> >>>>> unifies the controllers of the system to only expose one the OS. This
> >>>>> is problematic and should be changed but it's another topic.
> >>>>>
> >>>>>   
> >>>>>> On the other hand, you have the pSeries case under PowerVM that
> >>>>>> doesn't xc->chip_id, which isn't passed to any hcall AFAICT.
> >>>>>
> >>>>> yes "ibm,chip-id" is an OPAL concept unfortunately and it has no meaning
> >>>>> under PAPR. xc->chip_id on pseries (PowerVM) will contains an invalid
> >>>>> chip id.
> >>>>>
> >>>>> QEMU/KVM exposes "ibm,chip-id" but it's not used. (its value is not
> >>>>> always correct btw)
> >>>>
> >>>>
> >>>> If you have a way to reliably reproduce this, let me know and I'll fix it
> >>>> up in QEMU.
> >>>
> >>> with :
> >>>
> >>>     -smp 4,cores=1,maxcpus=8 -object memory-backend-ram,id=ram-node0,size=2G -numa node,nodeid=0,cpus=0-1,cpus=4-5,memdev=ram-node0 -object memory-backend-ram,id=ram-node1,size=2G -numa node,nodeid=1,cpus=2-3,cpus=6-7,memdev=ram-node1
> >>>
> >>> # dmesg | grep numa
> >>> [    0.013106] numa: Node 0 CPUs: 0-1
> >>> [    0.013136] numa: Node 1 CPUs: 2-3
> >>>
> >>> # dtc -I fs /proc/device-tree/cpus/ -f | grep ibm,chip-id
> >>> 		ibm,chip-id = <0x01>;
> >>> 		ibm,chip-id = <0x02>;
> >>> 		ibm,chip-id = <0x00>;
> >>> 		ibm,chip-id = <0x03>;
> >>>
> >>> with :
> >>>
> >>>    -smp 4,cores=4,maxcpus=8,threads=1 -object memory-backend-ram,id=ram-node0,size=2G -numa node,nodeid=0,cpus=0-1,cpus=4-5,memdev=ram-node0 -object memory-backend-ram,id=ram-node1,size=2G -numa node,nodeid=1,cpus=2-3,cpus=6-7,memdev=ram-node1
> >>>
> >>> # dmesg | grep numa
> >>> [    0.013106] numa: Node 0 CPUs: 0-1
> >>> [    0.013136] numa: Node 1 CPUs: 2-3
> >>>
> >>> # dtc -I fs /proc/device-tree/cpus/ -f | grep ibm,chip-id
> >>> 		ibm,chip-id = <0x00>;
> >>> 		ibm,chip-id = <0x00>;
> >>> 		ibm,chip-id = <0x00>;
> >>> 		ibm,chip-id = <0x00>;
> >>>
> >>> I think we should simply remove "ibm,chip-id" since it's not used and
> >>> not in the PAPR spec.
> >>
> >> As I mentioned to Daniel on our call this morning, oddly it *does*
> >> appear to be used in the RHEL kernel, even though that's 4.18 based.
> >> This patch seems to have caused a minor regression; not in the
> >> identification of NUMA nodes, but in the number of sockets shown be
> >> lscpu, etc.  See https://bugzilla.redhat.com/show_bug.cgi?id=1934421
> >> for more information.
> > 
> > Yes. The property "ibm,chip-id" is wrongly calculated in QEMU. If we
> > remove it, we get with 4.18.0-295.el8.ppc64le or 5.12.0-rc2 :
> > 
> >     [root@localhost ~]# lscpu
> >     Architecture:        ppc64le
> >     Byte Order:          Little Endian
> >     CPU(s):              128
> >     On-line CPU(s) list: 0-127
> >     Thread(s) per core:  4
> >     Core(s) per socket:  16
> >     Socket(s):           2
> >     NUMA node(s):        2
> >     Model:               2.2 (pvr 004e 1202)
> >     Model name:          POWER9 (architected), altivec supported
> >     Hypervisor vendor:   KVM
> >     Virtualization type: para
> >     L1d cache:           32K
> >     L1i cache:           32K
> >     NUMA node0 CPU(s):   0-63
> >     NUMA node1 CPU(s):   64-127
> > 
> >     [root@localhost ~]# grep . /sys/devices/system/cpu/*/topology/physical_package_id
> >     /sys/devices/system/cpu/cpu0/topology/physical_package_id:-1
> >     /sys/devices/system/cpu/cpu100/topology/physical_package_id:-1
> >     /sys/devices/system/cpu/cpu101/topology/physical_package_id:-1
> >     /sys/devices/system/cpu/cpu102/topology/physical_package_id:-1
> >     /sys/devices/system/cpu/cpu103/topology/physical_package_id:-1
> >     ....
> > 
> > "ibm,chip-id" is still being used on some occasion on pSeries machines.
> > This is wrong :/ The problem is :
> > 
> >    #define topology_physical_package_id(cpu)      (cpu_to_chip_id(cpu))
> > 
> > We should be using cpu_to_node().
> 
> 
> IIUC the "real fix" then is this change you mentioned above, together with
> this xive patch as well, to stop using ibm,chip-id for good in the pserie
>   kernel. With these changes QEMU can remove 'ibm,chip-id' from the pseries
> machine without impact. Is this correct?
> 
> If that's the case, then I believe it's ok to go forward with the QEMU side
> change (just for 6.0.0 and newer machines). Or should I wait for the kernel
> changes to be merged upstream first?
> 

I'd say the latter since this is a breaking change and people will want
to identify the upstream commits they have to backport to their kernel
in order to support the disappearance of "ibm,chip-id".

Cheers,

--
Greg

> 
> Thanks,
> 
> 
> DHB
> 
> 
> > 
> > C.
> > 
> >>
> >> Since the value was used by some PAPR kernels - even if they shouldn't
> >> have - I think we should only remove this for newer machine types.  We
> >> also need to check what we're not supplying that the guest kernel is
> >> showing a different number of sockets than specified on the qemu
> >> command line.
> >>
> >>>
> >>> Thanks,
> >>>
> >>> C.
> >>>
> >>>   
> >>>
> >>>   [...]
> >>>   [...]
> >>>   [...]
> >>>   [...]
> >>>   [...]
> >>>   [...]
> >>>   [...]
> >>>   [...]
> >>>   [...]
> >>>
> >>
> >>
> > 

Re: [PATCH v2 1/8] powerpc/xive: Use cpu_to_node() instead of ibm,chip-id property

[Cédric Le Goater]

On 3/12/21 1:18 PM, Daniel Henrique Barboza wrote:
> 
> 
> On 3/12/21 6:53 AM, Cédric Le Goater wrote:
>> On 3/12/21 2:55 AM, David Gibson wrote:
>>> On Tue, 9 Mar 2021 18:26:35 +0100
>>> Cédric Le Goater <clg@kaod.org> wrote:
>>>
>>>> On 3/9/21 6:08 PM, Daniel Henrique Barboza wrote:
>>>>>
>>>>>
>>>>> On 3/9/21 12:33 PM, Cédric Le Goater wrote:
>>>>>> On 3/8/21 6:13 PM, Greg Kurz wrote:
>>>>>>> On Wed, 3 Mar 2021 18:48:50 +0100
>>>>>>> Cédric Le Goater <clg@kaod.org> wrote:
>>>>>>>  
>>>>>>>> The 'chip_id' field of the XIVE CPU structure is used to choose a
>>>>>>>> target for a source located on the same chip when possible. This field
>>>>>>>> is assigned on the PowerNV platform using the "ibm,chip-id" property
>>>>>>>> on pSeries under KVM when NUMA nodes are defined but it is undefined
>>>>>>>
>>>>>>> This sentence seems to have a syntax problem... like it is missing an
>>>>>>> 'and' before 'on pSeries'.
>>>>>>
>>>>>> ah yes, or simply a comma.
>>>>>>  
>>>>>>>> under PowerVM. The XIVE source structure has a similar field
>>>>>>>> 'src_chip' which is only assigned on the PowerNV platform.
>>>>>>>>
>>>>>>>> cpu_to_node() returns a compatible value on all platforms, 0 being the
>>>>>>>> default node. It will also give us the opportunity to set the affinity
>>>>>>>> of a source on pSeries when we can localize them.
>>>>>>>>   
>>>>>>>
>>>>>>> IIUC this relies on the fact that the NUMA node id is == to chip id
>>>>>>> on PowerNV, i.e. xc->chip_id which is passed to OPAL remain stable
>>>>>>> with this change.
>>>>>>
>>>>>> Linux sets the NUMA node in numa_setup_cpu(). On pseries, the hcall
>>>>>> H_HOME_NODE_ASSOCIATIVITY returns the node id if I am correct (Daniel
>>>>>> in Cc:)
>>>>   [...]
>>>>>>
>>>>>> On PowerNV, Linux uses "ibm,associativity" property of the CPU to find
>>>>>> the node id. This value is built from the chip id in OPAL, so the
>>>>>> value returned by cpu_to_node(cpu) and the value of the "ibm,chip-id"
>>>>>> property are unlikely to be different.
>>>>>>
>>>>>> cpu_to_node(cpu) is used in many places to allocate the structures
>>>>>> locally to the owning node. XIVE is not an exception (see below in the
>>>>>> same patch), it is better to be consistent and get the same information
>>>>>> (node id) using the same routine.
>>>>>>
>>>>>>
>>>>>> In Linux, "ibm,chip-id" is only used in low level PowerNV drivers :
>>>>>> LPC, XSCOM, RNG, VAS, NX. XIVE should be in that list also but skiboot
>>>>>> unifies the controllers of the system to only expose one the OS. This
>>>>>> is problematic and should be changed but it's another topic.
>>>>>>
>>>>>>  
>>>>>>> On the other hand, you have the pSeries case under PowerVM that
>>>>>>> doesn't xc->chip_id, which isn't passed to any hcall AFAICT.
>>>>>>
>>>>>> yes "ibm,chip-id" is an OPAL concept unfortunately and it has no meaning
>>>>>> under PAPR. xc->chip_id on pseries (PowerVM) will contains an invalid
>>>>>> chip id.
>>>>>>
>>>>>> QEMU/KVM exposes "ibm,chip-id" but it's not used. (its value is not
>>>>>> always correct btw)
>>>>>
>>>>>
>>>>> If you have a way to reliably reproduce this, let me know and I'll fix it
>>>>> up in QEMU.
>>>>
>>>> with :
>>>>
>>>>     -smp 4,cores=1,maxcpus=8 -object memory-backend-ram,id=ram-node0,size=2G -numa node,nodeid=0,cpus=0-1,cpus=4-5,memdev=ram-node0 -object memory-backend-ram,id=ram-node1,size=2G -numa node,nodeid=1,cpus=2-3,cpus=6-7,memdev=ram-node1
>>>>
>>>> # dmesg | grep numa
>>>> [    0.013106] numa: Node 0 CPUs: 0-1
>>>> [    0.013136] numa: Node 1 CPUs: 2-3
>>>>
>>>> # dtc -I fs /proc/device-tree/cpus/ -f | grep ibm,chip-id
>>>>         ibm,chip-id = <0x01>;
>>>>         ibm,chip-id = <0x02>;
>>>>         ibm,chip-id = <0x00>;
>>>>         ibm,chip-id = <0x03>;
>>>>
>>>> with :
>>>>
>>>>    -smp 4,cores=4,maxcpus=8,threads=1 -object memory-backend-ram,id=ram-node0,size=2G -numa node,nodeid=0,cpus=0-1,cpus=4-5,memdev=ram-node0 -object memory-backend-ram,id=ram-node1,size=2G -numa node,nodeid=1,cpus=2-3,cpus=6-7,memdev=ram-node1
>>>>
>>>> # dmesg | grep numa
>>>> [    0.013106] numa: Node 0 CPUs: 0-1
>>>> [    0.013136] numa: Node 1 CPUs: 2-3
>>>>
>>>> # dtc -I fs /proc/device-tree/cpus/ -f | grep ibm,chip-id
>>>>         ibm,chip-id = <0x00>;
>>>>         ibm,chip-id = <0x00>;
>>>>         ibm,chip-id = <0x00>;
>>>>         ibm,chip-id = <0x00>;
>>>>
>>>> I think we should simply remove "ibm,chip-id" since it's not used and
>>>> not in the PAPR spec.
>>>
>>> As I mentioned to Daniel on our call this morning, oddly it *does*
>>> appear to be used in the RHEL kernel, even though that's 4.18 based.
>>> This patch seems to have caused a minor regression; not in the
>>> identification of NUMA nodes, but in the number of sockets shown be
>>> lscpu, etc.  See https://bugzilla.redhat.com/show_bug.cgi?id=1934421
>>> for more information.
>>
>> Yes. The property "ibm,chip-id" is wrongly calculated in QEMU. If we
>> remove it, we get with 4.18.0-295.el8.ppc64le or 5.12.0-rc2 :
>>
>>     [root@localhost ~]# lscpu
>>     Architecture:        ppc64le
>>     Byte Order:          Little Endian
>>     CPU(s):              128
>>     On-line CPU(s) list: 0-127
>>     Thread(s) per core:  4
>>     Core(s) per socket:  16
>>     Socket(s):           2
>>     NUMA node(s):        2
>>     Model:               2.2 (pvr 004e 1202)
>>     Model name:          POWER9 (architected), altivec supported
>>     Hypervisor vendor:   KVM
>>     Virtualization type: para
>>     L1d cache:           32K
>>     L1i cache:           32K
>>     NUMA node0 CPU(s):   0-63
>>     NUMA node1 CPU(s):   64-127
>>
>>     [root@localhost ~]# grep . /sys/devices/system/cpu/*/topology/physical_package_id
>>     /sys/devices/system/cpu/cpu0/topology/physical_package_id:-1
>>     /sys/devices/system/cpu/cpu100/topology/physical_package_id:-1
>>     /sys/devices/system/cpu/cpu101/topology/physical_package_id:-1
>>     /sys/devices/system/cpu/cpu102/topology/physical_package_id:-1
>>     /sys/devices/system/cpu/cpu103/topology/physical_package_id:-1
>>     ....
>>
>> "ibm,chip-id" is still being used on some occasion on pSeries machines.
>> This is wrong :/ The problem is :
>>
>>    #define topology_physical_package_id(cpu)      (cpu_to_chip_id(cpu))
>>
>> We should be using cpu_to_node().
> 
> 
> IIUC the "real fix" then is this change you mentioned above, together with
> this xive patch as well, 

These are independent. 

The XIVE patch just raised the issue because it's another usage example of 
cpu_to_chip_id() or directly "ibm,chip-id" in the XIVE case, on a pseries 
machine.  

The use of cpu_to_node(cpu) for topology_physical_package_id(cpu) is a fix 
for the sysfs issue reported in the redhat BZ. 

> to stop using ibm,chip-id for good in the pserie
>  kernel. With these changes QEMU can remove 'ibm,chip-id' from the pseries
> machine without impact. Is this correct?

Linux is already "broken" on PowerVM today since we don't have the "ibm,chip-id" 
property. QEMU is just hiding the problem on KVM. 

But we have to be bug compatible :) if the QEMU fix is under the pseries-6.x 
machine we should be fine.

> If that's the case, then I believe it's ok to go forward with the QEMU side
> change (just for 6.0.0 and newer machines). Or should I wait for the kernel
> changes to be merged upstream first?

Once Linux is fixed, we shouldn't care if QEMU exports 'ibm,chip-id' or not.
I don't think the order is very important. These are independent. 

C.