Re: [PATCH v2 3/5] xen/domain: Stricter configuration checking

[Julien Grall <julien.grall@arm.com>]

Hi Andrew,

On 12/11/2018 16:16, Andrew Cooper wrote:
> Currently, a number of options passed for domain creation are ignored, or have
> implicit fallback behaviour.  This is bad for forwards compatibility, and for
> end users to be certain that they got the configuration they asked for.
> 
> With this change:
>   * ARM now strictly requires that XEN_DOMCTL_CDF_hap is passed.  Previously,
>     only XEN_DOMCTL_CDF_hvm_guest was checked.
>   * For x86, requesting HAP without HVM is now prohibited, as the combination
>     makes no sense.
>   * For x86, requesting HAP on a non-HAP capable system will fail, rather than
>     silently fall back to Shadow.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

For the Arm bits:

Acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> CC: Ian Jackson <Ian.Jackson@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> 
> Semi RFC because this may cause a user-visible change in behaviour.  However,
> if the user has gone to the effort of specifying hap=1, silently falling back
> to shadow is unexpected, and IMO, a bug.
> 
> Alternatively, if this proves to be controversial, it can be dropped from the
> series to avoid blocking the main bugfix.
> 
> v2:
>   * New
> ---
>   xen/arch/arm/domain.c |  7 +++++++
>   xen/arch/x86/domain.c | 40 ++++++++++++++++++++++++++++++++++++++++
>   xen/common/domain.c   | 34 +++-------------------------------
>   3 files changed, 50 insertions(+), 31 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index c24ace6..08ba412 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -601,6 +601,13 @@ void vcpu_switch_to_aarch64_mode(struct vcpu *v)
>   
>   int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
>   {
> +    if ( !(config->flags & XEN_DOMCTL_CDF_hvm_guest) ||
> +         !(config->flags & XEN_DOMCTL_CDF_hap) )
> +    {
> +        dprintk(XENLOG_INFO, "Unsupported configuration %#x\n", config->flags);
> +        return -EINVAL;
> +    }
> +
>       /* Fill in the native GIC version, passed back to the toolstack. */
>       if ( config->arch.gic_version == XEN_DOMCTL_CONFIG_GIC_NATIVE )
>       {
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 28a145a..f47ad04 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -420,6 +420,46 @@ void arch_vcpu_destroy(struct vcpu *v)
>   
>   int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
>   {
> +    bool hvm;
> +
> +    if ( !IS_ENABLED(CONFIG_PV) && !(config->flags & XEN_DOMCTL_CDF_hvm_guest) )
> +    {
> +        dprintk(XENLOG_INFO, "PV support not available\n");
> +        return -EINVAL;
> +    }
> +
> +    if ( !hvm_enabled && (config->flags & XEN_DOMCTL_CDF_hvm_guest) )
> +    {
> +        dprintk(XENLOG_INFO, "HVM support not available\n");
> +        return -EINVAL;
> +    }
> +
> +    hvm = config->flags & XEN_DOMCTL_CDF_hvm_guest;
> +
> +    if ( !hvm )
> +    {
> +        if ( config->flags & XEN_DOMCTL_CDF_hap )
> +        {
> +            dprintk(XENLOG_INFO, "HAP inapplicable for PV guests\n");
> +            return -EINVAL;
> +        }
> +    }
> +    else
> +    {
> +        if ( !IS_ENABLED(CONFIG_SHADOW_PAGING) &&
> +             !(config->flags & XEN_DOMCTL_CDF_hap) )
> +        {
> +            dprintk(XENLOG_INFO, "SHADOW support not available\n");
> +            return -EINVAL;
> +        }
> +
> +        if ( !hvm_hap_supported() && (config->flags & XEN_DOMCTL_CDF_hap) )
> +        {
> +            dprintk(XENLOG_INFO, "HAP support not available\n");
> +            return -EINVAL;
> +        }
> +    }
> +
>       return 0;
>   }
>   
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index ddaf74a..f69f405 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -339,37 +339,9 @@ struct domain *domain_create(domid_t domid,
>           hardware_domain = d;
>       }
>   
> -    /* Sort out our idea of is_{pv,hvm}_domain(). */
> -    if ( config )
> -    {
> -        if ( config->flags & XEN_DOMCTL_CDF_hvm_guest )
> -        {
> -#ifdef CONFIG_HVM
> -            d->guest_type = guest_type_hvm;
> -#else
> -            err = -EINVAL;
> -            goto fail;
> -#endif
> -        }
> -        else
> -        {
> -#ifdef CONFIG_PV
> -            d->guest_type = guest_type_pv;
> -#else
> -            err = -EINVAL;
> -            goto fail;
> -#endif
> -        }
> -    }
> -    else
> -    {
> -        /*
> -         * At least the idle domain should be treated as PV domain
> -         * because it uses PV context switch functions. To err on the
> -         * safe side, leave all system domains to be guest_type_pv.
> -         */
> -        d->guest_type = guest_type_pv;
> -    }
> +    /* Sort out our idea of is_{pv,hvm}_domain().  All system domains are PV. */
> +    d->guest_type = ((config && (config->flags & XEN_DOMCTL_CDF_hvm_guest))
> +                     ? guest_type_hvm : guest_type_pv);
>   
>       TRACE_1D(TRC_DOM0_DOM_ADD, d->domain_id);
>   
> 

-- 
Julien Grall

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