Re: [Xen-devel] [PATCH v6.5 19/26] x86/hvm: Permit guests direct access to MSR_{SPEC_CTRL, PRED_CMD}

2018-01-09 Thread Jan Beulich 
>>> On 09.01.18 at 14:34,  wrote:
> On 09/01/18 13:28, Jan Beulich wrote:
> On 09.01.18 at 13:03,  wrote:
>>> On 04/01/18 09:52, Jan Beulich wrote:
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -132,7 +132,8 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, 
> uint64_t *val)
>  case MSR_SPEC_CTRL:
>  if ( !cp->feat.ibrsb )
>  goto gp_fault;
> -*val = vp->spec_ctrl.guest;
> +*val = (vp->spec_ctrl.direct_access
> +? vp->spec_ctrl.host : vp->spec_ctrl.guest);
>  break;
 To recap, I had asked whether this is valid ahead of later changes,
 which you replied to saying this won't have any "by not permitting
 the guest any access until patch 25". In which case at the very
 least the patch title is misleading. Yet I don't even agree with what
 you say - patch 25 only fiddles with CPUID bits. Did you perhaps
 mean to say "By not permitting a well behaved guest any access
 until patch 25," as one trying to access the MSRs without consulting
 the CPUID bits would be able to starting with the patch here aiui?
>>> The guest access bit being clear in cpufeatureset.h means that the
>>> maximum featureset calculations for guests will guarantee that
>>> cp->feat.ibrsb is currently false.
>> Well, that was the point of my reply: You mean well behaved
>> guests (ones consulting CPUID), but you don't say so, and - as
>> said - I think ones trying to access the MSRs anyway will observe
>> the accesses to work as of this patch, yet as it seems not fully
>> correctly (until that later patch is in place).
>>
>> As pointed out before - I fine with things not working fully right
>> until that later patch, but the situation should be stated clearly.
> 
> But it still functions correctly.  A guest which ignores CPUID will
> still find two MSRs which unconditionally #GP when poked.  The logic to
> allow passthrough is also derived from the cpuid policy, which disallows
> the passthrough until patch 25.

Oh, right - I keep not realizing that the variable named "cp" is
the cpuid policy of the guest.

Jan


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

Re: [Xen-devel] [PATCH v6.5 19/26] x86/hvm: Permit guests direct access to MSR_{SPEC_CTRL, PRED_CMD}

2018-01-09 Thread Andrew Cooper 
On 09/01/18 13:28, Jan Beulich wrote:
 On 09.01.18 at 13:03,  wrote:
>> On 04/01/18 09:52, Jan Beulich wrote:
 --- a/xen/arch/x86/msr.c
 +++ b/xen/arch/x86/msr.c
 @@ -132,7 +132,8 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, 
 uint64_t *val)
  case MSR_SPEC_CTRL:
  if ( !cp->feat.ibrsb )
  goto gp_fault;
 -*val = vp->spec_ctrl.guest;
 +*val = (vp->spec_ctrl.direct_access
 +? vp->spec_ctrl.host : vp->spec_ctrl.guest);
  break;
>>> To recap, I had asked whether this is valid ahead of later changes,
>>> which you replied to saying this won't have any "by not permitting
>>> the guest any access until patch 25". In which case at the very
>>> least the patch title is misleading. Yet I don't even agree with what
>>> you say - patch 25 only fiddles with CPUID bits. Did you perhaps
>>> mean to say "By not permitting a well behaved guest any access
>>> until patch 25," as one trying to access the MSRs without consulting
>>> the CPUID bits would be able to starting with the patch here aiui?
>> The guest access bit being clear in cpufeatureset.h means that the
>> maximum featureset calculations for guests will guarantee that
>> cp->feat.ibrsb is currently false.
> Well, that was the point of my reply: You mean well behaved
> guests (ones consulting CPUID), but you don't say so, and - as
> said - I think ones trying to access the MSRs anyway will observe
> the accesses to work as of this patch, yet as it seems not fully
> correctly (until that later patch is in place).
>
> As pointed out before - I fine with things not working fully right
> until that later patch, but the situation should be stated clearly.

But it still functions correctly.  A guest which ignores CPUID will
still find two MSRs which unconditionally #GP when poked.  The logic to
allow passthrough is also derived from the cpuid policy, which disallows
the passthrough until patch 25.

(In fact, before this patch, a guest which poked at the MSRs wouldn't
find a #GP everywhere, because our MSR infrastructure is leaky.)

~Andrew

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

Re: [Xen-devel] [PATCH v6.5 19/26] x86/hvm: Permit guests direct access to MSR_{SPEC_CTRL, PRED_CMD}

2018-01-09 Thread Jan Beulich 
>>> On 09.01.18 at 13:03,  wrote:
> On 04/01/18 09:52, Jan Beulich wrote:
>>
>>> --- a/xen/arch/x86/msr.c
>>> +++ b/xen/arch/x86/msr.c
>>> @@ -132,7 +132,8 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, 
>>> uint64_t *val)
>>>  case MSR_SPEC_CTRL:
>>>  if ( !cp->feat.ibrsb )
>>>  goto gp_fault;
>>> -*val = vp->spec_ctrl.guest;
>>> +*val = (vp->spec_ctrl.direct_access
>>> +? vp->spec_ctrl.host : vp->spec_ctrl.guest);
>>>  break;
>> To recap, I had asked whether this is valid ahead of later changes,
>> which you replied to saying this won't have any "by not permitting
>> the guest any access until patch 25". In which case at the very
>> least the patch title is misleading. Yet I don't even agree with what
>> you say - patch 25 only fiddles with CPUID bits. Did you perhaps
>> mean to say "By not permitting a well behaved guest any access
>> until patch 25," as one trying to access the MSRs without consulting
>> the CPUID bits would be able to starting with the patch here aiui?
> 
> The guest access bit being clear in cpufeatureset.h means that the
> maximum featureset calculations for guests will guarantee that
> cp->feat.ibrsb is currently false.

Well, that was the point of my reply: You mean well behaved
guests (ones consulting CPUID), but you don't say so, and - as
said - I think ones trying to access the MSRs anyway will observe
the accesses to work as of this patch, yet as it seems not fully
correctly (until that later patch is in place).

As pointed out before - I fine with things not working fully right
until that later patch, but the situation should be stated clearly.

Jan


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

Re: [Xen-devel] [PATCH v6.5 19/26] x86/hvm: Permit guests direct access to MSR_{SPEC_CTRL, PRED_CMD}

2018-01-09 Thread Andrew Cooper 
On 04/01/18 09:52, Jan Beulich wrote:
>
>> --- a/xen/arch/x86/msr.c
>> +++ b/xen/arch/x86/msr.c
>> @@ -132,7 +132,8 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, 
>> uint64_t *val)
>>  case MSR_SPEC_CTRL:
>>  if ( !cp->feat.ibrsb )
>>  goto gp_fault;
>> -*val = vp->spec_ctrl.guest;
>> +*val = (vp->spec_ctrl.direct_access
>> +? vp->spec_ctrl.host : vp->spec_ctrl.guest);
>>  break;
> To recap, I had asked whether this is valid ahead of later changes,
> which you replied to saying this won't have any "by not permitting
> the guest any access until patch 25". In which case at the very
> least the patch title is misleading. Yet I don't even agree with what
> you say - patch 25 only fiddles with CPUID bits. Did you perhaps
> mean to say "By not permitting a well behaved guest any access
> until patch 25," as one trying to access the MSRs without consulting
> the CPUID bits would be able to starting with the patch here aiui?

The guest access bit being clear in cpufeatureset.h means that the
maximum featureset calculations for guests will guarantee that
cp->feat.ibrsb is currently false.

~Andrew

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

Re: [Xen-devel] [PATCH v6.5 19/26] x86/hvm: Permit guests direct access to MSR_{SPEC_CTRL, PRED_CMD}

2018-01-04 Thread Jan Beulich 
>>> On 04.01.18 at 01:15,  wrote:
> @@ -292,6 +301,16 @@ static int update_domain_cpuid_info(struct domain *d,
>  d->arch.pv_domain.cpuidmasks->e1cd = mask;
>  }
>  break;
> +
> +case 0x8008:
> +/*
> + * If the IBRB policy has changed, we need to recalculate the MSR
> + * interception bitmaps.
> + */
> +call_policy_changed = (is_hvm_domain(d) &&
> +   ((old_e8b ^ p->extd.raw[8].b) &
> +(cpufeat_mask(X86_FEATURE_IBPB;

There's a stray pair of parentheses here.

> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -132,7 +132,8 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, 
> uint64_t *val)
>  case MSR_SPEC_CTRL:
>  if ( !cp->feat.ibrsb )
>  goto gp_fault;
> -*val = vp->spec_ctrl.guest;
> +*val = (vp->spec_ctrl.direct_access
> +? vp->spec_ctrl.host : vp->spec_ctrl.guest);
>  break;

To recap, I had asked whether this is valid ahead of later changes,
which you replied to saying this won't have any "by not permitting
the guest any access until patch 25". In which case at the very
least the patch title is misleading. Yet I don't even agree with what
you say - patch 25 only fiddles with CPUID bits. Did you perhaps
mean to say "By not permitting a well behaved guest any access
until patch 25," as one trying to access the MSRs without consulting
the CPUID bits would be able to starting with the patch here aiui?

I do realize that re-ordering the series may be impossible, so I'm
not necessarily asking for a code change. But at least the
description should explain the situation.

Jan


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

[Xen-devel] [PATCH v6.5 19/26] x86/hvm: Permit guests direct access to MSR_{SPEC_CTRL, PRED_CMD}

2018-01-03 Thread Andrew Cooper 
For performance reasons, HVM guests should have direct access to these MSRs
when possible.

Signed-off-by: Andrew Cooper 
---
v4:
 * Redo almost from scratch to support AMD
v6:
 * Allow direct access to PRED_CMD for IBPB
---
 xen/arch/x86/domctl.c  | 19 +++
 xen/arch/x86/hvm/svm/svm.c |  5 +
 xen/arch/x86/hvm/vmx/vmx.c | 18 ++
 xen/arch/x86/msr.c |  3 ++-
 xen/include/asm-x86/msr.h  |  5 -
 5 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 72b4489..81fbeaa 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -53,6 +53,7 @@ static int update_domain_cpuid_info(struct domain *d,
 struct cpuid_policy *p = d->arch.cpuid;
 const struct cpuid_leaf leaf = { ctl->eax, ctl->ebx, ctl->ecx, ctl->edx };
 int old_vendor = p->x86_vendor;
+unsigned int old_7d0 = p->feat.raw[0].d, old_e8b = p->extd.raw[8].b;
 bool call_policy_changed = false; /* Avoid for_each_vcpu() unnecessarily */
 
 /*
@@ -218,6 +219,14 @@ static int update_domain_cpuid_info(struct domain *d,
 
 d->arch.pv_domain.cpuidmasks->_7ab0 = mask;
 }
+
+/*
+ * If the IBSRB/STIBP policy has changed, we need to recalculate the
+ * MSR interception bitmaps and STIBP protection default.
+ */
+call_policy_changed = ((old_7d0 ^ p->feat.raw[0].d) &
+   (cpufeat_mask(X86_FEATURE_IBRSB) |
+cpufeat_mask(X86_FEATURE_STIBP)));
 break;
 
 case 0xa:
@@ -292,6 +301,16 @@ static int update_domain_cpuid_info(struct domain *d,
 d->arch.pv_domain.cpuidmasks->e1cd = mask;
 }
 break;
+
+case 0x8008:
+/*
+ * If the IBRB policy has changed, we need to recalculate the MSR
+ * interception bitmaps.
+ */
+call_policy_changed = (is_hvm_domain(d) &&
+   ((old_e8b ^ p->extd.raw[8].b) &
+(cpufeat_mask(X86_FEATURE_IBPB;
+break;
 }
 
 if ( call_policy_changed )
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index c48fdfa..ee47508 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -617,6 +617,7 @@ static void svm_cpuid_policy_changed(struct vcpu *v)
 {
 struct arch_svm_struct *arch_svm = >arch.hvm_svm;
 struct vmcb_struct *vmcb = arch_svm->vmcb;
+const struct cpuid_policy *cp = v->domain->arch.cpuid;
 u32 bitmap = vmcb_get_exception_intercepts(vmcb);
 
 if ( opt_hvm_fep ||
@@ -626,6 +627,10 @@ static void svm_cpuid_policy_changed(struct vcpu *v)
 bitmap &= ~(1U << TRAP_invalid_op);
 
 vmcb_set_exception_intercepts(vmcb, bitmap);
+
+/* Give access to MSR_PRED_CMD if the guest has been told about it. */
+svm_intercept_msr(v, MSR_PRED_CMD,
+  cp->extd.ibpb ? MSR_INTERCEPT_NONE : MSR_INTERCEPT_RW);
 }
 
 static void svm_sync_vmcb(struct vcpu *v)
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index e036303..8609de3 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -656,6 +656,8 @@ void vmx_update_exception_bitmap(struct vcpu *v)
 
 static void vmx_cpuid_policy_changed(struct vcpu *v)
 {
+const struct cpuid_policy *cp = v->domain->arch.cpuid;
+
 if ( opt_hvm_fep ||
  (v->domain->arch.cpuid->x86_vendor != boot_cpu_data.x86_vendor) )
 v->arch.hvm_vmx.exception_bitmap |= (1U << TRAP_invalid_op);
@@ -665,6 +667,22 @@ static void vmx_cpuid_policy_changed(struct vcpu *v)
 vmx_vmcs_enter(v);
 vmx_update_exception_bitmap(v);
 vmx_vmcs_exit(v);
+
+/*
+ * We can only pass though MSR_SPEC_CTRL if the guest knows about all bits
+ * in it.  Otherwise, Xen may be fixing up in the background.
+ */
+v->arch.msr->spec_ctrl.direct_access = cp->feat.ibrsb && cp->feat.stibp;
+if ( v->arch.msr->spec_ctrl.direct_access )
+vmx_clear_msr_intercept(v, MSR_SPEC_CTRL, VMX_MSR_RW);
+else
+vmx_set_msr_intercept(v, MSR_SPEC_CTRL, VMX_MSR_RW);
+
+/* MSR_PRED_CMD is safe to pass through if the guest knows about it. */
+if ( cp->feat.ibrsb || cp->extd.ibpb )
+vmx_clear_msr_intercept(v, MSR_PRED_CMD,  VMX_MSR_RW);
+else
+vmx_set_msr_intercept(v, MSR_PRED_CMD,  VMX_MSR_RW);
 }
 
 int vmx_guest_x86_mode(struct vcpu *v)
diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index 02a7b49..697cc6e 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -132,7 +132,8 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, 
uint64_t *val)
 case MSR_SPEC_CTRL:
 if ( !cp->feat.ibrsb )
 goto gp_fault;
-*val = vp->spec_ctrl.guest;
+*val = (vp->spec_ctrl.direct_access
+? vp->spec_ctrl.host : vp->spec_ctrl.guest);
 break;
 
 case MSR_INTEL_PLATFORM_INFO: