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

[Jan Beulich]

>>> On 26.02.18 at 18:35,  wrote:
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -183,6 +183,10 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, 
> uint64_t *val)
>  }
>  
>  /* Fallthrough. */
> +case 0x4200 ... 0x42ff:

These again want to have #define-s added.

> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -776,29 +776,26 @@ static void do_trap(struct cpu_user_regs *regs)
>trapnr, trapstr(trapnr), regs->error_code);
>  }
>  
> -/* Returns 0 if not handled, and non-0 for success. */
> -int rdmsr_hypervisor_regs(uint32_t idx, uint64_t *val)
> +int guest_rdmsr_xen(const struct vcpu *v, uint32_t idx, uint64_t *val)
>  {
> -struct domain *d = current->domain;
> +const struct domain *d = v->domain;
>  /* Optionally shift out of the way of Viridian architectural MSRs. */
>  uint32_t base = is_viridian_domain(d) ? 0x4200 : 0x4000;
>  
>  switch ( idx - base )
>  {
>  case 0: /* Write hypercall page MSR.  Read as zero. */
> -{
>  *val = 0;
> -return 1;
> -}
> -}
> +return X86EMUL_OKAY;
>  
> -return 0;
> +default:
> +return X86EMUL_EXCEPTION;
> +}
>  }

Could I talk you into adjusting the code to have a "return" at the
end of the function, e.g. by dropping the default case? Also on
the write path then?

With at least the #define-s added,
Reviewed-by: Jan Beulich 

Jan


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

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

[Roger Pau Monné]

On Mon, Feb 26, 2018 at 05:35:16PM +, Andrew Cooper wrote:
> Dispatch from the guest_{rd,wr}msr() functions, after falling through from the
> !is_viridian_domain() case.
> 
> Rename {rd,wr}msr_hypervisor_regs() to guest_{rd,wr}msr_xen() 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, and switch to using X86EMUL_* return values.
> 
> Signed-off-by: Andrew Cooper 

Reviewed-by: Roger Pau Monné 

> diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
> index 2ff9361..9f20fd8 100644
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -183,6 +183,10 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, 
> uint64_t *val)
>  }
>  
>  /* Fallthrough. */
> +case 0x4200 ... 0x42ff:
> +ret = guest_rdmsr_xen(v, msr, val);
> +goto out;
> +
>  default:
>  return X86EMUL_UNHANDLEABLE;
>  }
> @@ -274,6 +278,10 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t 
> val)
>  }
>  
>  /* Fallthrough. */
> +case 0x4200 ... 0x42ff:

A define would be better in order to prevent this two values from
getting out of sync with the ones in rdmsr maybe.

Thanks, Roger.

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

Re: [Xen-devel] [PATCH 3/6] x86: Handle the Xen 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 falling through from
> the
> !is_viridian_domain() case.
> 
> Rename {rd,wr}msr_hypervisor_regs() to guest_{rd,wr}msr_xen() 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, and switch to using 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 3/6] x86: Handle the Xen 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 falling through from the
> !is_viridian_domain() case.
>
> Rename {rd,wr}msr_hypervisor_regs() to guest_{rd,wr}msr_xen() 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, and switch to using 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  | 25 -
>  xen/arch/x86/hvm/vmx/vmx.c  | 24 
>  xen/arch/x86/msr.c  |  8 
>  xen/arch/x86/pv/emul-priv-op.c  |  6 --
>  xen/arch/x86/traps.c| 33 -
>  xen/include/asm-x86/processor.h |  4 ++--
>  6 files changed, 34 insertions(+), 66 deletions(-)


Reviewed-by: Boris Ostrovsky 

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

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

[Andrew Cooper]

Dispatch from the guest_{rd,wr}msr() functions, after falling through from the
!is_viridian_domain() case.

Rename {rd,wr}msr_hypervisor_regs() to guest_{rd,wr}msr_xen() 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, and switch to using 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  | 25 -
 xen/arch/x86/hvm/vmx/vmx.c  | 24 
 xen/arch/x86/msr.c  |  8 
 xen/arch/x86/pv/emul-priv-op.c  |  6 --
 xen/arch/x86/traps.c| 33 -
 xen/include/asm-x86/processor.h |  4 ++--
 6 files changed, 34 insertions(+), 66 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 6d8ed5c..f90a7b4 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1967,9 +1967,6 @@ static int svm_msr_read_intercept(unsigned int msr, 
uint64_t *msr_content)
 else if ( ret )
 break;
 
-if ( rdmsr_hypervisor_regs(msr, msr_content) )
-break;
-
 if ( rdmsr_safe(msr, *msr_content) == 0 )
 break;
 
@@ -2122,25 +2119,11 @@ static int svm_msr_write_intercept(unsigned int msr, 
uint64_t msr_content)
 else if ( ret )
 break;
 
-switch ( wrmsr_hypervisor_regs(msr, msr_content) )
-{
-case -ERESTART:
-result = X86EMUL_RETRY;
-break;
-case 0:
-/*
- * Match up with the RDMSR side for now; ultimately this entire
- * case block should go away.
- */
-if ( rdmsr_safe(msr, msr_content) == 0 )
-break;
-goto gpf;
-case 1:
+/* Match up with the RDMSR side; ultimately this should go away. */
+if ( rdmsr_safe(msr, msr_content) == 0 )
 break;
-default:
-goto gpf;
-}
-break;
+
+goto gpf;
 }
 
 if ( sync )
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 5bf6f62..6caaabc 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2871,9 +2871,6 @@ static int vmx_msr_read_intercept(unsigned int msr, 
uint64_t *msr_content)
 break;
 }
 
-if ( rdmsr_hypervisor_regs(msr, msr_content) )
-break;
-
 if ( rdmsr_safe(msr, *msr_content) == 0 )
 break;
 
@@ -3115,24 +3112,11 @@ static int vmx_msr_write_intercept(unsigned int msr, 
uint64_t msr_content)
  is_last_branch_msr(msr) )
 break;
 
-switch ( wrmsr_hypervisor_regs(msr, msr_content) )
-{
-case -ERESTART:
-return X86EMUL_RETRY;
-case 0:
-/*
- * Match up with the RDMSR side for now; ultimately this
- * entire case block should go away.
- */
-if ( rdmsr_safe(msr, msr_content) == 0 )
-break;
-goto gp_fault;
-case 1:
+/* Match up with the RDMSR side; ultimately this should go away. */
+if ( rdmsr_safe(msr, msr_content) == 0 )
 break;
-default:
-goto gp_fault;
-}
-break;
+
+goto gp_fault;
 }
 
 return X86EMUL_OKAY;
diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index 2ff9361..9f20fd8 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -183,6 +183,10 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, 
uint64_t *val)
 }
 
 /* Fallthrough. */
+case 0x4200 ... 0x42ff:
+ret = guest_rdmsr_xen(v, msr, val);
+goto out;
+
 default:
 return X86EMUL_UNHANDLEABLE;
 }
@@ -274,6 +278,10 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
 }
 
 /* Fallthrough. */
+case 0x4200 ... 0x42ff:
+ret = guest_wrmsr_xen(v, msr, val);
+goto out;
+
 default:
 return X86EMUL_UNHANDLEABLE;
 }
diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
index 17aaf97..97fcac0 100644
--- a/xen/arch/x86/pv/emul-priv-op.c
+++ b/xen/arch/x86/pv/emul-priv-op.c
@@ -974,9 +974,6 @@ static int read_msr(unsigned int reg, uint64_t *val,
 }
 /* fall through */
 default:
-if ( rdmsr_hypervisor_regs(reg, val) )
-return X86EMUL_OKAY;
-
 rc = vmce_rdmsr(reg, val);
 if ( rc < 0 )