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

[Alejandro Vallejo <agarciav@amd.com>]

On Tue Apr 15, 2025 at 7:38 AM BST, Jan Beulich wrote:
> 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),

Not sure I agree. Long conditions on ternary operators makes the
control flow harder to follow.

A nicer alternative that also removes the auxiliary variable is to have
a helper to convert from bootcaps to whatever createdomainflags are
required. That'd extend naturally for more bits.

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

If the commandline is parsed early enough (I see the early parse path in
head.S?) it would be better to add this logic to builder_init() and
prevent the capability from reaching the boot_domain in the first place.

Then there's no exception for the pv shim.

Cheers,
Alejandro