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

[Jan Beulich]

>>> On 01.03.18 at 13:29,  wrote:
> On 01/03/18 09:58, Jan Beulich wrote:
>>
> @@ -173,11 +175,26 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, 
>  
> uint64_t *val)
> _MSR_MISC_FEATURES_CPUID_FAULTING;
>  break;
>  
> +case 0x4000 ... 0x41ff:
 As was already suggested, these want to gain #define-s.

 Reviewed-by: Jan Beulich 
 with at least the latter taken care of.
>>> Just like on the CPUID side, the range of valid MSRs depend on the
>>> fallthrough pattern, and which hypervisor(s) we are emulating for.
>>>
>>> This is clearer by the end of the subsequent patch, but the logic is far
>>> easier to follow without these numbers being hidden.
>> I disagree (it's simply impossible to make the connection between
>> the read side and the right side this way, because the numbers
>> could also just happen to be the same, nor is it possible to
>> reasonably find all uses of those numbers via e.g. grep), but well,
>> I don't want to block the patch over this.
> 
> I'm not looking to try and railroad this through.
> 
> If you disagree, then what naming would you suggest instead?  I'd be
> happy to use any naming which doesn't impede the clarity of the logic,
> but I have spent a long time trying to find something suitable, and
> failed to do so.  Raw numbers really are clearer to follow than any
> naming scheme I tried.  The root of the problem is with the fact that
> the MSR spaces overlap, but this information disappears when you try to
> put named constants in.  Also notice that the number of CPUID leaves and
> the number of MSR leaves have a different stride, which adds more
> complexity.

The Viridian MSR range is fixed aiui, so any simple naming scheme
could be used in this patch (as e.g. suggested by Roger, to be lifted
from viridian.c). For the conflicting hypervisor range, how about

#define XEN_MSR_BASE(stride) (0x4000 + 0x200 * (stride))

? We'll anyway want to declare some upper bound on this range,
which would then be used to describe the upper end in case
labels.

Jan


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

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

[Andrew Cooper]

On 01/03/18 09:58, Jan Beulich wrote:
>
 @@ -173,11 +175,26 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr,  
 uint64_t *val)
 _MSR_MISC_FEATURES_CPUID_FAULTING;
  break;
  
 +case 0x4000 ... 0x41ff:
>>> As was already suggested, these want to gain #define-s.
>>>
>>> Reviewed-by: Jan Beulich 
>>> with at least the latter taken care of.
>> Just like on the CPUID side, the range of valid MSRs depend on the
>> fallthrough pattern, and which hypervisor(s) we are emulating for.
>>
>> This is clearer by the end of the subsequent patch, but the logic is far
>> easier to follow without these numbers being hidden.
> I disagree (it's simply impossible to make the connection between
> the read side and the right side this way, because the numbers
> could also just happen to be the same, nor is it possible to
> reasonably find all uses of those numbers via e.g. grep), but well,
> I don't want to block the patch over this.

I'm not looking to try and railroad this through.

If you disagree, then what naming would you suggest instead?  I'd be
happy to use any naming which doesn't impede the clarity of the logic,
but I have spent a long time trying to find something suitable, and
failed to do so.  Raw numbers really are clearer to follow than any
naming scheme I tried.  The root of the problem is with the fact that
the MSR spaces overlap, but this information disappears when you try to
put named constants in.  Also notice that the number of CPUID leaves and
the number of MSR leaves have a different stride, which adds more
complexity.

~Andrew

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

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

[Paul Durrant]

> -Original Message-
> From: Andrew Cooper
> Sent: 28 February 2018 18:22
> To: Paul Durrant ; Xen-devel  de...@lists.xen.org>
> Cc: Jan Beulich ; Jun Nakajima
> ; Kevin Tian ; Boris
> Ostrovsky ; Suravee Suthikulpanit
> ; Wei Liu ; Roger
> Pau Monne ; Sergey Dyasli
> 
> Subject: Re: [PATCH 2/6] x86/hvm: Handle viridian MSRs via the new
> guest_{rd,wr}msr() infrastructure
> 
> On 27/02/18 14:38, Paul Durrant wrote:
> >> @@ -698,13 +697,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 +722,7 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t *val)
> >>
> >>  case HV_X64_MSR_TSC_FREQUENCY:
> >>  if ( viridian_feature_mask(d) & HVMPV_no_freq )
> >> -return 0;
> >> +goto gp_fault;
> >>
> >>  perfc_incr(mshv_rdmsr_tsc_frequency);
> >>  *val = (uint64_t)d->arch.tsc_khz * 1000ull;
> >> @@ -733,7 +730,7 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t *val)
> >>
> >>  case HV_X64_MSR_APIC_FREQUENCY:
> >>  if ( viridian_feature_mask(d) & HVMPV_no_freq )
> >> -return 0;
> >> +goto gp_fault;
> >>
> >>  perfc_incr(mshv_rdmsr_apic_frequency);
> >>  *val = 10ull / APIC_BUS_CYCLE_NS;
> >> @@ -757,7 +754,7 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t *val)
> >>
> >>  case HV_X64_MSR_REFERENCE_TSC:
> >>  if ( !(viridian_feature_mask(d) & HVMPV_reference_tsc) )
> >> -return 0;
> > I have a recollection that for at least one version of Windows, when debug
> mode is enabled, it reads the reference TSC MSR regardless of whether the
> feature is enabled or not so this change may well cause guest boot failures.
> > In general I would be wary of #GP faulting where the current code does
> not. I think the current code is almost certainly too liberal even in the 
> face of
> buggy versions of Windows but the new code might be too conservative. It
> will need some testing.
> >
> > In principle though...
> >
> > Reviewed-by: Paul Durrant 
> 
> The current code is absolutely wrong, because it falls back into the
> default path and continues looking for a result.  On the read side, that
> ends up leaking in L$N-1 hypervisor leaves if they are present, while
> the write side ends up discarding the result.
> 
> ISTR it was only one single pre-release build of windows which failed to
> check for the TSC feature, so I'm not sure we need to worry.
> 

No, ISTR that turning debug mode on in recent Windows kernels makes them assume 
they have the timer MSR. (The bad breakage in pre-release Windows 8 was that it 
assumed it had a synIC just because it saw the basic viridian CPUID leaves).

> If we do find that it is a problem in practice, then the correct course
> of action is to explicitly fill with 0 and return X86EMUL_OKAY, which at
> least means that we've dealt with the request.
> 
> I've booted Win7 and Win10 with the code in this state.  Are you happy
> for us to go with this provisionally, and revert back to an explicit
> discard if we encounter problems?
> 

Yes, that's fine. If 7 and 10 are happy (in debug mode) then I'm happy. As you 
say, we'll just deal with any fallout on a case-by-case basis.

Cheers,

  Paul

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

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

[Jan Beulich]

>>> On 28.02.18 at 19:32,  wrote:
> On 28/02/18 16:40, Jan Beulich wrote:
> On 26.02.18 at 18:35,  wrote:
>>> --- a/xen/arch/x86/hvm/viridian.c
>>> +++ b/xen/arch/x86/hvm/viridian.c
>>> @@ -554,13 +554,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)
>> unsigned int would do instead of uint32_t (same on the read side).
> 
> MSRs are architecturally uint32_t, and your proposed coding style
> suggestions would have uint32_t here.  At the moment, uint32_t is used
> consistently throughout the new MSR infrastructure.

I don't really agree: Passing around MSR numbers can be done
using any type at least 32 bits wide. Hence - with our general
assumptions in mind - unsigned int would be suitable (see e.g.
cpuid_count()), as would be uint_least32_t or uint_fast32_t.
IOW as long as we prefer built in types over stdint.h-like ones,
there should really be only extremely few uses of uintNN_t
outside of the public headers and other interface definitions
where "exact width" is what we want.

But as indicated - I'm not going to insist here, as it's clear there
can be quite different views.

>>> @@ -173,11 +175,26 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr,  
>>> uint64_t *val)
>>> _MSR_MISC_FEATURES_CPUID_FAULTING;
>>>  break;
>>>  
>>> +case 0x4000 ... 0x41ff:
>> As was already suggested, these want to gain #define-s.
>>
>> Reviewed-by: Jan Beulich 
>> with at least the latter taken care of.
> 
> Just like on the CPUID side, the range of valid MSRs depend on the
> fallthrough pattern, and which hypervisor(s) we are emulating for.
> 
> This is clearer by the end of the subsequent patch, but the logic is far
> easier to follow without these numbers being hidden.

I disagree (it's simply impossible to make the connection between
the read side and the right side this way, because the numbers
could also just happen to be the same, nor is it possible to
reasonably find all uses of those numbers via e.g. grep), but well,
I don't want to block the patch over this.

Jan


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

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

[Andrew Cooper]

On 28/02/18 16:40, Jan Beulich wrote:
 On 26.02.18 at 18:35,  wrote:
>> --- a/xen/arch/x86/hvm/viridian.c
>> +++ b/xen/arch/x86/hvm/viridian.c
>> @@ -554,13 +554,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)
> unsigned int would do instead of uint32_t (same on the read side).

MSRs are architecturally uint32_t, and your proposed coding style
suggestions would have uint32_t here.  At the moment, uint32_t is used
consistently throughout the new MSR infrastructure.

>
>> @@ -173,11 +175,26 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr,  
>> uint64_t *val)
>> _MSR_MISC_FEATURES_CPUID_FAULTING;
>>  break;
>>  
>> +case 0x4000 ... 0x41ff:
> As was already suggested, these want to gain #define-s.
>
> Reviewed-by: Jan Beulich 
> with at least the latter taken care of.

Just like on the CPUID side, the range of valid MSRs depend on the
fallthrough pattern, and which hypervisor(s) we are emulating for.

This is clearer by the end of the subsequent patch, but the logic is far
easier to follow without these numbers being hidden.

~Andrew

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

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

[Andrew Cooper]

On 27/02/18 14:38, Paul Durrant wrote:
>> @@ -698,13 +697,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 +722,7 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t *val)
>>
>>  case HV_X64_MSR_TSC_FREQUENCY:
>>  if ( viridian_feature_mask(d) & HVMPV_no_freq )
>> -return 0;
>> +goto gp_fault;
>>
>>  perfc_incr(mshv_rdmsr_tsc_frequency);
>>  *val = (uint64_t)d->arch.tsc_khz * 1000ull;
>> @@ -733,7 +730,7 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t *val)
>>
>>  case HV_X64_MSR_APIC_FREQUENCY:
>>  if ( viridian_feature_mask(d) & HVMPV_no_freq )
>> -return 0;
>> +goto gp_fault;
>>
>>  perfc_incr(mshv_rdmsr_apic_frequency);
>>  *val = 10ull / APIC_BUS_CYCLE_NS;
>> @@ -757,7 +754,7 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t *val)
>>
>>  case HV_X64_MSR_REFERENCE_TSC:
>>  if ( !(viridian_feature_mask(d) & HVMPV_reference_tsc) )
>> -return 0;
> I have a recollection that for at least one version of Windows, when debug 
> mode is enabled, it reads the reference TSC MSR regardless of whether the 
> feature is enabled or not so this change may well cause guest boot failures.
> In general I would be wary of #GP faulting where the current code does not. I 
> think the current code is almost certainly too liberal even in the face of 
> buggy versions of Windows but the new code might be too conservative. It will 
> need some testing.
>
> In principle though...
>
> Reviewed-by: Paul Durrant 

The current code is absolutely wrong, because it falls back into the
default path and continues looking for a result.  On the read side, that
ends up leaking in L$N-1 hypervisor leaves if they are present, while
the write side ends up discarding the result.

ISTR it was only one single pre-release build of windows which failed to
check for the TSC feature, so I'm not sure we need to worry.

If we do find that it is a problem in practice, then the correct course
of action is to explicitly fill with 0 and return X86EMUL_OKAY, which at
least means that we've dealt with the request.

I've booted Win7 and Win10 with the code in this state.  Are you happy
for us to go with this provisionally, and revert back to an explicit
discard if we encounter problems?

~Andrew

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

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

[Andrew Cooper]

On 27/02/18 14:13, Roger Pau Monné wrote:
> On Mon, Feb 26, 2018 at 05:35:15PM +, Andrew Cooper wrote:
>> @@ -173,11 +175,26 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, 
>> uint64_t *val)
>> _MSR_MISC_FEATURES_CPUID_FAULTING;
>>  break;
>>  
>> +case 0x4000 ... 0x41ff:
> I think it would be clearer to use VIRIDIAN_MSR_MIN ...
> VIRIDIAN_MSR_MAX.
>
> Or else the defines should be removed because you are removing all of
> it's users.

Both of these would be wrong.  I'll remove them instead.

The problem here, and moreso with the following patch, is that the range
of acceptable MSRs depends on which hypervisor we are emulating.  Using
named labels here actually confuses rather than helps matters.

~Andrew

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

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

[Jan Beulich]

>>> On 26.02.18 at 18:35,  wrote:
> --- a/xen/arch/x86/hvm/viridian.c
> +++ b/xen/arch/x86/hvm/viridian.c
> @@ -554,13 +554,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)

unsigned int would do instead of uint32_t (same on the read side).

> @@ -173,11 +175,26 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr,  
> uint64_t *val)
> _MSR_MISC_FEATURES_CPUID_FAULTING;
>  break;
>  
> +case 0x4000 ... 0x41ff:

As was already suggested, these want to gain #define-s.

Reviewed-by: Jan Beulich 
with at least the latter taken care of.

Jan


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

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

[Paul Durrant]

> -Original Message-
> From: Andrew Cooper [mailto:andrew.coop...@citrix.com]
> Sent: 26 February 2018 17:35
> To: Xen-devel 
> Cc: Andrew Cooper ; Jan Beulich
> ; Jun Nakajima ; Paul
> Durrant ; Kevin Tian ; Boris
> Ostrovsky ; Suravee Suthikulpanit
> ; Wei Liu ; Roger
> Pau Monne ; Sergey Dyasli
> 
> Subject: [PATCH 2/6] x86/hvm: Handle viridian MSRs via the new
> guest_{rd,wr}msr() infrastructure
> 
> 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 
> ---
> CC: Jan Beulich 
> CC: Jun Nakajima 
> CC: Paul Durrant 
> CC: Kevin Tian 
> CC: Boris Ostrovsky 
> CC: Suravee Suthikulpanit 
> CC: Wei Liu 
> CC: Roger Pau Monné 
> CC: Sergey Dyasli 
> ---
>  xen/arch/x86/hvm/svm/svm.c |  6 +
>  xen/arch/x86/hvm/viridian.c| 49 ++---
> -
>  xen/arch/x86/hvm/vmx/vmx.c |  6 +
>  xen/arch/x86/msr.c | 41 +++
>  xen/include/asm-x86/hvm/viridian.h | 11 ++---
>  5 files changed, 64 insertions(+), 49 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 8b4cefd..6d8ed5c 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1967,8 +1967,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 )
> @@ -2123,9 +2122,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..23de433 100644
> --- a/xen/arch/x86/hvm/viridian.c
> +++ b/xen/arch/x86/hvm/viridian.c
> @@ -554,13 +554,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 +613,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 +653,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 +697,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 

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

[Roger Pau Monné]

On Mon, Feb 26, 2018 at 05:35:15PM +, Andrew Cooper wrote:
> 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: Roger Pau Monné 

Just two nits.

> ---
> CC: Jan Beulich 
> CC: Jun Nakajima 
> CC: Paul Durrant 
> CC: Kevin Tian 
> CC: Boris Ostrovsky 
> CC: Suravee Suthikulpanit 
> CC: Wei Liu 
> CC: Roger Pau Monné 
> CC: Sergey Dyasli 
> ---
>  xen/arch/x86/hvm/svm/svm.c |  6 +
>  xen/arch/x86/hvm/viridian.c| 49 
> ++
>  xen/arch/x86/hvm/vmx/vmx.c |  6 +
>  xen/arch/x86/msr.c | 41 +++
>  xen/include/asm-x86/hvm/viridian.h | 11 ++---
>  5 files changed, 64 insertions(+), 49 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 8b4cefd..6d8ed5c 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1967,8 +1967,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 )
> @@ -2123,9 +2122,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..23de433 100644
> --- a/xen/arch/x86/hvm/viridian.c
> +++ b/xen/arch/x86/hvm/viridian.c
> @@ -554,13 +554,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)

Since this now returns X86EMUL_* which doesn't have negative value,
should this be unsigned int?

> diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
> index 7aaa2b0..2ff9361 100644
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -139,9 +139,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 )
>  {
> @@ -173,11 +175,26 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, 
> uint64_t *val)
> _MSR_MISC_FEATURES_CPUID_FAULTING;
>  break;
>  
> +case 0x4000 ... 0x41ff:

I think it would be clearer to use VIRIDIAN_MSR_MIN ...
VIRIDIAN_MSR_MAX.

Or else the defines should be removed because you are removing all of
it's users.

Thanks, Roger.

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

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

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.coop...@citrix.com]
> Sent: Tuesday, February 27, 2018 1:35 AM
> 
> 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: Kevin Tian 
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Boris Ostrovsky]

On 02/26/2018 12:35 PM, Andrew Cooper wrote:
> 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 
> ---
> CC: Jan Beulich 
> CC: Jun Nakajima 
> CC: Paul Durrant 
> CC: Kevin Tian 
> CC: Boris Ostrovsky 
> CC: Suravee Suthikulpanit 
> CC: Wei Liu 
> CC: Roger Pau Monné 
> CC: Sergey Dyasli 

Reviewed-by: Boris Ostrovsky 



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

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

[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 
---
CC: Jan Beulich 
CC: Jun Nakajima 
CC: Paul Durrant 
CC: Kevin Tian 
CC: Boris Ostrovsky 
CC: Suravee Suthikulpanit 
CC: Wei Liu 
CC: Roger Pau Monné 
CC: Sergey Dyasli 
---
 xen/arch/x86/hvm/svm/svm.c |  6 +
 xen/arch/x86/hvm/viridian.c| 49 ++
 xen/arch/x86/hvm/vmx/vmx.c |  6 +
 xen/arch/x86/msr.c | 41 +++
 xen/include/asm-x86/hvm/viridian.h | 11 ++---
 5 files changed, 64 insertions(+), 49 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 8b4cefd..6d8ed5c 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1967,8 +1967,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 )
@@ -2123,9 +2122,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..23de433 100644
--- a/xen/arch/x86/hvm/viridian.c
+++ b/xen/arch/x86/hvm/viridian.c
@@ -554,13 +554,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 +613,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 +653,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 +697,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 +722,7 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t *val)
 
 case HV_X64_MSR_TSC_FREQUENCY:
 if ( viridian_feature_mask(d) & HVMPV_no_freq )
-return 0;
+goto gp_fault;
 
 perfc_incr(mshv_rdmsr_tsc_frequency);
 *val = (uint64_t)d->arch.tsc_khz * 1000ull;
@@ -733,7 +730,7 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t *val)
 
 case HV_X64_MSR_APIC_FREQUENCY:
 if ( viridian_feature_mask(d) & HVMPV_no_freq )
-return 0;
+goto gp_fault;
 
 perfc_incr(mshv_rdmsr_apic_frequency);
 *val = 10ull / APIC_BUS_CYCLE_NS;
@@ -757,7 +754,7 @@ int