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

[Alejandro Vallejo <agarciav@xxxxxxx>]

On Thu Apr 10, 2025 at 1:18 PM BST, Jan Beulich wrote:
> On 08.04.2025 18:07, Alejandro Vallejo wrote:
>> From: "Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>
>> 
>> Introduce the ability to assign capabilities to a domain via its definition 
>> in
>> device tree. The first capability enabled to select is the control domain
>> capability. The capability property is a bitfield in both the device tree and
>> `struct boot_domain`.
>> 
>> Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
>> Reviewed-by: Jason Andryuk <jason.andryuk@xxxxxxx>
>> Signed-off-by: Jason Andryuk <jason.andryuk@xxxxxxx>
>
> The R-b feels kind of redundant with the subsequent S-o-b.

I'll drop it.

>
>> --- 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)?

>
>> --- 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.

> (can't resist: when there's already a wall of local variables here)?

Heh :). Point taken.

Cheers,
Alejandro