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

[Julien Grall <julien@xxxxxxx>]

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).
Cheers,

--
Julien Grall