Re: [PATCH] xen/arm: Skip Xen specific nodes/properties from hwdom /chosen node

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien,

On 12/09/2023 12:03, Julien Grall wrote:
> 
> 
> On 11/09/2023 16:09, Michal Orzel wrote:
>>
>>
>> On 11/09/2023 16:48, Julien Grall wrote:
>>> Caution: This message originated from an External Source. Use proper 
>>> caution when opening attachments, clicking links, or responding.
>>>
>>>
>>> On 11/09/2023 15:01, Michal Orzel wrote:
>>>> Hi Julien,
>>>>
>>>> On 11/09/2023 15:14, Julien Grall wrote:
>>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>> On 11/09/2023 13:34, Michal Orzel wrote:
>>>>>> Host device tree nodes under /chosen with compatible string
>>>>>> "xen,evtchn-v1", "xen,domain-shared-memory-v1" are Xen specific and not
>>>>>> meant to be exposed to hardware domain.
>>>>>
>>>>> So, how dom0 is meant to discover the static event channel, shared
>>>>> memory it can use?
>>>>
>>>> For static shared memory:
>>>> A node with this compatible is only meant for Xen since it contains 
>>>> information like host-guest mapping.
>>>> Xen creates a different node for guests under /reserved-memory.
>>>> Linux binding:
>>>> https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/reserved-memory/xen,shared-memory.txt
>>>> Xen node generation:
>>>> https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/arch/arm/domain_build.c;hb=HEAD#l1407
>>>
>>> Ah good point. I agree with this one.
>>>
>>>>
>>>> For static event channels:
>>>> This is a bit weird so let me put it in a different way.
>>>> 1) Xen does not create any node for static evtchn for domU.
>>>> 2) The booting.txt states:
>>>> There is no need to describe the static event channel info in the domU 
>>>> device
>>>> tree. Static event channels are only useful in fully static configurations,
>>>> and in those configurations, the domU device tree dynamically generated by 
>>>> Xen
>>>> is not needed.
>>>> 3) The "xen,evtchn" property specifies the local port as well as phandle 
>>>> of domU node.
>>>> dom0 does not have access to domU nodes, therefore exposing a property 
>>>> with not existing phandle
>>>> makes me think that:
>>>
>>> You have a point for the phandle. Yet, this is the only way dom0 can
>>> find the event channel today. As we exposed it, I don't think we can
>>> remove it until we have a proper replacement.
>>>
>>> We might get away if the feature is not supported it at all. But there
>>> is no statement, so it is a little unclear whether the feature is meant
>>> to be in technical preview.
>>>
>>> In any case, I think the commit message deserve a bit more explanation
>>> as hiding "xen,evtchn-v1"/"xen,domain-shared-memory-v1" is not
>>> uncontroversial.
>>>
>>>> a) point 2) applies to dom0 as well and we should hide this node or > b) 
>>>> there is a missing property for both dom0 and domUs to tell them
>>> about static local port in a proper way
>>>>
>>>> Exposing Xen specific evtchn node only to dom0 and not to domU with 
>>>> required information happen to be found as first value
>>>> in xen,evtchn is definitely not a proper solution.
>>>
>>> My concern here is we exposed such information to dom0. So as I said
>>> above, I don't think we can simply remove it as there is no other way to
>>> find such information today.
>>>
>>> It doesn't matter that it wasn't exposed to domU.
>>>
>>>> Also, there is no Linux binding for it.
>>>
>>> AFAIK the static event channel support was not added in Linux until very
>>> recently. Also, I think the kernel doesn't directly use the static event
>>> channel. So it is possible, this is just an overlook.
>>
>> I've found this thread in which Stefano, Rahul and Bertrand agrees on not 
>> exposing
>> any dt property and the rationale behind:
>> https://patchwork.kernel.org/project/xen-devel/patch/4836304496e6fbbea41348ed8cc9fcf6b0f3e893.1648049827.git.rahul.singh@xxxxxxx/
> 
> Ok. So the expectation is that each end will have hardcoded event
> channel number. I feel this is going to lead to issue when someone will
> try to re-number event channel but forgot to update one of the
> component. Anyway...
> 
>>
>> I would not call exposing node to dom0 as something done deliberately given 
>> that it happens automatically by copying.
>> So my understanding is
>> that we did not want to expose any node and dom0 case was overlooked (since 
>> done automatically).
>>
>> Exposing half the interface (from system POV) in a way it should not be done 
>> (phandle) is not great IMO.
>> In any case, if you insist on keeping xen,evtchn, I can leave with it.
>>
>> This feature is marked as tech preview with no Linux binding in place so I 
>> would not be worried.
> 
> ... I overlooked it was a tech preview. So I am having less concern
> about remove the property. But I still think we need to find a way to
> describe it in the device-tree for future usage (see why above).

I fully agree that we shall have a property for both dom0 and domUs to make OS 
life easier
instead of hardcoding the values. This is something we need to keep in mind to 
do after the release.
That said, I hope you agree on removing this node right now from being exposed 
to dom0 which is what this patch aims for.
Please see Stefano reply and my proposal for a better commit msg.

~Michal