| Message ID | 20190816172001.3905-3-paul.durrant@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | use stashed domain create flags... | expand |
On Fri, Aug 16, 2019 at 06:19:53PM +0100, 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 > macro can be transformed into an inline function and re-located to > xen/sched.h. This also makes hap_enabled() common, thus allowing > removal of an ugly ifdef CONFIG_X86 from the common iommu code. > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks, Roger.
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
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/ > >> 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. One other thing. Why is this eval_nospec()? ~Andrew
> -----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
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"); + return -EINVAL; + } + return 0; } @@ -564,12 +570,7 @@ int arch_domain_create(struct domain *d, HYPERVISOR_COMPAT_VIRT_START(d) = is_pv_domain(d) ? __HYPERVISOR_COMPAT_VIRT_START : ~0u; - /* Need to determine if HAP is enabled before initialising paging */ - if ( is_hvm_domain(d) ) - d->arch.hvm.hap_enabled = - hvm_hap_supported() && (config->flags & XEN_DOMCTL_CDF_hap); - - if ( (rc = paging_domain_init(d, config->flags)) != 0 ) + if ( (rc = paging_domain_init(d)) != 0 ) goto fail; paging_initialised = true; diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c index 011089368a..097a27f608 100644 --- a/xen/arch/x86/mm/paging.c +++ b/xen/arch/x86/mm/paging.c @@ -632,7 +632,7 @@ void paging_log_dirty_init(struct domain *d, const struct log_dirty_ops *ops) /* CODE FOR PAGING SUPPORT */ /************************************************/ /* Domain paging struct initialization. */ -int paging_domain_init(struct domain *d, unsigned int domcr_flags) +int paging_domain_init(struct domain *d) { int rc; @@ -653,7 +653,7 @@ int paging_domain_init(struct domain *d, unsigned int domcr_flags) if ( hap_enabled(d) ) hap_domain_init(d); else - rc = shadow_domain_init(d, domcr_flags); + rc = shadow_domain_init(d, d->options); return rc; } 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"); + return -EINVAL; + } + return arch_sanitise_domain_config(config); } diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c index f8c3bf53bd..37eb0f7d01 100644 --- a/xen/drivers/passthrough/iommu.c +++ b/xen/drivers/passthrough/iommu.c @@ -573,9 +573,7 @@ int iommu_do_domctl( void iommu_share_p2m_table(struct domain* d) { -#ifdef CONFIG_X86 ASSERT(hap_enabled(d)); -#endif /* * iommu_use_hap_pt(d) cannot be used here because during domain * construction need_iommu(d) will always return false here. diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h index 6c7c4f5aa6..bcc5621797 100644 --- a/xen/include/asm-x86/hvm/domain.h +++ b/xen/include/asm-x86/hvm/domain.h @@ -156,7 +156,6 @@ struct hvm_domain { struct viridian_domain *viridian; - bool_t hap_enabled; bool_t mem_sharing_enabled; bool_t qemu_mapcache_invalidate; bool_t is_s3_suspended; @@ -195,12 +194,6 @@ struct hvm_domain { }; }; -#ifdef CONFIG_HVM -#define hap_enabled(d) (is_hvm_domain(d) && (d)->arch.hvm.hap_enabled) -#else -#define hap_enabled(d) ({(void)(d); false;}) -#endif - #endif /* __ASM_X86_HVM_DOMAIN_H__ */ /* diff --git a/xen/include/asm-x86/paging.h b/xen/include/asm-x86/paging.h index cf57ca708d..ab7887f23c 100644 --- a/xen/include/asm-x86/paging.h +++ b/xen/include/asm-x86/paging.h @@ -207,7 +207,7 @@ void paging_vcpu_init(struct vcpu *v); /* Set up the paging-assistance-specific parts of a domain struct at * start of day. Called for every domain from arch_domain_create() */ -int paging_domain_init(struct domain *d, unsigned int domcr_flags); +int paging_domain_init(struct domain *d); /* Handler for paging-control ops: operations from user-space to enable * and disable ephemeral shadow modes (test mode and log-dirty mode) and 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); +} + static inline bool is_hwdom_pinned_vcpu(const struct vcpu *v) { return (is_hardware_domain(v->domain) &&
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 macro can be transformed into an inline function and re-located to xen/sched.h. This also makes hap_enabled() common, thus allowing removal of an ugly ifdef CONFIG_X86 from the common iommu code. Signed-off-by: Paul Durrant <paul.durrant@citrix.com> --- Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: Wei Liu <wl@xen.org> Cc: "Roger Pau Monné" <roger.pau@citrix.com> Cc: George Dunlap <George.Dunlap@eu.citrix.com> Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Julien Grall <julien.grall@arm.com> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Tim Deegan <tim@xen.org> Cc: George Dunlap <george.dunlap@eu.citrix.com> v4: - Add missing perentheses and move hap_enabled() - Fix the shim build v3: - Re-worked as suggested by Jan - Not adding Roger's R-b as the patch has changed substantially v2: - Defer changes to shadow_domain_init() to patch #4 --- xen/arch/x86/domain.c | 13 +++++++------ xen/arch/x86/mm/paging.c | 4 ++-- xen/common/domain.c | 7 +++++++ xen/drivers/passthrough/iommu.c | 2 -- xen/include/asm-x86/hvm/domain.h | 7 ------- xen/include/asm-x86/paging.h | 2 +- xen/include/xen/sched.h | 6 ++++++ 7 files changed, 23 insertions(+), 18 deletions(-)