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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>
> Sent: 23 August 2019 13:26
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; Konrad 
> Rzeszutek Wilk
> <konrad.wilk@xxxxxxxxxx>; George Dunlap <George.Dunlap@xxxxxxxxxx>; Tim 
> (Xen.org) <tim@xxxxxxx>; Ian
> Jackson <Ian.Jackson@xxxxxxxxxx>; Julien Grall <julien.grall@xxxxxxx>; Jan 
> Beulich
> <jbeulich@xxxxxxxx>; Roger Pau Monne <roger.pau@xxxxxxxxxx>
> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel