Re: [PATCH v3 13/16] x86/hyperlaunch: specify dom0 mode with device tree

[Alejandro Vallejo <agarciav@xxxxxxx>]

On Wed Apr 9, 2025 at 11:24 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
>> 
>> 
>> 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 dpsmith@xxxxxxxxxxxxxxxxxxxx
>> 
>> Reviewed-by: Jason Andryuk jason.andryuk@xxxxxxx
>> 
>> ---
>> 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