Re: [Xen-devel] [PATCH 5/5] xen/arm: add dom0less device assignment info to docs

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 8 Aug 2019, Julien Grall wrote:
> Hi Stefano,
> 
> On 07/08/2019 22:01, Stefano Stabellini wrote:
> > On Tue, 15 Jan 2019, Julien Grall wrote:
> > > On 1/3/19 10:07 PM, Stefano Stabellini wrote:
> > > > On Mon, 24 Dec 2018, Julien Grall wrote:
> > > > > Hi Stefano,
> > > > > 
> > > > > On 12/5/18 5:28 PM, Stefano Stabellini wrote:
> > > > > > Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> > > > > > ---
> > > > > >     docs/misc/arm/device-tree/booting.txt | 108
> > > > > > ++++++++++++++++++++++++++++++++++
> > > > > >     1 file changed, 108 insertions(+)
> > > > > > 
> > > > > > diff --git a/docs/misc/arm/device-tree/booting.txt
> > > > > > b/docs/misc/arm/device-tree/booting.txt
> > > > > > index 317a9e9..f5aaf8f 100644
> > > > > > --- a/docs/misc/arm/device-tree/booting.txt
> > > > > > +++ b/docs/misc/arm/device-tree/booting.txt
> > > > > > @@ -226,3 +226,111 @@ chosen {
> > > > > >             };
> > > > > >         };
> > > > > >     };
> > > > > > +
> > > > > > +
> > > > > > +Device Assignment
> > > > > > +=================
> > > > > > +
> > > > > > +Device Assignment (Passthrough) is supported by adding another
> > > > > > module,
> > > > > > +alongside the kernel and ramdisk, with the device tree fragment
> > > > > > +corresponding to the device node to assign to the guest.
> > > > > > +
> > > > > > +The dtb sub-node should have the following properties:
> > > > > > +
> > > > > > +- compatible
> > > > > > +
> > > > > > +    "multiboot,dtb"
> > > > > 
> > > > > I would prefer "multiboot,device-tree"
> > > > 
> > > > I renamed it
> > > > 
> > > > 
> > > > > > +
> > > > > > +- reg
> > > > > > +
> > > > > > +    Specifies the physical address of the device tree binary
> > > > > > fragment
> > > > > > +    RAM and its length.
> > > > > > +
> > > > > > +As an example:
> > > > > > +
> > > > > > +        module@0xc000000 {
> > > > > > +            compatible = "multiboot,dtb", "multiboot,module";
> > > > > > +            reg = <0x0 0xc000000 0xffffff>;
> > > > > > +        };
> > > > > > +
> > > > > > +The DTB fragment (loaded in memory at 0xc000000 in the example
> > > > > > above)
> > > > > > +should follow the convention explained in
> > > > > > docs/misc/arm/passthrough.txt.
> > > > > > +The DTB fragment will be added to the guest device tree, so that
> > > > > > the
> > > > > > +guest kernel will be able to discover the device.
> > > > > > +
> > > > > > +In addition, the following properties for each device node in the
> > > > > > device
> > > > > > +tree fragment will be used for the device assignment setup:
> > > > > > +
> > > > > > +- reg
> > > > > > +
> > > > > > +  The reg property specifying the address and size of the device
> > > > > > memory.
> > > > > > +  The device memory will be automatically mapped to the guest
> > > > > > domain
> > > > > > +  with a 1:1 mapping (pseudo-physical address == physical address).
> > > > > 
> > > > > As said in a previous patch, I don't think this is correct to impose
> > > > > 1:1.
> > > > > The
> > > > > user is neither in control of the HW memory map nor the Guest memory
> > > > > map.
> > > > > So
> > > > > not many people are going to be able to use it without hacking Xen.
> > > > 
> > > > Yes, I'll fix this (and a couple of other issues) by introducing a new
> > > > "xen,reg" property, instead of trying to reuse the existing reg
> > > > property.
> > > > 
> > > > 
> > > > > > +
> > > > > > +- interrupts
> > > > > > +
> > > > > > +  The interrupts property specifies the interrupt of the device.
> > > > > > They
> > > > > > +  are automatically routed to the guest domain with virtual irqs ==
> > > > > > +  physical irqs.
> > > > > > +
> > > > > > +- interrupt-parent
> > > > > > +
> > > > > > +  It contains a reference to the interrupt controller node. It
> > > > > > should
> > > > > > be
> > > > > > +  65000, corresponding to GUEST_PHANDLE_GIC.
> > > > > 
> > > > > We managed to get away in the toolstack with this property. So why do
> > > > > you
> > > > > need
> > > > > it for the hypervisor? Furthermore, this would forbid to passthrough
> > > > > any
> > > > > other
> > > > > interrupt controller to the guest.
> > > > 
> > > > The toolstack does use GUEST_PHANDLE_GIC today for passthrough
> > > > interrupts, see tools/libxl/libxl_arm.c:make_root_properties and
> > > > docs/misc/arm/passthrough.txt:
> > > > 
> > > >     * The interrupt-parent property will be added by the toolstack in
> > > > the
> > > >       root node;
> > > 
> > > You misunderstood my point here. The toolstack is adding the property for
> > > the
> > > user. So why does you require the user to add this property for Dom0less
> > > case?
> > 
> > I did misunderstand. interrupt-parent came from the example I had at hand,
> > which had
> > already the property even if it is unnecessary. I comfirmed that it is
> > superflous and I am happy to remove it.
> > 
> > FYI dtc throws a warning if interrupt-parent is missing:
> > 
> > <stdout>: Warning (interrupts_property): Missing interrupt-parent for
> > /passthrough/ethernet@ff0e0000
> > 
> > It makes me guess that is why it was added to the example I had.
> 
> Hmmm, I didn't remember DTC were throwing a warning.
> 
> What I want to avoid is writing in the documentation the phandle value. The
> value has been chosen in random and we have no guarantee the phandle will not
> be used by DTC in the future.
> 
> The solution I can think of is requesting the user to add the following
> snippet in the partial DT.
> 
> interrupt-parent = &gic;
> 
> gic {
> };
> 
> This will let DTC to define the phandle. Xen can then lookup for the patch
> /gic and re-use the phandle for the guest GIC node.
> 
> What do you think?

That doesn't quite work because DTC would be unhappy about the gic node
being empty:

<stdout>: Warning (interrupts_property): Missing interrupt-controller or 
interrupt-map property in /gic
<stdout>: Warning (interrupts_property): Missing #interrupt-cells in 
interrupt-parent /gic

Also, adding a /gic node in the fragment I think would make it more
difficult to explain to the user and the fragment more complex.


Let's get back to the important goal. I agree we should not expose the
value of GUEST_PHANDLE_GIC, and I agree it is not a good idea to make
"GUEST_PHANDLE_GIC" part of the dom0less specification.

To avoid that, I would go with one of the following options:

1) No mentions to interrupt-parent in the dom0less spec
The user is expected not to have an interrupt-parent property in the
fragment (for gic interrupts) and live with the dtc warning, or add a
dummy value. The value doesn't matter because it will be automatically
re-written by Xen. Either way, we don't care, and we don't say anything
about it in the spec.

2) Say in the docs that interrupt-parent should be a specifc dummy value
For instance 0xd3adb33f. We replace 0xd3adb33f with GUEST_PHANDLE_GIC in
Xen.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel