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

From: "Cédric Le Goater" <clg@kaod.org>
To: David Gibson <david@gibson.dropbear.id.au>
Cc: linuxppc-dev@lists.ozlabs.org, Greg Kurz <groug@kaod.org>
Subject: Re: [PATCH v3 1/9] powerpc/xive: Use cpu_to_node() instead of "ibm,chip-id" property
Date: Thu, 1 Apr 2021 11:10:46 +0200	[thread overview]
Message-ID: <b9937a0d-659e-e56e-3d32-efce602a34e3@kaod.org> (raw)
In-Reply-To: <YGU0u4v2GVWBCrWw@yekko.fritz.box>

On 4/1/21 4:49 AM, David Gibson wrote:
> On Wed, Mar 31, 2021 at 04:45:06PM +0200, Cédric Le Goater 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. The XIVE
>> driver queries the chip id value from the "ibm,chip-id" DT property
>> but this property is not available on all platforms. It was first
>> introduced on the PowerNV platform and later, under QEMU for pseries.
>> However, the property does not exist under PowerVM since it is not
>> specified in PAPR.
>>
>> cpu_to_node() is a better alternative. On the PowerNV platform, the
>> node id is computed from the "ibm,associativity" property of the CPU.
>> Its value is built in the OPAL firmware from the physical chip id and
>> is equivalent to "ibm,chip-id".
> 
> Hrm... I mean, for powernv this is certainly correct, but seems to be
> relying on pretty specific specifics of the OPAL / chip behaviour,
> namely that the NUMA id == chip ID.

Yes. It seems so.  

>> On pSeries, the hcall H_HOME_NODE_ASSOCIATIVITY returns the node id.
> 
> AFAICT, the chip_id field is never actually used in the PAPR version
> of XIVE.  The only access to the field outside native.c is in
> xive_pick_irq_target(), and it only looks at chip_id if src_chip is
> valid.  

Yes.

> But src_chip is initialized to XIVE_INVALID_CHIP_ID in papr.c

Yes. The H_INT hcalls do no return any information on the source 
location.

> So it would make more sense to me to also initialize chip_id to
> XIVE_INVALID_CHIP_ID for PAPR to make it clearer that it's not
> relevant.

yes. That would clarify that chip_id is only relevant on PowerVM/KVM. 

We can drop this patch, it's not a requirement for patches 2-9, simply 
a cleanup. I will move the chip_id assignment to a platform handler 
in a other patch.

>> Also to be noted that under QEMU/KVM "ibm,chip-id" is badly calculated
>> with unusual SMT configuration. This leads to a bogus chip id value
>> being returned by of_get_ibm_chip_id().
> 
> I *still* don't clearly understand what you think is bogus about the
> chip id value that qemu generates.  It's clearly not a problem for
> XIVE, since PAPR XIVE never uses it.

I am getting confused by socket/node/chip concepts under PPC. 

However, when looking at PHB and MSI, there is definitely a "node" 
concept being used in the core IRQ layer for allocation and affinity. 
We will need to clarify that when we introduce MSI domains.  

Thanks,

C.  