[PATCH 00/11] Fixes to debugging facilities

[PATCH 00/11] Fixes to debugging facilities

[Andrew Cooper]

So this started as a small fix for the vmentry failure (penultimate patch),
and has snowballed...

I'm fairly confident that everything involving DEBUGCTL.BTF is broken, and
there are definitely bugs with configuring DEBUGCTL.RTM (which really isn't
helped by the fact that the GCC TSX intrinsics render the resulting code
un-debuggable.)  I'll defer fixing these swamps for now.

The first 4 patches probably want backporting to the stable trees, so I've
taken care to move them ahead of patch 6 for backport reasons.  While all
fixes would ideally be backported, I can't find a way of fixing %dr6 merging
(as it needs to be done precicely once) without a behavioural change in the
monitor subsystem.

Patch 8 probably breaks introspection, so can't be taken at this point.  See
that patch for discussion of the problem and my best guess at a solution.

Andrew Cooper (11):
  x86/svm Fixes and cleanup to svm_inject_event()
  x86/vmx: Don't clobber %dr6 while debugging state is lazy
  x86: Initialise debug registers correctly
  x86: Fix calculation of %dr6/7 reserved bits
  x86/emul: Unfold %cr4.de handling in x86emul_read_dr()
  x86: Reorganise and rename debug register fields in struct vcpu
  x86/emul: Add pending_dbg field to x86_event
  x86/hvm: RFC - PROBABLY BROKEN - Defer all debugging/monitor actions to {svm,vmx}_inject_event()
  x86: Fix merging of new status bits into %dr6
  x86/vmx: Work around VMEntry failure when Single Stepping in an STI shadow
  x86/dbg: Cleanup of legacy dr6 constants

 tools/libxc/xc_dom_x86.c               |  12 ++
 xen/arch/x86/acpi/suspend.c            |  14 +--
 xen/arch/x86/cpu/common.c              |  12 +-
 xen/arch/x86/domain.c                  |  32 ++++--
 xen/arch/x86/domctl.c                  |  15 +--
 xen/arch/x86/hvm/emulate.c             |   3 +-
 xen/arch/x86/hvm/hvm.c                 |  33 +++---
 xen/arch/x86/hvm/svm/svm.c             | 193 +++++++++++++++++----------------
 xen/arch/x86/hvm/vmx/vmx.c             | 161 +++++++++++++++------------
 xen/arch/x86/mm/shadow/multi.c         |   5 +-
 xen/arch/x86/pv/emul-priv-op.c         |  25 ++---
 xen/arch/x86/pv/emulate.c              |   6 +-
 xen/arch/x86/pv/ro-page-fault.c        |   3 +-
 xen/arch/x86/pv/traps.c                |  17 ++-
 xen/arch/x86/traps.c                   |  52 ++++-----
 xen/arch/x86/vm_event.c                |   2 +-
 xen/arch/x86/x86_emulate.c             |  27 +++--
 xen/arch/x86/x86_emulate/x86_emulate.h |   5 +-
 xen/include/asm-x86/debugreg.h         |  89 +++++++++++----
 xen/include/asm-x86/domain.h           |  23 +++-
 xen/include/asm-x86/hvm/hvm.h          |  15 ++-
 21 files changed, 441 insertions(+), 303 deletions(-)

-- 
2.1.4


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

[PATCH 01/11] x86/svm Fixes and cleanup to svm_inject_event()

[Andrew Cooper]

 * State adjustments (and debug tracing) for #DB/#BP/#PF should not be done
   for `int $n` instructions.  Updates to %cr2 occur even if the exception
   combines to #DF.
 * Don't opencode DR_STEP when updating %dr6.
 * Simplify the logic for calling svm_emul_swint_injection() as in the common
   case, every condition needs checking.
 * Fix comments which have become stale as code has moved between components.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
CC: Brian Woods <brian.woods@amd.com>
---
 xen/arch/x86/hvm/svm/svm.c | 41 +++++++++++++++++------------------------
 1 file changed, 17 insertions(+), 24 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 673a38c..49bb722 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1428,24 +1428,18 @@ static void svm_inject_event(const struct x86_event *event)
      * Xen must emulate enough of the event injection to be sure that a
      * further fault shouldn't occur during delivery.  This covers the fact
      * that hardware doesn't perform DPL checking on injection.
-     *
-     * Also, it accounts for proper positioning of %rip for an event with trap
-     * semantics (where %rip should point after the instruction) which suffers
-     * a fault during injection (at which point %rip should point at the
-     * instruction).
      */
     if ( event->type == X86_EVENTTYPE_PRI_SW_EXCEPTION ||
-         (!cpu_has_svm_nrips && (event->type == X86_EVENTTYPE_SW_INTERRUPT ||
-                                 event->type == X86_EVENTTYPE_SW_EXCEPTION)) )
+         (!cpu_has_svm_nrips && (event->type >= X86_EVENTTYPE_SW_INTERRUPT)) )
         svm_emul_swint_injection(&_event);
 
-    switch ( _event.vector )
+    switch ( _event.vector | -(_event.type == X86_EVENTTYPE_SW_INTERRUPT) )
     {
     case TRAP_debug:
         if ( regs->eflags & X86_EFLAGS_TF )
         {
             __restore_debug_registers(vmcb, curr);
-            vmcb_set_dr6(vmcb, vmcb_get_dr6(vmcb) | 0x4000);
+            vmcb_set_dr6(vmcb, vmcb_get_dr6(vmcb) | DR_STEP);
         }
         /* fall through */
     case TRAP_int3:
@@ -1455,6 +1449,13 @@ static void svm_inject_event(const struct x86_event *event)
             domain_pause_for_debugger();
             return;
         }
+        break;
+
+    case TRAP_page_fault:
+        ASSERT(_event.type == X86_EVENTTYPE_HW_EXCEPTION);
+        curr->arch.hvm_vcpu.guest_cr[2] = _event.cr2;
+        vmcb_set_cr2(vmcb, _event.cr2);
+        break;
     }
 
     if ( unlikely(eventinj.fields.v) &&
@@ -1477,13 +1478,9 @@ static void svm_inject_event(const struct x86_event *event)
      * icebp, software events with trap semantics need emulating, so %rip in
      * the trap frame points after the instruction.
      *
-     * The x86 emulator (if requested by the x86_swint_emulate_* choice) will
-     * have performed checks such as presence/dpl/etc and believes that the
-     * event injection will succeed without faulting.
-     *
-     * The x86 emulator will always provide fault semantics for software
-     * events, with _trap.insn_len set appropriately.  If the injection
-     * requires emulation, move %rip forwards at this point.
+     * svm_emul_swint_injection() has already confirmed that events with trap
+     * semantics won't fault on injection.  Position %rip/NextRIP suitably,
+     * and restrict the event type to what hardware will tolerate.
      */
     switch ( _event.type )
     {
@@ -1540,16 +1537,12 @@ static void svm_inject_event(const struct x86_event *event)
            eventinj.fields.errorcode == (uint16_t)eventinj.fields.errorcode);
     vmcb->eventinj = eventinj;
 
-    if ( _event.vector == TRAP_page_fault )
-    {
-        curr->arch.hvm_vcpu.guest_cr[2] = _event.cr2;
-        vmcb_set_cr2(vmcb, _event.cr2);
-        HVMTRACE_LONG_2D(PF_INJECT, _event.error_code, TRC_PAR_LONG(_event.cr2));
-    }
+    if ( _event.vector == TRAP_page_fault &&
+         _event.type == X86_EVENTTYPE_HW_EXCEPTION )
+        HVMTRACE_LONG_2D(PF_INJECT, _event.error_code,
+                         TRC_PAR_LONG(_event.cr2));
     else
-    {
         HVMTRACE_2D(INJ_EXC, _event.vector, _event.error_code);
-    }
 }
 
 static int svm_event_pending(struct vcpu *v)
-- 
2.1.4


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

Re: [PATCH 01/11] x86/svm Fixes and cleanup to svm_inject_event()

[Jan Beulich]

 >>> On 04.06.18 at 15:59, <andrew.cooper3@citrix.com> wrote:
> * State adjustments (and debug tracing) for #DB/#BP/#PF should not be done
>    for `int $n` instructions.  Updates to %cr2 occur even if the exception
>    combines to #DF.
>  * Don't opencode DR_STEP when updating %dr6.
>  * Simplify the logic for calling svm_emul_swint_injection() as in the common
>    case, every condition needs checking.
>  * Fix comments which have become stale as code has moved between components.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>



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

Re: [PATCH 01/11] x86/svm Fixes and cleanup to svm_inject_event()

[Andrew Cooper]

On 06/06/18 14:37, Jan Beulich wrote:
>  >>> On 04.06.18 at 15:59, <andrew.cooper3@citrix.com> wrote:
>> * State adjustments (and debug tracing) for #DB/#BP/#PF should not be done
>>    for `int $n` instructions.  Updates to %cr2 occur even if the exception
>>    combines to #DF.
>>  * Don't opencode DR_STEP when updating %dr6.
>>  * Simplify the logic for calling svm_emul_swint_injection() as in the common
>>    case, every condition needs checking.
>>  * Fix comments which have become stale as code has moved between components.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
>

Ping SVM?

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

Re: [PATCH 01/11] x86/svm Fixes and cleanup to svm_inject_event()

[Boris Ostrovsky]

On 07/16/2018 09:33 AM, Andrew Cooper wrote:
> On 06/06/18 14:37, Jan Beulich wrote:
>>  >>> On 04.06.18 at 15:59, <andrew.cooper3@citrix.com> wrote:
>>> * State adjustments (and debug tracing) for #DB/#BP/#PF should not be done
>>>    for `int $n` instructions.  Updates to %cr2 occur even if the exception
>>>    combines to #DF.
>>>  * Don't opencode DR_STEP when updating %dr6.
>>>  * Simplify the logic for calling svm_emul_swint_injection() as in the common
>>>    case, every condition needs checking.
>>>  * Fix comments which have become stale as code has moved between components.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>
>>
> Ping SVM?
>
>


Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

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

[PATCH 02/11] x86/vmx: Don't clobber %dr6 while debugging state is lazy

[Andrew Cooper]

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 <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
---
 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, &exit_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 )
             {
-- 
2.1.4


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

Re: [PATCH 02/11] x86/vmx: Don't clobber %dr6 while debugging state is lazy

[Roger Pau Monné]

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 <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Jun Nakajima <jun.nakajima@intel.com>
> CC: Kevin Tian <kevin.tian@intel.com>
> ---
>  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, &exit_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

Re: [PATCH 02/11] x86/vmx: Don't clobber %dr6 while debugging state is lazy

[Jan Beulich]

>>> On 04.06.18 at 15:59, <andrew.cooper3@citrix.com> 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, &exit_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: [PATCH 02/11] x86/vmx: Don't clobber %dr6 while debugging state is lazy

[Andrew Cooper]

On 06/06/18 14:50, Jan Beulich wrote:
>>>> On 04.06.18 at 15:59, <andrew.cooper3@citrix.com> 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, &exit_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: [PATCH 02/11] x86/vmx: Don't clobber %dr6 while debugging state is lazy

[Jan Beulich]

>>> On 06.06.18 at 16:16, <andrew.cooper3@citrix.com> wrote:
> On 06/06/18 14:50, Jan Beulich wrote:
>>>>> On 04.06.18 at 15:59, <andrew.cooper3@citrix.com> 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, &exit_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: [PATCH 02/11] x86/vmx: Don't clobber %dr6 while debugging state is lazy

[Andrew Cooper]

On 07/06/18 12:05, Jan Beulich wrote:
>>>> On 06.06.18 at 16:16, <andrew.cooper3@citrix.com> wrote:
>> On 06/06/18 14:50, Jan Beulich wrote:
>>>>>> On 04.06.18 at 15:59, <andrew.cooper3@citrix.com> 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, &exit_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: [PATCH 02/11] x86/vmx: Don't clobber %dr6 while debugging state is lazy

[Jan Beulich]

>>> On 08.06.18 at 17:58, <andrew.cooper3@citrix.com> wrote:
> On 07/06/18 12:05, Jan Beulich wrote:
>>>>> On 06.06.18 at 16:16, <andrew.cooper3@citrix.com> wrote:
>>> On 06/06/18 14:50, Jan Beulich wrote:
>>>>>>> On 04.06.18 at 15:59, <andrew.cooper3@citrix.com> 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, &exit_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: [PATCH 02/11] x86/vmx: Don't clobber %dr6 while debugging state is lazy

[Andrew Cooper]

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 <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Jun Nakajima <jun.nakajima@intel.com>
> CC: Kevin Tian <kevin.tian@intel.com>

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, &exit_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: [PATCH 02/11] x86/vmx: Don't clobber %dr6 while debugging state is lazy

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.cooper3@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 <andrew.cooper3@citrix.com>
> > ---
> > CC: Jan Beulich <JBeulich@suse.com>
> > CC: Wei Liu <wei.liu2@citrix.com>
> > CC: Roger Pau Monné <roger.pau@citrix.com>
> > CC: Jun Nakajima <jun.nakajima@intel.com>
> > CC: Kevin Tian <kevin.tian@intel.com>
> 
> Ping

Acked-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 03/11] x86: Initialise debug registers correctly

[Andrew Cooper]

In particular, initialising %dr6 with the value 0 is buggy, because on
hardware supporting Transnational Memory, it will cause the sticky RTM bit to
be asserted, even though a debug event from a transaction hasn't actually been
observed.

Introduce X86_DR7_DEFAULT to match the existing X86_DR6_DEFAULT, and use
correct defaults when resetting the debug registers in cpu_init().

For vcpus, %dr6/7 have never been initialised.  In practice, this means that
toolstack get/set operations see zeros until the vcpu has first touched its
debug registers (at which point hardware fixes up the reserved bits), and the
RTM corner case will persist beyond that point.

Introduce initialise_registers() to set register defaults (including eflags
while we are fixing this) and call it early in vcpu_initialise().  Make a
similar adjustment in hvm_vcpu_reset_state().

Finally, adjust the vcpu state initialising logic in libxc.  All 3 sites zero
memory before choosing the nonzero defaults, which propagates the RTM corner
case.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/libxc/xc_dom_x86.c       | 12 ++++++++++++
 xen/arch/x86/cpu/common.c      | 12 ++++++++----
 xen/arch/x86/domain.c          | 10 ++++++++++
 xen/arch/x86/hvm/hvm.c         |  6 +++++-
 xen/include/asm-x86/debugreg.h |  2 ++
 5 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index e33a288..3ab918c 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -53,6 +53,9 @@
 #define X86_CR0_PE 0x01
 #define X86_CR0_ET 0x10
 
+#define X86_DR6_DEFAULT 0xffff0ff0u
+#define X86_DR7_DEFAULT 0x00000400u
+
 #define SPECIALPAGE_PAGING   0
 #define SPECIALPAGE_ACCESS   1
 #define SPECIALPAGE_SHARING  2
@@ -860,6 +863,9 @@ static int vcpu_x86_32(struct xc_dom_image *dom)
         dom->parms.virt_base + (dom->start_info_pfn) * PAGE_SIZE_X86;
     ctxt->user_regs.eflags = 1 << 9; /* Interrupt Enable */
 
+    ctxt->debugreg[6] = X86_DR6_DEFAULT;
+    ctxt->debugreg[7] = X86_DR7_DEFAULT;
+
     ctxt->flags = VGCF_in_kernel_X86_32 | VGCF_online_X86_32;
     if ( dom->parms.pae == XEN_PAE_EXTCR3 ||
          dom->parms.pae == XEN_PAE_BIMODAL )
@@ -907,6 +913,9 @@ static int vcpu_x86_64(struct xc_dom_image *dom)
         dom->parms.virt_base + (dom->start_info_pfn) * PAGE_SIZE_X86;
     ctxt->user_regs.rflags = 1 << 9; /* Interrupt Enable */
 
+    ctxt->debugreg[6] = X86_DR6_DEFAULT;
+    ctxt->debugreg[7] = X86_DR7_DEFAULT;
+
     ctxt->flags = VGCF_in_kernel_X86_64 | VGCF_online_X86_64;
     cr3_pfn = xc_dom_p2m(dom, dom->pgtables_seg.pfn);
     ctxt->ctrlreg[3] = xen_pfn_to_cr3_x86_64(cr3_pfn);
@@ -1011,6 +1020,9 @@ static int vcpu_hvm(struct xc_dom_image *dom)
     /* Set the IP. */
     bsp_ctx.cpu.rip = dom->parms.phys_entry;
 
+    bsp_ctx.cpu.dr6 = X86_DR6_DEFAULT;
+    bsp_ctx.cpu.dr7 = X86_DR7_DEFAULT;
+
     if ( dom->start_info_seg.pfn )
         bsp_ctx.cpu.rbx = dom->start_info_seg.pfn << PAGE_SHIFT;
 
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index 528aff1..0872466 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -3,6 +3,7 @@
 #include <xen/delay.h>
 #include <xen/smp.h>
 #include <asm/current.h>
+#include <asm/debugreg.h>
 #include <asm/processor.h>
 #include <asm/xstate.h>
 #include <asm/msr.h>
@@ -823,10 +824,13 @@ void cpu_init(void)
 	/* Ensure FPU gets initialised for each domain. */
 	stts();
 
-	/* Clear all 6 debug registers: */
-#define CD(register) asm volatile ( "mov %0,%%db" #register : : "r"(0UL) );
-	CD(0); CD(1); CD(2); CD(3); /* no db4 and db5 */; CD(6); CD(7);
-#undef CD
+	/* Reset debug registers: */
+	write_debugreg(0, 0);
+	write_debugreg(1, 0);
+	write_debugreg(2, 0);
+	write_debugreg(3, 0);
+	write_debugreg(6, X86_DR6_DEFAULT);
+	write_debugreg(7, X86_DR7_DEFAULT);
 }
 
 void cpu_uninit(unsigned int cpu)
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 0ca820a..7ae9789 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -322,6 +322,14 @@ void free_vcpu_struct(struct vcpu *v)
     free_xenheap_page(v);
 }
 
+static void initialise_registers(struct vcpu *v)
+{
+    v->arch.user_regs.eflags = X86_EFLAGS_MBS;
+
+    v->arch.debugreg[6] = X86_DR6_DEFAULT;
+    v->arch.debugreg[7] = X86_DR7_DEFAULT;
+}
+
 int vcpu_initialise(struct vcpu *v)
 {
     struct domain *d = v->domain;
@@ -341,6 +349,8 @@ int vcpu_initialise(struct vcpu *v)
             return rc;
 
         vmce_init_vcpu(v);
+
+        initialise_registers(v);
     }
     else if ( (rc = xstate_alloc_save_area(v)) != 0 )
         return rc;
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index c23983c..10415e6 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -40,6 +40,7 @@
 #include <asm/shadow.h>
 #include <asm/hap.h>
 #include <asm/current.h>
+#include <asm/debugreg.h>
 #include <asm/e820.h>
 #include <asm/io.h>
 #include <asm/regs.h>
@@ -3907,7 +3908,10 @@ void hvm_vcpu_reset_state(struct vcpu *v, uint16_t cs, uint16_t ip)
     v->arch.user_regs.rflags = X86_EFLAGS_MBS;
     v->arch.user_regs.rdx = 0x00000f00;
     v->arch.user_regs.rip = ip;
-    memset(&v->arch.debugreg, 0, sizeof(v->arch.debugreg));
+
+    memset(&v->arch.debugreg, 0, sizeof(v->arch.debugreg) - 16);
+    v->arch.debugreg[6] = X86_DR6_DEFAULT;
+    v->arch.debugreg[7] = X86_DR7_DEFAULT;
 
     v->arch.hvm_vcpu.guest_cr[0] = X86_CR0_ET;
     hvm_update_guest_cr(v, 0);
diff --git a/xen/include/asm-x86/debugreg.h b/xen/include/asm-x86/debugreg.h
index b3b10ea..8e6a656 100644
--- a/xen/include/asm-x86/debugreg.h
+++ b/xen/include/asm-x86/debugreg.h
@@ -70,6 +70,8 @@
 #define DR_RTM_ENABLE            (0x00000800ul) /* RTM debugging enable */
 #define DR_GENERAL_DETECT        (0x00002000ul) /* General detect enable */
 
+#define X86_DR7_DEFAULT 0x00000400ul    /* Default %dr7 value. */
+
 #define write_debugreg(reg, val) do {                       \
     unsigned long __val = val;                              \
     asm volatile ( "mov %0,%%db" #reg : : "r" (__val) );    \
-- 
2.1.4


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

Re: [PATCH 03/11] x86: Initialise debug registers correctly

[Roger Pau Monné]

On Mon, Jun 04, 2018 at 02:59:07PM +0100, Andrew Cooper wrote:
> In particular, initialising %dr6 with the value 0 is buggy, because on
> hardware supporting Transnational Memory, it will cause the sticky RTM bit to
> be asserted, even though a debug event from a transaction hasn't actually been
> observed.
> 
> Introduce X86_DR7_DEFAULT to match the existing X86_DR6_DEFAULT, and use
> correct defaults when resetting the debug registers in cpu_init().
> 
> For vcpus, %dr6/7 have never been initialised.  In practice, this means that
> toolstack get/set operations see zeros until the vcpu has first touched its
> debug registers (at which point hardware fixes up the reserved bits), and the
> RTM corner case will persist beyond that point.
> 
> Introduce initialise_registers() to set register defaults (including eflags
> while we are fixing this) and call it early in vcpu_initialise().  Make a
> similar adjustment in hvm_vcpu_reset_state().
> 
> Finally, adjust the vcpu state initialising logic in libxc.  All 3 sites zero
> memory before choosing the nonzero defaults, which propagates the RTM corner
> case.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

LGTM:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Just a couple of questions below.

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  tools/libxc/xc_dom_x86.c       | 12 ++++++++++++
>  xen/arch/x86/cpu/common.c      | 12 ++++++++----
>  xen/arch/x86/domain.c          | 10 ++++++++++
>  xen/arch/x86/hvm/hvm.c         |  6 +++++-
>  xen/include/asm-x86/debugreg.h |  2 ++
>  5 files changed, 37 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> index e33a288..3ab918c 100644
> --- a/tools/libxc/xc_dom_x86.c
> +++ b/tools/libxc/xc_dom_x86.c
> @@ -53,6 +53,9 @@
>  #define X86_CR0_PE 0x01
>  #define X86_CR0_ET 0x10
>  
> +#define X86_DR6_DEFAULT 0xffff0ff0u
> +#define X86_DR7_DEFAULT 0x00000400u
> +
>  #define SPECIALPAGE_PAGING   0
>  #define SPECIALPAGE_ACCESS   1
>  #define SPECIALPAGE_SHARING  2
> @@ -860,6 +863,9 @@ static int vcpu_x86_32(struct xc_dom_image *dom)
>          dom->parms.virt_base + (dom->start_info_pfn) * PAGE_SIZE_X86;
>      ctxt->user_regs.eflags = 1 << 9; /* Interrupt Enable */
>  
> +    ctxt->debugreg[6] = X86_DR6_DEFAULT;
> +    ctxt->debugreg[7] = X86_DR7_DEFAULT;
> +
>      ctxt->flags = VGCF_in_kernel_X86_32 | VGCF_online_X86_32;
>      if ( dom->parms.pae == XEN_PAE_EXTCR3 ||
>           dom->parms.pae == XEN_PAE_BIMODAL )
> @@ -907,6 +913,9 @@ static int vcpu_x86_64(struct xc_dom_image *dom)
>          dom->parms.virt_base + (dom->start_info_pfn) * PAGE_SIZE_X86;
>      ctxt->user_regs.rflags = 1 << 9; /* Interrupt Enable */
>  
> +    ctxt->debugreg[6] = X86_DR6_DEFAULT;
> +    ctxt->debugreg[7] = X86_DR7_DEFAULT;
> +
>      ctxt->flags = VGCF_in_kernel_X86_64 | VGCF_online_X86_64;
>      cr3_pfn = xc_dom_p2m(dom, dom->pgtables_seg.pfn);
>      ctxt->ctrlreg[3] = xen_pfn_to_cr3_x86_64(cr3_pfn);
> @@ -1011,6 +1020,9 @@ static int vcpu_hvm(struct xc_dom_image *dom)
>      /* Set the IP. */
>      bsp_ctx.cpu.rip = dom->parms.phys_entry;
>  
> +    bsp_ctx.cpu.dr6 = X86_DR6_DEFAULT;
> +    bsp_ctx.cpu.dr7 = X86_DR7_DEFAULT;
> +
>      if ( dom->start_info_seg.pfn )
>          bsp_ctx.cpu.rbx = dom->start_info_seg.pfn << PAGE_SHIFT;
>  
> diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
> index 528aff1..0872466 100644
> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -3,6 +3,7 @@
>  #include <xen/delay.h>
>  #include <xen/smp.h>
>  #include <asm/current.h>
> +#include <asm/debugreg.h>
>  #include <asm/processor.h>
>  #include <asm/xstate.h>
>  #include <asm/msr.h>
> @@ -823,10 +824,13 @@ void cpu_init(void)
>  	/* Ensure FPU gets initialised for each domain. */
>  	stts();
>  
> -	/* Clear all 6 debug registers: */
> -#define CD(register) asm volatile ( "mov %0,%%db" #register : : "r"(0UL) );
> -	CD(0); CD(1); CD(2); CD(3); /* no db4 and db5 */; CD(6); CD(7);
> -#undef CD
> +	/* Reset debug registers: */
> +	write_debugreg(0, 0);
> +	write_debugreg(1, 0);
> +	write_debugreg(2, 0);
> +	write_debugreg(3, 0);
> +	write_debugreg(6, X86_DR6_DEFAULT);
> +	write_debugreg(7, X86_DR7_DEFAULT);

AFAICT you are writing the default init values, which should be there
already. So this is just a safebelt in case the CPU is initialized
with some bogus DR values?

>  }
>  
>  void cpu_uninit(unsigned int cpu)
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 0ca820a..7ae9789 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -322,6 +322,14 @@ void free_vcpu_struct(struct vcpu *v)
>      free_xenheap_page(v);
>  }
>  
> +static void initialise_registers(struct vcpu *v)
> +{
> +    v->arch.user_regs.eflags = X86_EFLAGS_MBS;
> +
> +    v->arch.debugreg[6] = X86_DR6_DEFAULT;
> +    v->arch.debugreg[7] = X86_DR7_DEFAULT;
> +}
> +
>  int vcpu_initialise(struct vcpu *v)
>  {
>      struct domain *d = v->domain;
> @@ -341,6 +349,8 @@ int vcpu_initialise(struct vcpu *v)
>              return rc;
>  
>          vmce_init_vcpu(v);
> +
> +        initialise_registers(v);
>      }
>      else if ( (rc = xstate_alloc_save_area(v)) != 0 )
>          return rc;
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index c23983c..10415e6 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -40,6 +40,7 @@
>  #include <asm/shadow.h>
>  #include <asm/hap.h>
>  #include <asm/current.h>
> +#include <asm/debugreg.h>
>  #include <asm/e820.h>
>  #include <asm/io.h>
>  #include <asm/regs.h>
> @@ -3907,7 +3908,10 @@ void hvm_vcpu_reset_state(struct vcpu *v, uint16_t cs, uint16_t ip)
>      v->arch.user_regs.rflags = X86_EFLAGS_MBS;
>      v->arch.user_regs.rdx = 0x00000f00;
>      v->arch.user_regs.rip = ip;
> -    memset(&v->arch.debugreg, 0, sizeof(v->arch.debugreg));
> +
> +    memset(&v->arch.debugreg, 0, sizeof(v->arch.debugreg) - 16);

For clarity I would use offsetof here, or rather just zero the whole
thing. This is not a hot path, so I would prefer to avoid the
optimization.

Roger.

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

Re: [PATCH 03/11] x86: Initialise debug registers correctly

[Andrew Cooper]

On 06/06/18 11:34, Roger Pau Monné wrote:
> On Mon, Jun 04, 2018 at 02:59:07PM +0100, Andrew Cooper wrote:
>> diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
>> index 528aff1..0872466 100644
>> --- a/xen/arch/x86/cpu/common.c
>> +++ b/xen/arch/x86/cpu/common.c
>> @@ -3,6 +3,7 @@
>>  #include <xen/delay.h>
>>  #include <xen/smp.h>
>>  #include <asm/current.h>
>> +#include <asm/debugreg.h>
>>  #include <asm/processor.h>
>>  #include <asm/xstate.h>
>>  #include <asm/msr.h>
>> @@ -823,10 +824,13 @@ void cpu_init(void)
>>  	/* Ensure FPU gets initialised for each domain. */
>>  	stts();
>>  
>> -	/* Clear all 6 debug registers: */
>> -#define CD(register) asm volatile ( "mov %0,%%db" #register : : "r"(0UL) );
>> -	CD(0); CD(1); CD(2); CD(3); /* no db4 and db5 */; CD(6); CD(7);
>> -#undef CD
>> +	/* Reset debug registers: */
>> +	write_debugreg(0, 0);
>> +	write_debugreg(1, 0);
>> +	write_debugreg(2, 0);
>> +	write_debugreg(3, 0);
>> +	write_debugreg(6, X86_DR6_DEFAULT);
>> +	write_debugreg(7, X86_DR7_DEFAULT);
> AFAICT you are writing the default init values, which should be there
> already. So this is just a safebelt in case the CPU is initialized
> with some bogus DR values?

Yes.  Its just to sanitise state.

>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> index c23983c..10415e6 100644
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -40,6 +40,7 @@
>>  #include <asm/shadow.h>
>>  #include <asm/hap.h>
>>  #include <asm/current.h>
>> +#include <asm/debugreg.h>
>>  #include <asm/e820.h>
>>  #include <asm/io.h>
>>  #include <asm/regs.h>
>> @@ -3907,7 +3908,10 @@ void hvm_vcpu_reset_state(struct vcpu *v, uint16_t cs, uint16_t ip)
>>      v->arch.user_regs.rflags = X86_EFLAGS_MBS;
>>      v->arch.user_regs.rdx = 0x00000f00;
>>      v->arch.user_regs.rip = ip;
>> -    memset(&v->arch.debugreg, 0, sizeof(v->arch.debugreg));
>> +
>> +    memset(&v->arch.debugreg, 0, sizeof(v->arch.debugreg) - 16);
> For clarity I would use offsetof here, or rather just zero the whole
> thing. This is not a hot path, so I would prefer to avoid the
> optimization.

This particular expression doesn't survive past patch 6.

~Andrew

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

Re: [PATCH 03/11] x86: Initialise debug registers correctly

[Jan Beulich]

>>> On 04.06.18 at 15:59, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -322,6 +322,14 @@ void free_vcpu_struct(struct vcpu *v)
>      free_xenheap_page(v);
>  }
>  
> +static void initialise_registers(struct vcpu *v)
> +{
> +    v->arch.user_regs.eflags = X86_EFLAGS_MBS;

If you used ->rflags here, you could (and imo better would) also ...

> @@ -3907,7 +3908,10 @@ void hvm_vcpu_reset_state(struct vcpu *v, uint16_t cs, uint16_t ip)
>      v->arch.user_regs.rflags = X86_EFLAGS_MBS;
>      v->arch.user_regs.rdx = 0x00000f00;
>      v->arch.user_regs.rip = ip;
> -    memset(&v->arch.debugreg, 0, sizeof(v->arch.debugreg));
> +
> +    memset(&v->arch.debugreg, 0, sizeof(v->arch.debugreg) - 16);
> +    v->arch.debugreg[6] = X86_DR6_DEFAULT;
> +    v->arch.debugreg[7] = X86_DR7_DEFAULT;

... call that function from here (dropping the setting of .rflags
visible in context).

If you decide to go that route,
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan



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

Re: [PATCH 03/11] x86: Initialise debug registers correctly

[Andrew Cooper]

On 06/06/18 14:56, Jan Beulich wrote:
>>>> On 04.06.18 at 15:59, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -322,6 +322,14 @@ void free_vcpu_struct(struct vcpu *v)
>>      free_xenheap_page(v);
>>  }
>>  
>> +static void initialise_registers(struct vcpu *v)
>> +{
>> +    v->arch.user_regs.eflags = X86_EFLAGS_MBS;
> If you used ->rflags here, you could (and imo better would) also ...
>
>> @@ -3907,7 +3908,10 @@ void hvm_vcpu_reset_state(struct vcpu *v, uint16_t cs, uint16_t ip)
>>      v->arch.user_regs.rflags = X86_EFLAGS_MBS;
>>      v->arch.user_regs.rdx = 0x00000f00;
>>      v->arch.user_regs.rip = ip;
>> -    memset(&v->arch.debugreg, 0, sizeof(v->arch.debugreg));
>> +
>> +    memset(&v->arch.debugreg, 0, sizeof(v->arch.debugreg) - 16);
>> +    v->arch.debugreg[6] = X86_DR6_DEFAULT;
>> +    v->arch.debugreg[7] = X86_DR7_DEFAULT;
> ... call that function from here (dropping the setting of .rflags
> visible in context).
>
> If you decide to go that route,
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Unforunately, I don't agree.  Large quantities of hvm_vcpu_reset_state()
would logically live in initialise_registers(), but definitely don't
want to be in the vcpu_initialise() path at this point.

initialise_registers() is restricted to the simple registers, and in
particular cannot cope with control registers which are wildly different
between PV and HVM guests.

I can't find a reasonable split (or for that matter, name) of a shared
helper function between part of hvm_vcpu_reset_state() and the vcpu
creation path.  For the sake of two constants, I'm not sure trying to
combine the paths is useful, unless you've got a better suggestion.

~Andrew

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

Re: [PATCH 03/11] x86: Initialise debug registers correctly

[Jan Beulich]

>>> On 08.06.18 at 17:42, <andrew.cooper3@citrix.com> wrote:
> On 06/06/18 14:56, Jan Beulich wrote:
>>>>> On 04.06.18 at 15:59, <andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/arch/x86/domain.c
>>> +++ b/xen/arch/x86/domain.c
>>> @@ -322,6 +322,14 @@ void free_vcpu_struct(struct vcpu *v)
>>>      free_xenheap_page(v);
>>>  }
>>>  
>>> +static void initialise_registers(struct vcpu *v)
>>> +{
>>> +    v->arch.user_regs.eflags = X86_EFLAGS_MBS;
>> If you used ->rflags here, you could (and imo better would) also ...
>>
>>> @@ -3907,7 +3908,10 @@ void hvm_vcpu_reset_state(struct vcpu *v, uint16_t 
> cs, uint16_t ip)
>>>      v->arch.user_regs.rflags = X86_EFLAGS_MBS;
>>>      v->arch.user_regs.rdx = 0x00000f00;
>>>      v->arch.user_regs.rip = ip;
>>> -    memset(&v->arch.debugreg, 0, sizeof(v->arch.debugreg));
>>> +
>>> +    memset(&v->arch.debugreg, 0, sizeof(v->arch.debugreg) - 16);
>>> +    v->arch.debugreg[6] = X86_DR6_DEFAULT;
>>> +    v->arch.debugreg[7] = X86_DR7_DEFAULT;
>> ... call that function from here (dropping the setting of .rflags
>> visible in context).
>>
>> If you decide to go that route,
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> Unforunately, I don't agree.  Large quantities of hvm_vcpu_reset_state()
> would logically live in initialise_registers(), but definitely don't
> want to be in the vcpu_initialise() path at this point.
> 
> initialise_registers() is restricted to the simple registers, and in
> particular cannot cope with control registers which are wildly different
> between PV and HVM guests.
> 
> I can't find a reasonable split (or for that matter, name) of a shared
> helper function between part of hvm_vcpu_reset_state() and the vcpu
> creation path.  For the sake of two constants, I'm not sure trying to
> combine the paths is useful, unless you've got a better suggestion.

What's wrong with utilizing the little bit of sharing that's possible at
this point, and extend it (if suitable) later? Any bit of avoided
redundancy is helpful imo. Let me say it that way - I'm not going to
make my R-b dependent on which route you go, but I'd like to ask
that you please consider the suggested alternative another time.

Jan



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

[PATCH 04/11] x86: Fix calculation of %dr6/7 reserved bits

[Andrew Cooper]

The reserved bit calculations for %dr6 and %dr7 depend on whether the VM has
the Restricted Transnational Memory feature available.

Introduce adjust_dr{6,7}_rsvd() and replace the opencoded logic and constants
(except for DR_STATUS_RESERVED_ONE which is (mis)used elsewhere and will be
removed after future bugfixes).  The use of these helpers in set_debugreg()
covers toolstack values for PV guests, but HVM guests need similar treatment.

The use of the guests cpuid policy is less than optimal in the create/restore
paths.  However in such cases, the policy will be the guest maximum policy,
which will be more permissive with respect to the RTM feature.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/domain.c          |  5 ++++-
 xen/arch/x86/hvm/hvm.c         |  5 +++--
 xen/arch/x86/traps.c           | 17 +++++-----------
 xen/include/asm-x86/debugreg.h | 44 ++++++++++++++++++++++++++++++++++++------
 4 files changed, 50 insertions(+), 21 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 7ae9789..cee57a8 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -783,6 +783,7 @@ int arch_set_info_guest(
     struct vcpu *v, vcpu_guest_context_u c)
 {
     struct domain *d = v->domain;
+    const struct cpuid_policy *cp = d->arch.cpuid;
     unsigned long cr3_gfn;
     struct page_info *cr3_page;
     unsigned long flags, cr4;
@@ -894,8 +895,10 @@ int arch_set_info_guest(
 
     if ( is_hvm_domain(d) )
     {
-        for ( i = 0; i < ARRAY_SIZE(v->arch.debugreg); ++i )
+        for ( i = 0; i < ARRAY_SIZE(v->arch.debugreg) - 2; ++i )
             v->arch.debugreg[i] = c(debugreg[i]);
+        v->arch.debugreg[6] = adjust_dr6_rsvd(c(debugreg[6]), cp->feat.rtm);
+        v->arch.debugreg[7] = adjust_dr7_rsvd(c(debugreg[7]), cp->feat.rtm);
 
         hvm_set_info_guest(v);
         goto out;
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 10415e6..7fddae1 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -977,6 +977,7 @@ unsigned long hvm_cr4_guest_valid_bits(const struct domain *d, bool restore)
 
 static int hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
 {
+    const struct cpuid_policy *cp = d->arch.cpuid;
     int vcpuid;
     struct vcpu *v;
     struct hvm_hw_cpu ctxt;
@@ -1154,8 +1155,8 @@ static int hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
     v->arch.debugreg[1] = ctxt.dr1;
     v->arch.debugreg[2] = ctxt.dr2;
     v->arch.debugreg[3] = ctxt.dr3;
-    v->arch.debugreg[6] = ctxt.dr6;
-    v->arch.debugreg[7] = ctxt.dr7;
+    v->arch.debugreg[6] = adjust_dr6_rsvd(ctxt.dr6, cp->feat.rtm);
+    v->arch.debugreg[7] = adjust_dr7_rsvd(ctxt.dr7, cp->feat.rtm);
 
     v->arch.vgc_flags = VGCF_online;
 
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 6990c67..e9bfbc7 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -2057,6 +2057,7 @@ void activate_debugregs(const struct vcpu *curr)
 long set_debugreg(struct vcpu *v, unsigned int reg, unsigned long value)
 {
     struct vcpu *curr = current;
+    const struct cpuid_policy *cp = curr->domain->arch.cpuid;
 
     switch ( reg )
     {
@@ -2086,12 +2087,8 @@ long set_debugreg(struct vcpu *v, unsigned int reg, unsigned long value)
         if ( value != (uint32_t)value )
             return -EINVAL;
 
-        /*
-         * DR6: Bits 4-11,16-31 reserved (set to 1).
-         *      Bit 12 reserved (set to 0).
-         */
-        value &= ~DR_STATUS_RESERVED_ZERO; /* reserved bits => 0 */
-        value |=  DR_STATUS_RESERVED_ONE;  /* reserved bits => 1 */
+        value = adjust_dr6_rsvd(value, cp->feat.rtm);
+
         if ( v == curr )
             write_debugreg(6, value);
         break;
@@ -2106,12 +2103,8 @@ long set_debugreg(struct vcpu *v, unsigned int reg, unsigned long value)
         if ( value != (uint32_t)value )
             return -EINVAL;
 
-        /*
-         * DR7: Bit 10 reserved (set to 1).
-         *      Bits 11-12,14-15 reserved (set to 0).
-         */
-        value &= ~DR_CONTROL_RESERVED_ZERO; /* reserved bits => 0 */
-        value |=  DR_CONTROL_RESERVED_ONE;  /* reserved bits => 1 */
+        value = adjust_dr7_rsvd(value, cp->feat.rtm);
+
         /*
          * Privileged bits:
          *      GD (bit 13): must be 0.
diff --git a/xen/include/asm-x86/debugreg.h b/xen/include/asm-x86/debugreg.h
index 8e6a656..8df566b 100644
--- a/xen/include/asm-x86/debugreg.h
+++ b/xen/include/asm-x86/debugreg.h
@@ -10,9 +10,18 @@
 #define DR_STATUS    6
 #define DR_CONTROL   7
 
-/* Define a few things for the status register.  We can use this to determine
-   which debugging register was responsible for the trap.  The other bits
-   are either reserved or not of interest to us. */
+/*
+ * DR6 status bits.
+ *   N.B. For backwards compatibility, X86_DR6_RTM has inverted polarity.
+ */
+#define X86_DR6_B0              (1u <<  0)  /* Breakpoint 0 triggered  */
+#define X86_DR6_B1              (1u <<  1)  /* Breakpoint 1 triggered  */
+#define X86_DR6_B2              (1u <<  2)  /* Breakpoint 2 triggered  */
+#define X86_DR6_B3              (1u <<  3)  /* Breakpoint 3 triggered  */
+#define X86_DR6_BD              (1u << 13)  /* Debug register accessed */
+#define X86_DR6_BS              (1u << 14)  /* Single step             */
+#define X86_DR6_BT              (1u << 15)  /* Task switch             */
+#define X86_DR6_RTM             (1u << 16)  /* #DB/#BP in RTM region   */
 
 #define DR_TRAP0        (0x1)           /* db0 */
 #define DR_TRAP1        (0x2)           /* db1 */
@@ -21,7 +30,6 @@
 #define DR_STEP         (0x4000)        /* single-step */
 #define DR_SWITCH       (0x8000)        /* task switch */
 #define DR_NOT_RTM      (0x10000)       /* clear: #BP inside RTM region */
-#define DR_STATUS_RESERVED_ZERO (~0xffffeffful) /* Reserved, read as zero */
 #define DR_STATUS_RESERVED_ONE  0xffff0ff0ul /* Reserved, read as one */
 
 #define X86_DR6_DEFAULT 0xffff0ff0ul    /* Default %dr6 value. */
@@ -63,8 +71,6 @@
    We can slow the instruction pipeline for instructions coming via the
    gdt or the ldt if we want to.  I am not sure why this is an advantage */
 
-#define DR_CONTROL_RESERVED_ZERO (~0xffff27fful) /* Reserved, read as zero */
-#define DR_CONTROL_RESERVED_ONE  (0x00000400ul) /* Reserved, read as one */
 #define DR_LOCAL_EXACT_ENABLE    (0x00000100ul) /* Local exact enable */
 #define DR_GLOBAL_EXACT_ENABLE   (0x00000200ul) /* Global exact enable */
 #define DR_RTM_ENABLE            (0x00000800ul) /* RTM debugging enable */
@@ -84,4 +90,30 @@
 long set_debugreg(struct vcpu *, unsigned int reg, unsigned long value);
 void activate_debugregs(const struct vcpu *);
 
+static inline unsigned long adjust_dr6_rsvd(unsigned long dr6, bool rtm)
+{
+    /*
+     * DR6: Bits 4-11,17-31 reserved (set to 1).
+     *      Bit  16 reserved (set to 1) if RTM unavailable.
+     *      Bit  12 reserved (set to 0).
+     */
+    dr6 |= 0xfffe0ff0 | (rtm ? 0 : X86_DR6_RTM);
+    dr6 &= 0xffffefff;
+
+    return dr6;
+}
+
+static inline unsigned long adjust_dr7_rsvd(unsigned long dr7, bool rtm)
+{
+    /*
+     * DR7: Bit  10 reserved (set to 1).
+     *      Bit  11 reserved (set to 0) if RTM unavailable.
+     *      Bits 12,14-15 reserved (set to 0).
+     */
+    dr7 |= 0x00000400;
+    dr7 &= 0xffff23ff & (rtm ? 0 : ~DR_RTM_ENABLE);
+
+    return dr7;
+}
+
 #endif /* _X86_DEBUGREG_H */
-- 
2.1.4


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

Re: [PATCH 04/11] x86: Fix calculation of %dr6/7 reserved bits

[Jan Beulich]

>>> On 04.06.18 at 15:59, <andrew.cooper3@citrix.com> wrote:
> The reserved bit calculations for %dr6 and %dr7 depend on whether the VM has
> the Restricted Transnational Memory feature available.

Transactional, I think.

> --- a/xen/include/asm-x86/debugreg.h
> +++ b/xen/include/asm-x86/debugreg.h
> @@ -10,9 +10,18 @@
>  #define DR_STATUS    6
>  #define DR_CONTROL   7
>  
> -/* Define a few things for the status register.  We can use this to determine
> -   which debugging register was responsible for the trap.  The other bits
> -   are either reserved or not of interest to us. */
> +/*
> + * DR6 status bits.
> + *   N.B. For backwards compatibility, X86_DR6_RTM has inverted polarity.
> + */
> +#define X86_DR6_B0              (1u <<  0)  /* Breakpoint 0 triggered  */
> +#define X86_DR6_B1              (1u <<  1)  /* Breakpoint 1 triggered  */
> +#define X86_DR6_B2              (1u <<  2)  /* Breakpoint 2 triggered  */
> +#define X86_DR6_B3              (1u <<  3)  /* Breakpoint 3 triggered  */
> +#define X86_DR6_BD              (1u << 13)  /* Debug register accessed */
> +#define X86_DR6_BS              (1u << 14)  /* Single step             */
> +#define X86_DR6_BT              (1u << 15)  /* Task switch             */
> +#define X86_DR6_RTM             (1u << 16)  /* #DB/#BP in RTM region   */

Could I talk you into introducing these into x86-defs.h instead, for the
emulator to use them eventually too?

> @@ -84,4 +90,30 @@
>  long set_debugreg(struct vcpu *, unsigned int reg, unsigned long value);
>  void activate_debugregs(const struct vcpu *);
>  
> +static inline unsigned long adjust_dr6_rsvd(unsigned long dr6, bool rtm)
> +{
> +    /*
> +     * DR6: Bits 4-11,17-31 reserved (set to 1).
> +     *      Bit  16 reserved (set to 1) if RTM unavailable.
> +     *      Bit  12 reserved (set to 0).
> +     */

Please also mention bits 32-63 (also for DR7).

> +    dr6 |= 0xfffe0ff0 | (rtm ? 0 : X86_DR6_RTM);
> +    dr6 &= 0xffffefff;

I'm not overly happy with this move to literal numbers. Could we
at least meet in the middle and adjust the first line to

    dr6 |= X86_DR6_DEFAULT & ~(rtm ? 0 : X86_DR6_RTM);

? Even the second line, it doesn't look unreasonable to me to
accompany the other X86_DR6_* values you introduce with
X86_DR6_MBZ (or some such, if you dislike this name). Of course
then for the first line using X86_DR6_MBS would also be an option,
allowing you to retain the | (which documentation-wise might be
slightly better than the & ~()).

> +    return dr6;
> +}
> +
> +static inline unsigned long adjust_dr7_rsvd(unsigned long dr7, bool rtm)
> +{
> +    /*
> +     * DR7: Bit  10 reserved (set to 1).
> +     *      Bit  11 reserved (set to 0) if RTM unavailable.
> +     *      Bits 12,14-15 reserved (set to 0).
> +     */
> +    dr7 |= 0x00000400;
> +    dr7 &= 0xffff23ff & (rtm ? 0 : ~DR_RTM_ENABLE);

Along those lines here then.

Jan



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

Re: [PATCH 04/11] x86: Fix calculation of %dr6/7 reserved bits

[Andrew Cooper]

On 06/06/18 15:16, Jan Beulich wrote:
>> +    dr6 |= 0xfffe0ff0 | (rtm ? 0 : X86_DR6_RTM);
>> +    dr6 &= 0xffffefff;
> I'm not overly happy with this move to literal numbers. Could we
> at least meet in the middle and adjust the first line to
>
>     dr6 |= X86_DR6_DEFAULT & ~(rtm ? 0 : X86_DR6_RTM);
>
> ? Even the second line, it doesn't look unreasonable to me to
> accompany the other X86_DR6_* values you introduce with
> X86_DR6_MBZ (or some such, if you dislike this name). Of course
> then for the first line using X86_DR6_MBS would also be an option,
> allowing you to retain the | (which documentation-wise might be
> slightly better than the & ~()).

At the moment, every single use of DR_*_RESERVED_* in the entire
codebase is buggy, although this is admittedly a side effect of the
introduction of RTM (and that the advice given the Intel manual has
caused software not to be forwards compatible to the introduction of
RTM.  I've also arranged for that advice to be fixed.)

Irrespective, the constants are now simply appropriate to express how
the reserved bit behaviour works, which is why I'm removing them and
introducing these functions instead.

As for naked numbers, they are like this because its the clearest I
could make the code.  Hiding these behind defines makes it harder to
cross reference with the comment.

An alternative might be to go for an ARM approach using GENMASK()/BIT(),
but I'm not a fan of those because it introduces more confusion as to
where the boundaries of the mask lie.

Like you, I'm not a fan of naked numbers, but in this instance, I chose
them because I think they are the least bad option available.

~Andrew

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

Re: [PATCH 04/11] x86: Fix calculation of %dr6/7 reserved bits

[Andrew Cooper]

On 06/06/18 15:50, Andrew Cooper wrote:
> On 06/06/18 15:16, Jan Beulich wrote:
>>> +    dr6 |= 0xfffe0ff0 | (rtm ? 0 : X86_DR6_RTM);
>>> +    dr6 &= 0xffffefff;
>> I'm not overly happy with this move to literal numbers. Could we
>> at least meet in the middle and adjust the first line to
>>
>>     dr6 |= X86_DR6_DEFAULT & ~(rtm ? 0 : X86_DR6_RTM);
>>
>> ? Even the second line, it doesn't look unreasonable to me to
>> accompany the other X86_DR6_* values you introduce with
>> X86_DR6_MBZ (or some such, if you dislike this name). Of course
>> then for the first line using X86_DR6_MBS would also be an option,
>> allowing you to retain the | (which documentation-wise might be
>> slightly better than the & ~()).
> At the moment, every single use of DR_*_RESERVED_* in the entire
> codebase is buggy, although this is admittedly a side effect of the
> introduction of RTM (and that the advice given the Intel manual has
> caused software not to be forwards compatible to the introduction of
> RTM.  I've also arranged for that advice to be fixed.)
>
> Irrespective, the constants are now simply appropriate to express how

Apologies - I meant "simply not appropriate".

~Andrew

> the reserved bit behaviour works, which is why I'm removing them and
> introducing these functions instead.
>
> As for naked numbers, they are like this because its the clearest I
> could make the code.  Hiding these behind defines makes it harder to
> cross reference with the comment.
>
> An alternative might be to go for an ARM approach using GENMASK()/BIT(),
> but I'm not a fan of those because it introduces more confusion as to
> where the boundaries of the mask lie.
>
> Like you, I'm not a fan of naked numbers, but in this instance, I chose
> them because I think they are the least bad option available.
>
> ~Andrew
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel


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

Re: [PATCH 04/11] x86: Fix calculation of %dr6/7 reserved bits

[Jan Beulich]

>>> On 06.06.18 at 16:50, <andrew.cooper3@citrix.com> wrote:
> On 06/06/18 15:16, Jan Beulich wrote:
>>> +    dr6 |= 0xfffe0ff0 | (rtm ? 0 : X86_DR6_RTM);
>>> +    dr6 &= 0xffffefff;
>> I'm not overly happy with this move to literal numbers. Could we
>> at least meet in the middle and adjust the first line to
>>
>>     dr6 |= X86_DR6_DEFAULT & ~(rtm ? 0 : X86_DR6_RTM);
>>
>> ? Even the second line, it doesn't look unreasonable to me to
>> accompany the other X86_DR6_* values you introduce with
>> X86_DR6_MBZ (or some such, if you dislike this name). Of course
>> then for the first line using X86_DR6_MBS would also be an option,
>> allowing you to retain the | (which documentation-wise might be
>> slightly better than the & ~()).
> 
> At the moment, every single use of DR_*_RESERVED_* in the entire
> codebase is buggy, although this is admittedly a side effect of the
> introduction of RTM (and that the advice given the Intel manual has
> caused software not to be forwards compatible to the introduction of
> RTM.  I've also arranged for that advice to be fixed.)
> 
> Irrespective, the constants are now simply not appropriate to express how
[I've taken the liberty to insert the omitted "not" above, to help someone,
 like me, going through this later (again) to have correct context]
> the reserved bit behaviour works, which is why I'm removing them and
> introducing these functions instead.

Hence my suggestion to introduce (correct) X86_DR6_MBZ and
X86_DR6_MBS.

> As for naked numbers, they are like this because its the clearest I
> could make the code.  Hiding these behind defines makes it harder to
> cross reference with the comment.

Whether the comment goes next to their definition or next to their
(currently single) use is a secondary aspect.

> An alternative might be to go for an ARM approach using GENMASK()/BIT(),
> but I'm not a fan of those because it introduces more confusion as to
> where the boundaries of the mask lie.

I don't like this option either.

Jan



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

Re: [PATCH 04/11] x86: Fix calculation of %dr6/7 reserved bits

[Roger Pau Monné]

On Mon, Jun 04, 2018 at 02:59:08PM +0100, Andrew Cooper wrote:
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 10415e6..7fddae1 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -977,6 +977,7 @@ unsigned long hvm_cr4_guest_valid_bits(const struct domain *d, bool restore)
>  
>  static int hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
>  {
> +    const struct cpuid_policy *cp = d->arch.cpuid;
>      int vcpuid;
>      struct vcpu *v;
>      struct hvm_hw_cpu ctxt;
> @@ -1154,8 +1155,8 @@ static int hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
>      v->arch.debugreg[1] = ctxt.dr1;
>      v->arch.debugreg[2] = ctxt.dr2;
>      v->arch.debugreg[3] = ctxt.dr3;
> -    v->arch.debugreg[6] = ctxt.dr6;
> -    v->arch.debugreg[7] = ctxt.dr7;
> +    v->arch.debugreg[6] = adjust_dr6_rsvd(ctxt.dr6, cp->feat.rtm);
> +    v->arch.debugreg[7] = adjust_dr7_rsvd(ctxt.dr7, cp->feat.rtm);

I don't know much, but if the value in dr6/dr7 is changed from the one
in the provided context, won't the guest experience issues?

Won't it be better to plain refuse to load a context that has
unsupported DR6/7 values?

Thanks, Roger.

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

Re: [PATCH 04/11] x86: Fix calculation of %dr6/7 reserved bits

[Andrew Cooper]

On 06/06/18 16:49, Roger Pau Monné wrote:
> On Mon, Jun 04, 2018 at 02:59:08PM +0100, Andrew Cooper wrote:
>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> index 10415e6..7fddae1 100644
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -977,6 +977,7 @@ unsigned long hvm_cr4_guest_valid_bits(const struct domain *d, bool restore)
>>  
>>  static int hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
>>  {
>> +    const struct cpuid_policy *cp = d->arch.cpuid;
>>      int vcpuid;
>>      struct vcpu *v;
>>      struct hvm_hw_cpu ctxt;
>> @@ -1154,8 +1155,8 @@ static int hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
>>      v->arch.debugreg[1] = ctxt.dr1;
>>      v->arch.debugreg[2] = ctxt.dr2;
>>      v->arch.debugreg[3] = ctxt.dr3;
>> -    v->arch.debugreg[6] = ctxt.dr6;
>> -    v->arch.debugreg[7] = ctxt.dr7;
>> +    v->arch.debugreg[6] = adjust_dr6_rsvd(ctxt.dr6, cp->feat.rtm);
>> +    v->arch.debugreg[7] = adjust_dr7_rsvd(ctxt.dr7, cp->feat.rtm);
> I don't know much, but if the value in dr6/dr7 is changed from the one
> in the provided context, won't the guest experience issues?
>
> Won't it be better to plain refuse to load a context that has
> unsupported DR6/7 values?

adjust_dr{6,7}_rsvd() match the adjustments the processor does when
loading values.

For backwards compatibility, the processor accepts any bit pattern for
the bottom 32 bits, then ignores the bits it doesn't care about.  When
reading the register back, the unimplemented bits appear as their
defaults, which aren't all 0.

In the case of hvm_load_cpu_ctxt(), we'd ideally strictly limit it to
architectural expectation, but there will be a good chunk of VMs out
there which haven't ever touched %dr6/%dr7 in their lifetime, and will
therefore have 0's in the migration record at this point.

~Andrew

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

Re: [PATCH 04/11] x86: Fix calculation of %dr6/7 reserved bits

[Roger Pau Monné]

On Wed, Jun 06, 2018 at 04:59:24PM +0100, Andrew Cooper wrote:
> On 06/06/18 16:49, Roger Pau Monné wrote:
> > On Mon, Jun 04, 2018 at 02:59:08PM +0100, Andrew Cooper wrote:
> >> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> >> index 10415e6..7fddae1 100644
> >> --- a/xen/arch/x86/hvm/hvm.c
> >> +++ b/xen/arch/x86/hvm/hvm.c
> >> @@ -977,6 +977,7 @@ unsigned long hvm_cr4_guest_valid_bits(const struct domain *d, bool restore)
> >>  
> >>  static int hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
> >>  {
> >> +    const struct cpuid_policy *cp = d->arch.cpuid;
> >>      int vcpuid;
> >>      struct vcpu *v;
> >>      struct hvm_hw_cpu ctxt;
> >> @@ -1154,8 +1155,8 @@ static int hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
> >>      v->arch.debugreg[1] = ctxt.dr1;
> >>      v->arch.debugreg[2] = ctxt.dr2;
> >>      v->arch.debugreg[3] = ctxt.dr3;
> >> -    v->arch.debugreg[6] = ctxt.dr6;
> >> -    v->arch.debugreg[7] = ctxt.dr7;
> >> +    v->arch.debugreg[6] = adjust_dr6_rsvd(ctxt.dr6, cp->feat.rtm);
> >> +    v->arch.debugreg[7] = adjust_dr7_rsvd(ctxt.dr7, cp->feat.rtm);
> > I don't know much, but if the value in dr6/dr7 is changed from the one
> > in the provided context, won't the guest experience issues?
> >
> > Won't it be better to plain refuse to load a context that has
> > unsupported DR6/7 values?
> 
> adjust_dr{6,7}_rsvd() match the adjustments the processor does when
> loading values.
> 
> For backwards compatibility, the processor accepts any bit pattern for
> the bottom 32 bits, then ignores the bits it doesn't care about.  When
> reading the register back, the unimplemented bits appear as their
> defaults, which aren't all 0.
> 
> In the case of hvm_load_cpu_ctxt(), we'd ideally strictly limit it to
> architectural expectation, but there will be a good chunk of VMs out
> there which haven't ever touched %dr6/%dr7 in their lifetime, and will
> therefore have 0's in the migration record at this point.

Right, I guess that's the best approach.

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

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

[PATCH 05/11] x86/emul: Unfold %cr4.de handling in x86emul_read_dr()

[Andrew Cooper]

No functional change (as curr->arch.debugreg[5] is zero when DE is clear), but
this change simplifies the following patch.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/x86_emulate.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/x86_emulate.c b/xen/arch/x86/x86_emulate.c
index 30f89ad..03b364a 100644
--- a/xen/arch/x86/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate.c
@@ -101,23 +101,29 @@ int x86emul_read_dr(unsigned int reg, unsigned long *val,
     switch ( reg )
     {
     case 0 ... 3:
-    case 6:
         *val = curr->arch.debugreg[reg];
         break;
 
+    case 4:
+        if ( curr->arch.pv_vcpu.ctrlreg[4] & X86_CR4_DE )
+            goto ud_fault;
+
+        /* Fallthrough */
+    case 6:
+        *val = curr->arch.debugreg[6];
+        break;
+
+    case 5:
+        if ( curr->arch.pv_vcpu.ctrlreg[4] & X86_CR4_DE )
+            goto ud_fault;
+
+        /* Fallthrough */
     case 7:
         *val = (curr->arch.debugreg[7] |
                 curr->arch.debugreg[5]);
         break;
 
-    case 4 ... 5:
-        if ( !(curr->arch.pv_vcpu.ctrlreg[4] & X86_CR4_DE) )
-        {
-            *val = curr->arch.debugreg[reg + 2];
-            break;
-        }
-
-        /* Fallthrough */
+    ud_fault:
     default:
         if ( ctxt )
             x86_emul_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC, ctxt);
-- 
2.1.4


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

Re: [PATCH 05/11] x86/emul: Unfold %cr4.de handling in x86emul_read_dr()

[Jan Beulich]

>>> On 04.06.18 at 15:59, <andrew.cooper3@citrix.com> wrote:
> No functional change (as curr->arch.debugreg[5] is zero when DE is clear), but
> this change simplifies the following patch.

A comment to this effect would be helpful ...

> --- a/xen/arch/x86/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate.c
> @@ -101,23 +101,29 @@ int x86emul_read_dr(unsigned int reg, unsigned long *val,
>      switch ( reg )
>      {
>      case 0 ... 3:
> -    case 6:
>          *val = curr->arch.debugreg[reg];
>          break;
>  
> +    case 4:
> +        if ( curr->arch.pv_vcpu.ctrlreg[4] & X86_CR4_DE )
> +            goto ud_fault;
> +
> +        /* Fallthrough */
> +    case 6:
> +        *val = curr->arch.debugreg[6];
> +        break;
> +
> +    case 5:
> +        if ( curr->arch.pv_vcpu.ctrlreg[4] & X86_CR4_DE )
> +            goto ud_fault;
> +
> +        /* Fallthrough */
>      case 7:
>          *val = (curr->arch.debugreg[7] |
>                  curr->arch.debugreg[5]);

... somewhere above here. With that
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan



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

Re: [PATCH 05/11] x86/emul: Unfold %cr4.de handling in x86emul_read_dr()

[Andrew Cooper]

On 06/06/18 15:20, Jan Beulich wrote:
>>>> On 04.06.18 at 15:59, <andrew.cooper3@citrix.com> wrote:
>> No functional change (as curr->arch.debugreg[5] is zero when DE is clear), but
>> this change simplifies the following patch.
> A comment to this effect would be helpful ...
>
>> --- a/xen/arch/x86/x86_emulate.c
>> +++ b/xen/arch/x86/x86_emulate.c
>> @@ -101,23 +101,29 @@ int x86emul_read_dr(unsigned int reg, unsigned long *val,
>>      switch ( reg )
>>      {
>>      case 0 ... 3:
>> -    case 6:
>>          *val = curr->arch.debugreg[reg];
>>          break;
>>  
>> +    case 4:
>> +        if ( curr->arch.pv_vcpu.ctrlreg[4] & X86_CR4_DE )
>> +            goto ud_fault;
>> +
>> +        /* Fallthrough */
>> +    case 6:
>> +        *val = curr->arch.debugreg[6];
>> +        break;
>> +
>> +    case 5:
>> +        if ( curr->arch.pv_vcpu.ctrlreg[4] & X86_CR4_DE )
>> +            goto ud_fault;
>> +
>> +        /* Fallthrough */
>>      case 7:
>>          *val = (curr->arch.debugreg[7] |
>>                  curr->arch.debugreg[5]);
> ... somewhere above here. With that
> Acked-by: Jan Beulich <jbeulich@suse.com>

Even in light of the rename turning this into

*val = curr->arch.dr7 | curr->arch.pv_vcpu.dr7_emul;

?

The only reason I made the comment was to justify the "no functional
change" part of the commit message, but PV guests' questionable use of
debugreg[5] in the first place has no bearing on the correctness of this
code.

~Andrew

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

Re: [PATCH 05/11] x86/emul: Unfold %cr4.de handling in x86emul_read_dr()

[Jan Beulich]

>>> On 08.06.18 at 18:03, <andrew.cooper3@citrix.com> wrote:
> On 06/06/18 15:20, Jan Beulich wrote:
>>>>> On 04.06.18 at 15:59, <andrew.cooper3@citrix.com> wrote:
>>> No functional change (as curr->arch.debugreg[5] is zero when DE is clear), 
> but
>>> this change simplifies the following patch.
>> A comment to this effect would be helpful ...
>>
>>> --- a/xen/arch/x86/x86_emulate.c
>>> +++ b/xen/arch/x86/x86_emulate.c
>>> @@ -101,23 +101,29 @@ int x86emul_read_dr(unsigned int reg, unsigned long 
> *val,
>>>      switch ( reg )
>>>      {
>>>      case 0 ... 3:
>>> -    case 6:
>>>          *val = curr->arch.debugreg[reg];
>>>          break;
>>>  
>>> +    case 4:
>>> +        if ( curr->arch.pv_vcpu.ctrlreg[4] & X86_CR4_DE )
>>> +            goto ud_fault;
>>> +
>>> +        /* Fallthrough */
>>> +    case 6:
>>> +        *val = curr->arch.debugreg[6];
>>> +        break;
>>> +
>>> +    case 5:
>>> +        if ( curr->arch.pv_vcpu.ctrlreg[4] & X86_CR4_DE )
>>> +            goto ud_fault;
>>> +
>>> +        /* Fallthrough */
>>>      case 7:
>>>          *val = (curr->arch.debugreg[7] |
>>>                  curr->arch.debugreg[5]);
>> ... somewhere above here. With that
>> Acked-by: Jan Beulich <jbeulich@suse.com>
> 
> Even in light of the rename turning this into
> 
> *val = curr->arch.dr7 | curr->arch.pv_vcpu.dr7_emul;
> 
> ?

Indeed, the later adjustment makes a comment pretty pointless.

Jan



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

Re: [PATCH 05/11] x86/emul: Unfold %cr4.de handling in x86emul_read_dr()

[Roger Pau Monné]

On Mon, Jun 04, 2018 at 02:59:09PM +0100, Andrew Cooper wrote:
> No functional change (as curr->arch.debugreg[5] is zero when DE is clear), but
> this change simplifies the following patch.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

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

[PATCH 06/11] x86: Reorganise and rename debug register fields in struct vcpu

[Andrew Cooper]

Reusing debugreg[5] for the PV emulated IO breakpoint information is confusing
to read.  Instead, introduce a dr7_emul field in pv_vcpu for the pupose.

With the PV emulation out of the way, debugreg[4,5] are entirely unused and
don't need to be stored.

Rename debugreg[0..3] to dr[0..3] to reduce code volume, but keep them as an
array because their behaviour is identical and this helps simplfy some of the
PV handling.  Introduce dr6 and dr7 fields to replace debugreg[6,7] which
removes the storage for debugreg[4,5].

Two minor alterations on the PV side is that merging of the emulated state
happens along with the other dr handling, rather than much later, and
arch_set_info_guest() now checks the return value from set_debugreg() fails
the hypercall rather than silently discarding the values.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
CC: Brian Woods <brian.woods@amd.com>
---
 xen/arch/x86/acpi/suspend.c    | 14 +++++++-------
 xen/arch/x86/domain.c          | 27 ++++++++++++++++-----------
 xen/arch/x86/domctl.c          | 15 +++++----------
 xen/arch/x86/hvm/hvm.c         | 30 +++++++++++++++---------------
 xen/arch/x86/hvm/svm/svm.c     | 27 ++++++++++++++-------------
 xen/arch/x86/hvm/vmx/vmx.c     | 26 +++++++++++++-------------
 xen/arch/x86/pv/emul-priv-op.c | 14 +++++++-------
 xen/arch/x86/pv/emulate.c      |  2 +-
 xen/arch/x86/traps.c           | 30 +++++++++++++++---------------
 xen/arch/x86/vm_event.c        |  2 +-
 xen/arch/x86/x86_emulate.c     |  7 +++----
 xen/include/asm-x86/domain.h   | 11 ++++++++++-
 12 files changed, 107 insertions(+), 98 deletions(-)

diff --git a/xen/arch/x86/acpi/suspend.c b/xen/arch/x86/acpi/suspend.c
index 044bd81..1a129b4 100644
--- a/xen/arch/x86/acpi/suspend.c
+++ b/xen/arch/x86/acpi/suspend.c
@@ -82,14 +82,14 @@ void restore_rest_processor_state(void)
 
     /* Maybe load the debug registers. */
     BUG_ON(!is_pv_vcpu(curr));
-    if ( !is_idle_vcpu(curr) && curr->arch.debugreg[7] )
+    if ( !is_idle_vcpu(curr) && curr->arch.dr7 )
     {
-        write_debugreg(0, curr->arch.debugreg[0]);
-        write_debugreg(1, curr->arch.debugreg[1]);
-        write_debugreg(2, curr->arch.debugreg[2]);
-        write_debugreg(3, curr->arch.debugreg[3]);
-        write_debugreg(6, curr->arch.debugreg[6]);
-        write_debugreg(7, curr->arch.debugreg[7]);
+        write_debugreg(0, curr->arch.dr[0]);
+        write_debugreg(1, curr->arch.dr[1]);
+        write_debugreg(2, curr->arch.dr[2]);
+        write_debugreg(3, curr->arch.dr[3]);
+        write_debugreg(6, curr->arch.dr6);
+        write_debugreg(7, curr->arch.dr7);
     }
 
     /* Reload FPU state on next FPU use. */
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index cee57a8..ba8eb05 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -326,8 +326,8 @@ static void initialise_registers(struct vcpu *v)
 {
     v->arch.user_regs.eflags = X86_EFLAGS_MBS;
 
-    v->arch.debugreg[6] = X86_DR6_DEFAULT;
-    v->arch.debugreg[7] = X86_DR7_DEFAULT;
+    v->arch.dr6 = X86_DR6_DEFAULT;
+    v->arch.dr7 = X86_DR7_DEFAULT;
 }
 
 int vcpu_initialise(struct vcpu *v)
@@ -895,10 +895,10 @@ int arch_set_info_guest(
 
     if ( is_hvm_domain(d) )
     {
-        for ( i = 0; i < ARRAY_SIZE(v->arch.debugreg) - 2; ++i )
-            v->arch.debugreg[i] = c(debugreg[i]);
-        v->arch.debugreg[6] = adjust_dr6_rsvd(c(debugreg[6]), cp->feat.rtm);
-        v->arch.debugreg[7] = adjust_dr7_rsvd(c(debugreg[7]), cp->feat.rtm);
+        for ( i = 0; i < ARRAY_SIZE(v->arch.dr); ++i )
+            v->arch.dr[i] = c(debugreg[i]);
+        v->arch.dr6 = adjust_dr6_rsvd(c(debugreg[6]), cp->feat.rtm);
+        v->arch.dr7 = adjust_dr7_rsvd(c(debugreg[7]), cp->feat.rtm);
 
         hvm_set_info_guest(v);
         goto out;
@@ -981,9 +981,14 @@ int arch_set_info_guest(
     v->arch.pv_vcpu.ctrlreg[4] = cr4 ? pv_guest_cr4_fixup(v, cr4) :
         real_cr4_to_pv_guest_cr4(mmu_cr4_features);
 
-    memset(v->arch.debugreg, 0, sizeof(v->arch.debugreg));
-    for ( i = 0; i < 8; i++ )
-        (void)set_debugreg(v, i, c(debugreg[i]));
+    for ( i = 0; !rc && i < ARRAY_SIZE(v->arch.dr); i++ )
+        rc = set_debugreg(v, i, c(debugreg[i]));
+    if ( !rc )
+        rc = set_debugreg(v, 6, c(debugreg[6]));
+    if ( !rc )
+        rc = set_debugreg(v, 7, c(debugreg[7]));
+    if ( rc )
+        return rc;
 
     if ( v->is_initialised )
         goto out;
@@ -1525,7 +1530,7 @@ void paravirt_ctxt_switch_from(struct vcpu *v)
      * inside Xen, before we get a chance to reload DR7, and this cannot always
      * safely be handled.
      */
-    if ( unlikely(v->arch.debugreg[7] & DR7_ACTIVE_MASK) )
+    if ( unlikely(v->arch.dr7 & DR7_ACTIVE_MASK) )
         write_debugreg(7, 0);
 }
 
@@ -1538,7 +1543,7 @@ void paravirt_ctxt_switch_to(struct vcpu *v)
             l4e_from_page(v->domain->arch.perdomain_l3_pg,
                           __PAGE_HYPERVISOR_RW);
 
-    if ( unlikely(v->arch.debugreg[7] & DR7_ACTIVE_MASK) )
+    if ( unlikely(v->arch.dr7 & DR7_ACTIVE_MASK) )
         activate_debugregs(v);
 
     if ( cpu_has_rdtscp )
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 8fbbf3a..54f9f0e 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1565,8 +1565,11 @@ void arch_get_info_guest(struct vcpu *v, vcpu_guest_context_u c)
         }
     }
 
-    for ( i = 0; i < ARRAY_SIZE(v->arch.debugreg); ++i )
-        c(debugreg[i] = v->arch.debugreg[i]);
+    for ( i = 0; i < ARRAY_SIZE(v->arch.dr); ++i )
+        c(debugreg[i] = v->arch.dr[i]);
+    c(debugreg[6] = v->arch.dr6);
+    c(debugreg[7] = v->arch.dr7 |
+      (is_pv_domain(d) ? v->arch.pv_vcpu.dr7_emul : 0));
 
     if ( is_hvm_domain(d) )
     {
@@ -1641,10 +1644,6 @@ void arch_get_info_guest(struct vcpu *v, vcpu_guest_context_u c)
             c.nat->ctrlreg[1] =
                 pagetable_is_null(v->arch.guest_table_user) ? 0
                 : xen_pfn_to_cr3(pagetable_get_pfn(v->arch.guest_table_user));
-
-            /* Merge shadow DR7 bits into real DR7. */
-            c.nat->debugreg[7] |= c.nat->debugreg[5];
-            c.nat->debugreg[5] = 0;
         }
         else
         {
@@ -1653,10 +1652,6 @@ void arch_get_info_guest(struct vcpu *v, vcpu_guest_context_u c)
 
             c.cmp->ctrlreg[3] = compat_pfn_to_cr3(l4e_get_pfn(*l4e));
             unmap_domain_page(l4e);
-
-            /* Merge shadow DR7 bits into real DR7. */
-            c.cmp->debugreg[7] |= c.cmp->debugreg[5];
-            c.cmp->debugreg[5] = 0;
         }
 
         if ( guest_kernel_mode(v, &v->arch.user_regs) )
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 7fddae1..4f825a2 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -883,12 +883,12 @@ static int hvm_save_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
         ctxt.r13 = v->arch.user_regs.r13;
         ctxt.r14 = v->arch.user_regs.r14;
         ctxt.r15 = v->arch.user_regs.r15;
-        ctxt.dr0 = v->arch.debugreg[0];
-        ctxt.dr1 = v->arch.debugreg[1];
-        ctxt.dr2 = v->arch.debugreg[2];
-        ctxt.dr3 = v->arch.debugreg[3];
-        ctxt.dr6 = v->arch.debugreg[6];
-        ctxt.dr7 = v->arch.debugreg[7];
+        ctxt.dr0 = v->arch.dr[0];
+        ctxt.dr1 = v->arch.dr[1];
+        ctxt.dr2 = v->arch.dr[2];
+        ctxt.dr3 = v->arch.dr[3];
+        ctxt.dr6 = v->arch.dr6;
+        ctxt.dr7 = v->arch.dr7;
 
         if ( hvm_save_entry(CPU, v->vcpu_id, h, &ctxt) != 0 )
             return 1; 
@@ -1151,12 +1151,12 @@ static int hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
     v->arch.user_regs.r13 = ctxt.r13;
     v->arch.user_regs.r14 = ctxt.r14;
     v->arch.user_regs.r15 = ctxt.r15;
-    v->arch.debugreg[0] = ctxt.dr0;
-    v->arch.debugreg[1] = ctxt.dr1;
-    v->arch.debugreg[2] = ctxt.dr2;
-    v->arch.debugreg[3] = ctxt.dr3;
-    v->arch.debugreg[6] = adjust_dr6_rsvd(ctxt.dr6, cp->feat.rtm);
-    v->arch.debugreg[7] = adjust_dr7_rsvd(ctxt.dr7, cp->feat.rtm);
+    v->arch.dr[0] = ctxt.dr0;
+    v->arch.dr[1] = ctxt.dr1;
+    v->arch.dr[2] = ctxt.dr2;
+    v->arch.dr[3] = ctxt.dr3;
+    v->arch.dr6 = adjust_dr6_rsvd(ctxt.dr6, cp->feat.rtm);
+    v->arch.dr7 = adjust_dr7_rsvd(ctxt.dr7, cp->feat.rtm);
 
     v->arch.vgc_flags = VGCF_online;
 
@@ -3910,9 +3910,9 @@ void hvm_vcpu_reset_state(struct vcpu *v, uint16_t cs, uint16_t ip)
     v->arch.user_regs.rdx = 0x00000f00;
     v->arch.user_regs.rip = ip;
 
-    memset(&v->arch.debugreg, 0, sizeof(v->arch.debugreg) - 16);
-    v->arch.debugreg[6] = X86_DR6_DEFAULT;
-    v->arch.debugreg[7] = X86_DR7_DEFAULT;
+    memset(v->arch.dr, 0, sizeof(v->arch.dr));
+    v->arch.dr6 = X86_DR6_DEFAULT;
+    v->arch.dr7 = X86_DR7_DEFAULT;
 
     v->arch.hvm_vcpu.guest_cr[0] = X86_CR0_ET;
     hvm_update_guest_cr(v, 0);
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 49bb722..dabb96f 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -215,12 +215,12 @@ static void svm_save_dr(struct vcpu *v)
         rdmsrl(MSR_AMD64_DR3_ADDRESS_MASK, v->arch.hvm_svm.dr_mask[3]);
     }
 
-    v->arch.debugreg[0] = read_debugreg(0);
-    v->arch.debugreg[1] = read_debugreg(1);
-    v->arch.debugreg[2] = read_debugreg(2);
-    v->arch.debugreg[3] = read_debugreg(3);
-    v->arch.debugreg[6] = vmcb_get_dr6(vmcb);
-    v->arch.debugreg[7] = vmcb_get_dr7(vmcb);
+    v->arch.dr[0] = read_debugreg(0);
+    v->arch.dr[1] = read_debugreg(1);
+    v->arch.dr[2] = read_debugreg(2);
+    v->arch.dr[3] = read_debugreg(3);
+    v->arch.dr6 = vmcb_get_dr6(vmcb);
+    v->arch.dr7 = vmcb_get_dr7(vmcb);
 }
 
 static void __restore_debug_registers(struct vmcb_struct *vmcb, struct vcpu *v)
@@ -246,12 +246,12 @@ static void __restore_debug_registers(struct vmcb_struct *vmcb, struct vcpu *v)
         wrmsrl(MSR_AMD64_DR3_ADDRESS_MASK, v->arch.hvm_svm.dr_mask[3]);
     }
 
-    write_debugreg(0, v->arch.debugreg[0]);
-    write_debugreg(1, v->arch.debugreg[1]);
-    write_debugreg(2, v->arch.debugreg[2]);
-    write_debugreg(3, v->arch.debugreg[3]);
-    vmcb_set_dr6(vmcb, v->arch.debugreg[6]);
-    vmcb_set_dr7(vmcb, v->arch.debugreg[7]);
+    write_debugreg(0, v->arch.dr[0]);
+    write_debugreg(1, v->arch.dr[1]);
+    write_debugreg(2, v->arch.dr[2]);
+    write_debugreg(3, v->arch.dr[3]);
+    vmcb_set_dr6(vmcb, v->arch.dr6);
+    vmcb_set_dr7(vmcb, v->arch.dr7);
 }
 
 /*
@@ -263,7 +263,8 @@ static void __restore_debug_registers(struct vmcb_struct *vmcb, struct vcpu *v)
 static void svm_restore_dr(struct vcpu *v)
 {
     struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
-    if ( unlikely(v->arch.debugreg[7] & DR7_ACTIVE_MASK) )
+
+    if ( unlikely(v->arch.dr7 & DR7_ACTIVE_MASK) )
         __restore_debug_registers(vmcb, v);
 }
 
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 8dbe838..bfa3a0d 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -610,13 +610,13 @@ static void vmx_save_dr(struct vcpu *v)
     v->arch.hvm_vmx.exec_control |= CPU_BASED_MOV_DR_EXITING;
     vmx_update_cpu_exec_control(v);
 
-    v->arch.debugreg[0] = read_debugreg(0);
-    v->arch.debugreg[1] = read_debugreg(1);
-    v->arch.debugreg[2] = read_debugreg(2);
-    v->arch.debugreg[3] = read_debugreg(3);
-    v->arch.debugreg[6] = read_debugreg(6);
+    v->arch.dr[0] = read_debugreg(0);
+    v->arch.dr[1] = read_debugreg(1);
+    v->arch.dr[2] = read_debugreg(2);
+    v->arch.dr[3] = read_debugreg(3);
+    v->arch.dr6   = read_debugreg(6);
     /* DR7 must be saved as it is used by vmx_restore_dr(). */
-    __vmread(GUEST_DR7, &v->arch.debugreg[7]);
+    __vmread(GUEST_DR7, &v->arch.dr7);
 }
 
 static void __restore_debug_registers(struct vcpu *v)
@@ -626,11 +626,11 @@ static void __restore_debug_registers(struct vcpu *v)
 
     v->arch.hvm_vcpu.flag_dr_dirty = 1;
 
-    write_debugreg(0, v->arch.debugreg[0]);
-    write_debugreg(1, v->arch.debugreg[1]);
-    write_debugreg(2, v->arch.debugreg[2]);
-    write_debugreg(3, v->arch.debugreg[3]);
-    write_debugreg(6, v->arch.debugreg[6]);
+    write_debugreg(0, v->arch.dr[0]);
+    write_debugreg(1, v->arch.dr[1]);
+    write_debugreg(2, v->arch.dr[2]);
+    write_debugreg(3, v->arch.dr[3]);
+    write_debugreg(6, v->arch.dr6);
     /* DR7 is loaded from the VMCS. */
 }
 
@@ -643,7 +643,7 @@ static void __restore_debug_registers(struct vcpu *v)
 static void vmx_restore_dr(struct vcpu *v)
 {
     /* NB. __vmread() is not usable here, so we cannot read from the VMCS. */
-    if ( unlikely(v->arch.debugreg[7] & DR7_ACTIVE_MASK) )
+    if ( unlikely(v->arch.dr7 & DR7_ACTIVE_MASK) )
         __restore_debug_registers(v);
 }
 
@@ -1870,7 +1870,7 @@ static void vmx_set_info_guest(struct vcpu *v)
 
     vmx_vmcs_enter(v);
 
-    __vmwrite(GUEST_DR7, v->arch.debugreg[7]);
+    __vmwrite(GUEST_DR7, v->arch.dr7);
 
     /* 
      * If the interruptibility-state field indicates blocking by STI,
diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
index ce2ec76..dfde70e 100644
--- a/xen/arch/x86/pv/emul-priv-op.c
+++ b/xen/arch/x86/pv/emul-priv-op.c
@@ -286,20 +286,20 @@ static unsigned int check_guest_io_breakpoint(struct vcpu *v,
     unsigned int width, i, match = 0;
     unsigned long start;
 
-    if ( !(v->arch.debugreg[5]) ||
+    if ( !v->arch.pv_vcpu.dr7_emul ||
          !(v->arch.pv_vcpu.ctrlreg[4] & X86_CR4_DE) )
         return 0;
 
     for ( i = 0; i < 4; i++ )
     {
-        if ( !(v->arch.debugreg[5] &
+        if ( !(v->arch.pv_vcpu.dr7_emul &
                (3 << (i * DR_ENABLE_SIZE))) )
             continue;
 
-        start = v->arch.debugreg[i];
+        start = v->arch.dr[i];
         width = 0;
 
-        switch ( (v->arch.debugreg[7] >>
+        switch ( (v->arch.dr7 >>
                   (DR_CONTROL_SHIFT + i * DR_CONTROL_SIZE)) & 0xc )
         {
         case DR_LEN_1: width = 1; break;
@@ -1116,7 +1116,7 @@ static int write_msr(unsigned int reg, uint64_t val,
         if ( !boot_cpu_has(X86_FEATURE_DBEXT) || (val >> 32) )
             break;
         curr->arch.pv_vcpu.dr_mask[0] = val;
-        if ( curr->arch.debugreg[7] & DR7_ACTIVE_MASK )
+        if ( curr->arch.dr7 & DR7_ACTIVE_MASK )
             wrmsrl(MSR_AMD64_DR0_ADDRESS_MASK, val);
         return X86EMUL_OKAY;
 
@@ -1124,7 +1124,7 @@ static int write_msr(unsigned int reg, uint64_t val,
         if ( !boot_cpu_has(X86_FEATURE_DBEXT) || (val >> 32) )
             break;
         curr->arch.pv_vcpu.dr_mask[reg - MSR_AMD64_DR1_ADDRESS_MASK + 1] = val;
-        if ( curr->arch.debugreg[7] & DR7_ACTIVE_MASK )
+        if ( curr->arch.dr7 & DR7_ACTIVE_MASK )
             wrmsrl(reg, val);
         return X86EMUL_OKAY;
 
@@ -1368,7 +1368,7 @@ int pv_emulate_privileged_op(struct cpu_user_regs *regs)
             ctxt.bpmatch |= DR_STEP;
         if ( ctxt.bpmatch )
         {
-            curr->arch.debugreg[6] |= ctxt.bpmatch | DR_STATUS_RESERVED_ONE;
+            curr->arch.dr6 |= ctxt.bpmatch | DR_STATUS_RESERVED_ONE;
             if ( !(curr->arch.pv_vcpu.trap_bounce.flags & TBF_EXCEPTION) )
                 pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
         }
diff --git a/xen/arch/x86/pv/emulate.c b/xen/arch/x86/pv/emulate.c
index 1b60911..757ffd1 100644
--- a/xen/arch/x86/pv/emulate.c
+++ b/xen/arch/x86/pv/emulate.c
@@ -78,7 +78,7 @@ void pv_emul_instruction_done(struct cpu_user_regs *regs, unsigned long rip)
     regs->eflags &= ~X86_EFLAGS_RF;
     if ( regs->eflags & X86_EFLAGS_TF )
     {
-        current->arch.debugreg[6] |= DR_STEP | DR_STATUS_RESERVED_ONE;
+        current->arch.dr6 |= DR_STEP | DR_STATUS_RESERVED_ONE;
         pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
     }
 }
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index e9bfbc7..d0d9011 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1861,8 +1861,8 @@ void do_debug(struct cpu_user_regs *regs)
     }
 
     /* Save debug status register where guest OS can peek at it */
-    v->arch.debugreg[6] |= (dr6 & ~X86_DR6_DEFAULT);
-    v->arch.debugreg[6] &= (dr6 | ~X86_DR6_DEFAULT);
+    v->arch.dr6 |= (dr6 & ~X86_DR6_DEFAULT);
+    v->arch.dr6 &= (dr6 | ~X86_DR6_DEFAULT);
 
     pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
 }
@@ -2025,19 +2025,19 @@ void activate_debugregs(const struct vcpu *curr)
 {
     ASSERT(curr == current);
 
-    write_debugreg(0, curr->arch.debugreg[0]);
-    write_debugreg(1, curr->arch.debugreg[1]);
-    write_debugreg(2, curr->arch.debugreg[2]);
-    write_debugreg(3, curr->arch.debugreg[3]);
-    write_debugreg(6, curr->arch.debugreg[6]);
+    write_debugreg(0, curr->arch.dr[0]);
+    write_debugreg(1, curr->arch.dr[1]);
+    write_debugreg(2, curr->arch.dr[2]);
+    write_debugreg(3, curr->arch.dr[3]);
+    write_debugreg(6, curr->arch.dr6);
 
     /*
      * Avoid writing the subsequently getting replaced value when getting
      * called from set_debugreg() below. Eventual future callers will need
      * to take this into account.
      */
-    if ( curr->arch.debugreg[7] & DR7_ACTIVE_MASK )
-        write_debugreg(7, curr->arch.debugreg[7]);
+    if ( curr->arch.dr7 & DR7_ACTIVE_MASK )
+        write_debugreg(7, curr->arch.dr7);
 
     if ( boot_cpu_has(X86_FEATURE_DBEXT) )
     {
@@ -2065,6 +2065,7 @@ long set_debugreg(struct vcpu *v, unsigned int reg, unsigned long value)
         if ( !access_ok(value, sizeof(long)) )
             return -EPERM;
 
+        v->arch.dr[reg] = value;
         if ( v == curr )
         {
             switch ( reg )
@@ -2089,6 +2090,7 @@ long set_debugreg(struct vcpu *v, unsigned int reg, unsigned long value)
 
         value = adjust_dr6_rsvd(value, cp->feat.rtm);
 
+        v->arch.dr6 = value;
         if ( v == curr )
             write_debugreg(6, value);
         break;
@@ -2127,8 +2129,7 @@ long set_debugreg(struct vcpu *v, unsigned int reg, unsigned long value)
                 }
             }
 
-            /* Guest DR5 is a handy stash for I/O intercept information. */
-            v->arch.debugreg[5] = io_enable;
+            v->arch.pv_vcpu.dr7_emul = io_enable;
             value &= ~io_enable;
 
             /*
@@ -2136,14 +2137,14 @@ long set_debugreg(struct vcpu *v, unsigned int reg, unsigned long value)
              * debug registers at this point as they were not restored during
              * context switch.  Updating DR7 itself happens later.
              */
-            if ( (v == curr) &&
-                 !(v->arch.debugreg[7] & DR7_ACTIVE_MASK) )
+            if ( (v == curr) && !(v->arch.dr7 & DR7_ACTIVE_MASK) )
                 activate_debugregs(v);
         }
         else
             /* Zero the emulated controls if %dr7 isn't active. */
-            v->arch.debugreg[5] = 0;
+            v->arch.pv_vcpu.dr7_emul = 0;
 
+        v->arch.dr7 = value;
         if ( v == curr )
             write_debugreg(7, value);
         break;
@@ -2152,7 +2153,6 @@ long set_debugreg(struct vcpu *v, unsigned int reg, unsigned long value)
         return -ENODEV;
     }
 
-    v->arch.debugreg[reg] = value;
     return 0;
 }
 
diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
index f91aade..81facb0 100644
--- a/xen/arch/x86/vm_event.c
+++ b/xen/arch/x86/vm_event.c
@@ -155,7 +155,7 @@ void vm_event_fill_regs(vm_event_request_t *req)
     req->data.regs.x86.rflags = regs->rflags;
     req->data.regs.x86.rip    = regs->rip;
 
-    req->data.regs.x86.dr7 = curr->arch.debugreg[7];
+    req->data.regs.x86.dr7 = curr->arch.dr7;
     req->data.regs.x86.cr0 = ctxt.cr0;
     req->data.regs.x86.cr2 = ctxt.cr2;
     req->data.regs.x86.cr3 = ctxt.cr3;
diff --git a/xen/arch/x86/x86_emulate.c b/xen/arch/x86/x86_emulate.c
index 03b364a..9d1942b 100644
--- a/xen/arch/x86/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate.c
@@ -101,7 +101,7 @@ int x86emul_read_dr(unsigned int reg, unsigned long *val,
     switch ( reg )
     {
     case 0 ... 3:
-        *val = curr->arch.debugreg[reg];
+        *val = curr->arch.dr[reg];
         break;
 
     case 4:
@@ -110,7 +110,7 @@ int x86emul_read_dr(unsigned int reg, unsigned long *val,
 
         /* Fallthrough */
     case 6:
-        *val = curr->arch.debugreg[6];
+        *val = curr->arch.dr6;
         break;
 
     case 5:
@@ -119,8 +119,7 @@ int x86emul_read_dr(unsigned int reg, unsigned long *val,
 
         /* Fallthrough */
     case 7:
-        *val = (curr->arch.debugreg[7] |
-                curr->arch.debugreg[5]);
+        *val = curr->arch.dr7 | curr->arch.pv_vcpu.dr7_emul;
         break;
 
     ud_fault:
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 197f8d6..59d5e4a 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -500,6 +500,12 @@ struct pv_vcpu
     unsigned long shadow_ldt_mapcnt;
     spinlock_t shadow_ldt_lock;
 
+    /*
+     * %dr7 bits the guest has set, but aren't loaded into hardware, and are
+     * completely emulated.
+     */
+    uint32_t dr7_emul;
+
     /* data breakpoint extension MSRs */
     uint32_t dr_mask[4];
 
@@ -518,7 +524,10 @@ struct arch_vcpu
     void              *fpu_ctxt;
     unsigned long      vgc_flags;
     struct cpu_user_regs user_regs;
-    unsigned long      debugreg[8];
+
+    /* Debug registers. */
+    unsigned long dr[4];
+    unsigned long dr6, dr7;
 
     /* other state */
 
-- 
2.1.4


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

Re: [PATCH 06/11] x86: Reorganise and rename debug register fields in struct vcpu

[Jan Beulich]

>>> On 04.06.18 at 15:59, <andrew.cooper3@citrix.com> wrote:
> @@ -981,9 +981,14 @@ int arch_set_info_guest(
>      v->arch.pv_vcpu.ctrlreg[4] = cr4 ? pv_guest_cr4_fixup(v, cr4) :
>          real_cr4_to_pv_guest_cr4(mmu_cr4_features);
>  
> -    memset(v->arch.debugreg, 0, sizeof(v->arch.debugreg));
> -    for ( i = 0; i < 8; i++ )
> -        (void)set_debugreg(v, i, c(debugreg[i]));
> +    for ( i = 0; !rc && i < ARRAY_SIZE(v->arch.dr); i++ )
> +        rc = set_debugreg(v, i, c(debugreg[i]));
> +    if ( !rc )
> +        rc = set_debugreg(v, 6, c(debugreg[6]));
> +    if ( !rc )
> +        rc = set_debugreg(v, 7, c(debugreg[7]));
> +    if ( rc )
> +        return rc;

There is certainly a good intention behind this change, but it treats
one problem for another: The function has a pre-existing partial-
update-and-then-fail problem, which you now widen. Proper
behavior would imo be to never update any state when returning
an error, at least as far as anything ahead of

>      if ( v->is_initialised )
>          goto out;

goes (for not yet initialized vCPU-s the function would need to be
called again anyway before the vCPU could be started). On the
whole, until said problem gets addressed, I think I'd prefer original
behavior over the new one you switch to.

> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -1861,8 +1861,8 @@ void do_debug(struct cpu_user_regs *regs)
>      }
>  
>      /* Save debug status register where guest OS can peek at it */
> -    v->arch.debugreg[6] |= (dr6 & ~X86_DR6_DEFAULT);
> -    v->arch.debugreg[6] &= (dr6 | ~X86_DR6_DEFAULT);
> +    v->arch.dr6 |= (dr6 & ~X86_DR6_DEFAULT);
> +    v->arch.dr6 &= (dr6 | ~X86_DR6_DEFAULT);

Shouldn't this have been changed by patch 4?

> @@ -518,7 +524,10 @@ struct arch_vcpu
>      void              *fpu_ctxt;
>      unsigned long      vgc_flags;
>      struct cpu_user_regs user_regs;
> -    unsigned long      debugreg[8];
> +
> +    /* Debug registers. */
> +    unsigned long dr[4];
> +    unsigned long dr6, dr7;

Since you make the last two separate fields, and since their upper
32 bits are reserved-zero, why not make them uint32_t, just like
dr7_emul is?

Jan



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

Re: [PATCH 06/11] x86: Reorganise and rename debug register fields in struct vcpu

[Andrew Cooper]

On 06/06/18 16:00, Jan Beulich wrote:
>>>> On 04.06.18 at 15:59, <andrew.cooper3@citrix.com> wrote:
>> @@ -981,9 +981,14 @@ int arch_set_info_guest(
>>      v->arch.pv_vcpu.ctrlreg[4] = cr4 ? pv_guest_cr4_fixup(v, cr4) :
>>          real_cr4_to_pv_guest_cr4(mmu_cr4_features);
>>  
>> -    memset(v->arch.debugreg, 0, sizeof(v->arch.debugreg));
>> -    for ( i = 0; i < 8; i++ )
>> -        (void)set_debugreg(v, i, c(debugreg[i]));
>> +    for ( i = 0; !rc && i < ARRAY_SIZE(v->arch.dr); i++ )
>> +        rc = set_debugreg(v, i, c(debugreg[i]));
>> +    if ( !rc )
>> +        rc = set_debugreg(v, 6, c(debugreg[6]));
>> +    if ( !rc )
>> +        rc = set_debugreg(v, 7, c(debugreg[7]));
>> +    if ( rc )
>> +        return rc;
> There is certainly a good intention behind this change, but it treats
> one problem for another: The function has a pre-existing partial-
> update-and-then-fail problem, which you now widen. Proper
> behavior would imo be to never update any state when returning
> an error, at least as far as anything ahead of
>
>>      if ( v->is_initialised )
>>          goto out;
> goes (for not yet initialized vCPU-s the function would need to be
> called again anyway before the vCPU could be started). On the
> whole, until said problem gets addressed, I think I'd prefer original
> behavior over the new one you switch to.

Hmm ok.

>
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -1861,8 +1861,8 @@ void do_debug(struct cpu_user_regs *regs)
>>      }
>>  
>>      /* Save debug status register where guest OS can peek at it */
>> -    v->arch.debugreg[6] |= (dr6 & ~X86_DR6_DEFAULT);
>> -    v->arch.debugreg[6] &= (dr6 | ~X86_DR6_DEFAULT);
>> +    v->arch.dr6 |= (dr6 & ~X86_DR6_DEFAULT);
>> +    v->arch.dr6 &= (dr6 | ~X86_DR6_DEFAULT);
> Shouldn't this have been changed by patch 4?

This gets fixed by patch 9.

>
>> @@ -518,7 +524,10 @@ struct arch_vcpu
>>      void              *fpu_ctxt;
>>      unsigned long      vgc_flags;
>>      struct cpu_user_regs user_regs;
>> -    unsigned long      debugreg[8];
>> +
>> +    /* Debug registers. */
>> +    unsigned long dr[4];
>> +    unsigned long dr6, dr7;
> Since you make the last two separate fields, and since their upper
> 32 bits are reserved-zero, why not make them uint32_t, just like
> dr7_emul is?

Because __vmread() takes unsigned long *.

I did initially make them uint32_t, then had the compiler complain at
me.  I indented to leave a note in the commit message, but I forgot.  Sorry.

~Andrew

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

Re: [PATCH 06/11] x86: Reorganise and rename debug register fields in struct vcpu

[Jan Beulich]

>>> On 06.06.18 at 17:21, <andrew.cooper3@citrix.com> wrote:
> On 06/06/18 16:00, Jan Beulich wrote:
>>>>> On 04.06.18 at 15:59, <andrew.cooper3@citrix.com> wrote:
>>> @@ -518,7 +524,10 @@ struct arch_vcpu
>>>      void              *fpu_ctxt;
>>>      unsigned long      vgc_flags;
>>>      struct cpu_user_regs user_regs;
>>> -    unsigned long      debugreg[8];
>>> +
>>> +    /* Debug registers. */
>>> +    unsigned long dr[4];
>>> +    unsigned long dr6, dr7;
>> Since you make the last two separate fields, and since their upper
>> 32 bits are reserved-zero, why not make them uint32_t, just like
>> dr7_emul is?
> 
> Because __vmread() takes unsigned long *.
> 
> I did initially make them uint32_t, then had the compiler complain at
> me.  I indented to leave a note in the commit message, but I forgot.  Sorry.

That's for dr7 only, then. dr6 and dr7_emul could then share the other
64-bit slot.

Jan



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

Re: [PATCH 06/11] x86: Reorganise and rename debug register fields in struct vcpu

[Roger Pau Monné]

On Mon, Jun 04, 2018 at 02:59:10PM +0100, Andrew Cooper wrote:
> Reusing debugreg[5] for the PV emulated IO breakpoint information is confusing
> to read.  Instead, introduce a dr7_emul field in pv_vcpu for the pupose.
> 
> With the PV emulation out of the way, debugreg[4,5] are entirely unused and
> don't need to be stored.
> 
> Rename debugreg[0..3] to dr[0..3] to reduce code volume, but keep them as an
> array because their behaviour is identical and this helps simplfy some of the
> PV handling.  Introduce dr6 and dr7 fields to replace debugreg[6,7] which
> removes the storage for debugreg[4,5].
> 
> Two minor alterations on the PV side is that merging of the emulated state
> happens along with the other dr handling, rather than much later, and
> arch_set_info_guest() now checks the return value from set_debugreg() fails
> the hypercall rather than silently discarding the values.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
> index 197f8d6..59d5e4a 100644
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -500,6 +500,12 @@ struct pv_vcpu
>      unsigned long shadow_ldt_mapcnt;
>      spinlock_t shadow_ldt_lock;
>  
> +    /*
> +     * %dr7 bits the guest has set, but aren't loaded into hardware, and are
> +     * completely emulated.
> +     */
> +    uint32_t dr7_emul;

Upper 32bit are reserved ATM, but won't it be better to just use a
unsigned long (like it's used below to store the registers)?

Roger.

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

[PATCH 07/11] x86/emul: Add pending_dbg field to x86_event

[Andrew Cooper]

All #DB exceptions result in an update of %dr6, but this isn't captured in
Xen's handling.

PV guests generally work by modifying %dr6 before raising #DB, whereas HVM
guests do nothing and have a single-step special case in the lowest levels of
{vmx,svm}_inject_event().  All of this is buggy, but in particular, task
switches with the trace flag never end up signalling BT in %dr6.

To begin resolving this issue, add a new pending_dbg field to x86_event
(unioned with cr2 to avoid taking any extra space), and introduce
{pv,hvm}_inject_debug_exn() helpers to replace the current callers using
{pv,hvm}_inject_hw_exception().

A key property is that pending_dbg is taken with positive polarity to deal
with RTM sensibly.  Most callers pass in a constant, but callers passing in a
hardware %dr6 value need to xor the value with X86_DR6_DEFAULT to flip the
polarity of RTM and reserved fields.

For PV guests, move the ad-hoc updating of %dr6 into pv_inject_event().  This
in principle breaks the handing of RTM in do_debug(), but PV guests can't
actually enable MSR_DEBUGCTL.RTM yet, so this doesn't matter in practice.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
CC: Brian Woods <brian.woods@amd.com>
CC: Tim Deegan <tim@xen.org>
---
 xen/arch/x86/hvm/emulate.c             |  3 ++-
 xen/arch/x86/hvm/hvm.c                 |  2 +-
 xen/arch/x86/hvm/svm/svm.c             |  9 ++++++---
 xen/arch/x86/hvm/vmx/vmx.c             | 13 ++++++++-----
 xen/arch/x86/mm/shadow/multi.c         |  5 +++--
 xen/arch/x86/pv/emul-priv-op.c         | 11 +++++------
 xen/arch/x86/pv/emulate.c              |  6 ++----
 xen/arch/x86/pv/ro-page-fault.c        |  3 ++-
 xen/arch/x86/pv/traps.c                | 16 ++++++++++++----
 xen/arch/x86/traps.c                   |  7 +------
 xen/arch/x86/x86_emulate/x86_emulate.h |  5 ++++-
 xen/include/asm-x86/domain.h           | 12 ++++++++++++
 xen/include/asm-x86/hvm/hvm.h          | 15 ++++++++++++++-
 13 files changed, 72 insertions(+), 35 deletions(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index c9aa188..0292058 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -15,6 +15,7 @@
 #include <xen/paging.h>
 #include <xen/trace.h>
 #include <xen/vm_event.h>
+#include <asm/debugreg.h>
 #include <asm/event.h>
 #include <asm/i387.h>
 #include <asm/xstate.h>
@@ -2283,7 +2284,7 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt,
     }
 
     if ( hvmemul_ctxt->ctxt.retire.singlestep )
-        hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
+        hvm_inject_debug_exn(X86_DR6_BS);
 
     new_intr_shadow = hvmemul_ctxt->intr_shadow;
 
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 4f825a2..b35cf54 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3115,7 +3115,7 @@ void hvm_task_switch(
     }
 
     if ( (tss.trace & 1) && !exn_raised )
-        hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
+        hvm_inject_debug_exn(X86_DR6_BT);
 
  out:
     hvm_unmap_entry(optss_desc);
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index dabb96f..c06bd68 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -119,7 +119,7 @@ void __update_guest_eip(struct cpu_user_regs *regs, unsigned int inst_len)
     curr->arch.hvm_svm.vmcb->interrupt_shadow = 0;
 
     if ( regs->eflags & X86_EFLAGS_TF )
-        hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
+        hvm_inject_debug_exn(X86_DR6_BS);
 }
 
 static void svm_cpu_down(void)
@@ -2798,7 +2798,10 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
                 goto unexpected_exit_type;
             if ( !rc )
                 hvm_inject_exception(TRAP_debug,
-                                     trap_type, inst_len, X86_EVENT_NO_EC);
+                                     trap_type, inst_len, X86_EVENT_NO_EC,
+                                     exit_reason == VMEXIT_ICEBP ? 0 :
+                                     /* #DB - Hardware already updated dr6. */
+                                     vmcb_get_dr6(vmcb) ^ X86_DR6_DEFAULT);
         }
         else
             domain_pause_for_debugger();
@@ -2830,7 +2833,7 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
            if ( !rc )
                hvm_inject_exception(TRAP_int3,
                                     X86_EVENTTYPE_SW_EXCEPTION,
-                                    inst_len, X86_EVENT_NO_EC);
+                                    inst_len, X86_EVENT_NO_EC, 0 /* N/A */);
         }
         break;
 
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index bfa3a0d..39c9ddc 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2483,7 +2483,7 @@ void update_guest_eip(void)
     }
 
     if ( regs->eflags & X86_EFLAGS_TF )
-        hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
+        hvm_inject_debug_exn(X86_DR6_BS);
 }
 
 static void vmx_fpu_dirty_intercept(void)
@@ -3382,7 +3382,7 @@ static int vmx_handle_eoi_write(void)
  * It is the callers responsibility to ensure that this function is only used
  * in the context of an appropriate vmexit.
  */
-static void vmx_propagate_intr(unsigned long intr)
+static void vmx_propagate_intr(unsigned long intr, unsigned long pending_dbg)
 {
     struct x86_event event = {
         .vector = MASK_EXTR(intr, INTR_INFO_VECTOR_MASK),
@@ -3406,6 +3406,9 @@ static void vmx_propagate_intr(unsigned long intr)
     else
         event.insn_len = 0;
 
+    if ( event.vector == TRAP_debug )
+        event.pending_dbg = pending_dbg;
+
     hvm_inject_event(&event);
 }
 
@@ -3715,7 +3718,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
                 if ( rc < 0 )
                     goto exit_and_crash;
                 if ( !rc )
-                    vmx_propagate_intr(intr_info);
+                    vmx_propagate_intr(intr_info, exit_qualification);
             }
             else
                 domain_pause_for_debugger();
@@ -3736,7 +3739,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
                 if ( rc < 0 )
                     goto exit_and_crash;
                 if ( !rc )
-                    vmx_propagate_intr(intr_info);
+                    vmx_propagate_intr(intr_info, 0 /* N/A */);
             }
             else
             {
@@ -3776,7 +3779,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
             break;
         case TRAP_alignment_check:
             HVMTRACE_1D(TRAP, vector);
-            vmx_propagate_intr(intr_info);
+            vmx_propagate_intr(intr_info, 0 /* N/A */);
             break;
         case TRAP_nmi:
             if ( MASK_EXTR(intr_info, INTR_INFO_INTR_TYPE_MASK) !=
diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c
index da586c2..57d24d9 100644
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -30,6 +30,7 @@ asm(".file \"" __OBJECT_FILE__ "\"");
 #include <xen/perfc.h>
 #include <xen/domain_page.h>
 #include <xen/iocap.h>
+#include <asm/debugreg.h>
 #include <xsm/xsm.h>
 #include <asm/page.h>
 #include <asm/current.h>
@@ -3453,7 +3454,7 @@ static int sh_page_fault(struct vcpu *v,
 #endif
 
     if ( emul_ctxt.ctxt.retire.singlestep )
-        hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
+        hvm_inject_debug_exn(X86_DR6_BS);
 
 #if GUEST_PAGING_LEVELS == 3 /* PAE guest */
     /*
@@ -3494,7 +3495,7 @@ static int sh_page_fault(struct vcpu *v,
                 TRACE_SHADOW_PATH_FLAG(TRCE_SFLAG_EMULATION_LAST_FAILED);
 
                 if ( emul_ctxt.ctxt.retire.singlestep )
-                    hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
+                    hvm_inject_debug_exn(X86_DR6_BS);
 
                 break; /* Don't emulate again if we failed! */
             }
diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
index dfde70e..45788b2 100644
--- a/xen/arch/x86/pv/emul-priv-op.c
+++ b/xen/arch/x86/pv/emul-priv-op.c
@@ -1366,12 +1366,11 @@ int pv_emulate_privileged_op(struct cpu_user_regs *regs)
 
         if ( ctxt.ctxt.retire.singlestep )
             ctxt.bpmatch |= DR_STEP;
-        if ( ctxt.bpmatch )
-        {
-            curr->arch.dr6 |= ctxt.bpmatch | DR_STATUS_RESERVED_ONE;
-            if ( !(curr->arch.pv_vcpu.trap_bounce.flags & TBF_EXCEPTION) )
-                pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
-        }
+
+        if ( ctxt.bpmatch &&
+             !(curr->arch.pv_vcpu.trap_bounce.flags & TBF_EXCEPTION) )
+            pv_inject_debug_exn(ctxt.bpmatch);
+
         /* fall through */
     case X86EMUL_RETRY:
         return EXCRET_fault_fixed;
diff --git a/xen/arch/x86/pv/emulate.c b/xen/arch/x86/pv/emulate.c
index 757ffd1..542b6d0 100644
--- a/xen/arch/x86/pv/emulate.c
+++ b/xen/arch/x86/pv/emulate.c
@@ -76,11 +76,9 @@ void pv_emul_instruction_done(struct cpu_user_regs *regs, unsigned long rip)
 {
     regs->rip = rip;
     regs->eflags &= ~X86_EFLAGS_RF;
+
     if ( regs->eflags & X86_EFLAGS_TF )
-    {
-        current->arch.dr6 |= DR_STEP | DR_STATUS_RESERVED_ONE;
-        pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
-    }
+        pv_inject_debug_exn(X86_DR6_BS);
 }
 
 /*
diff --git a/xen/arch/x86/pv/ro-page-fault.c b/xen/arch/x86/pv/ro-page-fault.c
index aa8d5a7..33873c9 100644
--- a/xen/arch/x86/pv/ro-page-fault.c
+++ b/xen/arch/x86/pv/ro-page-fault.c
@@ -25,6 +25,7 @@
 #include <xen/sched.h>
 #include <xen/trace.h>
 
+#include <asm/debugreg.h>
 #include <asm/domain.h>
 #include <asm/mm.h>
 #include <asm/pci.h>
@@ -387,7 +388,7 @@ int pv_ro_page_fault(unsigned long addr, struct cpu_user_regs *regs)
         /* Fallthrough */
     case X86EMUL_OKAY:
         if ( ctxt.retire.singlestep )
-            pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
+            pv_inject_debug_exn(X86_DR6_BS);
 
         /* Fallthrough */
     case X86EMUL_RETRY:
diff --git a/xen/arch/x86/pv/traps.c b/xen/arch/x86/pv/traps.c
index f48db92..7d48d83 100644
--- a/xen/arch/x86/pv/traps.c
+++ b/xen/arch/x86/pv/traps.c
@@ -26,6 +26,7 @@
 #include <xen/softirq.h>
 
 #include <asm/apic.h>
+#include <asm/debugreg.h>
 #include <asm/shared.h>
 #include <asm/traps.h>
 
@@ -70,9 +71,9 @@ void pv_inject_event(const struct x86_event *event)
     tb->cs    = ti->cs;
     tb->eip   = ti->address;
 
-    if ( event->type == X86_EVENTTYPE_HW_EXCEPTION &&
-         vector == TRAP_page_fault )
+    switch ( vector | -(event->type == X86_EVENTTYPE_SW_INTERRUPT) )
     {
+    case TRAP_page_fault:
         curr->arch.pv_vcpu.ctrlreg[2] = event->cr2;
         arch_set_cr2(curr, event->cr2);
 
@@ -82,9 +83,16 @@ void pv_inject_event(const struct x86_event *event)
             error_code |= PFEC_user_mode;
 
         trace_pv_page_fault(event->cr2, error_code);
-    }
-    else
+        break;
+
+    case TRAP_debug:
+        curr->arch.dr6 |= event->pending_dbg;
+        /* Fallthrough */
+
+    default:
         trace_pv_trap(vector, regs->rip, use_error_code, error_code);
+        break;
+    }
 
     if ( use_error_code )
     {
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index d0d9011..8ef22b4 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1768,7 +1768,6 @@ void do_device_not_available(struct cpu_user_regs *regs)
 void do_debug(struct cpu_user_regs *regs)
 {
     unsigned long dr6;
-    struct vcpu *v = current;
 
     /* Stash dr6 as early as possible. */
     dr6 = read_debugreg(6);
@@ -1860,11 +1859,7 @@ void do_debug(struct cpu_user_regs *regs)
         return;
     }
 
-    /* Save debug status register where guest OS can peek at it */
-    v->arch.dr6 |= (dr6 & ~X86_DR6_DEFAULT);
-    v->arch.dr6 &= (dr6 | ~X86_DR6_DEFAULT);
-
-    pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
+    pv_inject_debug_exn(dr6 ^ X86_DR6_DEFAULT);
 }
 
 static void __init noinline __set_intr_gate(unsigned int n,
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h b/xen/arch/x86/x86_emulate/x86_emulate.h
index c22e774..d85a84a 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.h
+++ b/xen/arch/x86/x86_emulate/x86_emulate.h
@@ -88,7 +88,10 @@ struct x86_event {
     uint8_t       type;         /* X86_EVENTTYPE_* */
     uint8_t       insn_len;     /* Instruction length */
     int32_t       error_code;   /* X86_EVENT_NO_EC if n/a */
-    unsigned long cr2;          /* Only for TRAP_page_fault h/w exception */
+    union {
+        unsigned long cr2;         /* #PF */
+        unsigned long pending_dbg; /* #DB (new DR6 bits, positive polarity) */
+    };
 };
 
 /*
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 59d5e4a..dfe995f 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -648,6 +648,18 @@ static inline void pv_inject_hw_exception(unsigned int vector, int errcode)
     pv_inject_event(&event);
 }
 
+static inline void pv_inject_debug_exn(unsigned long pending_dbg)
+{
+    struct x86_event event = {
+        .vector      = X86_EXC_DB,
+        .type        = X86_EVENTTYPE_HW_EXCEPTION,
+        .error_code  = X86_EVENT_NO_EC,
+        .pending_dbg = pending_dbg,
+    };
+
+    pv_inject_event(&event);
+}
+
 static inline void pv_inject_page_fault(int errcode, unsigned long cr2)
 {
     const struct x86_event event = {
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index ef5e198..65d512e 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -410,13 +410,14 @@ void hvm_inject_event(const struct x86_event *event);
 
 static inline void hvm_inject_exception(
     unsigned int vector, unsigned int type,
-    unsigned int insn_len, int error_code)
+    unsigned int insn_len, int error_code, unsigned long extra)
 {
     struct x86_event event = {
         .vector = vector,
         .type = type,
         .insn_len = insn_len,
         .error_code = error_code,
+        .cr2 = extra, /* Any union field will do. */
     };
 
     hvm_inject_event(&event);
@@ -433,6 +434,18 @@ static inline void hvm_inject_hw_exception(unsigned int vector, int errcode)
     hvm_inject_event(&event);
 }
 
+static inline void hvm_inject_debug_exn(unsigned long pending_dbg)
+{
+    struct x86_event event = {
+        .vector      = X86_EXC_DB,
+        .type        = X86_EVENTTYPE_HW_EXCEPTION,
+        .error_code  = X86_EVENT_NO_EC,
+        .pending_dbg = pending_dbg,
+    };
+
+    hvm_inject_event(&event);
+}
+
 static inline void hvm_inject_page_fault(int errcode, unsigned long cr2)
 {
     struct x86_event event = {
-- 
2.1.4


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

Re: [PATCH 07/11] x86/emul: Add pending_dbg field to x86_event

[Roger Pau Monné]

On Mon, Jun 04, 2018 at 02:59:11PM +0100, Andrew Cooper wrote:
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index bfa3a0d..39c9ddc 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2483,7 +2483,7 @@ void update_guest_eip(void)
>      }
>  
>      if ( regs->eflags & X86_EFLAGS_TF )
> -        hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
> +        hvm_inject_debug_exn(X86_DR6_BS);
>  }
>  
>  static void vmx_fpu_dirty_intercept(void)
> @@ -3382,7 +3382,7 @@ static int vmx_handle_eoi_write(void)
>   * It is the callers responsibility to ensure that this function is only used
>   * in the context of an appropriate vmexit.
>   */
> -static void vmx_propagate_intr(unsigned long intr)
> +static void vmx_propagate_intr(unsigned long intr, unsigned long pending_dbg)
>  {
>      struct x86_event event = {
>          .vector = MASK_EXTR(intr, INTR_INFO_VECTOR_MASK),
> @@ -3406,6 +3406,9 @@ static void vmx_propagate_intr(unsigned long intr)
>      else
>          event.insn_len = 0;
>  
> +    if ( event.vector == TRAP_debug )
> +        event.pending_dbg = pending_dbg;
> +
>      hvm_inject_event(&event);
>  }
>  
> @@ -3715,7 +3718,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>                  if ( rc < 0 )
>                      goto exit_and_crash;
>                  if ( !rc )
> -                    vmx_propagate_intr(intr_info);
> +                    vmx_propagate_intr(intr_info, exit_qualification);
>              }
>              else
>                  domain_pause_for_debugger();
> @@ -3736,7 +3739,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>                  if ( rc < 0 )
>                      goto exit_and_crash;
>                  if ( !rc )
> -                    vmx_propagate_intr(intr_info);
> +                    vmx_propagate_intr(intr_info, 0 /* N/A */);
>              }
>              else
>              {
> @@ -3776,7 +3779,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>              break;
>          case TRAP_alignment_check:
>              HVMTRACE_1D(TRAP, vector);
> -            vmx_propagate_intr(intr_info);
> +            vmx_propagate_intr(intr_info, 0 /* N/A */);
>              break;
>          case TRAP_nmi:
>              if ( MASK_EXTR(intr_info, INTR_INFO_INTR_TYPE_MASK) !=

I'm kind of lost here. Don't you need to update vmx_inject_event so
that it does something with the pending_dbg field in x86_event?

(and the same to svm_inject_event)

Roger.

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

Re: [PATCH 07/11] x86/emul: Add pending_dbg field to x86_event

[Andrew Cooper]

On 06/06/18 17:46, Roger Pau Monné wrote:
> On Mon, Jun 04, 2018 at 02:59:11PM +0100, Andrew Cooper wrote:
>> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
>> index bfa3a0d..39c9ddc 100644
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -2483,7 +2483,7 @@ void update_guest_eip(void)
>>      }
>>  
>>      if ( regs->eflags & X86_EFLAGS_TF )
>> -        hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
>> +        hvm_inject_debug_exn(X86_DR6_BS);
>>  }
>>  
>>  static void vmx_fpu_dirty_intercept(void)
>> @@ -3382,7 +3382,7 @@ static int vmx_handle_eoi_write(void)
>>   * It is the callers responsibility to ensure that this function is only used
>>   * in the context of an appropriate vmexit.
>>   */
>> -static void vmx_propagate_intr(unsigned long intr)
>> +static void vmx_propagate_intr(unsigned long intr, unsigned long pending_dbg)
>>  {
>>      struct x86_event event = {
>>          .vector = MASK_EXTR(intr, INTR_INFO_VECTOR_MASK),
>> @@ -3406,6 +3406,9 @@ static void vmx_propagate_intr(unsigned long intr)
>>      else
>>          event.insn_len = 0;
>>  
>> +    if ( event.vector == TRAP_debug )
>> +        event.pending_dbg = pending_dbg;
>> +
>>      hvm_inject_event(&event);
>>  }
>>  
>> @@ -3715,7 +3718,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>>                  if ( rc < 0 )
>>                      goto exit_and_crash;
>>                  if ( !rc )
>> -                    vmx_propagate_intr(intr_info);
>> +                    vmx_propagate_intr(intr_info, exit_qualification);
>>              }
>>              else
>>                  domain_pause_for_debugger();
>> @@ -3736,7 +3739,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>>                  if ( rc < 0 )
>>                      goto exit_and_crash;
>>                  if ( !rc )
>> -                    vmx_propagate_intr(intr_info);
>> +                    vmx_propagate_intr(intr_info, 0 /* N/A */);
>>              }
>>              else
>>              {
>> @@ -3776,7 +3779,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>>              break;
>>          case TRAP_alignment_check:
>>              HVMTRACE_1D(TRAP, vector);
>> -            vmx_propagate_intr(intr_info);
>> +            vmx_propagate_intr(intr_info, 0 /* N/A */);
>>              break;
>>          case TRAP_nmi:
>>              if ( MASK_EXTR(intr_info, INTR_INFO_INTR_TYPE_MASK) !=
> I'm kind of lost here. Don't you need to update vmx_inject_event so
> that it does something with the pending_dbg field in x86_event?
>
> (and the same to svm_inject_event)

The commit message specifically says "To begin resolving this issue, add
a ..."

This patch is plumbing.  Later patches are fixes, and it takes until
patch 9 to get working sensibly.

~Andrew

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

Re: [PATCH 07/11] x86/emul: Add pending_dbg field to x86_event

[Roger Pau Monné]

On Wed, Jun 06, 2018 at 05:50:38PM +0100, Andrew Cooper wrote:
> On 06/06/18 17:46, Roger Pau Monné wrote:
> > On Mon, Jun 04, 2018 at 02:59:11PM +0100, Andrew Cooper wrote:
> >> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> >> index bfa3a0d..39c9ddc 100644
> >> --- a/xen/arch/x86/hvm/vmx/vmx.c
> >> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> >> @@ -2483,7 +2483,7 @@ void update_guest_eip(void)
> >>      }
> >>  
> >>      if ( regs->eflags & X86_EFLAGS_TF )
> >> -        hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
> >> +        hvm_inject_debug_exn(X86_DR6_BS);
> >>  }
> >>  
> >>  static void vmx_fpu_dirty_intercept(void)
> >> @@ -3382,7 +3382,7 @@ static int vmx_handle_eoi_write(void)
> >>   * It is the callers responsibility to ensure that this function is only used
> >>   * in the context of an appropriate vmexit.
> >>   */
> >> -static void vmx_propagate_intr(unsigned long intr)
> >> +static void vmx_propagate_intr(unsigned long intr, unsigned long pending_dbg)
> >>  {
> >>      struct x86_event event = {
> >>          .vector = MASK_EXTR(intr, INTR_INFO_VECTOR_MASK),
> >> @@ -3406,6 +3406,9 @@ static void vmx_propagate_intr(unsigned long intr)
> >>      else
> >>          event.insn_len = 0;
> >>  
> >> +    if ( event.vector == TRAP_debug )
> >> +        event.pending_dbg = pending_dbg;
> >> +
> >>      hvm_inject_event(&event);
> >>  }
> >>  
> >> @@ -3715,7 +3718,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
> >>                  if ( rc < 0 )
> >>                      goto exit_and_crash;
> >>                  if ( !rc )
> >> -                    vmx_propagate_intr(intr_info);
> >> +                    vmx_propagate_intr(intr_info, exit_qualification);
> >>              }
> >>              else
> >>                  domain_pause_for_debugger();
> >> @@ -3736,7 +3739,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
> >>                  if ( rc < 0 )
> >>                      goto exit_and_crash;
> >>                  if ( !rc )
> >> -                    vmx_propagate_intr(intr_info);
> >> +                    vmx_propagate_intr(intr_info, 0 /* N/A */);
> >>              }
> >>              else
> >>              {
> >> @@ -3776,7 +3779,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
> >>              break;
> >>          case TRAP_alignment_check:
> >>              HVMTRACE_1D(TRAP, vector);
> >> -            vmx_propagate_intr(intr_info);
> >> +            vmx_propagate_intr(intr_info, 0 /* N/A */);
> >>              break;
> >>          case TRAP_nmi:
> >>              if ( MASK_EXTR(intr_info, INTR_INFO_INTR_TYPE_MASK) !=
> > I'm kind of lost here. Don't you need to update vmx_inject_event so
> > that it does something with the pending_dbg field in x86_event?
> >
> > (and the same to svm_inject_event)
> 
> The commit message specifically says "To begin resolving this issue, add
> a ..."
> 
> This patch is plumbing.  Later patches are fixes, and it takes until
> patch 9 to get working sensibly.

Sorry, realized this when reviewing the next patch:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

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

Re: [PATCH 07/11] x86/emul: Add pending_dbg field to x86_event

[Jan Beulich]

>>> On 04.06.18 at 15:59, <andrew.cooper3@citrix.com> wrote:
> @@ -82,9 +83,16 @@ void pv_inject_event(const struct x86_event *event)
>              error_code |= PFEC_user_mode;
>  
>          trace_pv_page_fault(event->cr2, error_code);
> -    }
> -    else
> +        break;
> +
> +    case TRAP_debug:
> +        curr->arch.dr6 |= event->pending_dbg;

Considering what you've been telling me over and over, shouldn't
you mask out the low four bits here before ORing in new state?

> +        /* Fallthrough */
> +
> +    default:
>          trace_pv_trap(vector, regs->rip, use_error_code, error_code);

I also wonder whether tracing wouldn't benefit from being informed
about pending_dbg here instead of the error code. Not something
for this patch (or series) of course, just as a remark.

Since the equivalent HVM code looks to be missing, I take it that's
going to be addressed in later patches of the series?

> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
> @@ -88,7 +88,10 @@ struct x86_event {
>      uint8_t       type;         /* X86_EVENTTYPE_* */
>      uint8_t       insn_len;     /* Instruction length */
>      int32_t       error_code;   /* X86_EVENT_NO_EC if n/a */
> -    unsigned long cr2;          /* Only for TRAP_page_fault h/w exception */
> +    union {
> +        unsigned long cr2;         /* #PF */
> +        unsigned long pending_dbg; /* #DB (new DR6 bits, positive polarity) */
> +    };
>  };

Seeing that this is the only x86_emulate* change, I don't suppose
you fancy making the emulator correctly raise X86_DR6_BD at the
same time?

Jan



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

Re: [PATCH 07/11] x86/emul: Add pending_dbg field to x86_event

[Andrew Cooper]

On 08/06/18 13:34, Jan Beulich wrote:
>>>> On 04.06.18 at 15:59, <andrew.cooper3@citrix.com> wrote:
>> @@ -82,9 +83,16 @@ void pv_inject_event(const struct x86_event *event)
>>              error_code |= PFEC_user_mode;
>>  
>>          trace_pv_page_fault(event->cr2, error_code);
>> -    }
>> -    else
>> +        break;
>> +
>> +    case TRAP_debug:
>> +        curr->arch.dr6 |= event->pending_dbg;
> Considering what you've been telling me over and over, shouldn't
> you mask out the low four bits here before ORing in new state?

Yes, but that is covered in a later patch (9 to be specific) once the
HVM side is fixed to not update %dr6 twice.

Until all paths have been updated to touch %dr6 exactly once,
implementing the overwriting of the bottom 4 bits will lead to losing
all breakpoint information all the time, which is worse guest behaviour
than having them act as if they were sticky (which works in practice as
the #DB handlers reset %dr6 to 0).

>
>> +        /* Fallthrough */
>> +
>> +    default:
>>          trace_pv_trap(vector, regs->rip, use_error_code, error_code);
> I also wonder whether tracing wouldn't benefit from being informed
> about pending_dbg here instead of the error code. Not something
> for this patch (or series) of course, just as a remark.
>
> Since the equivalent HVM code looks to be missing, I take it that's
> going to be addressed in later patches of the series?

Tracing is a mess, and completely inconsistent.  I'm opting for "no
functional change" until I've got time to rationalise it all.

>
>> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
>> @@ -88,7 +88,10 @@ struct x86_event {
>>      uint8_t       type;         /* X86_EVENTTYPE_* */
>>      uint8_t       insn_len;     /* Instruction length */
>>      int32_t       error_code;   /* X86_EVENT_NO_EC if n/a */
>> -    unsigned long cr2;          /* Only for TRAP_page_fault h/w exception */
>> +    union {
>> +        unsigned long cr2;         /* #PF */
>> +        unsigned long pending_dbg; /* #DB (new DR6 bits, positive polarity) */
>> +    };
>>  };
> Seeing that this is the only x86_emulate* change, I don't suppose
> you fancy making the emulator correctly raise X86_DR6_BD at the
> same time?

Access to %dr registers is only implemented for PV guests at the
moment.  HVM guests don't provide {read,write}_dr() hooks.  Furthermore,
PV guests are disallowed from enabling general detect.

I toyed with supporting GD for PV guests, but it complicates the
hypercall access, and I'm not sure whether it is worth supporting.  The
feature exists for In Circuit Emulators, and PV guests have been fine
without the ability to use it for more than a decade now.

For HVM guests, I do plan to hook up full emulation, once I've
rationalised the state handling logic.

~Andrew

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

[PATCH 08/11] x86/hvm: RFC - PROBABLY BROKEN - Defer all debugging/monitor actions to {svm, vmx}_inject_event()

[Andrew Cooper]

Currently, there is a lot of functionality in the #DB intercepts, and some
repeated functionality in the *_inject_event() logic.

The gdbsx code is implemented at both levels (albeit differently for #BP,
which is presumably due to the fact that the old emulator behaviour used to be
to move %rip forwards for traps), while the monitor behaviour only exists at
the intercept level.

Updating of %dr6 is implemented (buggily) at both levels, but having it at
both levels is problematic to implement correctly.

Rearrange the logic to have nothing interesting at the intercept level, and
everything implemented at the injection level.  Amongst other things, this
means that the monitor subsystem will pick up debug actions from emulated
events.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
CC: Brian Woods <brian.woods@amd.com>
CC: Razvan Cojocaru <rcojocaru@bitdefender.com>
CC: Tamas K Lengyel <tamas@tklengyel.com>

This is RFC because it probably breaks introspection, as injection replies
from the introspection engine will (probably, but I haven't confirmed) trigger
new monitor events.

First of all, monitoring emulated debug events is a change in behaviour,
although IMO it is more of a bugfix than a new feature.  Also, similar changes
will happen to other monitored events as we try to unify the
intercept/emulation paths.

As for the recursive triggering of monitor events, I was considering extending
the monitor infrastructure to have a "in monitor reply" boolean which causes
hvm_monitor_debug() to ignore the recursive request.

Does this plan sound ok, or have I missed something more subtle with monitor
handling?
---
 xen/arch/x86/hvm/svm/svm.c | 127 ++++++++++++++++++++++++---------------------
 xen/arch/x86/hvm/vmx/vmx.c | 102 +++++++++++++++---------------------
 2 files changed, 110 insertions(+), 119 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index c06bd68..df5f9ed 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1437,19 +1437,49 @@ static void svm_inject_event(const struct x86_event *event)
     switch ( _event.vector | -(_event.type == X86_EVENTTYPE_SW_INTERRUPT) )
     {
     case TRAP_debug:
-        if ( regs->eflags & X86_EFLAGS_TF )
-        {
-            __restore_debug_registers(vmcb, curr);
-            vmcb_set_dr6(vmcb, vmcb_get_dr6(vmcb) | DR_STEP);
-        }
+        /*
+         * On AMD hardware, a #DB exception:
+         *  1) Merges new status bits into %dr6
+         *  2) Clears %dr7.gd and MSR_DEBUGCTL.{LBR,BTF}
+         *
+         * Item 1 is done by hardware before a #DB intercepted vmexit, but we
+         * may end up here from emulation so have to repeat it ourselves.
+         * Item 2 is done by hardware when injecting a #DB exception.
+         */
+        __restore_debug_registers(vmcb, curr);
+        vmcb_set_dr6(vmcb, vmcb_get_dr6(vmcb) | event->pending_dbg);
+
         /* fall through */
     case TRAP_int3:
         if ( curr->domain->debugger_attached )
         {
             /* Debug/Int3: Trap to debugger. */
+            if ( _event.vector == TRAP_int3 )
+            {
+                /* N.B. Can't use __update_guest_eip() for risk of recusion. */
+                regs->rip += _event.insn_len;
+                regs->eflags &= ~X86_EFLAGS_RF;
+                curr->arch.gdbsx_vcpu_event = TRAP_int3;
+            }
+
             domain_pause_for_debugger();
             return;
         }
+        else
+        {
+            int rc = hvm_monitor_debug(regs->rip,
+                                       _event.vector == TRAP_debug
+                                       ? HVM_MONITOR_DEBUG_EXCEPTION
+                                       : HVM_MONITOR_SOFTWARE_BREAKPOINT,
+                                       _event.type, _event.insn_len);
+            if ( rc < 0 )
+            {
+                gprintk(XENLOG_ERR, "Monitor debug error %d\n", rc);
+                return svm_crash_or_fault(curr);
+            }
+            if ( rc )
+                return; /* VCPU paused.  Wait for monitor. */
+        }
         break;
 
     case TRAP_page_fault:
@@ -2775,67 +2805,46 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
 
     case VMEXIT_ICEBP:
     case VMEXIT_EXCEPTION_DB:
-        if ( !v->domain->debugger_attached )
+    case VMEXIT_EXCEPTION_BP:
+    {
+        unsigned int vec, type, len, extra;
+
+        switch ( exit_reason )
         {
-            int rc;
-            unsigned int trap_type;
+        case VMEXIT_ICEBP:
+            vec   = TRAP_debug;
+            type  = X86_EVENTTYPE_PRI_SW_EXCEPTION;
+            len   = __get_instruction_length(v, INSTR_ICEBP);
+            extra = 0;
+            break;
 
-            if ( likely(exit_reason != VMEXIT_ICEBP) )
-            {
-                trap_type = X86_EVENTTYPE_HW_EXCEPTION;
-                inst_len = 0;
-            }
-            else
-            {
-                trap_type = X86_EVENTTYPE_PRI_SW_EXCEPTION;
-                inst_len = __get_instruction_length(v, INSTR_ICEBP);
-            }
+        case VMEXIT_EXCEPTION_DB:
+            vec   = TRAP_debug;
+            type  = X86_EVENTTYPE_HW_EXCEPTION;
+            len   = 0;
+            /* #DB - Hardware has already updated %dr6 for us. */
+            extra = vmcb_get_dr6(vmcb) ^ X86_DR6_DEFAULT;
+            break;
 
-            rc = hvm_monitor_debug(regs->rip,
-                                   HVM_MONITOR_DEBUG_EXCEPTION,
-                                   trap_type, inst_len);
-            if ( rc < 0 )
-                goto unexpected_exit_type;
-            if ( !rc )
-                hvm_inject_exception(TRAP_debug,
-                                     trap_type, inst_len, X86_EVENT_NO_EC,
-                                     exit_reason == VMEXIT_ICEBP ? 0 :
-                                     /* #DB - Hardware already updated dr6. */
-                                     vmcb_get_dr6(vmcb) ^ X86_DR6_DEFAULT);
-        }
-        else
-            domain_pause_for_debugger();
-        break;
+        case VMEXIT_EXCEPTION_BP:
+            vec   = TRAP_int3;
+            type  = X86_EVENTTYPE_SW_EXCEPTION;
+            len   = __get_instruction_length(v, INSTR_INT3);
+            extra = 0; /* N/A */
+            break;
 
-    case VMEXIT_EXCEPTION_BP:
-        inst_len = __get_instruction_length(v, INSTR_INT3);
+        default:
+            ASSERT_UNREACHABLE();
+            goto unexpected_exit_type;
+        }
 
-        if ( inst_len == 0 )
-             break;
+        /* __get_instruction_length() failure.  #GP queued up. */
+        if ( type >= X86_EVENTTYPE_SW_INTERRUPT && !len )
+            break;
 
-        if ( v->domain->debugger_attached )
-        {
-            /* AMD Vol2, 15.11: INT3, INTO, BOUND intercepts do not update RIP. */
-            __update_guest_eip(regs, inst_len);
-            current->arch.gdbsx_vcpu_event = TRAP_int3;
-            domain_pause_for_debugger();
-        }
-        else
-        {
-           int rc;
-
-           rc = hvm_monitor_debug(regs->rip,
-                                  HVM_MONITOR_SOFTWARE_BREAKPOINT,
-                                  X86_EVENTTYPE_SW_EXCEPTION,
-                                  inst_len);
-           if ( rc < 0 )
-               goto unexpected_exit_type;
-           if ( !rc )
-               hvm_inject_exception(TRAP_int3,
-                                    X86_EVENTTYPE_SW_EXCEPTION,
-                                    inst_len, X86_EVENT_NO_EC, 0 /* N/A */);
-        }
+        hvm_inject_exception(vec, type, len, X86_EVENT_NO_EC, extra);
         break;
+    }
 
     case VMEXIT_EXCEPTION_NM:
         svm_fpu_dirty_intercept();
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 39c9ddc..f59ef88 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1778,15 +1778,21 @@ static void vmx_inject_event(const struct x86_event *event)
     unsigned long intr_info;
     struct vcpu *curr = current;
     struct x86_event _event = *event;
+    struct cpu_user_regs *regs = guest_cpu_user_regs();
 
     switch ( _event.vector | -(_event.type == X86_EVENTTYPE_SW_INTERRUPT) )
     {
     case TRAP_debug:
-        if ( guest_cpu_user_regs()->eflags & X86_EFLAGS_TF )
-        {
-            __restore_debug_registers(curr);
-            write_debugreg(6, read_debugreg(6) | DR_STEP);
-        }
+        /*
+         * On Intel hardware, a #DB exception:
+         *  1) Merges new status bits into %dr6
+         *  2) Clears %dr7.gd and MSR_DEBUGCTL.LBR
+         *
+         * All actions are left up to the hypervisor to perform.
+         */
+        __restore_debug_registers(curr);
+        write_debugreg(6, read_debugreg(6) | event->pending_dbg);
+
         if ( !nestedhvm_vcpu_in_guestmode(curr) ||
              !nvmx_intercepts_exception(curr, TRAP_debug, _event.error_code) )
         {
@@ -1797,16 +1803,39 @@ static void vmx_inject_event(const struct x86_event *event)
             __vmread(GUEST_IA32_DEBUGCTL, &val);
             __vmwrite(GUEST_IA32_DEBUGCTL, val & ~IA32_DEBUGCTLMSR_LBR);
         }
-        if ( cpu_has_monitor_trap_flag )
-            break;
+
         /* fall through */
     case TRAP_int3:
         if ( curr->domain->debugger_attached )
         {
             /* Debug/Int3: Trap to debugger. */
+            if ( _event.vector == TRAP_int3 )
+            {
+                /* N.B. Can't use __update_guest_eip() for risk of recusion. */
+                regs->rip += _event.insn_len;
+                regs->eflags &= ~X86_EFLAGS_RF;
+                curr->arch.gdbsx_vcpu_event = TRAP_int3;
+            }
+
             domain_pause_for_debugger();
             return;
         }
+        else
+        {
+            int rc = hvm_monitor_debug(regs->rip,
+                                       _event.vector == TRAP_debug
+                                       ? HVM_MONITOR_DEBUG_EXCEPTION
+                                       : HVM_MONITOR_SOFTWARE_BREAKPOINT,
+                                       _event.type, _event.insn_len);
+            if ( rc < 0 )
+            {
+                gprintk(XENLOG_ERR, "Monitor debug error %d\n", rc);
+                domain_crash(curr->domain);
+                return;
+            }
+            if ( rc )
+                return; /* VCPU paused.  Wait for monitor. */
+        }
         break;
 
     case TRAP_page_fault:
@@ -3693,61 +3722,17 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
         switch ( vector )
         {
         case TRAP_debug:
-            /*
-             * Updates DR6 where debugger can peek (See 3B 23.2.1,
-             * Table 23-1, "Exit Qualification for Debug Exceptions").
-             */
             __vmread(EXIT_QUALIFICATION, &exit_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 )
-            {
-                unsigned long insn_len = 0;
-                int rc;
-                unsigned long trap_type = MASK_EXTR(intr_info,
-                                                    INTR_INFO_INTR_TYPE_MASK);
-
-                if ( trap_type >= X86_EVENTTYPE_SW_INTERRUPT )
-                    __vmread(VM_EXIT_INSTRUCTION_LEN, &insn_len);
-
-                rc = hvm_monitor_debug(regs->rip,
-                                       HVM_MONITOR_DEBUG_EXCEPTION,
-                                       trap_type, insn_len);
-
-                if ( rc < 0 )
-                    goto exit_and_crash;
-                if ( !rc )
-                    vmx_propagate_intr(intr_info, exit_qualification);
-            }
-            else
-                domain_pause_for_debugger();
+            vmx_propagate_intr(intr_info, exit_qualification);
             break;
+
         case TRAP_int3:
+        case TRAP_alignment_check:
             HVMTRACE_1D(TRAP, vector);
-            if ( !v->domain->debugger_attached )
-            {
-                unsigned long insn_len;
-                int rc;
-
-                __vmread(VM_EXIT_INSTRUCTION_LEN, &insn_len);
-                rc = hvm_monitor_debug(regs->rip,
-                                       HVM_MONITOR_SOFTWARE_BREAKPOINT,
-                                       X86_EVENTTYPE_SW_EXCEPTION,
-                                       insn_len);
-
-                if ( rc < 0 )
-                    goto exit_and_crash;
-                if ( !rc )
-                    vmx_propagate_intr(intr_info, 0 /* N/A */);
-            }
-            else
-            {
-                update_guest_eip(); /* Safe: INT3 */
-                v->arch.gdbsx_vcpu_event = TRAP_int3;
-                domain_pause_for_debugger();
-            }
+            vmx_propagate_intr(intr_info, 0 /* N/A */);
             break;
+
         case TRAP_no_device:
             HVMTRACE_1D(TRAP, vector);
             vmx_fpu_dirty_intercept();
@@ -3777,10 +3762,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
 
             hvm_inject_page_fault(regs->error_code, exit_qualification);
             break;
-        case TRAP_alignment_check:
-            HVMTRACE_1D(TRAP, vector);
-            vmx_propagate_intr(intr_info, 0 /* N/A */);
-            break;
+
         case TRAP_nmi:
             if ( MASK_EXTR(intr_info, INTR_INFO_INTR_TYPE_MASK) !=
                  X86_EVENTTYPE_NMI )
-- 
2.1.4


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

Re: [PATCH 08/11] x86/hvm: RFC - PROBABLY BROKEN - Defer all debugging/monitor actions to {svm, vmx}_inject_event()

[Razvan Cojocaru]

On 06/04/2018 04:59 PM, Andrew Cooper wrote:
> Currently, there is a lot of functionality in the #DB intercepts, and some
> repeated functionality in the *_inject_event() logic.
> 
> The gdbsx code is implemented at both levels (albeit differently for #BP,
> which is presumably due to the fact that the old emulator behaviour used to be
> to move %rip forwards for traps), while the monitor behaviour only exists at
> the intercept level.
> 
> Updating of %dr6 is implemented (buggily) at both levels, but having it at
> both levels is problematic to implement correctly.
> 
> Rearrange the logic to have nothing interesting at the intercept level, and
> everything implemented at the injection level.  Amongst other things, this
> means that the monitor subsystem will pick up debug actions from emulated
> events.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Jun Nakajima <jun.nakajima@intel.com>
> CC: Kevin Tian <kevin.tian@intel.com>
> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> CC: Brian Woods <brian.woods@amd.com>
> CC: Razvan Cojocaru <rcojocaru@bitdefender.com>
> CC: Tamas K Lengyel <tamas@tklengyel.com>
> 
> This is RFC because it probably breaks introspection, as injection replies
> from the introspection engine will (probably, but I haven't confirmed) trigger
> new monitor events.
> 
> First of all, monitoring emulated debug events is a change in behaviour,
> although IMO it is more of a bugfix than a new feature.  Also, similar changes
> will happen to other monitored events as we try to unify the
> intercept/emulation paths.
> 
> As for the recursive triggering of monitor events, I was considering extending
> the monitor infrastructure to have a "in monitor reply" boolean which causes
> hvm_monitor_debug() to ignore the recursive request.
> 
> Does this plan sound ok, or have I missed something more subtle with monitor
> handling?

The plan does sound OK, but I'm not convinced the problem is real: the
only way an introspection agent can inject something that I'm aware of
is via xc_hvm_inject_trap() (which admittedly we do use).

But calling xc_hvm_inject_trap() does not lead to an immediate
injection. Instead, the information is only recorded, and the action is
taken if possible only in hvm_do_resume() - which gets called only after
a VCPU-pause-causing vm_event has been handled:

 509 void hvm_do_resume(struct vcpu *v)
 510 {
 511     check_wakeup_from_wait();
 512
 513     pt_restore_timer(v);
 514
 515     if ( !handle_hvm_io_completion(v) )
 516         return;
 517
 518     if ( unlikely(v->arch.vm_event) )
 519         hvm_vm_event_do_resume(v);
 520
 521     /* Inject pending hw/sw event */
 522     if ( v->arch.hvm_vcpu.inject_event.vector >= 0 )
 523     {
 524         smp_rmb();
 525
 526         if ( !hvm_event_pending(v) )
 527             hvm_inject_event(&v->arch.hvm_vcpu.inject_event);
 528
 529         v->arch.hvm_vcpu.inject_event.vector = HVM_EVENT_VECTOR_UNSET;
 530     }

An example of this is the breakpoint events test in xen-access.c:

762             case VM_EVENT_REASON_SOFTWARE_BREAKPOINT:
763                 printf("Breakpoint: rip=%016"PRIx64", gfn=%"PRIx64"
(vcpu %d)\n",
764                        req.data.regs.x86.rip,
765                        req.u.software_breakpoint.gfn,
766                        req.vcpu_id);
767
768                 /* Reinject */
769                 rc = xc_hvm_inject_trap(xch, domain_id, req.vcpu_id,
770                                         X86_TRAP_INT3,
771
req.u.software_breakpoint.type, -1,
772
req.u.software_breakpoint.insn_length, 0);
773                 if (rc < 0)
774                 {
775                     ERROR("Error %d injecting breakpoint\n", rc);
776                     interrupted = -1;
777                     continue;
778                 }
779                 break;

I did try to apply your series and test it as much as I can think of
(several times), but for reasons which escape me I found that it doesn't
apply cleanly on either the current staging or master (starting with
patch 6). Maybe something's going wrong in my email client.


Thanks,
Razvan

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

Re: [PATCH 08/11] x86/hvm: RFC - PROBABLY BROKEN - Defer all debugging/monitor actions to {svm, vmx}_inject_event()

[Razvan Cojocaru]

On 06/04/2018 05:53 PM, Razvan Cojocaru wrote:
> On 06/04/2018 04:59 PM, Andrew Cooper wrote:
>> Currently, there is a lot of functionality in the #DB intercepts, and some
>> repeated functionality in the *_inject_event() logic.
>>
>> The gdbsx code is implemented at both levels (albeit differently for #BP,
>> which is presumably due to the fact that the old emulator behaviour used to be
>> to move %rip forwards for traps), while the monitor behaviour only exists at
>> the intercept level.
>>
>> Updating of %dr6 is implemented (buggily) at both levels, but having it at
>> both levels is problematic to implement correctly.
>>
>> Rearrange the logic to have nothing interesting at the intercept level, and
>> everything implemented at the injection level.  Amongst other things, this
>> means that the monitor subsystem will pick up debug actions from emulated
>> events.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Jun Nakajima <jun.nakajima@intel.com>
>> CC: Kevin Tian <kevin.tian@intel.com>
>> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> CC: Brian Woods <brian.woods@amd.com>
>> CC: Razvan Cojocaru <rcojocaru@bitdefender.com>
>> CC: Tamas K Lengyel <tamas@tklengyel.com>
>>
>> This is RFC because it probably breaks introspection, as injection replies
>> from the introspection engine will (probably, but I haven't confirmed) trigger
>> new monitor events.
>>
>> First of all, monitoring emulated debug events is a change in behaviour,
>> although IMO it is more of a bugfix than a new feature.  Also, similar changes
>> will happen to other monitored events as we try to unify the
>> intercept/emulation paths.
>>
>> As for the recursive triggering of monitor events, I was considering extending
>> the monitor infrastructure to have a "in monitor reply" boolean which causes
>> hvm_monitor_debug() to ignore the recursive request.
>>
>> Does this plan sound ok, or have I missed something more subtle with monitor
>> handling?
> 
> The plan does sound OK, but I'm not convinced the problem is real: the
> only way an introspection agent can inject something that I'm aware of
> is via xc_hvm_inject_trap() (which admittedly we do use).
> 
> But calling xc_hvm_inject_trap() does not lead to an immediate
> injection. Instead, the information is only recorded, and the action is
> taken if possible only in hvm_do_resume() - which gets called only after
> a VCPU-pause-causing vm_event has been handled:
> 
>  509 void hvm_do_resume(struct vcpu *v)
>  510 {
>  511     check_wakeup_from_wait();
>  512
>  513     pt_restore_timer(v);
>  514
>  515     if ( !handle_hvm_io_completion(v) )
>  516         return;
>  517
>  518     if ( unlikely(v->arch.vm_event) )
>  519         hvm_vm_event_do_resume(v);
>  520
>  521     /* Inject pending hw/sw event */
>  522     if ( v->arch.hvm_vcpu.inject_event.vector >= 0 )
>  523     {
>  524         smp_rmb();
>  525
>  526         if ( !hvm_event_pending(v) )
>  527             hvm_inject_event(&v->arch.hvm_vcpu.inject_event);
>  528
>  529         v->arch.hvm_vcpu.inject_event.vector = HVM_EVENT_VECTOR_UNSET;
>  530     }
Actually, on further talking with Andrew, he is right - this
hvm_inject_event() call should _not_ trigger a vm_event if it is a
result of calling xc_hvm_inject_trap() - which is the current behaviour.

The "simplest" solution I can think of is to somehow record that this
injection has been caused by xc_hvm_inject_trap() (i.e. deliberately
caused by an external agent).


Thanks,
Razvan

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

Re: [PATCH 08/11] x86/hvm: RFC - PROBABLY BROKEN - Defer all debugging/monitor actions to {svm, vmx}_inject_event()

[Roger Pau Monné]

On Mon, Jun 04, 2018 at 02:59:12PM +0100, Andrew Cooper wrote:
> Currently, there is a lot of functionality in the #DB intercepts, and some
> repeated functionality in the *_inject_event() logic.
> 
> The gdbsx code is implemented at both levels (albeit differently for #BP,
> which is presumably due to the fact that the old emulator behaviour used to be
> to move %rip forwards for traps), while the monitor behaviour only exists at
> the intercept level.
> 
> Updating of %dr6 is implemented (buggily) at both levels, but having it at
> both levels is problematic to implement correctly.
> 
> Rearrange the logic to have nothing interesting at the intercept level, and
> everything implemented at the injection level.  Amongst other things, this
> means that the monitor subsystem will pick up debug actions from emulated
> events.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

> @@ -1797,16 +1803,39 @@ static void vmx_inject_event(const struct x86_event *event)
>              __vmread(GUEST_IA32_DEBUGCTL, &val);
>              __vmwrite(GUEST_IA32_DEBUGCTL, val & ~IA32_DEBUGCTLMSR_LBR);
>          }
> -        if ( cpu_has_monitor_trap_flag )
> -            break;
> +
>          /* fall through */
>      case TRAP_int3:
>          if ( curr->domain->debugger_attached )
>          {
>              /* Debug/Int3: Trap to debugger. */
> +            if ( _event.vector == TRAP_int3 )
> +            {
> +                /* N.B. Can't use __update_guest_eip() for risk of recusion. */
> +                regs->rip += _event.insn_len;
> +                regs->eflags &= ~X86_EFLAGS_RF;
> +                curr->arch.gdbsx_vcpu_event = TRAP_int3;
> +            }
> +
>              domain_pause_for_debugger();
>              return;
>          }
> +        else
> +        {
> +            int rc = hvm_monitor_debug(regs->rip,
> +                                       _event.vector == TRAP_debug
> +                                       ? HVM_MONITOR_DEBUG_EXCEPTION
> +                                       : HVM_MONITOR_SOFTWARE_BREAKPOINT,
> +                                       _event.type, _event.insn_len);
> +            if ( rc < 0 )
> +            {
> +                gprintk(XENLOG_ERR, "Monitor debug error %d\n", rc);
> +                domain_crash(curr->domain);
> +                return;
> +            }
> +            if ( rc )
> +                return; /* VCPU paused.  Wait for monitor. */
> +        }
>          break;

This look fairly similar to the svm_inject_event code, I wonder if
those could be merged somehow (or certain part of it shared).

Roger.

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

Re: [PATCH 08/11] x86/hvm: RFC - PROBABLY BROKEN - Defer all debugging/monitor actions to {svm, vmx}_inject_event()

[Jan Beulich]

>>> On 04.06.18 at 15:59, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1437,19 +1437,49 @@ static void svm_inject_event(const struct x86_event *event)
>      switch ( _event.vector | -(_event.type == X86_EVENTTYPE_SW_INTERRUPT) )
>      {
>      case TRAP_debug:
> -        if ( regs->eflags & X86_EFLAGS_TF )
> -        {
> -            __restore_debug_registers(vmcb, curr);
> -            vmcb_set_dr6(vmcb, vmcb_get_dr6(vmcb) | DR_STEP);
> -        }
> +        /*
> +         * On AMD hardware, a #DB exception:
> +         *  1) Merges new status bits into %dr6
> +         *  2) Clears %dr7.gd and MSR_DEBUGCTL.{LBR,BTF}
> +         *
> +         * Item 1 is done by hardware before a #DB intercepted vmexit, but we
> +         * may end up here from emulation so have to repeat it ourselves.
> +         * Item 2 is done by hardware when injecting a #DB exception.
> +         */
> +        __restore_debug_registers(vmcb, curr);
> +        vmcb_set_dr6(vmcb, vmcb_get_dr6(vmcb) | event->pending_dbg);
> +
>          /* fall through */
>      case TRAP_int3:
>          if ( curr->domain->debugger_attached )
>          {
>              /* Debug/Int3: Trap to debugger. */
> +            if ( _event.vector == TRAP_int3 )
> +            {
> +                /* N.B. Can't use __update_guest_eip() for risk of recusion. */
> +                regs->rip += _event.insn_len;

Not all callers provide a non-zero insn length. Is that a potential
problem here (and in the equivalent VMX code)?

> +                regs->eflags &= ~X86_EFLAGS_RF;
> +                curr->arch.gdbsx_vcpu_event = TRAP_int3;
> +            }
> +
>              domain_pause_for_debugger();
>              return;
>          }
> +        else
> +        {

Considering the "return" above, could you avoid the "else" and
the extra level of indentation here? Same for VMX then again.

> +            int rc = hvm_monitor_debug(regs->rip,

"rc" can surely be declared in slightly wider a scope.

> @@ -2775,67 +2805,46 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
>  
>      case VMEXIT_ICEBP:
>      case VMEXIT_EXCEPTION_DB:
> -        if ( !v->domain->debugger_attached )
> +    case VMEXIT_EXCEPTION_BP:
> +    {
> +        unsigned int vec, type, len, extra;
> +
> +        switch ( exit_reason )
>          {
> -            int rc;
> -            unsigned int trap_type;
> +        case VMEXIT_ICEBP:

I don't understand this structuring of the code: The outer switch()
has the exact same control expression, and there's no fall through
- why the nesting? Move the variable declarations to the beginning
of the outer switch() and drop the inner one? You may not even
need all of the variables if you replicated the little bit of code
currently shared by the three (immediately after the inner switch()).

> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -1778,15 +1778,21 @@ static void vmx_inject_event(const struct x86_event *event)
>      unsigned long intr_info;
>      struct vcpu *curr = current;
>      struct x86_event _event = *event;
> +    struct cpu_user_regs *regs = guest_cpu_user_regs();
>  
>      switch ( _event.vector | -(_event.type == X86_EVENTTYPE_SW_INTERRUPT) )
>      {
>      case TRAP_debug:
> -        if ( guest_cpu_user_regs()->eflags & X86_EFLAGS_TF )
> -        {
> -            __restore_debug_registers(curr);
> -            write_debugreg(6, read_debugreg(6) | DR_STEP);
> -        }
> +        /*
> +         * On Intel hardware, a #DB exception:
> +         *  1) Merges new status bits into %dr6
> +         *  2) Clears %dr7.gd and MSR_DEBUGCTL.LBR
> +         *
> +         * All actions are left up to the hypervisor to perform.
> +         */
> +        __restore_debug_registers(curr);
> +        write_debugreg(6, read_debugreg(6) | event->pending_dbg);

Same as for the PV case in the earlier patch: Don't you need to clear the
low four bits first?

Jan



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

Re: [PATCH 08/11] x86/hvm: RFC - PROBABLY BROKEN - Defer all debugging/monitor actions to {svm, vmx}_inject_event()

[Andrew Cooper]

On 08/06/18 14:00, Jan Beulich wrote:
>>>> On 04.06.18 at 15:59, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -1437,19 +1437,49 @@ static void svm_inject_event(const struct x86_event *event)
>>      switch ( _event.vector | -(_event.type == X86_EVENTTYPE_SW_INTERRUPT) )
>>      {
>>      case TRAP_debug:
>> -        if ( regs->eflags & X86_EFLAGS_TF )
>> -        {
>> -            __restore_debug_registers(vmcb, curr);
>> -            vmcb_set_dr6(vmcb, vmcb_get_dr6(vmcb) | DR_STEP);
>> -        }
>> +        /*
>> +         * On AMD hardware, a #DB exception:
>> +         *  1) Merges new status bits into %dr6
>> +         *  2) Clears %dr7.gd and MSR_DEBUGCTL.{LBR,BTF}
>> +         *
>> +         * Item 1 is done by hardware before a #DB intercepted vmexit, but we
>> +         * may end up here from emulation so have to repeat it ourselves.
>> +         * Item 2 is done by hardware when injecting a #DB exception.
>> +         */
>> +        __restore_debug_registers(vmcb, curr);
>> +        vmcb_set_dr6(vmcb, vmcb_get_dr6(vmcb) | event->pending_dbg);
>> +
>>          /* fall through */
>>      case TRAP_int3:
>>          if ( curr->domain->debugger_attached )
>>          {
>>              /* Debug/Int3: Trap to debugger. */
>> +            if ( _event.vector == TRAP_int3 )
>> +            {
>> +                /* N.B. Can't use __update_guest_eip() for risk of recusion. */
>> +                regs->rip += _event.insn_len;
> Not all callers provide a non-zero insn length. Is that a potential
> problem here (and in the equivalent VMX code)?

TRAP_int3 is strictly a software exception (and/or software interrupt if
started via `int $n`), so should always have a nonzero length.

We should be able to assert this property, and I was considering some
extra checks in the {pv,hvm}_inject_event() logic, although it would
require XEN_DMOP_inject_event gaining more sanity checking than it
currently has (which is probably a good thing).

I'll fold something suitable into v2, which is already quite a bit
longer to help with monitor problem identified here.

>> @@ -2775,67 +2805,46 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
>>  
>>      case VMEXIT_ICEBP:
>>      case VMEXIT_EXCEPTION_DB:
>> -        if ( !v->domain->debugger_attached )
>> +    case VMEXIT_EXCEPTION_BP:
>> +    {
>> +        unsigned int vec, type, len, extra;
>> +
>> +        switch ( exit_reason )
>>          {
>> -            int rc;
>> -            unsigned int trap_type;
>> +        case VMEXIT_ICEBP:
> I don't understand this structuring of the code: The outer switch()
> has the exact same control expression, and there's no fall through
> - why the nesting? Move the variable declarations to the beginning
> of the outer switch() and drop the inner one? You may not even
> need all of the variables if you replicated the little bit of code
> currently shared by the three (immediately after the inner switch()).

Good question - I think this layout is a side effect of me changing my
mind several times during its development.

~Andrew

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

[PATCH 09/11] x86: Fix merging of new status bits into %dr6

[Andrew Cooper]

The current logic used to update %dr6 when injecting #DB is buggy.  The
architectural behaviour is to overwrite B{0..3} (rather than accumulate) and
accumulate all other bits.

Introduce a new merge_dr6() helper, which also takes care of handing RTM
correctly.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
CC: Brian Woods <brian.woods@amd.com>
---
 xen/arch/x86/hvm/svm/svm.c     |  3 ++-
 xen/arch/x86/hvm/vmx/vmx.c     |  3 ++-
 xen/arch/x86/pv/traps.c        |  3 ++-
 xen/include/asm-x86/debugreg.h | 26 +++++++++++++++++++++++++-
 4 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index df5f9ed..b1efa5e 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1447,7 +1447,8 @@ static void svm_inject_event(const struct x86_event *event)
          * Item 2 is done by hardware when injecting a #DB exception.
          */
         __restore_debug_registers(vmcb, curr);
-        vmcb_set_dr6(vmcb, vmcb_get_dr6(vmcb) | event->pending_dbg);
+        vmcb_set_dr6(vmcb, merge_dr6(vmcb_get_dr6(vmcb), event->pending_dbg,
+                                     curr->domain->arch.cpuid->feat.rtm));
 
         /* fall through */
     case TRAP_int3:
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index f59ef88..82ef3aa 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1791,7 +1791,8 @@ static void vmx_inject_event(const struct x86_event *event)
          * All actions are left up to the hypervisor to perform.
          */
         __restore_debug_registers(curr);
-        write_debugreg(6, read_debugreg(6) | event->pending_dbg);
+        write_debugreg(6, merge_dr6(read_debugreg(6), event->pending_dbg,
+                                    curr->domain->arch.cpuid->feat.rtm));
 
         if ( !nestedhvm_vcpu_in_guestmode(curr) ||
              !nvmx_intercepts_exception(curr, TRAP_debug, _event.error_code) )
diff --git a/xen/arch/x86/pv/traps.c b/xen/arch/x86/pv/traps.c
index 7d48d83..c2955ea 100644
--- a/xen/arch/x86/pv/traps.c
+++ b/xen/arch/x86/pv/traps.c
@@ -86,7 +86,8 @@ void pv_inject_event(const struct x86_event *event)
         break;
 
     case TRAP_debug:
-        curr->arch.dr6 |= event->pending_dbg;
+        curr->arch.dr6 = merge_dr6(curr->arch.dr6, event->pending_dbg,
+                                   curr->domain->arch.cpuid->feat.rtm);
         /* Fallthrough */
 
     default:
diff --git a/xen/include/asm-x86/debugreg.h b/xen/include/asm-x86/debugreg.h
index 8df566b..f6b361e 100644
--- a/xen/include/asm-x86/debugreg.h
+++ b/xen/include/asm-x86/debugreg.h
@@ -23,6 +23,12 @@
 #define X86_DR6_BT              (1u << 15)  /* Task switch             */
 #define X86_DR6_RTM             (1u << 16)  /* #DB/#BP in RTM region   */
 
+#define X86_DR6_BP_MASK                                 \
+    (X86_DR6_B0 | X86_DR6_B1 | X86_DR6_B2 | X86_DR6_B3)
+
+#define X86_DR6_KNOWN_MASK                                              \
+    (X86_DR6_BP_MASK | X86_DR6_BD | X86_DR6_BS | X86_DR6_BT | X86_DR6_RTM)
+
 #define DR_TRAP0        (0x1)           /* db0 */
 #define DR_TRAP1        (0x2)           /* db1 */
 #define DR_TRAP2        (0x4)           /* db2 */
@@ -30,7 +36,6 @@
 #define DR_STEP         (0x4000)        /* single-step */
 #define DR_SWITCH       (0x8000)        /* task switch */
 #define DR_NOT_RTM      (0x10000)       /* clear: #BP inside RTM region */
-#define DR_STATUS_RESERVED_ONE  0xffff0ff0ul /* Reserved, read as one */
 
 #define X86_DR6_DEFAULT 0xffff0ff0ul    /* Default %dr6 value. */
 
@@ -103,6 +108,25 @@ static inline unsigned long adjust_dr6_rsvd(unsigned long dr6, bool rtm)
     return dr6;
 }
 
+static inline unsigned long merge_dr6(unsigned long dr6, unsigned long new,
+                                      bool rtm)
+{
+    /* Flip dr6 to have positive polarity. */
+    dr6 ^= X86_DR6_DEFAULT;
+
+    /* Sanity check that only known values are passed in. */
+    ASSERT(!(dr6 & ~X86_DR6_KNOWN_MASK));
+    ASSERT(!(new & ~X86_DR6_KNOWN_MASK));
+
+    /* Breakpoints 0-3 overridden.  BD, BS, BT and RTM accumulate. */
+    dr6 = (dr6 & ~X86_DR6_BP_MASK) | new;
+
+    /* Flip dr6 back to having default polarity. */
+    dr6 ^= X86_DR6_DEFAULT;
+
+    return adjust_dr6_rsvd(dr6, rtm);
+}
+
 static inline unsigned long adjust_dr7_rsvd(unsigned long dr7, bool rtm)
 {
     /*
-- 
2.1.4


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

Re: [PATCH 09/11] x86: Fix merging of new status bits into %dr6

[Roger Pau Monné]

On Mon, Jun 04, 2018 at 02:59:13PM +0100, Andrew Cooper wrote:
> The current logic used to update %dr6 when injecting #DB is buggy.  The
> architectural behaviour is to overwrite B{0..3} (rather than accumulate) and
> accumulate all other bits.
> 
> Introduce a new merge_dr6() helper, which also takes care of handing RTM
> correctly.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

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

Re: [PATCH 09/11] x86: Fix merging of new status bits into %dr6

[Jan Beulich]

>>> On 04.06.18 at 15:59, <andrew.cooper3@citrix.com> wrote:
> The current logic used to update %dr6 when injecting #DB is buggy.  The
> architectural behaviour is to overwrite B{0..3} (rather than accumulate) and
> accumulate all other bits.
> 
> Introduce a new merge_dr6() helper, which also takes care of handing RTM
> correctly.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Ah, here we go. This as well as patch 7:
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan



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

[PATCH 10/11] x86/vmx: Work around VMEntry failure when Single Stepping in an STI shadow

[Andrew Cooper]

See the code comment for the details.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>

Jun/Kevin: This workaround is as suggested by Gil, and there is expected to be
an SDM update discussing the corner case.

Note that, like elsewhere dealing with eflags.tf, this is probably buggy in
combination with MSR_DEBUGCTL.BTF.  I'll untangle the BTF swamp at some later
point.
---
 xen/arch/x86/hvm/vmx/vmx.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 82ef3aa..58ff8c7 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1794,6 +1794,36 @@ static void vmx_inject_event(const struct x86_event *event)
         write_debugreg(6, merge_dr6(read_debugreg(6), event->pending_dbg,
                                     curr->domain->arch.cpuid->feat.rtm));
 
+        /*
+         * Work around SS/STI vmentry bug.
+         *
+         * If kernel code is single stepping itself and executes an STI
+         * instruction resulting in an STI shadow, a vmexit occurs due to #DB
+         * interception, but the vmentry fails due to a failed consistency
+         * check.  (Hardware comes to the conclusion that there should be a
+         * pending debug exception, but doesn't account for the pending #DB in
+         * VMENTRY_INTR_INFO.)
+         *
+         * Manually adjust the pending debug exception field to mark BS as
+         * pending, which satisfies the consistency check and allows the
+         * vmentry to succeed.
+         */
+        if ( unlikely(regs->eflags & X86_EFLAGS_TF) )
+        {
+            unsigned long int_info;
+
+            __vmread(GUEST_INTERRUPTIBILITY_INFO, &int_info);
+
+            if ( int_info & VMX_INTR_SHADOW_STI )
+            {
+                unsigned long pending_dbg;
+
+                __vmread(GUEST_PENDING_DBG_EXCEPTIONS, &pending_dbg);
+                __vmwrite(GUEST_PENDING_DBG_EXCEPTIONS,
+                          pending_dbg | X86_DR6_BS);
+            }
+        }
+
         if ( !nestedhvm_vcpu_in_guestmode(curr) ||
              !nvmx_intercepts_exception(curr, TRAP_debug, _event.error_code) )
         {
-- 
2.1.4


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

Ping VT-x: [PATCH 10/11] x86/vmx: Work around VMEntry failure when Single Stepping in an STI shadow

[Andrew Cooper]

On 04/06/18 14:59, Andrew Cooper wrote:
> See the code comment for the details.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Jun Nakajima <jun.nakajima@intel.com>
> CC: Kevin Tian <kevin.tian@intel.com>
>
> Jun/Kevin: This workaround is as suggested by Gil, and there is expected to be
> an SDM update discussing the corner case.
>
> Note that, like elsewhere dealing with eflags.tf, this is probably buggy in
> combination with MSR_DEBUGCTL.BTF.  I'll untangle the BTF swamp at some later
> point.
> ---
>  xen/arch/x86/hvm/vmx/vmx.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 82ef3aa..58ff8c7 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -1794,6 +1794,36 @@ static void vmx_inject_event(const struct x86_event *event)
>          write_debugreg(6, merge_dr6(read_debugreg(6), event->pending_dbg,
>                                      curr->domain->arch.cpuid->feat.rtm));
>  
> +        /*
> +         * Work around SS/STI vmentry bug.
> +         *
> +         * If kernel code is single stepping itself and executes an STI
> +         * instruction resulting in an STI shadow, a vmexit occurs due to #DB
> +         * interception, but the vmentry fails due to a failed consistency
> +         * check.  (Hardware comes to the conclusion that there should be a
> +         * pending debug exception, but doesn't account for the pending #DB in
> +         * VMENTRY_INTR_INFO.)
> +         *
> +         * Manually adjust the pending debug exception field to mark BS as
> +         * pending, which satisfies the consistency check and allows the
> +         * vmentry to succeed.
> +         */
> +        if ( unlikely(regs->eflags & X86_EFLAGS_TF) )
> +        {
> +            unsigned long int_info;
> +
> +            __vmread(GUEST_INTERRUPTIBILITY_INFO, &int_info);
> +
> +            if ( int_info & VMX_INTR_SHADOW_STI )
> +            {
> +                unsigned long pending_dbg;
> +
> +                __vmread(GUEST_PENDING_DBG_EXCEPTIONS, &pending_dbg);
> +                __vmwrite(GUEST_PENDING_DBG_EXCEPTIONS,
> +                          pending_dbg | X86_DR6_BS);
> +            }
> +        }
> +
>          if ( !nestedhvm_vcpu_in_guestmode(curr) ||
>               !nvmx_intercepts_exception(curr, TRAP_debug, _event.error_code) )
>          {


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

Re: Ping VT-x: [PATCH 10/11] x86/vmx: Work around VMEntry failure when Single Stepping in an STI shadow

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Monday, September 3, 2018 6:40 PM
> 
> On 04/06/18 14:59, Andrew Cooper wrote:
> > See the code comment for the details.
> >
> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 11/11] x86/dbg: Cleanup of legacy dr6 constants

[Andrew Cooper]

Replace the few remaining uses with X86_DR6_* constants.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/pv/emul-priv-op.c |  2 +-
 xen/arch/x86/traps.c           |  2 +-
 xen/include/asm-x86/debugreg.h | 17 -----------------
 3 files changed, 2 insertions(+), 19 deletions(-)

diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
index 45788b2..bec6ee9 100644
--- a/xen/arch/x86/pv/emul-priv-op.c
+++ b/xen/arch/x86/pv/emul-priv-op.c
@@ -1365,7 +1365,7 @@ int pv_emulate_privileged_op(struct cpu_user_regs *regs)
         }
 
         if ( ctxt.ctxt.retire.singlestep )
-            ctxt.bpmatch |= DR_STEP;
+            ctxt.bpmatch |= X86_DR6_BS;
 
         if ( ctxt.bpmatch &&
              !(curr->arch.pv_vcpu.trap_bounce.flags & TBF_EXCEPTION) )
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 8ef22b4..362e209 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1830,7 +1830,7 @@ void do_debug(struct cpu_user_regs *regs)
             fatal_trap(regs, 0);
         }
 
-        if ( dr6 & (DR_TRAP3 | DR_TRAP2 | DR_TRAP1 | DR_TRAP0) )
+        if ( dr6 & X86_DR6_BP_MASK )
         {
             unsigned int bp, dr7 = read_debugreg(7) >> DR_CONTROL_SHIFT;
 
diff --git a/xen/include/asm-x86/debugreg.h b/xen/include/asm-x86/debugreg.h
index f6b361e..587ed9d 100644
--- a/xen/include/asm-x86/debugreg.h
+++ b/xen/include/asm-x86/debugreg.h
@@ -1,15 +1,6 @@
 #ifndef _X86_DEBUGREG_H
 #define _X86_DEBUGREG_H
 
-
-/* Indicate the register numbers for a number of the specific
-   debug registers.  Registers 0-3 contain the addresses we wish to trap on */
-
-#define DR_FIRSTADDR 0
-#define DR_LASTADDR  3
-#define DR_STATUS    6
-#define DR_CONTROL   7
-
 /*
  * DR6 status bits.
  *   N.B. For backwards compatibility, X86_DR6_RTM has inverted polarity.
@@ -29,14 +20,6 @@
 #define X86_DR6_KNOWN_MASK                                              \
     (X86_DR6_BP_MASK | X86_DR6_BD | X86_DR6_BS | X86_DR6_BT | X86_DR6_RTM)
 
-#define DR_TRAP0        (0x1)           /* db0 */
-#define DR_TRAP1        (0x2)           /* db1 */
-#define DR_TRAP2        (0x4)           /* db2 */
-#define DR_TRAP3        (0x8)           /* db3 */
-#define DR_STEP         (0x4000)        /* single-step */
-#define DR_SWITCH       (0x8000)        /* task switch */
-#define DR_NOT_RTM      (0x10000)       /* clear: #BP inside RTM region */
-
 #define X86_DR6_DEFAULT 0xffff0ff0ul    /* Default %dr6 value. */
 
 /* Now define a bunch of things for manipulating the control register.
-- 
2.1.4


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

Re: [PATCH 11/11] x86/dbg: Cleanup of legacy dr6 constants

[Roger Pau Monné]

On Mon, Jun 04, 2018 at 02:59:15PM +0100, Andrew Cooper wrote:
> Replace the few remaining uses with X86_DR6_* constants.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

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

Re: [PATCH 11/11] x86/dbg: Cleanup of legacy dr6 constants

[Jan Beulich]

>>> On 04.06.18 at 15:59, <andrew.cooper3@citrix.com> wrote:
> Replace the few remaining uses with X86_DR6_* constants.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>



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

Re: [PATCH 00/11] Fixes to debugging facilities

[Andrew Cooper]

On 04/06/18 14:59, Andrew Cooper wrote:
> So this started as a small fix for the vmentry failure (penultimate patch),
> and has snowballed...
>
> I'm fairly confident that everything involving DEBUGCTL.BTF is broken, and
> there are definitely bugs with configuring DEBUGCTL.RTM (which really isn't
> helped by the fact that the GCC TSX intrinsics render the resulting code
> un-debuggable.)  I'll defer fixing these swamps for now.
>
> The first 4 patches probably want backporting to the stable trees, so I've
> taken care to move them ahead of patch 6 for backport reasons.  While all
> fixes would ideally be backported, I can't find a way of fixing %dr6 merging
> (as it needs to be done precicely once) without a behavioural change in the
> monitor subsystem.
>
> Patch 8 probably breaks introspection, so can't be taken at this point.  See
> that patch for discussion of the problem and my best guess at a solution.

As spotted by Razvan, I forgot to mention that this series is built on
top of "x86/vmx: Fix handing of MSR_DEBUGCTL on VMExit".  It can be
found in git form here:

http://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=shortlog;h=refs/heads/debug-fixes-v1

~Andrew

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

Re: [PATCH 00/11] Fixes to debugging facilities

[Razvan Cojocaru]

On 06/04/2018 06:39 PM, Andrew Cooper wrote:
> On 04/06/18 14:59, Andrew Cooper wrote:
>> So this started as a small fix for the vmentry failure (penultimate patch),
>> and has snowballed...
>>
>> I'm fairly confident that everything involving DEBUGCTL.BTF is broken, and
>> there are definitely bugs with configuring DEBUGCTL.RTM (which really isn't
>> helped by the fact that the GCC TSX intrinsics render the resulting code
>> un-debuggable.)  I'll defer fixing these swamps for now.
>>
>> The first 4 patches probably want backporting to the stable trees, so I've
>> taken care to move them ahead of patch 6 for backport reasons.  While all
>> fixes would ideally be backported, I can't find a way of fixing %dr6 merging
>> (as it needs to be done precicely once) without a behavioural change in the
>> monitor subsystem.
>>
>> Patch 8 probably breaks introspection, so can't be taken at this point.  See
>> that patch for discussion of the problem and my best guess at a solution.
> 
> As spotted by Razvan, I forgot to mention that this series is built on
> top of "x86/vmx: Fix handing of MSR_DEBUGCTL on VMExit".  It can be
> found in git form here:
> 
> http://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=shortlog;h=refs/heads/debug-fixes-v1

FWIW, you're exactly right about the recursive injection vm_events. I've
tested this with xen-access and the test-hvm64-swint-emulation XTF test:

1. xl create xl create -p ./test-hvm64-swint-emulation.cfg
2. xen-access <DOMID> breakpoint
3. xl unpause <DOMID>

The test domain will not be able to finish until xen-access is stopped
(with ^C).

So this does indeed break introspection the way it is now implemented.

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

Re: [PATCH 00/11] Fixes to debugging facilities

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 2215 bytes --]

On 04/06/18 18:09, Razvan Cojocaru wrote:
> On 06/04/2018 06:39 PM, Andrew Cooper wrote:
>> On 04/06/18 14:59, Andrew Cooper wrote:
>>> So this started as a small fix for the vmentry failure (penultimate patch),
>>> and has snowballed...
>>>
>>> I'm fairly confident that everything involving DEBUGCTL.BTF is broken, and
>>> there are definitely bugs with configuring DEBUGCTL.RTM (which really isn't
>>> helped by the fact that the GCC TSX intrinsics render the resulting code
>>> un-debuggable.)  I'll defer fixing these swamps for now.
>>>
>>> The first 4 patches probably want backporting to the stable trees, so I've
>>> taken care to move them ahead of patch 6 for backport reasons.  While all
>>> fixes would ideally be backported, I can't find a way of fixing %dr6 merging
>>> (as it needs to be done precicely once) without a behavioural change in the
>>> monitor subsystem.
>>>
>>> Patch 8 probably breaks introspection, so can't be taken at this point.  See
>>> that patch for discussion of the problem and my best guess at a solution.
>> As spotted by Razvan, I forgot to mention that this series is built on
>> top of "x86/vmx: Fix handing of MSR_DEBUGCTL on VMExit".  It can be
>> found in git form here:
>>
>> http://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=shortlog;h=refs/heads/debug-fixes-v1
> FWIW, you're exactly right about the recursive injection vm_events. I've
> tested this with xen-access and the test-hvm64-swint-emulation XTF test:
>
> 1. xl create xl create -p ./test-hvm64-swint-emulation.cfg
> 2. xen-access <DOMID> breakpoint
> 3. xl unpause <DOMID>
>
> The test domain will not be able to finish until xen-access is stopped
> (with ^C).
>
> So this does indeed break introspection the way it is now implemented.

Ack.  I'm attempting to implement the "performing agent-caused action"
boolean as discussed.

Another issue I've encountered is that the changes to #DB injection
require that pending_dbg gets sent to the introspection agent so it can
be fed back suitably in xc_hvm_inject_trap().  OTOH, this does mean that
in principle, introspection of debug exceptions could become selective
on the exact source if that is a feature anyone is interested in.

~Andrew

[-- Attachment #1.2: Type: text/html, Size: 2979 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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