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

2019-08-27 Thread Paul Durrant 
> -Original Message-
> From: Andrew Cooper 
> Sent: 23 August 2019 13:26
> To: Paul Durrant ; xen-devel@lists.xenproject.org
> Cc: Stefano Stabellini ; Wei Liu ; 
> Konrad Rzeszutek Wilk
> ; George Dunlap ; Tim 
> (Xen.org) ; Ian
> Jackson ; Julien Grall ; Jan 
> Beulich
> ; Roger Pau Monne 
> 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

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

2019-08-23 Thread Andrew Cooper 
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

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

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

2019-08-23 Thread Andrew Cooper 
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

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

2019-08-23 Thread Roger Pau Monné 
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 

Reviewed-by: Roger Pau Monné 

Thanks, Roger.

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

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

2019-08-16 Thread Paul Durrant 
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 
---
Cc: Jan Beulich 
Cc: Andrew Cooper 
Cc: Wei Liu 
Cc: "Roger Pau Monné" 
Cc: George Dunlap 
Cc: Ian Jackson 
Cc: Julien Grall 
Cc: Konrad Rzeszutek Wilk 
Cc: Stefano Stabellini 
Cc: Tim Deegan 
Cc: George Dunlap 

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(-)

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