| Message ID | 20190808231242.26424-6-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/6] xen/arm: introduce handle_interrupts | expand |
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@xilinx.com> > > --- > 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? > + 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. > + > +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. You would need to do the same update for multiboot,{kernel, ramdisk}. > + > +- 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? > +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? > + > +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,
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@xilinx.com> > > > > --- > > 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 >
Hi, On 20/08/2019 01:41, Stefano Stabellini wrote: > 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@xilinx.com> >>> >>> --- >>> 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. I am not sure why dom0less assignment should be in booting.txt. This is not entirely related to booting and make the file more difficult to digest. Furthermore, it feels quite natural to put that in passthrough.txt because this is likely where someone will look at passthrough first. >>> + >>> +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 This definitely does not belong to this patch and I already send a patch to fix it because I got annoyed when testing dom0less. > > >>> + >>> +- 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. I will make the same statement as I did on reserved-memory in the past. While the implementation may be deferred, I still think you should have a think what can happen if there are used. If you look at patch #3, your solution here may not fit with "interrupt-map". So can you have a think how this will work? Regarding "interrupts-extended", I don't think this series should go in without the implementation. We want people to be able to test dom0less without having to hack heavily there device-tree. It is also just a couple of line change. Cheers,
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 + 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 +================= + +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" + +- 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 +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. + +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>; + }; + }; + }; +};
Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> --- 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(+)