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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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...