On Wed Apr 9, 2025 at 11:24 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
>>
>>
>> Enable selecting the mode in which the domain will be built and ran. This
>> includes:
>>
>> - whether it will be either a 32/64 bit domain
>> - if it will be run as a PV or HVM domain
>> - and if it will require a device model (not applicable for dom0)
>>
>> In the device tree, this will be represented as a bit map that will be
>> carried
>> through into struct boot_domain.
>>
>> Signed-off-by: Daniel P. Smith dpsm...@apertussolutions.com
>>
>> Reviewed-by: Jason Andryuk jason.andr...@amd.com
>>
>> ---
>> xen/arch/x86/domain-builder/fdt.c | 19 +++++++++++++++++++
>> xen/arch/x86/include/asm/boot-domain.h | 5 +++++
>> xen/arch/x86/setup.c | 3 ++-
>> 3 files changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/x86/domain-builder/fdt.c
>> b/xen/arch/x86/domain-builder/fdt.c
>> index 4c6aafe195..da65f6a5a0 100644
>> --- a/xen/arch/x86/domain-builder/fdt.c
>> +++ b/xen/arch/x86/domain-builder/fdt.c
>> @@ -193,6 +193,25 @@ static int __init process_domain_node(
>> bd->domid = (domid_t)val;
>>
>> printk(" domid: %d\n", bd->domid);
>>
>> }
>> + else if ( strncmp(prop_name, "mode", name_len) == 0 )
>> + {
>> + if ( fdt_prop_as_u32(prop, &bd->mode) != 0 )
>>
>> + {
>> + printk(" failed processing mode for domain %s\n", name);
>> + return -EINVAL;
>> + }
>> +
>> + printk(" mode: ");
>> + if ( !(bd->mode & BUILD_MODE_PARAVIRT) )
>>
>> + {
>> + if ( bd->mode & BUILD_MODE_ENABLE_DM )
>>
>> + printk("HVM\n");
>> + else
>> + printk("PVH\n");
>> + }
>> + else
>> + printk("PV\n");
>> + }
>> }
>
> I would re-write so the positive condition is processed first, e.g.:
>
> if ( bd->mode & BUILD_MODE_PARAVIRT )
> printk("PV\n");
> else if ( bd->mode & BUILD_MODE_ENABLE_DM )
> printk("HVM\n");
> else
> printk("PVH\n");
>
> I think it will reduce indentation and make code block a bit easier to read.
>
For sure. You're absolutely right.
> Also, if there are more uses for printing string representation of a
> boot module mode in the future, perhaps move it to a separate function?
>
> What do you think?
If there's more existing cases I'm happy to unify them, but otherwise
I'd rather keep the code inlined to avoid too much indirection.
Cheers,
Alejandro
