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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien,

> On 1 Sep 2022, at 19:15, Julien Grall <julien@xxxxxxx> wrote:
> 
> 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.

I think that could be a good solution if we do it this way:
- define a dom0less feature field and have defines like the following:
#define DOM0LESS_GNTTAB
#define DOM0LESS_EVENTCHN
#define DOM0LESS_XENSTORE

- define dom0less enhanced as the right combination:
#define DOM0LESS_ENHANCED = (DOM0LESS_GNTTAB| DOM0LESS_EVENTCHN| 
DOM0LESS_XENSTORE)

This way we have a proper feature bitset and ENHANCED is properly defined as a 
combination of the 3 features.

What do you guys think ?

Cheers
Bertrand


> 
> Cheers,
> 
> -- 
> Julien Grall