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

[Julien Grall <julien@xxxxxxx>]

On 12/09/2023 08:18, Michal Orzel wrote:
> 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.
The new commit message LGTM.

Cheers,

--
Julien Grall