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