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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 9 Aug 2019, Julien Grall wrote:
> Hi Stefano,
> 
> I figured out I may want to read the docs before looking at the code :).
> 
> On 09/08/2019 00:12, Stefano Stabellini wrote:
> > Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> > 
> > ---
> > Changes in v3:
> > - add nr_spis
> > - change description of interrupts and interrupt-parent
> > 
> > Changes in v2:
> > - device tree fragment loaded in cacheable memory
> > - rename multiboot,dtb to multiboot,device-tree
> > - rename "path" to "xen,path"
> > - add a note about device memory mapping
> > - introduce xen,reg
> > - specify only the GIC is supported
> > ---
> >   docs/misc/arm/device-tree/booting.txt | 117 ++++++++++++++++++++++++++
> >   1 file changed, 117 insertions(+)
> > 
> > diff --git a/docs/misc/arm/device-tree/booting.txt
> > b/docs/misc/arm/device-tree/booting.txt
> > index 317a9e962a..ec2f7ba605 100644
> > --- a/docs/misc/arm/device-tree/booting.txt
> > +++ b/docs/misc/arm/device-tree/booting.txt
> > @@ -148,6 +148,12 @@ with the following properties:
> >         An empty property to enable/disable a virtual pl011 for the guest to
> > use.
> >   +- nr_spis
> > +    Optional. A 32-bit integer specifying the number of SPIs (shared
> > +    perhiperal interrupts) to allocate for the domain. If nr_spis is
> 
> s/perhiperal/peripheral/. Also can you uppercase the first letter of each
> wording as you are spelling out an acronym?

OK


> > +    missing, the max number of SPIs supported by the physical GIC is
> > +    used.
> > +
> >   - #address-cells and #size-cells
> >         Both #address-cells and #size-cells need to be specified because
> > @@ -226,3 +232,114 @@ chosen {
> >           };
> >       };
> >   };
> > +
> > +
> > +Device Assignment
> > +=================
> 
> Couldn't we just extend the file misc/arm/passthrough.txt? After all there
> should be no major difference between the two.

Do you mean instead of introducing this sub-chapter, or in addition?  I
think it would be best if we avoid any duplication of information with
docs/misc/arm/passthrough.txt by referencing
docs/misc/arm/passthrough.txt as much as possible. But I would keep the
dom0less device assignment details here. I tried to do that in this
patch by only providing examples and details about the differences
compared to docs/misc/arm/passthrough.txt (xen,path, xen,reg, interrupt
mapping) here in booting.txt.
 

> > +
> > +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,device-tree"
> 
> Can we follow the convention for dom0? I.e you always have to pass
> "multiboot,module"? And probably suggest an order if the compatible
> "multiboot,device-tree" is not specified.
>
> This would help to keep everything similar between dom0 and domU. Longer term,
> I would love to see Dom0 described exactly the same way as domU.

Yes, I think it is a good idea.


> You would need to do the same update for multiboot,{kernel, ramdisk}.

OK, I'll do it in this patch


> > +
> > +- reg
> > +
> > +    Specifies the physical address of the device tree binary fragment
> > +    RAM and its length.
> > +
> > +As an example:
> > +
> > +        module@0xc000000 {
> > +            compatible = "multiboot,device-tree", "multiboot,module";
> > +            reg = <0x0 0xc000000 0xffffff>;
> > +        };
> > +
> > +The DTB fragment is loaded in cacheable memory, at 0xc000000 in the
> > +example above. It should follow the convention explained in
> 
> This is a bit confusing, how does the user know that 0xc0000000 is cachable
> memory? Also why you do mention it for device-tree and not kernel and
> initramfs?

That is a mistake, sorry. I'll remove it as it doesn't add useful
information and it is confusing.


> > +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:
> > +
> > +- xen,reg
> > +
> > +  The xen,reg property is an array of:
> > +
> > +    <phys_addr size guest_addr>
> > +
> > +  They specify the physical address and size of the device memory
> > +  ranges together with the corresponding guest address to map them to.
> > +  The size of `phys_addr' and `guest_addr' is determined by
> > +  #address_cells; the size of `size' is determined by #size_cells.
> > +  The memory will be mapped as device memory in the guest
> > +  (p2m_mmio_direct_dev).
> > +
> > +- xen,path
> > +
> > +  A new string property named "xen,path" holds the path in the host device
> > +  tree to the corresponding device node.
> > +
> > +Please note that for GIC interrupts, the interrupts and interrupt-parent
> > +device tree properties should not be present in the device tree
> > +fragment, because they are automatically generated by Xen starting from
> > +the corresponding information on the host device tree node for the
> > +device. For GIC interrupts, only the interrupts property is currently
> > +supported, not the newer interrupts-extended property.
> 
> Why so? There are a lot of Device-Tree that are switching to interrupts-extend
> for GIC interrupts....
>
> Also, what about the property interrupt-map?

Only because they aren't implemented yet. I thought I should document
this limitation.


> > +
> > +The following is a real-world example of a device tree fragment for the
> > +network card on Xilinx MPSoC boards:
> > +
> > +/dts-v1/;
> > +
> > +/ {
> > +    #address-cells = <0x2>;
> > +    #size-cells = <0x1>;
> > +
> > +    passthrough {
> > +        compatible = "simple-bus";
> > +        ranges;
> > +        #address-cells = <0x2>;
> > +        #size-cells = <0x1>;
> > +
> > +        misc_clk {
> > +            #clock-cells = <0x0>;
> > +            clock-frequency = <0x7735940>;
> > +            compatible = "fixed-clock";
> > +            linux,phandle = <0x1>;
> > +            phandle = <0x1>;
> > +        };
> > +
> > +        ethernet@ff0e0000 {
> > +            compatible = "cdns,zynqmp-gem";
> > +            status = "okay";
> > +            reg = <0x0 0xff0e0000 0x1000>;
> > +            clock-names = "pclk", "hclk", "tx_clk", "rx_clk";
> > +            #address-cells = <0x1>;
> > +            #size-cells = <0x0>;
> > +            clocks = <0x1 0x1 0x1 0x1>;
> > +            phy-mode = "rgmii-id";
> > +            xlnx,ptp-enet-clock = <0x0>;
> > +            local-mac-address = [00 0a 35 00 22 01];
> > +            phy-handle = <0x2>;
> > +            xen,path = "/amba/ethernet@ff0e0000";
> > +            xen,reg = <0x0 0xff0e0000 0x1000 0x0 0xff0e0000>;
> > +
> > +            phy@c {
> > +                reg = <0xc>;
> > +                ti,rx-internal-delay = <0x8>;
> > +                ti,tx-internal-delay = <0xa>;
> > +                ti,fifo-depth = <0x1>;
> > +                ti,rxctrl-strap-worka;
> > +                linux,phandle = <0x2>;
> > +                phandle = <0x2>;
> > +            };
> > +        };
> > +    };
> > +};
> > 
> 
> Cheers,
> 
> -- 
> Julien Grall
> 

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