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. The same applies to
"xen,static-heap" property. Skip them from being included into hwdom
/chosen node.

Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
 xen/arch/arm/domain_build.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 29dcbb8a2ee6..413568c0e2fd 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1149,7 +1149,7 @@ static int __init write_properties(struct domain *d, struct kernel_info *kinfo,
          * * remember xen,dom0-bootargs if we don't already have
          *   bootargs (from module #1, above).
          * * remove bootargs,  xen,dom0-bootargs, xen,xen-bootargs,
-         *   linux,initrd-start and linux,initrd-end.
+         *   xen,static-heap, linux,initrd-start and linux,initrd-end.
          * * remove stdout-path.
          * * remove bootargs, linux,uefi-system-table,
          *   linux,uefi-mmap-start, linux,uefi-mmap-size,
@@ -1158,7 +1158,8 @@ static int __init write_properties(struct domain *d, struct kernel_info *kinfo,
          */
         if ( dt_node_path_is_equal(node, "/chosen") )
         {
-            if ( dt_property_name_is_equal(prop, "xen,xen-bootargs") ||
+            if ( dt_property_name_is_equal(prop, "xen,static-heap") ||
+                 dt_property_name_is_equal(prop, "xen,xen-bootargs") ||
                  dt_property_name_is_equal(prop, "linux,initrd-start") ||
                  dt_property_name_is_equal(prop, "linux,initrd-end") ||
                  dt_property_name_is_equal(prop, "stdout-path") ||
@@ -2300,6 +2301,8 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo,
     static const struct dt_device_match skip_matches[] __initconst =
     {
         DT_MATCH_COMPATIBLE("xen,domain"),
+        DT_MATCH_COMPATIBLE("xen,domain-shared-memory-v1"),
+        DT_MATCH_COMPATIBLE("xen,evtchn-v1"),
         DT_MATCH_COMPATIBLE("xen,xen"),
         DT_MATCH_COMPATIBLE("xen,multiboot-module"),
         DT_MATCH_COMPATIBLE("multiboot,module"),
-- 
2.25.1

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?

> The same applies to
> "xen,static-heap" property. Skip them from being included into hwdom
> /chosen node.

I agree with hiding "xen,static-heap" property.

Cheers,

-- 
Julien Grall

On Mon, 11 Sep 2023, Julien Grall wrote:
> 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?

As they are static, they don't necessarily need to be discovered: they
could be hard-coded. I realize we didn't commit the design doc for the
static event channel feature, but it had:
https://marc.info/?l=xen-devel&m=165245272905876

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

But in any case, even if we wanted to generated/expose the static event
channels via Device Tree, the information on the host device tree is in
the "wrong" format. That's meant for Xen and has all the static event
channels, including the ones not relevant for Dom0. So I think that in
any case we should filter them out here (and regenerate as necessary).

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

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:
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. Also, there is no Linux binding for it.
All that makes me think that the author's idea was not to expose it.

~Michal

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.

Cheers,

-- 
Julien Grall

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@arm.com/

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.

~Michal

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@arm.com/

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

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@arm.com/
> 
> 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

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@arm.com/

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

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@arm.com/
> 
> 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

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@arm.com/
>>
>> 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