Re: [RFC PATCH v2] docs/design: boot domain device tree design

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>]

On Tue, 2 Feb 2021, Daniel P. Smith wrote:
> This is a Request For Comments on the adoption of Device Tree as the
> format for the Launch Control Module as described in the previously
> posted DomB RFC.
> 
> For RFC purposes, a rendered of this file can be found here:
> ihttps://drive.google.com/file/d/1l3fo4FylvZCQs1V00DcwifyLjl5Jw3W8/view?usp=sharing
> 
> Details on the DomB boot domain can be found on Xen wiki:
> https://wiki.xenproject.org/wiki/DomB_mode_of_dom0less
> 
> Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Christopher Clark <christopher.clark@xxxxxxxxxx>
> 
> Version 2
> ---------
> 
>  - cleaned up wording
>  - updated example to reflect a real configuration
>  - add explanation of mb2 modules that would be loaded
>  - add the config node
> ---
>  docs/designs/boot-domain-device-tree.rst | 235 
> +++++++++++++++++++++++++++++++
>  1 file changed, 235 insertions(+)
>  create mode 100644 docs/designs/boot-domain-device-tree.rst
> 
> diff --git a/docs/designs/boot-domain-device-tree.rst 
> b/docs/designs/boot-domain-device-tree.rst
> new file mode 100644
> index 0000000000..558d75a796
> --- /dev/null
> +++ b/docs/designs/boot-domain-device-tree.rst
> @@ -0,0 +1,235 @@
> +====================================
> +Xen Boot Domain Device Tree Bindings
> +====================================
> +
> +The Xen Boot Domain device tree adopts the dom0less device tree structure and
> +extends it to meet the requirements for the Boot Domain capability. The 
> primary
> +difference is the introduction of the ``xen`` node that is under the 
> ``/chosen``
> +node. The move to a dedicated node was driven by:
> +
> +1. Reduces the need to walk over nodes that are not of interest, e.g. only
> +nodes of interest should be in ``/chosen/xen``
> +
> +2. Enables the use of the ``#address-cells`` and ``#size-cells`` fields on 
> the
> +xen node.
> +
> +3. Allows for the domain construction information to easily be sanitized by
> +simple removing the ``/chosen/xen`` node.
> +
> +Below is an example device tree definition for a xen node followed by an
> +explanation of each section and field:

This is great!

You should know that there is an effort ongoing to standardize a set of
device tree nodes for static hypervisors and heterogeneous computing
configurations. It is called system device tree and it is also aligned
with our dom0less nodes, in fact, you could say that dom0less is the
predecessor of system device tree. It is great to see that this proposal
is also very well aligned with it. We are all going in the same
direction, excellent! :-)

https://www.youtube.com/watch?v=2o-B21unV9M
https://github.com/devicetree-org/lopper/blob/68c35bdb92d25a24a1a0b9b3eaf258c034b1f5db/device-trees/system-device-tree.dts#L784


I am going to suggest to make a few changes below to make it even more
aligned and more device tree compatible. It might even allow us to
reuse system device tree tools such as lopper in the long term.




> +::
> +    xen {

This needs a compatible string too. In system device tree every domain
(Xen domain or AMP domain) has "openamp,domain" as compatible. I think
it would be good to reuse it here. We could also use something else, but
either way, I'd add a compatible string.


> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        // Configuration container
> +        config@0 {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +            compatible = "xen,config";
> +
> +            // reg is required but ignored for "xen,config" node
> +            reg = <0>;

I think it might be better to avoid using regs for the domid, and
instead add a new property for it.


> +            module@1 {
> +                compatible = "multiboot,microcode", "multiboot,module";
> +                reg = <1>;
> +            };
> +
> +            module@2 {
> +                compatible = "multiboot,xsm-policy", "multiboot,module";
> +                reg = <2>;
> +            };
> +        };
> +
> +        // Boot Domain definition
> +        domain@0x7FF5 {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +            compatible = "xen,domain";

I would add the domid here, e.g.:

domid = <1>;


> +            reg = <0x7FF5>;
> +            memory = <0x0 0x20000>;
> +            cpus = <1>;
> +            module@3 {
> +                compatible = "multiboot,kernel", "multiboot,module";
> +                reg = <3>;
> +            };
> +
> +            module@4 {
> +                compatible = "multiboot,ramdisk", "multiboot,module";
> +                reg = <4>;
> +            };
> +            module@5 {
> +                compatible = "multiboot,config", "multiboot,module";
> +                reg = <5>;
> +            };
> +
> +        // Classic Dom0 definition
> +        domain@0 {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +            compatible = "xen,domain";
> +
> +            reg = <0>;
> +
> +            // PERMISSION_NONE          (0)
> +            // PERMISSION_CONTROL       (1 << 0)
> +            // PERMISSION_HARDWARE      (1 << 1)
> +            permissions = <3>;
> +
> +            // FUNCTION_NONE            (0)
> +            // FUNCTION_BOOT            (1 << 1)
> +            // FUNCTION_CRASH           (1 << 2)
> +            // FUNCTION_CONSOLE         (1 << 3)
> +            // FUNCTION_XENSTORE        (1 << 30)
> +            // FUNCTION_LEGACY_DOM0     (1 << 31)
> +            functions = <0xFFFFFFFF>;
> +
> +            // MODE_PARAVIRTUALIZED     (1 << 0) /* PV | PVH/HVM */
> +            // MODE_ENABLE_DEVICE_MODEL (1 << 1) /* HVM | PVH */
> +            // MODE_LONG                (1 << 2) /* 64 BIT | 32 BIT */
> +            mode = <5>; /* 64 BIT, PV */
> +
> +            // UUID
> +            domain-handle = [B3 FB 98 FB 8F 9F 67 A3];

let's use uuid, see also below


> +            cpus = <1>;
> +            memory = <0x0 0x20000>;
> +            security-id = <0>;
> +
> +            module@6 {
> +                compatible = "multiboot,kernel", "multiboot,module";
> +                reg = <6>;
> +                bootargs = "console=hvc0";
> +            };
> +            module@7 {
> +                compatible = "multiboot,ramdisk", "multiboot,module";
> +                reg = <7>;
> +            };
> +    };
> +
> +The multiboot modules that would be supplied when using the above config 
> would
> +be, in order:
> + - (the above config, compiled)
> + - CPU microcode
> + - XSM policy
> + - kernel for boot domain
> + - ramdisk for boot domain
> + - boot domain configuration file
> + - kernel for the classic dom0 domain
> + - ramdisk for the classic dom0 domain
> +
> +The Xen node
> +------------
> +The xen node is a top level container for the domains that will be built by
> +hypervisor on start up. On the ``xen`` node the ``#address-cells`` is set to 
> one
> +and the ``#size-cells`` is set to zero. This is done to enforce that each 
> domain
> +node must define a reg property and the hypervisor will use it to determine 
> the
> +``domid`` for the domain.

I understand that it would be good to specify the domid, but why not
using a new device tree property for it? Reusing reg seems a bit
strange, and it causes a bit of awkwardness like reg = <0> for
xen,config.

You could just use one if the following:

domid = <0x1>;
domain-id = <0x1>;

and also:

uuid = "12345678-9abc-def0-1234-56789abcdef0";

The latter one is used in a recent secure enclave proposal:
https://lore.kernel.org/linux-devicetree/20201204121137.2966778-2-sudeep.holla@xxxxxxx/

I think uuid would be better than domain-handle.


> +The Config node
> +---------------
> +
> +A config node is for detailing any multiboot modules that are of interest to 
> Xen
> +itself. For example this would be where Xen would be informed of microcode or
> +XSM policy locations.

If you make the top-level xen node a domain too, then the module can be
put directly under it and you don't need a config node at all.


> For locating the multiboot modules, the #address-cells and
> +#size-cells are set according to how the multiboot modules are identified and
> +located. If the multiboot modules will be located by index within the module
> +chain, the values should be “1” and “0” respectively. If the multiboot module
> +will be located by physical memory address, then the values should be “1” and
> +“1” respectively.

This is an interesting problem. The device tree way to do this would be
to use the "ranges" property of the parent.

If the multiboot module is located by physical memory address, the
parent node is going to have a range property:

xen {
    compatible = "openamp,domain";
    ranges;
    #address-cells = <0x1>;
    #size-cells = <0x1>;

    module@1 {
        compatible = "multiboot,microcode", "multiboot,module";
        reg = <0x0 0x10000>;
    };
    
    module@2 {
        compatible = "multiboot,xsm-policy", "multiboot,module";
        reg = <0x10000 0x10000>;
    };
}


Instead, if the multiboot module is not a physical memory address, but
an index in the multiboot chain, then you'd remove ranges from the
parent:

xen {
    compatible = "openamp,domain";
    #address-cells = <0x1>;
    #size-cells = <0x0>;

    module@1 {
        compatible = "multiboot,microcode", "multiboot,module";
        reg = <0x1>;
    };
    
    module@2 {
        compatible = "multiboot,xsm-policy", "multiboot,module";
        reg = <0x2>;
    };
}


ranges is meant to express if the children are supposed to map to the
CPU address space or not.

This is going to work fine as long as all the modules are either in
physical memory or have a multiboot index.  This is going to work only
if all the modules are either in physical memory or a multiboot index.
There is also the issue that the existing dom0less spec doesn't use
ranges and we don't want to break compatibility (interesting, even
system device tree doesn't use ranges for domains.)

So maybe it is better to avoid the problem and just use a different way
to specify that you are referring to a multiboot index. Something like:

xen {
    compatible = "openamp,domain";
    #address-cells = <0x1>;
    #size-cells = <0x1>;

    module@1 {
        compatible = "multiboot,microcode", "multiboot,module";
        multiboot-index = <0x1>;
    };
    
    module@2 {
        compatible = "multiboot,xsm-policy", "multiboot,module";
        reg = <0x10000 0x10000>;
    };
}

This way we can mix the two for different modules and don't break
compatibility with dom0less. Any thoughts?



> +\#address-cells
> +  Identifies number of fields for address. Required.
> +
> +\#size-cells
> +  Identifies number of fields for size. Required.
> +
> +compatible
> +  Identifies the type of node. Required.
> +
> +reg
> +  Unused. Required.
> +
> +The Domain node
> +---------------
> +A domain node is for describing the construction of a domain. It is free to 
> set
> +the #address-cells and #size-cells depending on how the multiboot modules
> +identified and located. If the multiboot modules will be located by index 
> within
> +the module chain, the values should be “1” and “0” respectively. If the
> +multiboot module will be located by physical memory address, then the values
> +should be “1” and “1” respectively.
> +
> +As previously mentioned a domain node must have a reg property which will be
> +used as the requested domain id for the domain with a value of “0” 
> signifying to
> +use the next available domain id. A domain configuration is not able to 
> request
> +a domid of “0”. After that a domain node may have any of the following
> +parameters,
> +
> +\#address-cells
> +  Identifies number of fields for address. Required.
> +
> +\#size-cells
> +  Identifies number of fields for size. Required.
> +
> +compatible
> +  Identifies the type of node. Required.
> +
> +reg
> +  Identifies the domid requested to assign to the domain. Required.
> +
> +permissions
> +  This sets what Discretionary Access Control permissions
> +  a domain is assigned. Optional, default is none.
> +
> +functions
> +  This identifies what system functions a domain will fulfill.
> +  Optional, the default is none.
> +
> +.. note::  The `functions` bits that have been selected to indicate 
> ``FUNCTION_XENSTORE`` and ``FUNCTION_LEGACY_DOM0`` are the last two bits (30, 
> 31) such that should these features ever be fully retired, the flags may be 
> dropped without leaving a gap in the flag set.
> +
> +mode
> +  The mode the domain will be executed under. Required.
> +
> +domain-handle
> +  A globally unique identifier for the domain. Optional,
> +  the default is NULL.
> +
> +cpus
> +  The number of vCPUs to be assigned to the domain. Optional,
> +  the default is “1”.
> +
> +memory
> +  The amount of memory to assign to the domain, in KBs.
> +  Required.
> +
> +security-id
> +  The security identity to be assigned to the domain when XSM
> +  is the access control mechanism being used. Optional,
> +  the default is “domu”.
> +
> +The Module node
> +---------------
> +This node describes a multiboot module loaded by the boot loader. The 
> required
> +compatible property follows the format: multiboot,<type> where type can be
> +“module”, “kernel”, “ramdisk”, “device-tree”, “microcode”, “xsm-policy” or
> +“config”. The reg property is required and identifies how to locate the
> +multiboot module.
> +
> +compatible
> +  This identifies what the module is and thus what the hypervisor
> +  should use the module for during domain construction. Required.
> +
> +reg
> +  This identifies where this module is located within the multiboot
> +  module chain. Required.
> +
> +bootargs
> +  This is used to override the boot params carried with the
> +  multiboot module.
> +
> +.. note::  The bootargs property is intended for situations where the same 
> kernel multiboot module is used for more than one domain.
> +