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

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Julien,

> On 23 Aug 2022, at 11:05 am, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi,
> 
> On 19/08/2022 11:02, Rahul Singh wrote:
>> Introduce a new "xen,enhanced" dom0less property value "evtchn" to
>> enable/disable event-channel interfaces for dom0less guests.
> 
> The documentation in docs/misc/arm/device-tree/booting.txt is missing. Also, 
> you probably wants to update docs/feature/dom0less.pandoc because the section 
> "PV drivers" suggests that if the property "xen,enhanced" is specified, then 
> we would end up to allocate information for PV drivers.
> 
> AFAIU, this is not the case when "evtchn" is specified.
> 
>> The configurable option is for domUs only. For dom0 we always set the
>> corresponding property in the Xen code to true.
>> Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>
>> ---
>> Changes in v2:
>>  - no change
>> ---
>> ---
>>  xen/arch/arm/domain_build.c       | 149 ++++++++++++++++--------------
>>  xen/arch/arm/include/asm/kernel.h |   3 +
>>  2 files changed, 82 insertions(+), 70 deletions(-)
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 5101bca979..bd8b8475b7 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -1396,85 +1396,92 @@ static int __init make_hypervisor_node(struct domain 
>> *d,
>>      if ( res )
>>          return res;
>>  
> 
> 
> The diff below is quite difficult to read. I have applied to have a look. You 
> seem to have simply indented the code and now some of the
> lines are over the 80 characters mark.
> 
> Ideally, I would like to avoid large 'if'. So I would suggest to either
> re-ordering the code or split in multiple functions.
> 
> However, reading the binding of "xen,xen", the property "reg" and 
> "interrupts" are not optional.
> 
> I also don't think can make them optional because some OSes may not boot if 
> it can't find one of the property.
> 
> In any case, at minimum you should explain why this is fine to make them 
> optional.
> 
> [...]

If we want to expose the "reg” and “interrupts” property always to guests and 
these properties are not 
optional then we can discard this patch and add support for "xen,enhanced” 
property for domUs for
static evtchn to work for domUs

Please let me know your view on this.

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index bfe7bc6b36..a1e23eee59 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -3562,12 +3562,7 @@ static int __init construct_domU(struct domain *d,
   if ( rc == -EILSEQ ||
     rc == -ENODATA ||
     (rc == 0 && !strcmp(dom0less_enhanced, “enabled”)) )
-  {
-    if ( hardware_domain )
       kinfo.dom0less_enhanced = true;
-    else
-      panic(“Tried to use xen,enhanced without dom0\n”);
-  }
   if ( vcpu_create(d, 0) == NULL )
     return -ENOMEM;
 
 
Regards,
Rahul