Re: [PATCH v3 16/16] x86/hyperlaunch: add capabilities to boot domain

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.04.2025 21:31, Alejandro Vallejo wrote:
> On Thu Apr 10, 2025 at 1:18 PM BST, Jan Beulich wrote:
>> On 08.04.2025 18:07, Alejandro Vallejo wrote:
>>> --- a/xen/arch/x86/domain-builder/fdt.c
>>> +++ b/xen/arch/x86/domain-builder/fdt.c
>>> @@ -257,6 +257,18 @@ static int __init process_domain_node(
>>>              bd->max_vcpus = val;
>>>              printk("  max vcpus: %d\n", bd->max_vcpus);
>>>          }
>>> +        else if ( strncmp(prop_name, "capabilities", name_len) == 0 )
>>> +        {
>>> +            if ( fdt_prop_as_u32(prop, &bd->capabilities) != 0 )
>>> +            {
>>> +                printk("  failed processing domain id for domain %s\n", 
>>> name);
>>> +                return -EINVAL;
>>> +            }
>>> +            printk("  caps: ");
>>> +            if ( bd->capabilities & BUILD_CAPS_CONTROL )
>>> +                printk("c");
>>> +            printk("\n");
>>> +        }
>>
>> Like for the other patch: What about other bits being set in the value read?
> 
> I take it that the non-worded suggestion is to have a mask of reserved
> bits for each case and check they are not set (giving a warning if they are)?

Whether a warning is sufficient I can't tell. I would have expected such to be
outright rejected.

>>> --- a/xen/arch/x86/setup.c
>>> +++ b/xen/arch/x86/setup.c
>>> @@ -1006,6 +1006,7 @@ static struct domain *__init create_dom0(struct 
>>> boot_info *bi)
>>>  {
>>>      char *cmdline = NULL;
>>>      size_t cmdline_size;
>>> +    unsigned int create_flags = 0;
>>>      struct xen_domctl_createdomain dom0_cfg = {
>>>          .flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity : 
>>> 0,
>>>          .max_evtchn_port = -1,
>>> @@ -1037,7 +1038,10 @@ static struct domain *__init create_dom0(struct 
>>> boot_info *bi)
>>>      if ( bd->domid == DOMID_INVALID )
>>>          /* Create initial domain.  Not d0 for pvshim. */
>>>          bd->domid = get_initial_domain_id();
>>> -    d = domain_create(bd->domid, &dom0_cfg, pv_shim ? 0 : CDF_privileged);
>>> +    if ( bd->capabilities & BUILD_CAPS_CONTROL )
>>> +        create_flags |= CDF_privileged;
>>
>> Seeing that builder_init() in the non-DT case sets the new bit 
>> unconditionally,
>> isn't the shim's only domain suddenly getting CDF_privileged set this way? 
>> Oh,
>> no, you then ...
>>
>>> +    d = domain_create(bd->domid, &dom0_cfg,
>>> +                      pv_shim ? 0 : create_flags);
>>
>> ... hide the flag here. Any reason to have the intermediate variable in the
>> first place
> 
> Well, the logic would end up fairly convoluted otherwise. As things
> stand this can be encoded in an if-else fashion with 2 calls, but
> there's 2 capability flags coming that need integrating together.
> 
> This is just avoiding further code motion down the line.

Is it?

-    d = domain_create(bd->domid, &dom0_cfg, pv_shim ? 0 : CDF_privileged);
+    d = domain_create(bd->domid, &dom0_cfg,
+                      ((bd->capabilities & BUILD_CAPS_CONTROL) && !pv_shim
+                       ? CDF_privileged : 0));

isn't really worse (imo), but is highlighting the problem more clearly: Why
would the shim have BUILD_CAPS_CONTROL set in the first place? Without that
the statement would remain pretty similar to what it was before.

Jan