Re: [PATCH v6 4/7] xen/arm: configure dom0less domain for enabling xenstore after boot

[Julien Grall <julien@xxxxxxx>]

Hi Stefano,

On 05/05/2022 01:16, Stefano Stabellini wrote:
> From: Luca Miccio <lucmiccio@xxxxxxxxx>
>
> Export evtchn_alloc_unbound and make it __must_check.
>
> If "xen,enhanced" is enabled, then add to dom0less domains:
>
> - the hypervisor node in device tree
> - the xenstore event channel
>
> The xenstore event channel is also used for the first notification to
> let the guest know that xenstore has become available.
>
> Signed-off-by: Luca Miccio <lucmiccio@xxxxxxxxx>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> Reviewed-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> CC: Julien Grall <julien@xxxxxxx>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> CC: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> CC: jbeulich@xxxxxxxx
>
> ---
> Changes in v5:
> - merge with "xen: export evtchn_alloc_unbound"
> - __must_check
>
> Changes in v3:
> - use evtchn_alloc_unbound
>
> Changes in v2:
> - set HVM_PARAM_STORE_PFN to ~0ULL at domain creation
> - in alloc_xenstore_evtchn do not call _evtchn_alloc_unbound
>
> xen: export evtchn_alloc_unbound
>
> It will be used during dom0less domains construction.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> ---
>   xen/arch/arm/domain_build.c | 37 +++++++++++++++++++++++++++++++++++++
>   xen/common/event_channel.c  |  2 +-
>   xen/include/xen/event.h     |  3 +++
>   3 files changed, 41 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 016f56a99f..bb430f2189 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -27,6 +27,7 @@
>   #include <asm/setup.h>
>   #include <asm/cpufeature.h>
>   #include <asm/domain_build.h>
> +#include <xen/event.h>
>   
>   #include <xen/irq.h>
>   #include <xen/grant_table.h>
> @@ -2810,6 +2811,8 @@ static int __init prepare_dtb_domU(struct domain *d, 
> struct kernel_info *kinfo)
>       int ret;
>   
>       kinfo->phandle_gic = GUEST_PHANDLE_GIC;
> +    kinfo->gnttab_start = GUEST_GNTTAB_BASE;
> +    kinfo->gnttab_size = GUEST_GNTTAB_SIZE;
>   
>       addrcells = GUEST_ROOT_ADDRESS_CELLS;
>       sizecells = GUEST_ROOT_SIZE_CELLS;
> @@ -2884,6 +2887,13 @@ static int __init prepare_dtb_domU(struct domain *d, 
> struct kernel_info *kinfo)
>               goto err;
>       }
>   
> +    if ( kinfo->dom0less_enhanced )
> +    {
> +        ret = make_hypervisor_node(d, kinfo, addrcells, sizecells);
> +        if ( ret )
> +            goto err;
> +    }
> +
>       ret = fdt_end_node(kinfo->fdt);
>       if ( ret < 0 )
>           goto err;
> @@ -3150,6 +3160,25 @@ static int __init construct_domain(struct domain *d, 
> struct kernel_info *kinfo)
>       return 0;
>   }
>   
> +static int __init alloc_xenstore_evtchn(struct domain *d)
> +{
> +    evtchn_alloc_unbound_t alloc;
> +    int rc;
> +
> +    alloc.dom = d->domain_id;
> +    alloc.remote_dom = hardware_domain->domain_id;
> +    rc = evtchn_alloc_unbound(&alloc);
> +    if ( rc )
> +    {
> +        printk("Failed allocating event channel for domain\n");
> +        return rc;
> +    }
> +
> +    d->arch.hvm.params[HVM_PARAM_STORE_EVTCHN] = alloc.port;
> +
> +    return 0;
> +}
> +
>   static int __init construct_domU(struct domain *d,
>                                    const struct dt_device_node *node)
>   {
> @@ -3214,6 +3243,14 @@ static int __init construct_domU(struct domain *d,
>       if ( rc < 0 )
>           return rc;
>   
> +    if ( kinfo.dom0less_enhanced )
> +    {
> +        rc = alloc_xenstore_evtchn(d);
> +        if ( rc < 0 )
> +            return rc;
> +        d->arch.hvm.params[HVM_PARAM_STORE_PFN] = ~0ULL;
This sounds a little bit odd that we set the value to ~0ULL with 
dom0less_enhanced but keep it to 0 outside of dom0less.
If there are any rationale for that, then I would suggest to add a 
comment. (Can be done on commit).
For the common part:

Acked-by: Julien Grall <jgrall@xxxxxxxxxx> # common

Cheers,

--
Julien Grall