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

[Alejandro Vallejo <agarciav@xxxxxxx>]

On Wed Apr 9, 2025 at 11:39 PM BST, Denis Mukhin wrote:
> On Tuesday, April 8th, 2025 at 9:07 AM, Alejandro Vallejo <agarciav@xxxxxxx> 
> wrote:
>
>> 
>> 
>> From: "Daniel P. Smith" dpsmith@xxxxxxxxxxxxxxxxxxxx
>> 
>> 
>> 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 dpsmith@xxxxxxxxxxxxxxxxxxxx
>> 
>> Reviewed-by: Jason Andryuk jason.andryuk@xxxxxxx
>> 
>> Signed-off-by: Jason Andryuk jason.andryuk@xxxxxxx
>> 
>> ---
>> xen/arch/x86/domain-builder/core.c | 1 +
>> xen/arch/x86/domain-builder/fdt.c | 12 ++++++++++++
>> xen/arch/x86/include/asm/boot-domain.h | 4 ++++
>> xen/arch/x86/setup.c | 6 +++++-
>> 4 files changed, 22 insertions(+), 1 deletion(-)
>> 
>> diff --git a/xen/arch/x86/domain-builder/core.c 
>> b/xen/arch/x86/domain-builder/core.c
>> index 510a74a675..6ab4e6fe53 100644
>> --- a/xen/arch/x86/domain-builder/core.c
>> +++ b/xen/arch/x86/domain-builder/core.c
>> @@ -96,6 +96,7 @@ void __init builder_init(struct boot_info *bi)
>> i = first_boot_module_index(bi, BOOTMOD_UNKNOWN);
>> bi->mods[i].type = BOOTMOD_KERNEL;
>> 
>> bi->domains[0].kernel = &bi->mods[i];
>> 
>> + bi->domains[0].capabilities |= BUILD_CAPS_CONTROL;
>> 
>> bi->nr_domains = 1;
>> 
>> }
>> }
>> diff --git a/xen/arch/x86/domain-builder/fdt.c 
>> b/xen/arch/x86/domain-builder/fdt.c
>> index 5fcb767bdd..dbfbcffb0a 100644
>> --- 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);
>
> Suggest adding XENLOG_ERR to the error message.

Yes, and the message itself seems bogus. The dangers of copy-paste...

Will fix both.

>
>> + return -EINVAL;
>> + }
>> + printk(" caps: ");
>> + if ( bd->capabilities & BUILD_CAPS_CONTROL )
>> 
>> + printk("c");
>
> Perhaps wrap string generation into a separate function?
>
> That will help if the number of capabilities will grow over time
> and if there will be a need to use string representation somewhere else
> in the code.
>
> Thoughts?

If/when such other code appears I'm happy to unify them, but until then
I'd rather reduce indirection if possible and keep it inlined.

Cheers,
Alejandro