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

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Stefano,

On 12/09/2023 01:21, Stefano Stabellini wrote:
> Caution: This message originated from an External Source. Use proper caution 
> when opening attachments, clicking links, or responding.
> 
> 
> On Mon, 11 Sep 2023, Michal Orzel wrote:
>> On 11/09/2023 16:48, Julien Grall wrote:
>>> 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/
> 
> yes it was done on purpose
> 
> 
>> 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.
> 
> Yes I agree. I don't think we risk breaking anything. I would remove
> that info from Dom0. Even if we wanted to expose it to Dom0, this is not
> the right way to do it...

Ok, so my understanding was correct and the code itself need no change. 
Following Julien advice to add more
information to commit msg, my proposal for it is as follows:

Skip the following Xen specific host device tree nodes/properties
from being included into hardware domain /chosen node:
 - xen,static-heap: this property informs Xen about memory regions
   reserved exclusively as static heap,
 - xen,domain-shared-memory-v1: node with this compatible informs Xen
   about static shared memory region for a domain. Xen exposes a different
   node (under /reserved-memory with compatible "xen,shared-memory-v1") to
   let domain know about the shared region,
 - xen,evtchn-v1: node with this compatible informs Xen about static
   event channel configuration for a domain. Xen does not expose information
   about static event channels to domUs and dom0 case was overlooked (by
   default nodes from host dt are copied to dom0 fdt unless explicitly marked
   to be skipped), since the author's idea was not to expose it (refer
   docs/misc/arm/device-tree/booting.txt, "Static Event Channel"). Even if we
   wanted to expose the static event channel information, the current node
   is in the wrong format (i.e. contains phandle to domU node not visible by
   dom0). Lastly, this feature is marked as tech-preview and there is no
   Linux binding in place.

~Michal