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
