Re: [PATCH v2 1/2] docs: update hyperlaunch device tree

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

On 8/4/23 05:03, Julien Grall wrote:
> Hi Daniel,
>
> On 03/08/2023 11:44, Daniel P. Smith wrote:
> > +compatible
> > +  Identifies which hypervisors the configuration is compatible. 
> > Required.
> > -    hypervisor {
> > -        compatible = “hypervisor,xen”
> > +  Format: "hypervisor,<hypervisor name>", e.g "hypervisor,xen"
> I read "e.g" as "for example". You don't explicitely say which 
> compatible will be supported by Xen, so one could write "hypervisor,foo" 
> and expect to work.
> Also, it is not fully clear why you need both the hypervisor and each 
> domain node to have a compatible with the hypervisor name in it.
Ack, it should be explicit to what is expected for a Xen configuration. 
As for compatible on the domain node, I think that was from being overly 
cautious, it can be dropped.
This did get me reflecting that the compatibility was added there as 
there was some initial participation in standardization efforts going on 
at the time. I am not sure what has come of those, but the question it 
raised is that domain is a Xen specific term, whereas generally vm is 
accepted as the generic term. Maybe this node needs renaming?
> > +compatible
> > +  Identifies the hypervisor the confiugration is intended. Required.
> Also typo: s/confiugration/configuration/
Ack.

> > -The modules that would be supplied when using the above config would be:
> > +  Format: "<hypervisor name>,config", e.g "xen,config"
> > -* (the above config, compiled into hardware tree)
> > -* 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
> > +bootargs
> > +  This is used to provide the boot params for Xen.
> How is this different from the command line parameter chosen? And if you 
> want to keep both, what is the expected priority if a user provides both?
A DT file for x86, there is only a need to provide the hypervisor node, 
for which we already needed a hypervisor config section to describe XSM 
policy and microcode, at this point at least. It was decided in an 
effort to provide flexibility, the ability to specify command line 
parameters to the hypervisor in DT config. It is expected these 
parameters would function as a base parameters that would be overridden 
by those provided via the multiboot kernel entry.
Now as you point out for Arm, this becomes a bit redundant, to a degree, 
as the Xen command line is already under the /chosen node. But even here 
it would be beneficial, as a hyperlaunch configuration could be 
distributed consisting of a dtb that has core parameters set with base 
values along with a set of kernels and ramdisks. At boot, the 
hyperlaunch dtb could then be concatenated with the device specific dtb.

> > -The hypervisor device tree would be compiled into the hardware device 
> > tree and
> > -provided to Xen using the standard method currently in use. The 
> > remaining
> > -modules would need to be loaded in the respective addresses specified 
> > in the
> > -`module-addr` property.
> > +  Format: String, e.g. "flask=silo"
> > +Child Nodes
> > +"""""""""""
> > -The Hypervisor node
> > --------------------
> > +* module
> > -The hypervisor node is a top level container for the domains that 
> > will be built
> > -by hypervisor on start up. On the ``hypervisor`` node the ``compatible``
> > -property is used to identify the type of hypervisor node present..
> > +Domain Node
> > +-----------
> > -compatible
> > -  Identifies the type of node. Required.
> > +A ``domain`` node is for describing the construction of a domain. 
> > Since there
> > +may be one or more domain nodes, each one requires a unique, DTB 
> > compliant name
> > +and a ``compatible`` property to identify as a domain node.
> > -The Config node
> > ----------------
> > +A ``domain`` node  may provide a ``domid`` 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, which is the default behavior if omitted. 
> > It should
> > +be noted that a domain configuration is not able to request a domid 
> > of “0”
> Why do you need this restriction? And more importantly how would you 
> describe dom0 in hyperlaunch?
I would start by saying one of the goals/purposes behind hyperlaunch is 
to remove the binding that "dom0" is identified by having domid 0. That 
is what the roles patch recently submitted is working to achieve. "Dom0" 
is a role in the system, which I tried calling the "unbounded role" but 
that seems to have caused some confusion.
That aside, IMHO because of the legacy around domid 0, I don't think it 
should be assignable through hyperlaunch. Additionally, there should be 
an identifier that signifies auto-assign the domid and that value should 
not conflict/limit what domids are usable by the hypervisor. Given we 
should not be assigning domid 0 through this interface, it makes it the 
perfect candidate value.
> > +Beyond that, a domain node may have any of the following optional 
> > properties.
> > -A config node is for detailing any 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 the modules are multiboot modules and are able 
> > to be
> > -located by index within the module chain, the ``mb-index`` property 
> > should be
> > -used to specify the index in the multiboot module chain.. If the 
> > module will be
> > -located by physical memory address, then the ``module-addr`` property 
> > should be
> > -used to identify the location and size of the module.
> > +Properties
> > +""""""""""
> >   compatible
> > -  Identifies the type of node. Required.
> > -
> > -The Domain node
> > ----------------
> > +  Identifies the node as a domain node and for which hypervisor. 
> > Required.
> > -A domain node is for describing the construction of a domain. It may 
> > provide a
> > -domid 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, 
> > which is
> > -the default behavior if omitted. A domain configuration is not able 
> > to request
> > -a domid of “0”. After that a domain node may have any of the following
> > -parameters,
> > -
> > -compatible
> > -  Identifies the type of node. Required.
> > +  Format: "<hypervisor name>,domain", e.g "xen,domain"
> >   domid
> > -  Identifies the domid requested to assign to the domain. Required.
> > +  Identifies the domid requested to assign to the domain.
> > -permissions
> > +  Format: Integer, e.g <0>
> > +
> > +role
> >     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.
> > +  Format: Bitfield, e.g <3> or <0x00000003>
> > +
> > +          ROLE_NONE                (0)
> > +          ROLE_UNBOUNDED_DOMAIN    (1U<<0)
> > +          ROLE_CONTROL_DOMAIN      (1U<<1)
> > +          ROLE_HARDWARE_DOMAIN     (1U<<2)
> > +          ROLE_XENSTORE_DOMAIN     (1U<<3)
> Please describe what each roles are meant for.
Agreed, but this obviously is in flux based on the current review of the 
roles patch.
> > +
> > +capability
> > +  This identifies what system capabilities a domain may have beyond 
> > the role it
> > +  was assigned.
> >     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.
> > +  Format: Bitfield, e.g <3221225487> or <0xC0000007>
> I thik we should favor the hexadecimal version because this will be 
> somewhat easier to read.
I too favor the hex version, but as I recall, DT/libfdt doesn't 
care/enforce.
> Also, the Device-Tree values work in term of 32-bit cell. Also, how do 
> you plan to handle the case where you have more than 32 capabilities?
I would add a capabalities2 field, this is how SELinux/Flask handle the 
same problem.
> > +
> > +          CAP_NONE            (0)
> > +          CAP_CONSOLE_IO      (1U<<0)
> Please describe the capabilities.
Agreed and again, will change based on final version of roles patch.

> >   mode
> >     The mode the domain will be executed under. Required.
> > +  Format: Bitfield, e.g <5> or <0x00000005>
> > +
> > +          MODE_PARAVIRTUALIZED     (1 << 0) PV | PVH/HVM
> > +          MODE_ENABLE_DEVICE_MODEL (1 << 1) HVM | PVH
> > +          MODE_LONG                (1 << 2) 64 BIT | 32 BIT
> > +
> >   domain-uuid
> >     A globally unique identifier for the domain. Optional,
> >     the default is NULL.
> > +  Format: Byte Array, e.g [B3 FB 98 FB 8F 9F 67 A3]
> > +
> >   cpus
> >     The number of vCPUs to be assigned to the domain. Optional,
> >     the default is “1”.
> > +  Format: Integer, e.g <0>
> This is odd to suggest to give '0' as an example. Wouldn't Xen throw an 
> error if a user provide it?
Good catch, though I am now thinking individual examples are not needed 
and the full example below should be sufficient.
> > +
> >   memory
> > -  The amount of memory to assign to the domain, in KBs.
> > +  The amount of memory to assign to the domain, in KBs. This field 
> > uses a DTB
> > +  Reg which contains a start and size. For memory allocation start 
> > may or may
> > +  not have significance but size will always be used for the amount 
> > of memory
> >     Required.
> The description doesn't match...
Ack, others caught that as well. Will be fixed.

> > +  Format: String  min:<sz> | max:<sz> | <sz>, e.g. "256M"
> ... the format. But strings are difficult to parse. If you want to 
> provide 3 different values (possibly optional), then it would be best to 
> have 3 different properties.
That format comes from the command line dom0 memory parameter, in the 
hyperlaunch series I reused that existing parser that I am fairly 
confident will be maintained.
> I will continue to review the rest later.
Great, thank you so much for your feedback thus far.

v/r,
dps