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
