Re: [PATCH v3 7/7] xen/arm: introduce new xen,enhanced property value

[Julien Grall <julien@xxxxxxx>]

Hi Rahul,

On 01/09/2022 10:13, Rahul Singh wrote:
> Introduce a new "xen,enhanced" dom0less property value "no-xenstore" to
> disable xenstore interface for dom0less guests.
>
> Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>
> ---
> Changes in v3:
>   - new patch in this version
> ---
>   docs/misc/arm/device-tree/booting.txt |  4 ++++
>   xen/arch/arm/domain_build.c           | 10 +++++++---
>   xen/arch/arm/include/asm/kernel.h     |  3 +++
>   3 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/docs/misc/arm/device-tree/booting.txt 
> b/docs/misc/arm/device-tree/booting.txt
> index edef98e6d1..87f57f8889 100644
> --- a/docs/misc/arm/device-tree/booting.txt
> +++ b/docs/misc/arm/device-tree/booting.txt
> @@ -204,6 +204,10 @@ with the following properties:
>       - "disabled"
>       Xen PV interfaces are disabled.
>   
> +    - no-xenstore
> +    Xen PV interfaces, including grant-table will be enabled for the VM but
> +    xenstore will be disabled for the VM.
NIT: I would drop one of the "for the VM" as it seems to be redundant.

> +
>       If the xen,enhanced property is present with no value, it defaults
>       to "enabled". If the xen,enhanced property is not present, PV
>       interfaces are disabled.
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 4b24261825..8dd9984225 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -3336,10 +3336,14 @@ static int __init construct_domU(struct domain *d,
>            (rc == 0 && !strcmp(dom0less_enhanced, "enabled")) )
>       {
>           if ( hardware_domain )
> -            kinfo.dom0less_enhanced = true;
> +            kinfo.dom0less_xenstore = true;
>           else
> -            panic("Tried to use xen,enhanced without dom0\n");
> +            panic("Tried to use xen,enhanced without dom0 without 
> no-xenstore\n");
This is a bit hard to parse. How about:

"At the moment, Xenstore support requires dom0 to be present"

>       }
> +    else if ( rc == 0 && !strcmp(dom0less_enhanced, "no-xenstore") )
> +        kinfo.dom0less_xenstore = false;
> +
> +    kinfo.dom0less_enhanced = true;
Wouldn't this now set dom0less_enhanced unconditionally?

>   
>       if ( vcpu_create(d, 0) == NULL )
>           return -ENOMEM;
> @@ -3379,7 +3383,7 @@ static int __init construct_domU(struct domain *d,
>       if ( rc < 0 )
>           return rc;
>   
> -    if ( kinfo.dom0less_enhanced )
> +    if ( kinfo.dom0less_xenstore )
>       {
>           ASSERT(hardware_domain);
>           rc = alloc_xenstore_evtchn(d);
> diff --git a/xen/arch/arm/include/asm/kernel.h 
> b/xen/arch/arm/include/asm/kernel.h
> index c4dc039b54..3d7fa94910 100644
> --- a/xen/arch/arm/include/asm/kernel.h
> +++ b/xen/arch/arm/include/asm/kernel.h
> @@ -39,6 +39,9 @@ struct kernel_info {
>       /* Enable PV drivers */
>       bool dom0less_enhanced;
>   
> +    /* Enable Xenstore */
> +    bool dom0less_xenstore;
> +
AFAIU, it is not possible to have *_xenstore = true and *_enhanced = 
false. I think it would be clearer if ``dom0less_enhanced`` is turned to 
an enum with 3 values:
 - None
 - NOXENSTORE/BASIC
 - FULLY_ENHANCED

If we want to be future proof, I would use a field 'flags' where 
non-zero means enhanced. Each bit would indicate which features of Xen 
is exposed.
Cheers,

--
Julien Grall