Re: [Xen-devel] [PATCH v6 02/10] x86/hvm/domain: remove the 'hap_enabled' flag

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Paul Durrant <paul.durrant@citrix.com>, <xen-devel@lists.xenproject.org>
Cc: "Stefano Stabellini" <sstabellini@kernel.org>,
	"Wei Liu" <wl@xen.org>,
	"Konrad Rzeszutek Wilk" <konrad.wilk@oracle.com>,
	"George Dunlap" <George.Dunlap@eu.citrix.com>,
	"Tim Deegan" <tim@xen.org>,
	"Ian Jackson" <ian.jackson@eu.citrix.com>,
	"Julien Grall" <julien.grall@arm.com>,
	"Jan Beulich" <jbeulich@suse.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH v6 02/10] x86/hvm/domain: remove the 'hap_enabled' flag
Date: Fri, 23 Aug 2019 13:23:59 +0100	[thread overview]
Message-ID: <a109294b-6970-cf25-a2bb-fbb032ec24c9@citrix.com> (raw)
In-Reply-To: <20190816172001.3905-3-paul.durrant@citrix.com>

On 16/08/2019 18:19, Paul Durrant wrote:
> The hap_enabled() macro can determine whether the feature is available
> using the domain 'options'; there is no need for a separate flag.
>
> NOTE: Furthermore, by extending sanitiziing of the domain 'options', the

s/ii/i/

> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 9a6eb89ddc..bc0db03387 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -460,6 +460,12 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
>          return -EINVAL;
>      }
>  
> +    if ( (config->flags & XEN_DOMCTL_CDF_hap) && !hvm_hap_supported() )
> +    {
> +        dprintk(XENLOG_INFO, "HAP enabled but not supported\n");

s/enabled/requested/

> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 744b572195..6109623730 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -313,6 +313,13 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config)
>          return -EINVAL;
>      }
>  
> +    if ( !(config->flags & XEN_DOMCTL_CDF_hvm_guest) &&
> +         (config->flags & XEN_DOMCTL_CDF_hap) )
> +    {
> +        dprintk(XENLOG_INFO, "HAP enabled for non-HVM guest\n");

Again, I think 'requested' would be better here.

> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 2e6e0d3488..07a64947ed 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -954,6 +954,12 @@ static inline bool is_hvm_vcpu(const struct vcpu *v)
>      return is_hvm_domain(v->domain);
>  }
>  
> +static inline bool hap_enabled(const struct domain *d)
> +{
> +    return IS_ENABLED(CONFIG_HVM) && /* necessary for pv shim build */
> +        evaluate_nospec(d->options & XEN_DOMCTL_CDF_hap);

I'm not sure how helpful this comment is.  What should be here however
is a note saying that this logic depends on domain_create() rejecting
!HVM  and HAP.

All can be adjusted on commit if there are no other concerns.

~Andrew

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