Re: [Xen-devel] [PATCH 02/11] x86/vmx: Don't clobber %dr6 while debugging state is lazy
> From: Andrew Cooper [mailto:andrew.coop...@citrix.com] > Sent: Tuesday, July 17, 2018 5:28 PM > > On 04/06/18 14:59, Andrew Cooper wrote: > > c/s 4f36452b63 introduced a write to %dr6 in the #DB intercept case, but > the > > guests debug registers may be lazy at this point, at which point the guests > > later attempt to read %dr6 will discard this value and use the older stale > > value. > > > > Signed-off-by: Andrew Cooper > > --- > > CC: Jan Beulich > > CC: Wei Liu > > CC: Roger Pau Monné > > CC: Jun Nakajima > > CC: Kevin Tian > > Ping Acked-by: Kevin Tian ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 02/11] x86/vmx: Don't clobber %dr6 while debugging state is lazy
On 04/06/18 14:59, Andrew Cooper wrote:
> c/s 4f36452b63 introduced a write to %dr6 in the #DB intercept case, but the
> guests debug registers may be lazy at this point, at which point the guests
> later attempt to read %dr6 will discard this value and use the older stale
> value.
>
> Signed-off-by: Andrew Cooper
> ---
> CC: Jan Beulich
> CC: Wei Liu
> CC: Roger Pau Monné
> CC: Jun Nakajima
> CC: Kevin Tian
Ping
> ---
> xen/arch/x86/hvm/vmx/vmx.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 33d39f6..8dbe838 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -3696,6 +3696,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
> */
> __vmread(EXIT_QUALIFICATION, _qualification);
> HVMTRACE_1D(TRAP_DEBUG, exit_qualification);
> +__restore_debug_registers(v);
> write_debugreg(6, exit_qualification | DR_STATUS_RESERVED_ONE);
> if ( !v->domain->debugger_attached )
> {
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 02/11] x86/vmx: Don't clobber %dr6 while debugging state is lazy
>>> On 08.06.18 at 17:58, wrote:
> On 07/06/18 12:05, Jan Beulich wrote:
> On 06.06.18 at 16:16, wrote:
>>> On 06/06/18 14:50, Jan Beulich wrote:
>>> On 04.06.18 at 15:59, wrote:
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -3696,6 +3696,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
> */
> __vmread(EXIT_QUALIFICATION, _qualification);
> HVMTRACE_1D(TRAP_DEBUG, exit_qualification);
> +__restore_debug_registers(v);
> write_debugreg(6, exit_qualification |
> DR_STATUS_RESERVED_ONE);
The change is certainly correct as is, but I'd still like to put out for
discussion the alternative option:
if ( v->arch.hvm_vcpu.flag_dr_dirty )
write_debugreg(6, exit_qualification | DR_STATUS_RESERVED_ONE);
else
v->arch.debugreg[6] = exit_qualification | DR_STATUS_RESERVED_ONE;
After all the guest may know it's single stepping, and may not care to
read DR6 at all.
>>> All of this code changes across the series (so this specific suggestion
>>> is incorrect), but to the recommendation in general...
>> While I've not made it through the second half of the series yet,
>> another consideration: To avoid the double DR6 write, yet still avoid
>> an immediate further exit to restore debug registers, why not
>>
>> if ( v->arch.hvm_vcpu.flag_dr_dirty )
>> write_debugreg(6, exit_qualification | DR_STATUS_RESERVED_ONE);
>> else
>> {
>> v->arch.debugreg[6] = exit_qualification | DR_STATUS_RESERVED_ONE;
>> __restore_debug_registers(v);
>> }
>
> Do you really think the added complexity is worth shaving a few cycles
> off an almost unused cold path? The double %dr6 write (which isn't
> serialising) happens at most once per vcpu context switch, and only ever
> when the guest is debugging.
Yeah, you're right - keep it as is.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 02/11] x86/vmx: Don't clobber %dr6 while debugging state is lazy
On 07/06/18 12:05, Jan Beulich wrote:
On 06.06.18 at 16:16, wrote:
>> On 06/06/18 14:50, Jan Beulich wrote:
>> On 04.06.18 at 15:59, wrote:
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3696,6 +3696,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
*/
__vmread(EXIT_QUALIFICATION, _qualification);
HVMTRACE_1D(TRAP_DEBUG, exit_qualification);
+__restore_debug_registers(v);
write_debugreg(6, exit_qualification |
DR_STATUS_RESERVED_ONE);
>>> The change is certainly correct as is, but I'd still like to put out for
>>> discussion the alternative option:
>>>
>>> if ( v->arch.hvm_vcpu.flag_dr_dirty )
>>> write_debugreg(6, exit_qualification | DR_STATUS_RESERVED_ONE);
>>> else
>>> v->arch.debugreg[6] = exit_qualification | DR_STATUS_RESERVED_ONE;
>>>
>>> After all the guest may know it's single stepping, and may not care to
>>> read DR6 at all.
>> All of this code changes across the series (so this specific suggestion
>> is incorrect), but to the recommendation in general...
> While I've not made it through the second half of the series yet,
> another consideration: To avoid the double DR6 write, yet still avoid
> an immediate further exit to restore debug registers, why not
>
> if ( v->arch.hvm_vcpu.flag_dr_dirty )
> write_debugreg(6, exit_qualification | DR_STATUS_RESERVED_ONE);
> else
> {
> v->arch.debugreg[6] = exit_qualification | DR_STATUS_RESERVED_ONE;
> __restore_debug_registers(v);
> }
Do you really think the added complexity is worth shaving a few cycles
off an almost unused cold path? The double %dr6 write (which isn't
serialising) happens at most once per vcpu context switch, and only ever
when the guest is debugging.
Also remember that this patch wants backporting to all the stable trees,
because the code has been broken for about 10 years.
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 02/11] x86/vmx: Don't clobber %dr6 while debugging state is lazy
>>> On 06.06.18 at 16:16, wrote:
> On 06/06/18 14:50, Jan Beulich wrote:
> On 04.06.18 at 15:59, wrote:
>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>> @@ -3696,6 +3696,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>>> */
>>> __vmread(EXIT_QUALIFICATION, _qualification);
>>> HVMTRACE_1D(TRAP_DEBUG, exit_qualification);
>>> +__restore_debug_registers(v);
>>> write_debugreg(6, exit_qualification | DR_STATUS_RESERVED_ONE);
>> The change is certainly correct as is, but I'd still like to put out for
>> discussion the alternative option:
>>
>> if ( v->arch.hvm_vcpu.flag_dr_dirty )
>> write_debugreg(6, exit_qualification | DR_STATUS_RESERVED_ONE);
>> else
>> v->arch.debugreg[6] = exit_qualification | DR_STATUS_RESERVED_ONE;
>>
>> After all the guest may know it's single stepping, and may not care to
>> read DR6 at all.
>
> All of this code changes across the series (so this specific suggestion
> is incorrect), but to the recommendation in general...
While I've not made it through the second half of the series yet,
another consideration: To avoid the double DR6 write, yet still avoid
an immediate further exit to restore debug registers, why not
if ( v->arch.hvm_vcpu.flag_dr_dirty )
write_debugreg(6, exit_qualification | DR_STATUS_RESERVED_ONE);
else
{
v->arch.debugreg[6] = exit_qualification | DR_STATUS_RESERVED_ONE;
__restore_debug_registers(v);
}
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 02/11] x86/vmx: Don't clobber %dr6 while debugging state is lazy
On 06/06/18 14:50, Jan Beulich wrote:
On 04.06.18 at 15:59, wrote:
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -3696,6 +3696,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>> */
>> __vmread(EXIT_QUALIFICATION, _qualification);
>> HVMTRACE_1D(TRAP_DEBUG, exit_qualification);
>> +__restore_debug_registers(v);
>> write_debugreg(6, exit_qualification | DR_STATUS_RESERVED_ONE);
> The change is certainly correct as is, but I'd still like to put out for
> discussion the alternative option:
>
> if ( v->arch.hvm_vcpu.flag_dr_dirty )
> write_debugreg(6, exit_qualification | DR_STATUS_RESERVED_ONE);
> else
> v->arch.debugreg[6] = exit_qualification | DR_STATUS_RESERVED_ONE;
>
> After all the guest may know it's single stepping, and may not care to
> read DR6 at all.
All of this code changes across the series (so this specific suggestion
is incorrect), but to the recommendation in general...
I considered this, particularly when looking at the AMD side where %dr6
lives in the VMCB, and is unrelated to the dirty state of the other
The first thing a #DB handler will do is read %dr6 and reset it (as
recommended by both manuals), so forcing the %dr state to dirty (and
therefore clearing %dr interception) is a direct benefit to the guests
subsequent performance.
TBH, I'm still not terribly happy with these accessors, but I was
planning to defer reworking how the lazy state handing works. We've got
ad-hoc lazy logic for each type of state, and its about to get very very
complicated with guest_{rd,wr}msr().
I'm planning to build a more generic piece of state-syncing logic where
every piece of emulation / access (particularly remote access code) code
starts with vcpu_sync_state(state_$X) which takes care of pulling that
specific piece of state out of hardware (if necessary) and into struct
vcpu, and vcpu_state_dirtied(state_$X) which identifies that state in
struct vcpu has been modified.
The idea is to make all the emulation and access code agnostic to how a
certain piece of state is stored, whether that be in VMCS/VMCB, in
hardware registers for current context, switched on entry/exit, or fully
emulated.
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 02/11] x86/vmx: Don't clobber %dr6 while debugging state is lazy
>>> On 04.06.18 at 15:59, wrote: > --- a/xen/arch/x86/hvm/vmx/vmx.c > +++ b/xen/arch/x86/hvm/vmx/vmx.c > @@ -3696,6 +3696,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs) > */ > __vmread(EXIT_QUALIFICATION, _qualification); > HVMTRACE_1D(TRAP_DEBUG, exit_qualification); > +__restore_debug_registers(v); > write_debugreg(6, exit_qualification | DR_STATUS_RESERVED_ONE); The change is certainly correct as is, but I'd still like to put out for discussion the alternative option: if ( v->arch.hvm_vcpu.flag_dr_dirty ) write_debugreg(6, exit_qualification | DR_STATUS_RESERVED_ONE); else v->arch.debugreg[6] = exit_qualification | DR_STATUS_RESERVED_ONE; After all the guest may know it's single stepping, and may not care to read DR6 at all. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 02/11] x86/vmx: Don't clobber %dr6 while debugging state is lazy
On Mon, Jun 04, 2018 at 02:59:06PM +0100, Andrew Cooper wrote: > c/s 4f36452b63 introduced a write to %dr6 in the #DB intercept case, but the > guests debug registers may be lazy at this point, at which point the guests > later attempt to read %dr6 will discard this value and use the older stale > value. > > Signed-off-by: Andrew Cooper Reviewed-by: Roger Pau Monné > --- > CC: Jan Beulich > CC: Wei Liu > CC: Roger Pau Monné > CC: Jun Nakajima > CC: Kevin Tian > --- > xen/arch/x86/hvm/vmx/vmx.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c > index 33d39f6..8dbe838 100644 > --- a/xen/arch/x86/hvm/vmx/vmx.c > +++ b/xen/arch/x86/hvm/vmx/vmx.c > @@ -3696,6 +3696,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs) > */ > __vmread(EXIT_QUALIFICATION, _qualification); > HVMTRACE_1D(TRAP_DEBUG, exit_qualification); > +__restore_debug_registers(v); If I understood this correctly, you only call __restore_debug_registers in order to make sure the DR registers are marked as dirty? Thanks, Roger. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
