Re: [Xen-devel] [PATCH v2 1/5] x86/hvm: Handle viridian MSRs via the new guest_{rd, wr}msr() infrastructure

2018-03-13 Thread Jan Beulich 
>>> On 13.03.18 at 16:47,  wrote:
> On 13/03/18 15:20, Jan Beulich wrote:
> On 07.03.18 at 19:58,  wrote:
>>> @@ -175,11 +177,26 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, 
> uint64_t *val)
>>> _MSR_MISC_FEATURES_CPUID_FAULTING;
>>>  break;
>>>  
>>> +case MSR_HYPERVISOR_START ... MSR_HYPERVISOR_START + NR_VIRIDIAN_MSRS 
>>> - 
> 
>>> 1:
>>> +if ( is_viridian_domain(d) )
>>> +{
>>> +ret = guest_rdmsr_viridian(v, msr, val);
>>> +goto out;
>>> +}
>>> +
>>> +/* Fallthrough. */
>>>  default:
>>>  return X86EMUL_UNHANDLEABLE;
>>>  }
>>>  
>>> -return X86EMUL_OKAY;
>>> + out:
>> I've noticed this only in the context of patch 4, but why is this label
>> and yet another unnecessary "goto" here? That "goto" could simply
>> be "break" afaics.
> 
> Ah - that is for changes which I haven't posted yet.
> 
> When we get onto MSRs which might be in the load/save lists, or may be
> stashed in the VMCB/VMCS rather than in real hardware, we need to call
> back into arch specific code when an update is completed.

But I don't think this requires a goto here, does it? If that future
code structure really can't get away without, so be it (then). But
please let's evaluate that at the time you have that code ready.

Jan


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

Re: [Xen-devel] [PATCH v2 1/5] x86/hvm: Handle viridian MSRs via the new guest_{rd, wr}msr() infrastructure

2018-03-13 Thread Andrew Cooper 
On 13/03/18 15:20, Jan Beulich wrote:
 On 07.03.18 at 19:58,  wrote:
>> @@ -175,11 +177,26 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, 
>> uint64_t *val)
>> _MSR_MISC_FEATURES_CPUID_FAULTING;
>>  break;
>>  
>> +case MSR_HYPERVISOR_START ... MSR_HYPERVISOR_START + NR_VIRIDIAN_MSRS - 
>> 1:
>> +if ( is_viridian_domain(d) )
>> +{
>> +ret = guest_rdmsr_viridian(v, msr, val);
>> +goto out;
>> +}
>> +
>> +/* Fallthrough. */
>>  default:
>>  return X86EMUL_UNHANDLEABLE;
>>  }
>>  
>> -return X86EMUL_OKAY;
>> + out:
> I've noticed this only in the context of patch 4, but why is this label
> and yet another unnecessary "goto" here? That "goto" could simply
> be "break" afaics.

Ah - that is for changes which I haven't posted yet.

When we get onto MSRs which might be in the load/save lists, or may be
stashed in the VMCB/VMCS rather than in real hardware, we need to call
back into arch specific code when an update is completed.

~Andrew

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

Re: [Xen-devel] [PATCH v2 1/5] x86/hvm: Handle viridian MSRs via the new guest_{rd, wr}msr() infrastructure

2018-03-13 Thread Jan Beulich 
>>> On 07.03.18 at 19:58,  wrote:
> @@ -554,13 +551,11 @@ static void update_reference_tsc(struct domain *d, 
> bool_t initialize)
>  put_page_and_type(page);
>  }
>  
> -int wrmsr_viridian_regs(uint32_t idx, uint64_t val)
> +int guest_wrmsr_viridian(struct vcpu *v, uint32_t idx, uint64_t val)
>  {
> -struct vcpu *v = current;
>  struct domain *d = v->domain;
>  
> -if ( !is_viridian_domain(d) )
> -return 0;
> +ASSERT(is_viridian_domain(d));
>  
>  switch ( idx )
>  {
> @@ -615,7 +610,7 @@ int wrmsr_viridian_regs(uint32_t idx, uint64_t val)
>  
>  case HV_X64_MSR_REFERENCE_TSC:
>  if ( !(viridian_feature_mask(d) & HVMPV_reference_tsc) )
> -return 0;
> +goto gp_fault;
>  
>  perfc_incr(mshv_wrmsr_tsc_msr);
>  d->arch.hvm_domain.viridian.reference_tsc.raw = val;
> @@ -655,14 +650,15 @@ int wrmsr_viridian_regs(uint32_t idx, uint64_t val)
>  }
>  
>  default:
> -if ( idx >= VIRIDIAN_MSR_MIN && idx <= VIRIDIAN_MSR_MAX )
> -gprintk(XENLOG_WARNING, "write to unimplemented MSR %#x\n",
> -idx);
> -
> -return 0;
> +gdprintk(XENLOG_WARNING,
> + "Write %016"PRIx64" to unimplemented MSR %#x\n", val, idx);
> +goto gp_fault;
>  }
>  
> -return 1;
> +return X86EMUL_OKAY;
> +
> + gp_fault:
> +return X86EMUL_EXCEPTION;
>  }

Still these ugly goto-s to just a single return statement. But well,
Paul is happy with them ...

> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -141,9 +141,11 @@ int init_vcpu_msr_policy(struct vcpu *v)
>  
>  int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
>  {
> -const struct cpuid_policy *cp = v->domain->arch.cpuid;
> -const struct msr_domain_policy *dp = v->domain->arch.msr;
> +const struct domain *d = v->domain;
> +const struct cpuid_policy *cp = d->arch.cpuid;
> +const struct msr_domain_policy *dp = d->arch.msr;
>  const struct msr_vcpu_policy *vp = v->arch.msr;
> +int ret = X86EMUL_OKAY;
>  
>  switch ( msr )
>  {
> @@ -175,11 +177,26 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, 
> uint64_t *val)
> _MSR_MISC_FEATURES_CPUID_FAULTING;
>  break;
>  
> +case MSR_HYPERVISOR_START ... MSR_HYPERVISOR_START + NR_VIRIDIAN_MSRS - 
> 1:

The "HYPERVISOR" in here starts to make sense in the next patch,
but its combination with "VIRIDIAN" is still suspicious. I'll comment
on this further for the next patch.

> --- a/xen/include/asm-x86/msr-index.h
> +++ b/xen/include/asm-x86/msr-index.h
> @@ -540,4 +540,8 @@
>  #define MSR_PKGC9_IRTL   0x0634
>  #define MSR_PKGC10_IRTL  0x0635
>  
> +/* Hypervisor leaves in the 0x4xxx range. */
> +#define MSR_HYPERVISOR_START0x4000
> +#define NR_VIRIDIAN_MSRS0x0200

Is "leaves" really an appropriate term for MSRs?

Jan


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

[Xen-devel] [PATCH v2 1/5] x86/hvm: Handle viridian MSRs via the new guest_{rd, wr}msr() infrastructure

2018-03-07 Thread Andrew Cooper 
Dispatch from the guest_{rd,wr}msr() functions, after confirming that the
domain is configured to use viridian.  This allows for simplifiction of the
viridian helpers as they don't need to cope with the "not a viridian MSR"
case.  It also means that viridian MSRs which are unimplemented, or excluded
because of features, don't fall back into default handling path.

Rename {rd,wr}msr_viridian_regs() to guest_{rd,wr}msr_viridian() for
consistency, and because the _regs suffix isn't very appropriate.

Update them to take a vcpu pointer rather than presuming that they act on
current, which is safe for all implemented operations.  Also update them to
use X86EMUL_* return values.

Signed-off-by: Andrew Cooper 
Reviewed-by: Boris Ostrovsky 
Reviewed-by: Kevin Tian 
Reviewed-by: Paul Durrant 
Reviewed-by: Roger Pau Monné 
---
CC: Jan Beulich 
CC: Wei Liu 
CC: Sergey Dyasli 

v2:
 * Introduce some constants for the hypervisor range.  However, I do not think
   the current code is an improvement over bare numbers.
---
 xen/arch/x86/hvm/svm/svm.c |  6 +
 xen/arch/x86/hvm/viridian.c| 52 +-
 xen/arch/x86/hvm/vmx/vmx.c |  6 +
 xen/arch/x86/msr.c | 41 +++---
 xen/include/asm-x86/hvm/viridian.h | 11 ++--
 xen/include/asm-x86/msr-index.h|  4 +++
 6 files changed, 68 insertions(+), 52 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index c34f5b5..e8ac2d8 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1966,8 +1966,7 @@ static int svm_msr_read_intercept(unsigned int msr, 
uint64_t *msr_content)
 else if ( ret )
 break;
 
-if ( rdmsr_viridian_regs(msr, msr_content) ||
- rdmsr_hypervisor_regs(msr, msr_content) )
+if ( rdmsr_hypervisor_regs(msr, msr_content) )
 break;
 
 if ( rdmsr_safe(msr, *msr_content) == 0 )
@@ -2122,9 +2121,6 @@ static int svm_msr_write_intercept(unsigned int msr, 
uint64_t msr_content)
 else if ( ret )
 break;
 
-if ( wrmsr_viridian_regs(msr, msr_content) )
-break;
-
 switch ( wrmsr_hypervisor_regs(msr, msr_content) )
 {
 case -ERESTART:
diff --git a/xen/arch/x86/hvm/viridian.c b/xen/arch/x86/hvm/viridian.c
index 70aab52..074d920 100644
--- a/xen/arch/x86/hvm/viridian.c
+++ b/xen/arch/x86/hvm/viridian.c
@@ -88,9 +88,6 @@
 #define HV_X64_MSR_CRASH_P4  0x4104
 #define HV_X64_MSR_CRASH_CTL 0x4105
 
-#define VIRIDIAN_MSR_MIN HV_X64_MSR_GUEST_OS_ID
-#define VIRIDIAN_MSR_MAX HV_X64_MSR_CRASH_CTL
-
 /* Viridian Hypercall Status Codes. */
 #define HV_STATUS_SUCCESS   0x
 #define HV_STATUS_INVALID_HYPERCALL_CODE0x0002
@@ -554,13 +551,11 @@ static void update_reference_tsc(struct domain *d, bool_t 
initialize)
 put_page_and_type(page);
 }
 
-int wrmsr_viridian_regs(uint32_t idx, uint64_t val)
+int guest_wrmsr_viridian(struct vcpu *v, uint32_t idx, uint64_t val)
 {
-struct vcpu *v = current;
 struct domain *d = v->domain;
 
-if ( !is_viridian_domain(d) )
-return 0;
+ASSERT(is_viridian_domain(d));
 
 switch ( idx )
 {
@@ -615,7 +610,7 @@ int wrmsr_viridian_regs(uint32_t idx, uint64_t val)
 
 case HV_X64_MSR_REFERENCE_TSC:
 if ( !(viridian_feature_mask(d) & HVMPV_reference_tsc) )
-return 0;
+goto gp_fault;
 
 perfc_incr(mshv_wrmsr_tsc_msr);
 d->arch.hvm_domain.viridian.reference_tsc.raw = val;
@@ -655,14 +650,15 @@ int wrmsr_viridian_regs(uint32_t idx, uint64_t val)
 }
 
 default:
-if ( idx >= VIRIDIAN_MSR_MIN && idx <= VIRIDIAN_MSR_MAX )
-gprintk(XENLOG_WARNING, "write to unimplemented MSR %#x\n",
-idx);
-
-return 0;
+gdprintk(XENLOG_WARNING,
+ "Write %016"PRIx64" to unimplemented MSR %#x\n", val, idx);
+goto gp_fault;
 }
 
-return 1;
+return X86EMUL_OKAY;
+
+ gp_fault:
+return X86EMUL_EXCEPTION;
 }
 
 static int64_t raw_trc_val(struct domain *d)
@@ -698,13 +694,11 @@ void viridian_time_ref_count_thaw(struct domain *d)
 trc->off = (int64_t)trc->val - raw_trc_val(d);
 }
 
-int rdmsr_viridian_regs(uint32_t idx, uint64_t *val)
+int guest_rdmsr_viridian(const struct vcpu *v, uint32_t idx, uint64_t *val)
 {
-struct vcpu *v = current;
 struct domain *d = v->domain;
-
-if ( !is_viridian_domain(d) )
-return 0;
+
+ASSERT(is_viridian_domain(d));
 
 switch ( idx )
 {
@@ -725,7 +719,7 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t *val)
 
 case HV_X64_MSR_TSC_FREQUENCY:
 if ( viridian_feature_mask(d) &