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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.04.2025 14:22, Alejandro Vallejo wrote:
> 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/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.

The parsing from head.S is only partial. But surely DT is being looked at
far later than when the full parsing (cmdline_parse()) is done?

Jan