On Wed Apr 9, 2025 at 11:39 PM BST, Denis Mukhin wrote:
> On Tuesday, April 8th, 2025 at 9:07 AM, Alejandro Vallejo <agarc...@amd.com>
> 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
>>
>> ---
>> 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
