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" <dpsm...@apertussolutions.com>
>>
>> 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 <dpsm...@apertussolutions.com>
>> Reviewed-by: Jason Andryuk <jason.andr...@amd.com>
>> Signed-off-by: Jason Andryuk <jason.andr...@amd.com>
>
> 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
