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

[Julien Grall <julien@xxxxxxx>]

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.
[...]


> -    /*
> -     * Interrupt event channel upcall:
> -     *  - Active-low level-sensitive
> -     *  - All CPUs
> -     *  TODO: Handle properly the cpumask;
> -     */
> -    set_interrupt(intr, d->arch.evtchn_irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
> -    res = fdt_property_interrupts(kinfo, &intr, 1);
> -    if ( res )
> -        return res;
> +    if ( kinfo->dom0less_evtchn )
So I understand why you want to make the first part optional. But this 
is not clear why this one become conditional to "dom0less_evtchn". Do 
you have any plan to only present the node "xen,xen" where neither event 
channels nor PV interfaces would be used?
> +    {
> +        BUG_ON(d->arch.evtchn_irq == 0);
> +
> +        /*
> +         * Interrupt event channel upcall:
> +         *  - Active-low level-sensitive
> +         *  - All CPUs
> +         *  TODO: Handle properly the cpumask;
> +        */
> +        set_interrupt(intr, d->arch.evtchn_irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
> +        res = fdt_property_interrupts(kinfo, &intr, 1);
> +        if ( res )
> +            return res;
> +    }
>   
>       res = fdt_end_node(fdt);
>   
> @@ -2891,7 +2898,7 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
>               goto err;
>       }
>   
> -    if ( kinfo->dom0less_enhanced )
> +    if ( kinfo->dom0less_enhanced || kinfo->dom0less_evtchn )
I think the first part of the if can be removed because you can't do 
without event channel.
>       {
>           ret = make_hypervisor_node(d, kinfo, addrcells, sizecells);
>           if ( ret )
> @@ -3343,11 +3350,11 @@ static int __init construct_domU(struct domain *d,
>            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");
> +        kinfo.dom0less_enhanced = true;
> +        kinfo.dom0less_evtchn = true;
>       }
> +    else if ( rc == 0 && !strcmp(dom0less_enhanced, "evtchn") )
> +        kinfo.dom0less_evtchn = true;
>   
>       if ( vcpu_create(d, 0) == NULL )
>           return -ENOMEM;
> @@ -3526,6 +3533,8 @@ static int __init construct_dom0(struct domain *d)
>   
>       kinfo.unassigned_mem = dom0_mem;
>       kinfo.d = d;
> +    kinfo.dom0less_enhanced = true;
> +    kinfo.dom0less_evtchn = true;
>   
>       rc = kernel_probe(&kinfo, NULL);
>       if ( rc < 0 )
> diff --git a/xen/arch/arm/include/asm/kernel.h 
> b/xen/arch/arm/include/asm/kernel.h
> index c4dc039b54..7cff19b997 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 event-channel interface */
> +    bool dom0less_evtchn;
So technically, the event channel interface is still exposed even if 
this is false. This is because we are still allocate the PPI and set the 
number of events to a non-zero value.
IMHO, if dom0less_evtchn is false, then we should properly disable the 
event channels interface not just hide it.
Cheers,


--
Julien Grall