Re: [Xen-devel] [PATCH 5/9] x86/vmx: Fix handing of MSR_DEBUGCTL on VMExit

2018-05-28 Thread Andrew Cooper 
On 27/05/18 04:56, Tian, Kevin wrote:
>> From: Andrew Cooper [mailto:andrew.coop...@citrix.com]
>> Sent: Tuesday, May 22, 2018 7:21 PM
>>
>> Currently, whenever the guest writes a nonzero value to MSR_DEBUGCTL,
>> Xen
>> updates a host MSR load list entry with the current hardware value of
>> MSR_DEBUGCTL.  This is wrong.
>>
>> On VMExit, hardware automatically resets MSR_DEBUGCTL to 0.  The only
>> case
>> where different behaviour is needed is if Xen is debugging itself, and this
>> needs setting up unconditionally for the lifetime of the VM.
>>
>> The `ler` command line boolean is the only way to configure any use of
>> MSR_DEBUGCTL for Xen, so tie the host load list entry to this setting in
>> construct_vmcs().  Any runtime update of Xen's MSR_DEBUGCTL setting
>> requires
>> more complicated synchronisation across all the running VMs.
>>
>> In the exceedingly common case, this avoids the unnecessary overhead of
>> having
>> a host load entry performing the same zeroing operation that hardware
>> has
>> already performed as part of the VMExit.
> I didn't get "unnecessary overhead" part. if "ler' is disabled, as you
> said earlier it's a bug to save/restore thus overhead doesn't matter.

The current behaviour (bug or otherwise), means that when ler is
disabled, we end up with a host load list entry zeroing MSR_DEBUGCTL as
soon as the guest first writes to the MSR.

This causes unnecessary overhead because host load/save lists are slow.

Irrespective, I've reworked this patch differently, to fully disentangle
it from the EFER series.  See patch 1 of my "x86/vmx: Misc fixes and
improvements" series.

~Andrew

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

Re: [Xen-devel] [PATCH 5/9] x86/vmx: Fix handing of MSR_DEBUGCTL on VMExit

2018-05-26 Thread Tian, Kevin 
> From: Andrew Cooper [mailto:andrew.coop...@citrix.com]
> Sent: Tuesday, May 22, 2018 7:21 PM
> 
> Currently, whenever the guest writes a nonzero value to MSR_DEBUGCTL,
> Xen
> updates a host MSR load list entry with the current hardware value of
> MSR_DEBUGCTL.  This is wrong.
> 
> On VMExit, hardware automatically resets MSR_DEBUGCTL to 0.  The only
> case
> where different behaviour is needed is if Xen is debugging itself, and this
> needs setting up unconditionally for the lifetime of the VM.
> 
> The `ler` command line boolean is the only way to configure any use of
> MSR_DEBUGCTL for Xen, so tie the host load list entry to this setting in
> construct_vmcs().  Any runtime update of Xen's MSR_DEBUGCTL setting
> requires
> more complicated synchronisation across all the running VMs.
> 
> In the exceedingly common case, this avoids the unnecessary overhead of
> having
> a host load entry performing the same zeroing operation that hardware
> has
> already performed as part of the VMExit.

I didn't get "unnecessary overhead" part. if "ler' is disabled, as you
said earlier it's a bug to save/restore thus overhead doesn't matter.
If "ler" is enabled, then save/restore is anyway required then where
is saved overhead coming from?

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

Re: [Xen-devel] [PATCH 5/9] x86/vmx: Fix handing of MSR_DEBUGCTL on VMExit

2018-05-24 Thread Andrew Cooper 
On 24/05/18 16:08, Jan Beulich wrote:
 On 22.05.18 at 13:20,  wrote:
>> Currently, whenever the guest writes a nonzero value to MSR_DEBUGCTL, Xen
>> updates a host MSR load list entry with the current hardware value of
>> MSR_DEBUGCTL.  This is wrong.
>>
>> On VMExit, hardware automatically resets MSR_DEBUGCTL to 0.
> I'm pretty certain that back when I write this, the SDM didn't spell this out.

I wondered as much.  Gen-1 VT-x is well documented as forcing the
VM_EXIT_SAVE_DEBUG_CTLS control to be set in non-root operation, which
clearly suggests that it has always had this behaviour.

>
>>  The only case
>> where different behaviour is needed is if Xen is debugging itself, and this
>> needs setting up unconditionally for the lifetime of the VM.
>>
>> The `ler` command line boolean is the only way to configure any use of
>> MSR_DEBUGCTL for Xen, so tie the host load list entry to this setting in
>> construct_vmcs().  Any runtime update of Xen's MSR_DEBUGCTL setting requires
>> more complicated synchronisation across all the running VMs.
>>
>> In the exceedingly common case, this avoids the unnecessary overhead of 
>> having
>> a host load entry performing the same zeroing operation that hardware has
>> already performed as part of the VMExit.
>>
>> Signed-off-by: Andrew Cooper 
> Reviewed-by: Jan Beulich 
>
>> Notes for backporting: This change probably does want backporting, but 
>> depends
>> on the previous patch "Support remote access to the MSR lists", and adds an
>> extra rdmsr to the vcpu construction path (resolved in a later patch).
> I wonder if for backporting purposes we couldn't stick this function
> invocation somewhere else, like vmx_ctxt_switch_to() or
> vmx_do_resume(). I realize the potential allocation failure is a problem here,
> but for that we could either allocate the memory at the place you now
> invoke vmx_add_host_load_msr(), or take the brute force approach and
> crash the domain upon failure (the net effect won't be much different to
> allocation failing during domain destruction - the domain won't start in 
> either
> case).
>
> I mention this because it seems to me that pulling in the previous patch
> would in turn require pulling in earlier ones.

Yeah - to backport this change, you need 6 patches from the series.

That said, I think I've come up with a much safer, faster, alternative
which I can disentangle completely from this series.

I was already planning to clean up the host debugctl handling to have a
single read_mostly value.  With a trivial alternative block in the vmx
vmexit handler, we can do away with the host load entry entirely (and,
as I've been reliably informed, doing things this way is faster than
having microcode walk the host/guest load/save lists).

~Andrew

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

Re: [Xen-devel] [PATCH 5/9] x86/vmx: Fix handing of MSR_DEBUGCTL on VMExit

2018-05-24 Thread Jan Beulich 
>>> On 22.05.18 at 13:20,  wrote:
> Currently, whenever the guest writes a nonzero value to MSR_DEBUGCTL, Xen
> updates a host MSR load list entry with the current hardware value of
> MSR_DEBUGCTL.  This is wrong.
> 
> On VMExit, hardware automatically resets MSR_DEBUGCTL to 0.

I'm pretty certain that back when I write this, the SDM didn't spell this out.

>  The only case
> where different behaviour is needed is if Xen is debugging itself, and this
> needs setting up unconditionally for the lifetime of the VM.
> 
> The `ler` command line boolean is the only way to configure any use of
> MSR_DEBUGCTL for Xen, so tie the host load list entry to this setting in
> construct_vmcs().  Any runtime update of Xen's MSR_DEBUGCTL setting requires
> more complicated synchronisation across all the running VMs.
> 
> In the exceedingly common case, this avoids the unnecessary overhead of having
> a host load entry performing the same zeroing operation that hardware has
> already performed as part of the VMExit.
> 
> Signed-off-by: Andrew Cooper 

Reviewed-by: Jan Beulich 

> Notes for backporting: This change probably does want backporting, but depends
> on the previous patch "Support remote access to the MSR lists", and adds an
> extra rdmsr to the vcpu construction path (resolved in a later patch).

I wonder if for backporting purposes we couldn't stick this function
invocation somewhere else, like vmx_ctxt_switch_to() or
vmx_do_resume(). I realize the potential allocation failure is a problem here,
but for that we could either allocate the memory at the place you now
invoke vmx_add_host_load_msr(), or take the brute force approach and
crash the domain upon failure (the net effect won't be much different to
allocation failing during domain destruction - the domain won't start in either
case).

I mention this because it seems to me that pulling in the previous patch
would in turn require pulling in earlier ones.

Jan



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

Re: [Xen-devel] [PATCH 5/9] x86/vmx: Fix handing of MSR_DEBUGCTL on VMExit

2018-05-24 Thread Jan Beulich 
>>> On 24.05.18 at 14:39,  wrote:
> On 24/05/18 13:14, Roger Pau Monné wrote:
>> On Tue, May 22, 2018 at 12:20:42PM +0100, Andrew Cooper wrote:
>>> Currently, whenever the guest writes a nonzero value to MSR_DEBUGCTL, Xen
>>> updates a host MSR load list entry with the current hardware value of
>>> MSR_DEBUGCTL.  This is wrong.
>>>
>>> On VMExit, hardware automatically resets MSR_DEBUGCTL to 0.  The only case
>>> where different behaviour is needed is if Xen is debugging itself, and this
>>> needs setting up unconditionally for the lifetime of the VM.
>>>
>>> The `ler` command line boolean is the only way to configure any use of
>>> MSR_DEBUGCTL for Xen,
>> Hm, there's no documentation at all for the 'ler' option.
> 
> :(  ISTR Jan introducing it for debugging purposes, but I've never used
> it myself.

Indeed I've been using this a couple of times to have an idea how execution
lead to the point where an exception was raised. The option pre-dates the
existence of xen-command-line.markdown by several releases.

Jan


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

Re: [Xen-devel] [PATCH 5/9] x86/vmx: Fix handing of MSR_DEBUGCTL on VMExit

2018-05-24 Thread Andrew Cooper 
On 24/05/18 13:14, Roger Pau Monné wrote:
> On Tue, May 22, 2018 at 12:20:42PM +0100, Andrew Cooper wrote:
>> Currently, whenever the guest writes a nonzero value to MSR_DEBUGCTL, Xen
>> updates a host MSR load list entry with the current hardware value of
>> MSR_DEBUGCTL.  This is wrong.
>>
>> On VMExit, hardware automatically resets MSR_DEBUGCTL to 0.  The only case
>> where different behaviour is needed is if Xen is debugging itself, and this
>> needs setting up unconditionally for the lifetime of the VM.
>>
>> The `ler` command line boolean is the only way to configure any use of
>> MSR_DEBUGCTL for Xen,
> Hm, there's no documentation at all for the 'ler' option.

:(  ISTR Jan introducing it for debugging purposes, but I've never used
it myself.

>
>> so tie the host load list entry to this setting in
>> construct_vmcs().  Any runtime update of Xen's MSR_DEBUGCTL setting requires
>> more complicated synchronisation across all the running VMs.
>>
>> In the exceedingly common case, this avoids the unnecessary overhead of 
>> having
>> a host load entry performing the same zeroing operation that hardware has
>> already performed as part of the VMExit.
>>
>> Signed-off-by: Andrew Cooper 
>> ---
>> CC: Jan Beulich 
>> CC: Jun Nakajima 
>> CC: Kevin Tian 
>> CC: Wei Liu 
>> CC: Roger Pau Monné 
>>
>> Notes for backporting: This change probably does want backporting, but 
>> depends
>> on the previous patch "Support remote access to the MSR lists", and adds an
>> extra rdmsr to the vcpu construction path (resolved in a later patch).
>> ---
>>  xen/arch/x86/hvm/vmx/vmcs.c | 6 ++
>>  xen/arch/x86/hvm/vmx/vmx.c  | 3 +--
>>  2 files changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
>> index 8bf54c4..2035a6d 100644
>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>> @@ -996,6 +996,7 @@ static int construct_vmcs(struct vcpu *v)
>>  struct domain *d = v->domain;
>>  u32 vmexit_ctl = vmx_vmexit_control;
>>  u32 vmentry_ctl = vmx_vmentry_control;
>> +int rc;
>>  
>>  vmx_vmcs_enter(v);
>>  
>> @@ -1266,6 +1267,11 @@ static int construct_vmcs(struct vcpu *v)
>>  if ( cpu_has_vmx_tsc_scaling )
>>  __vmwrite(TSC_MULTIPLIER, d->arch.hvm_domain.tsc_scaling_ratio);
>>  
>> +/* If using host debugging, restore Xen's setting on vmexit. */
>> +if ( this_cpu(ler_msr) &&
>> + (rc = vmx_add_host_load_msr(v, MSR_IA32_DEBUGCTLMSR))  )
>> +return rc;
> Isn't this missing a vmx_vmcs_exit on error?

It is indeed.  Will fix.

~Andrew

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

Re: [Xen-devel] [PATCH 5/9] x86/vmx: Fix handing of MSR_DEBUGCTL on VMExit

2018-05-24 Thread Roger Pau Monné 
On Tue, May 22, 2018 at 12:20:42PM +0100, Andrew Cooper wrote:
> Currently, whenever the guest writes a nonzero value to MSR_DEBUGCTL, Xen
> updates a host MSR load list entry with the current hardware value of
> MSR_DEBUGCTL.  This is wrong.
> 
> On VMExit, hardware automatically resets MSR_DEBUGCTL to 0.  The only case
> where different behaviour is needed is if Xen is debugging itself, and this
> needs setting up unconditionally for the lifetime of the VM.
> 
> The `ler` command line boolean is the only way to configure any use of
> MSR_DEBUGCTL for Xen,

Hm, there's no documentation at all for the 'ler' option.

> so tie the host load list entry to this setting in
> construct_vmcs().  Any runtime update of Xen's MSR_DEBUGCTL setting requires
> more complicated synchronisation across all the running VMs.
> 
> In the exceedingly common case, this avoids the unnecessary overhead of having
> a host load entry performing the same zeroing operation that hardware has
> already performed as part of the VMExit.
> 
> Signed-off-by: Andrew Cooper 
> ---
> CC: Jan Beulich 
> CC: Jun Nakajima 
> CC: Kevin Tian 
> CC: Wei Liu 
> CC: Roger Pau Monné 
> 
> Notes for backporting: This change probably does want backporting, but depends
> on the previous patch "Support remote access to the MSR lists", and adds an
> extra rdmsr to the vcpu construction path (resolved in a later patch).
> ---
>  xen/arch/x86/hvm/vmx/vmcs.c | 6 ++
>  xen/arch/x86/hvm/vmx/vmx.c  | 3 +--
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> index 8bf54c4..2035a6d 100644
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -996,6 +996,7 @@ static int construct_vmcs(struct vcpu *v)
>  struct domain *d = v->domain;
>  u32 vmexit_ctl = vmx_vmexit_control;
>  u32 vmentry_ctl = vmx_vmentry_control;
> +int rc;
>  
>  vmx_vmcs_enter(v);
>  
> @@ -1266,6 +1267,11 @@ static int construct_vmcs(struct vcpu *v)
>  if ( cpu_has_vmx_tsc_scaling )
>  __vmwrite(TSC_MULTIPLIER, d->arch.hvm_domain.tsc_scaling_ratio);
>  
> +/* If using host debugging, restore Xen's setting on vmexit. */
> +if ( this_cpu(ler_msr) &&
> + (rc = vmx_add_host_load_msr(v, MSR_IA32_DEBUGCTLMSR))  )
> +return rc;

Isn't this missing a vmx_vmcs_exit on error?

Thanks, Roger.

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

Re: [Xen-devel] [PATCH 5/9] x86/vmx: Fix handing of MSR_DEBUGCTL on VMExit

2018-05-22 Thread Andrew Cooper 
On 22/05/18 12:20, Andrew Cooper wrote:
> Currently, whenever the guest writes a nonzero value to MSR_DEBUGCTL, Xen
> updates a host MSR load list entry with the current hardware value of
> MSR_DEBUGCTL.  This is wrong.
>
> On VMExit, hardware automatically resets MSR_DEBUGCTL to 0.  The only case
> where different behaviour is needed is if Xen is debugging itself, and this
> needs setting up unconditionally for the lifetime of the VM.
>
> The `ler` command line boolean is the only way to configure any use of
> MSR_DEBUGCTL for Xen, so tie the host load list entry to this setting in
> construct_vmcs().  Any runtime update of Xen's MSR_DEBUGCTL setting requires
> more complicated synchronisation across all the running VMs.
>
> In the exceedingly common case, this avoids the unnecessary overhead of having
> a host load entry performing the same zeroing operation that hardware has
> already performed as part of the VMExit.
>
> Signed-off-by: Andrew Cooper 

So after doing some archaeology, the last meaningful change to DEBUGCTL
handing was c/s dfa625e1 "VMX: fix DebugCtl MSR clearing" in Xen 4.5,
but the underlying logic has been broken since its introduction in
0ae9ef55a "vmx: last branch recording MSR emulation" in 2007.

~Andrew

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

[Xen-devel] [PATCH 5/9] x86/vmx: Fix handing of MSR_DEBUGCTL on VMExit

2018-05-22 Thread Andrew Cooper 
Currently, whenever the guest writes a nonzero value to MSR_DEBUGCTL, Xen
updates a host MSR load list entry with the current hardware value of
MSR_DEBUGCTL.  This is wrong.

On VMExit, hardware automatically resets MSR_DEBUGCTL to 0.  The only case
where different behaviour is needed is if Xen is debugging itself, and this
needs setting up unconditionally for the lifetime of the VM.

The `ler` command line boolean is the only way to configure any use of
MSR_DEBUGCTL for Xen, so tie the host load list entry to this setting in
construct_vmcs().  Any runtime update of Xen's MSR_DEBUGCTL setting requires
more complicated synchronisation across all the running VMs.

In the exceedingly common case, this avoids the unnecessary overhead of having
a host load entry performing the same zeroing operation that hardware has
already performed as part of the VMExit.

Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
CC: Jun Nakajima 
CC: Kevin Tian 
CC: Wei Liu 
CC: Roger Pau Monné 

Notes for backporting: This change probably does want backporting, but depends
on the previous patch "Support remote access to the MSR lists", and adds an
extra rdmsr to the vcpu construction path (resolved in a later patch).
---
 xen/arch/x86/hvm/vmx/vmcs.c | 6 ++
 xen/arch/x86/hvm/vmx/vmx.c  | 3 +--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 8bf54c4..2035a6d 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -996,6 +996,7 @@ static int construct_vmcs(struct vcpu *v)
 struct domain *d = v->domain;
 u32 vmexit_ctl = vmx_vmexit_control;
 u32 vmentry_ctl = vmx_vmentry_control;
+int rc;
 
 vmx_vmcs_enter(v);
 
@@ -1266,6 +1267,11 @@ static int construct_vmcs(struct vcpu *v)
 if ( cpu_has_vmx_tsc_scaling )
 __vmwrite(TSC_MULTIPLIER, d->arch.hvm_domain.tsc_scaling_ratio);
 
+/* If using host debugging, restore Xen's setting on vmexit. */
+if ( this_cpu(ler_msr) &&
+ (rc = vmx_add_host_load_msr(v, MSR_IA32_DEBUGCTLMSR))  )
+return rc;
+
 vmx_vmcs_exit(v);
 
 /* will update HOST & GUEST_CR3 as reqd */
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 3950b12..f9cfb6d 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3120,8 +3120,7 @@ static int vmx_msr_write_intercept(unsigned int msr, 
uint64_t msr_content)
 }
 }
 
-if ( (rc < 0) ||
- (msr_content && (vmx_add_host_load_msr(v, msr) < 0)) )
+if ( rc < 0 )
 hvm_inject_hw_exception(TRAP_machine_check, X86_EVENT_NO_EC);
 else
 __vmwrite(GUEST_IA32_DEBUGCTL, msr_content);
-- 
2.1.4


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