[PATCH] powerpc/pseries/mobility: ignore ibm, platform-facilities updates

[Tyrel Datwyler]

On 10/20/21 8:54 AM, Nathan Lynch wrote:
> Tyrel Datwyler <tyreld at linux.ibm.com> writes:
>> On 10/19/21 2:36 PM, Nathan Lynch wrote:
>>> Tyrel Datwyler <tyreld at linux.ibm.com> writes:
>>>> On 10/18/21 9:34 AM, Nathan Lynch wrote:
>>>>> On VMs with NX encryption, compression, and/or RNG offload, these
>>>>> capabilities are described by nodes in the ibm,platform-facilities device
>>>>> tree hierarchy:
>>>>>
>>>>>   $ tree -d /sys/firmware/devicetree/base/ibm,platform-facilities/
>>>>>   /sys/firmware/devicetree/base/ibm,platform-facilities/
>>>>>   ├── ibm,compression-v1
>>>>>   ├── ibm,random-v1
>>>>>   └── ibm,sym-encryption-v1
>>>>>
>>>>>   3 directories
>>>>>
>>>>> The acceleration functions that these nodes describe are not disrupted by
>>>>> live migration, not even temporarily.
>>>>>
>>>>> But the post-migration ibm,update-nodes sequence firmware always sends
>>>>> "delete" messages for this hierarchy, followed by an "add" directive to
>>>>> reconstruct it via ibm,configure-connector (log with debugging statements
>>>>> enabled in mobility.c):
>>>>>
>>>>>   mobility: removing node /ibm,platform-facilities/ibm,random-v1:4294967285
>>>>>   mobility: removing node /ibm,platform-facilities/ibm,compression-v1:4294967284
>>>>>   mobility: removing node /ibm,platform-facilities/ibm,sym-encryption-v1:4294967283
>>>>>   mobility: removing node /ibm,platform-facilities:4294967286
>>>>>   ...
>>>>>   mobility: added node /ibm,platform-facilities:4294967286
>>>>
>>>> It always bothered me that the update from firmware here was an delete/add at
>>>> the entire '/ibm,platform-facilities' node level instead of update properties
>>>> calls. When I asked about it years ago the claim was it was just easier to pass
>>>> an entire sub-tree as a single node add.
>>>
>>> Yes, I believe this is for ease of implementation on their part.
>>
>> I'd still argue that a full node removal or addition is a platform
>> reconfiguration on par with a hotplug operation.
> 
> Well... I think we might be better served by a slightly more flexible
> view of things :-) These are just a series of messages from firmware
> that should be considered as a whole, and they don't dictate exactly
> what the OS must do to its own data structures. Nothing mandates that
> the OS immediately and literally apply the changes as they are
> individually reported by the update-nodes sequence.

Agree to disagree. Not saying we can do much about it here, but I think being
flexible leads to scope creep down the road that may eventually complicate
things worse. A node removal doesn't guarantee an immediate node addition. So we
are just papering over the issue with a plan to paper over it some more in a
more complicated fashion.

> 
> Anyway, whether the firmware behavior is reasonable is sort of beside
> the point since we're stuck with it.

I'm aware. Moaning for the sake of moaning about it.

> 
> 
>>>>> This is because add_dt_node() -> dlpar_attach_node() attaches only the
>>>>> parent node returned from configure-connector, ignoring any children. This
>>>>> should be corrected for the general case, but fixing that won't help with
>>>>> the stale OF node references, which is the more urgent problem.
>>>>
>>>> I don't follow. If the code path you mention is truly broken in the way you say
>>>> then DLPAR operations involving nodes with child nodes should also be
>>>> broken.
>>>
>>> Hmm I don't know of any kernel-based DLPAR workflow that attaches
>>> sub-trees i.e. parent + children. Processor addition attaches CPU nodes
>>> and possibly cache nodes, but they have sibling relationships. If you're
>>> thinking of I/O adapters, the DT updates are (still!) driven from user
>>> space. As undesirable as that is, that use case actually works correctly
>>> AFAIK.
>>>
>>> What happens for the platfac LPM scenario is that
>>> dlpar_configure_connector() returns the node pointer for the root
>>> ibm,platform-facilities node, with children attached. That node pointer
>>> is passed from add_dt_node() -> dlpar_attach_node() -> of_attach_node()
>>> -> __of_attach_node(), where its child and sibling fields are
>>> overwritten in the process of attaching it to the tree.
>>
>> Well it worked back in 2013 when I got all the in kernel device tree update
>> stuff working. Looks like I missed this one which now explains one area where
>> platform-facilities update originally went to shit.
>>
>> commit 6162dbe49a451f96431a23b4821f05e3bd925bc1
>> Author: Grant Likely <grant.likely at linaro.org>
>> Date:   Wed Jul 16 08:48:46 2014 -0600
>>
>>     of: Make sure attached nodes don't carry along extra children
>>
>>     The child pointer does not get cleared when attaching new nodes which
>>     could cause the tree to be inconsistent. Clear the child pointer in
>>     __of_attach_node() to be absolutely sure that the structure remains in a
>>     consistent layout.
>>
>>     Signed-off-by: Grant Likely <grant.likely at linaro.org>
>>
>> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
>> index c875787fa394..b96d83100987 100644
>> --- a/drivers/of/dynamic.c
>> +++ b/drivers/of/dynamic.c
>> @@ -98,6 +98,7 @@ int of_property_notify(int action, struct device_node *np,
>>
>>  void __of_attach_node(struct device_node *np)
>>  {
>> +       np->child = NULL;
>>         np->sibling = np->parent->child;
>>         np->allnext = np->parent->allnext;
>>         np->parent->allnext = np;
>>
>> Not sure what the reasoning was here since this prevents attaching a node that
>> is a sub tree. Either need to revert that, rewrite dlpar_attach_node to iterate
>> over the sub-tree, or probably best rewrite dlpar_configure_connector to use a
>> of_changeset instead.
> 
> Good find. I'd guess that adding a subtree used to sort of work, except
> that children of np were not added to the allnext list, which would
> effectively hide them from some lookups.

Pretty sure allnodes/allnext list is a relic of the past. It guaranteed depth
first traversal after boot, but dynamic nodes broke that guarantee. It was
removed because you can make the same traversal via the parent<->child lists.

> 
> Regardless, yes, the pseries code needs to change to attach and detach
> nodes individually. I don't think the OF core supports more complex
> changes.
> 

Indeed. I clearly capitalized on the behavior at the time while missing the
intended usage.

-Tyrel