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

From: Paul Durrant <Paul.Durrant@citrix.com>
To: Andrew Cooper <Andrew.Cooper3@citrix.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Cc: Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	"Tim \(Xen.org\)" <tim@xen.org>,
	George Dunlap <George.Dunlap@citrix.com>,
	Julien Grall <julien.grall@arm.com>,
	Jan Beulich <jbeulich@suse.com>,
	Ian Jackson <Ian.Jackson@citrix.com>,
	Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH v6 02/10] x86/hvm/domain: remove the 'hap_enabled' flag
Date: Tue, 27 Aug 2019 08:19:39 +0000	[thread overview]
Message-ID: <8349b70cdcdf434bbc6da7794d25e46d@AMSPEX02CL03.citrite.net> (raw)
In-Reply-To: <f3e88434-6800-2271-f5a0-9ea6776f9d40@citrix.com>

> -----Original Message-----
> From: Andrew Cooper <Andrew.Cooper3@citrix.com>
> Sent: 23 August 2019 13:26
> 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@citrix.com>; Tim (Xen.org) <tim@xen.org>; Ian
> Jackson <Ian.Jackson@citrix.com>; Julien Grall <julien.grall@arm.com>; Jan Beulich
> <jbeulich@suse.com>; Roger Pau Monne <roger.pau@citrix.com>
> Subject: Re: [Xen-devel] [PATCH v6 02/10] x86/hvm/domain: remove the 'hap_enabled' flag
> 
> On 23/08/2019 13:23, Andrew Cooper wrote:
> > 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/

Oh yes.

> >
> >> 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/
> >

I'm not fussed... I just went with the incumbent flag name.

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

Ok.

> One other thing.  Why is this eval_nospec()?
> 

General paranoia about what might happen in speculation if the inline evaluates false and we wander into e.g. shadow code.

  Paul

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