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

[Julien Grall <julien.grall@arm.com>]

Hi,

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@xilinx.com>
>>> ---
>>>    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?

> 
> interrupt-parent always points to the guest GIC node, for virtual
> interrupts such as the vuart, as well as physical passthrough
> interrupts. Also see tools/libxl/libxl_arm.c:fdt_property_interrupts and
> tools/libxl/libxl_arm.c:make_gicv2_node.

The "interrupt-parent" in those nodes are not necessary because the root 
contain "interrupt-parent". The property is only useful if you are going 
to use a different controller than the default one. For instance if you 
are using a GPIO controller.

> 
> I don't think the scenario you are describing is supported today.

See my answer on patch #2. I can't see why it is not supported today.

Cheers,

-- 
Julien Grall

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