Re: [PATCH v2 12/12] xen/domain: Allocate d->vcpu[] in domain_create()

["Roger Pau Monné" <roger.pau@citrix.com>]

On Mon, Aug 13, 2018 at 11:01:09AM +0100, Andrew Cooper wrote:
> For ARM, the call to arch_domain_create() needs to have completed before
> domain_max_vcpus() will return the correct upper bound.
> 
> For each arch's dom0's, drop the temporary max_vcpus parameter, and allocation
> of dom0->vcpu.
> 
> With d->max_vcpus now correctly configured before evtchn_init(), the poll mask
> can be constructed suitably for the domain, rather than for the worst-case
> setting.
> 
> Due to the evtchn_init() fixes, it no longer calls domain_max_vcpus(), and
> ARM's two implementations of vgic_max_vcpus() no longer need work around the
> out-of-order call.
> 
> From this point on, d->max_vcpus and d->vcpus[] are valid for any domain which
> can be looked up by domid.
> 
> The XEN_DOMCTL_max_vcpus hypercall is modified to reject any call attempt with
> max != d->max_vcpus, which does match the older semantics (not that it is
> obvious from the code).  The logic to allocate d->vcpu[] is dropped, but at
> this point the hypercall still needs making to allocate each vcpu.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> 
> v2:
>  * Allocate in domain_create() rather than arch_domain_create().
>  * Retain domain_max_vcpus().
> ---
>  xen/arch/arm/domain_build.c |  8 +-------
>  xen/arch/arm/setup.c        |  2 +-
>  xen/arch/arm/vgic.c         | 11 +----------
>  xen/arch/arm/vgic/vgic.c    | 22 +---------------------
>  xen/arch/x86/dom0_build.c   |  8 +-------
>  xen/arch/x86/setup.c        |  2 +-
>  xen/common/domain.c         | 18 ++++++++++++++++++
>  xen/common/domctl.c         | 39 +--------------------------------------
>  xen/common/event_channel.c  |  3 +--
>  xen/include/xen/domain.h    |  2 +-
>  10 files changed, 27 insertions(+), 88 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index f4a1225..6f45e56 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -72,14 +72,8 @@ unsigned int __init dom0_max_vcpus(void)
>      return opt_dom0_max_vcpus;
>  }
>  
> -struct vcpu *__init alloc_dom0_vcpu0(struct domain *dom0,
> -                                     unsigned int max_vcpus)
> +struct vcpu *__init alloc_dom0_vcpu0(struct domain *dom0)
>  {
> -    dom0->vcpu = xzalloc_array(struct vcpu *, max_vcpus);
> -    if ( !dom0->vcpu )
> -        return NULL;
> -    dom0->max_vcpus = max_vcpus;
> -
>      return alloc_vcpu(dom0, 0, 0);
>  }
>  
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 72e42e8..a3e1ef7 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -863,7 +863,7 @@ void __init start_xen(unsigned long boot_phys_offset,
>      dom0_cfg.max_vcpus = dom0_max_vcpus();
>  
>      dom0 = domain_create(0, &dom0_cfg, true);
> -    if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0, dom0_cfg.max_vcpus) == NULL) )
> +    if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
>              panic("Error creating domain 0");
>  
>      if ( construct_dom0(dom0) != 0)
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 7a2c455..5a4f082 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -669,16 +669,7 @@ void vgic_free_virq(struct domain *d, unsigned int virq)
>  
>  unsigned int vgic_max_vcpus(const struct domain *d)
>  {
> -    /*
> -     * Since evtchn_init would call domain_max_vcpus for poll_mask
> -     * allocation when the vgic_ops haven't been initialised yet,
> -     * we return MAX_VIRT_CPUS if d->arch.vgic.handler is null.
> -     */
> -    if ( !d->arch.vgic.handler )
> -        return MAX_VIRT_CPUS;
> -    else
> -        return min_t(unsigned int, MAX_VIRT_CPUS,
> -                     d->arch.vgic.handler->max_vcpus);
> +    return min_t(unsigned int, MAX_VIRT_CPUS, d->arch.vgic.handler->max_vcpus);
>  }
>  
>  /*
> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
> index 832632a..4124817 100644
> --- a/xen/arch/arm/vgic/vgic.c
> +++ b/xen/arch/arm/vgic/vgic.c
> @@ -951,27 +951,7 @@ void vgic_sync_hardware_irq(struct domain *d,
>  
>  unsigned int vgic_max_vcpus(const struct domain *d)
>  {
> -    unsigned int vgic_vcpu_limit;
> -
> -    switch ( d->arch.vgic.version )
> -    {
> -    case GIC_INVALID:
> -        /*
> -         * Since evtchn_init would call domain_max_vcpus for poll_mask
> -         * allocation before the VGIC has been initialised, we need to
> -         * return some safe value in this case. As this is for allocation
> -         * purposes, go with the maximum value.
> -         */
> -        vgic_vcpu_limit = MAX_VIRT_CPUS;
> -        break;
> -    case GIC_V2:
> -        vgic_vcpu_limit = VGIC_V2_MAX_CPUS;
> -        break;
> -    default:
> -        BUG();
> -    }
> -
> -    return min_t(unsigned int, MAX_VIRT_CPUS, vgic_vcpu_limit);
> +    return min_t(unsigned int, MAX_VIRT_CPUS, d->arch.vgic.handler->max_vcpus);
>  }

Since both implementations are equal now, can you place this in vgic.h
as a static inline function?

With that:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel