Re: [Xen-devel] [PATCH v3 2/3] x86/svm: add EFER SVME support for VGIF/VLOAD

2018-02-14 Thread Jan Beulich 
>>> On 13.02.18 at 19:37,  wrote:
> Pardon any weird formatting, I'm replying on my phone. 
> 
> Because they are two different things.  One is an assert to make sure 
> nothing wrong is happening with the EFER.SVME bit, and the other changes what 
> features are enabled.  
> 
> IIRC, most asserts are on their on ifs and not in a if statement with 
> something else.  I guess I should have put the assert higher in the function 
> though but that's a small detail.  
> 
> Yes, you can squeeze both in one if statement but, but it being cleaner and 
> easier to read (at least more logical) is better than getting rid of one if 
> in my opinion.  Plus assuming asserts are disabled for release, I'd assume 
> the extra if would get optimized out by gcc anyway. 

In that case it would better be

ASSERT(nestedhvm_enabled(v->domain) || !(v->arch.hvm_vcpu.guest_efer & 
EFER_SVME));

(suitably line wrapped if necessary). But I continue to think the
if/else variant looks better overall. It'll be the SVM maintainers to
decide, though.

Jan

> On February 13, 2018 03:31:40 Jan Beulich  wrote:
> 
> On 08.02.18 at 18:01,  wrote:
>>> --- a/xen/arch/x86/hvm/svm/svm.c
>>> +++ b/xen/arch/x86/hvm/svm/svm.c
>>> @@ -611,6 +611,12 @@ static void svm_update_guest_efer(struct vcpu *v)
>>>  if ( lma )
>>>  new_efer |= EFER_LME;
>>>  vmcb_set_efer(vmcb, new_efer);
>>> +
>>> +if ( !nestedhvm_enabled(v->domain) )
>>> +ASSERT(!(v->arch.hvm_vcpu.guest_efer & EFER_SVME));
>>> +
>>> +if ( nestedhvm_enabled(v->domain) )
>>> +svm_nested_features_on_efer_update(v);
>>>  }
>>
>> Why not
>>
>> if ( nestedhvm_enabled(v->domain) )
>> svm_nested_features_on_efer_update(v);
>> else
>> ASSERT(!(v->arch.hvm_vcpu.guest_efer & EFER_SVME));
>>
>> ?
>>
>> Jan
>>
>>
>>




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

Re: [Xen-devel] [PATCH v3 2/3] x86/svm: add EFER SVME support for VGIF/VLOAD

2018-02-13 Thread Woods, Brian 
Pardon any weird formatting, I'm replying on my phone. 

Because they are two different things.  One is an assert to make sure nothing 
wrong is happening with the EFER.SVME bit, and the other changes what features 
are enabled.  

IIRC, most asserts are on their on ifs and not in a if statement with something 
else.  I guess I should have put the assert higher in the function though but 
that's a small detail.  

Yes, you can squeeze both in one if statement but, but it being cleaner and 
easier to read (at least more logical) is better than getting rid of one if in 
my opinion.  Plus assuming asserts are disabled for release, I'd assume the 
extra if would get optimized out by gcc anyway. 

Brian


On February 13, 2018 03:31:40 Jan Beulich  wrote:

 On 08.02.18 at 18:01,  wrote:
>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -611,6 +611,12 @@ static void svm_update_guest_efer(struct vcpu *v)
>>  if ( lma )
>>  new_efer |= EFER_LME;
>>  vmcb_set_efer(vmcb, new_efer);
>> +
>> +if ( !nestedhvm_enabled(v->domain) )
>> +ASSERT(!(v->arch.hvm_vcpu.guest_efer & EFER_SVME));
>> +
>> +if ( nestedhvm_enabled(v->domain) )
>> +svm_nested_features_on_efer_update(v);
>>  }
>
> Why not
>
> if ( nestedhvm_enabled(v->domain) )
> svm_nested_features_on_efer_update(v);
> else
> ASSERT(!(v->arch.hvm_vcpu.guest_efer & EFER_SVME));
>
> ?
>
> Jan
>
>
>

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

Re: [Xen-devel] [PATCH v3 2/3] x86/svm: add EFER SVME support for VGIF/VLOAD

2018-02-13 Thread Jan Beulich 
>>> On 08.02.18 at 18:01,  wrote:
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -611,6 +611,12 @@ static void svm_update_guest_efer(struct vcpu *v)
>  if ( lma )
>  new_efer |= EFER_LME;
>  vmcb_set_efer(vmcb, new_efer);
> +
> +if ( !nestedhvm_enabled(v->domain) )
> +ASSERT(!(v->arch.hvm_vcpu.guest_efer & EFER_SVME));
> +
> +if ( nestedhvm_enabled(v->domain) )
> +svm_nested_features_on_efer_update(v);
>  }

Why not

if ( nestedhvm_enabled(v->domain) )
svm_nested_features_on_efer_update(v);
else
ASSERT(!(v->arch.hvm_vcpu.guest_efer & EFER_SVME));

?

Jan




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

[Xen-devel] [PATCH v3 2/3] x86/svm: add EFER SVME support for VGIF/VLOAD

2018-02-08 Thread Brian Woods 
Only enable virtual VMLOAD/SAVE and VGIF if the guest EFER.SVME is set.

Reported-by: Andrew Cooper 
Signed-off-by: Brian Woods 
---
 xen/arch/x86/hvm/svm/nestedsvm.c| 66 +
 xen/arch/x86/hvm/svm/svm.c  |  6 +++
 xen/arch/x86/hvm/svm/vmcb.c | 17 -
 xen/include/asm-x86/hvm/svm/nestedsvm.h |  1 +
 4 files changed, 73 insertions(+), 17 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
index 1f7b0d3e88..9295e583d7 100644
--- a/xen/arch/x86/hvm/svm/nestedsvm.c
+++ b/xen/arch/x86/hvm/svm/nestedsvm.c
@@ -1659,3 +1659,69 @@ void svm_vmexit_do_clgi(struct cpu_user_regs *regs, 
struct vcpu *v)
 
 __update_guest_eip(regs, inst_len);
 }
+
+/*
+ * This runs on EFER change to see if nested features need to either be
+ * turned off or on.
+ */
+void svm_nested_features_on_efer_update(struct vcpu *v)
+{
+struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
+struct nestedsvm *svm = _nestedsvm(v);
+u32 general2_intercepts;
+vintr_t vintr;
+
+/*
+ * Need state for transfering the nested gif status so only write on
+ * the hvm_vcpu EFER.SVME changing.
+ */
+if ( v->arch.hvm_vcpu.guest_efer & EFER_SVME )
+{
+if ( !vmcb->virt_ext.fields.vloadsave_enable &&
+ paging_mode_hap(v->domain) &&
+ cpu_has_svm_vloadsave )
+{
+vmcb->virt_ext.fields.vloadsave_enable = 1;
+general2_intercepts  = vmcb_get_general2_intercepts(vmcb);
+general2_intercepts &= ~(GENERAL2_INTERCEPT_VMLOAD |
+ GENERAL2_INTERCEPT_VMSAVE);
+vmcb_set_general2_intercepts(vmcb, general2_intercepts);
+}
+
+if ( !vmcb->_vintr.fields.vgif_enable &&
+ cpu_has_svm_vgif )
+{
+vintr = vmcb_get_vintr(vmcb);
+vintr.fields.vgif = svm->ns_gif;
+vintr.fields.vgif_enable = 1;
+vmcb_set_vintr(vmcb, vintr);
+general2_intercepts  = vmcb_get_general2_intercepts(vmcb);
+general2_intercepts &= ~(GENERAL2_INTERCEPT_STGI |
+ GENERAL2_INTERCEPT_CLGI);
+vmcb_set_general2_intercepts(vmcb, general2_intercepts);
+}
+}
+else
+{
+if ( vmcb->virt_ext.fields.vloadsave_enable )
+{
+vmcb->virt_ext.fields.vloadsave_enable = 0;
+general2_intercepts  = vmcb_get_general2_intercepts(vmcb);
+general2_intercepts |= (GENERAL2_INTERCEPT_VMLOAD |
+GENERAL2_INTERCEPT_VMSAVE);
+vmcb_set_general2_intercepts(vmcb, general2_intercepts);
+}
+
+if ( vmcb->_vintr.fields.vgif_enable )
+{
+vintr = vmcb_get_vintr(vmcb);
+svm->ns_gif = vintr.fields.vgif;
+vintr.fields.vgif_enable = 0;
+vmcb_set_vintr(vmcb, vintr);
+general2_intercepts  = vmcb_get_general2_intercepts(vmcb);
+general2_intercepts |= (GENERAL2_INTERCEPT_STGI |
+GENERAL2_INTERCEPT_CLGI);
+vmcb_set_general2_intercepts(vmcb, general2_intercepts);
+}
+}
+}
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index c48fdfaa5d..be08a5aa5e 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -611,6 +611,12 @@ static void svm_update_guest_efer(struct vcpu *v)
 if ( lma )
 new_efer |= EFER_LME;
 vmcb_set_efer(vmcb, new_efer);
+
+if ( !nestedhvm_enabled(v->domain) )
+ASSERT(!(v->arch.hvm_vcpu.guest_efer & EFER_SVME));
+
+if ( nestedhvm_enabled(v->domain) )
+svm_nested_features_on_efer_update(v);
 }
 
 static void svm_cpuid_policy_changed(struct vcpu *v)
diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
index 0e6cba5b7b..997e7597e0 100644
--- a/xen/arch/x86/hvm/svm/vmcb.c
+++ b/xen/arch/x86/hvm/svm/vmcb.c
@@ -200,29 +200,12 @@ static int construct_vmcb(struct vcpu *v)
 
 /* PAT is under complete control of SVM when using nested paging. */
 svm_disable_intercept_for_msr(v, MSR_IA32_CR_PAT);
-
-/* Use virtual VMLOAD/VMSAVE if available. */
-if ( cpu_has_svm_vloadsave )
-{
-vmcb->virt_ext.fields.vloadsave_enable = 1;
-vmcb->_general2_intercepts &= ~GENERAL2_INTERCEPT_VMLOAD;
-vmcb->_general2_intercepts &= ~GENERAL2_INTERCEPT_VMSAVE;
-}
 }
 else
 {
 vmcb->_exception_intercepts |= (1U << TRAP_page_fault);
 }
 
-/* if available, enable and configure virtual gif */
-if ( cpu_has_svm_vgif )
-{
-vmcb->_vintr.fields.vgif = 1;
-vmcb->_vintr.fields.vgif_enable = 1;
-vmcb->_general2_intercepts &= ~GENERAL2_INTERCEPT_STGI;
-vmcb->_general2_intercepts &= ~GENERAL2_INTERCEPT_CLGI;
-}
-