[v2,0/7] x86/pv: #DB vs %dr6 fixes, part 2

Message

Andrew Cooper Sept. 15, 2023, 8:36 p.m. UTC

This time with a bit of sanity testing.  See patches for details.

Andrew Cooper (7):
  x86/emul: ASSERT that X86EMUL_DONE doesn't escape to callers
  x86/emul: Fix and extend #DB trap handling
  x86/pv: Fix the determiniation of whether to inject #DB
  x86/pv: Drop priv_op_ctxt.bpmatch and use pending_dbg instead
  x86: Introduce x86_merge_dr6()
  x86: Extend x86_event with a pending_dbg field
  x86/pv: Rewrite %dr6 handling

 xen/arch/x86/debug.c                   | 20 +++++++++++++++++
 xen/arch/x86/include/asm/debugreg.h    |  7 ++++++
 xen/arch/x86/include/asm/domain.h      | 18 ++++++++++++++--
 xen/arch/x86/include/asm/hvm/hvm.h     |  3 ++-
 xen/arch/x86/include/asm/x86-defns.h   |  7 ++++++
 xen/arch/x86/pv/emul-priv-op.c         | 30 +++++++++++++-------------
 xen/arch/x86/pv/emulate.c              |  9 ++++++--
 xen/arch/x86/pv/ro-page-fault.c        |  4 ++--
 xen/arch/x86/pv/traps.c                | 17 +++++++++++----
 xen/arch/x86/traps.c                   | 23 ++++++++++----------
 xen/arch/x86/x86_emulate/x86_emulate.c | 26 ++++++++++++++++------
 xen/arch/x86/x86_emulate/x86_emulate.h | 19 ++++++++++++----
 12 files changed, 134 insertions(+), 49 deletions(-)

Comments

Andrew Cooper Sept. 19, 2023, 8:53 p.m. UTC | #1

On 15/09/2023 9:36 pm, Andrew Cooper wrote:
> This time with a bit of sanity testing.  See patches for details.
>
> Andrew Cooper (7):
>   x86/emul: ASSERT that X86EMUL_DONE doesn't escape to callers
>   x86/emul: Fix and extend #DB trap handling
>   x86/pv: Fix the determiniation of whether to inject #DB
>   x86/pv: Drop priv_op_ctxt.bpmatch and use pending_dbg instead
>   x86: Introduce x86_merge_dr6()
>   x86: Extend x86_event with a pending_dbg field
>   x86/pv: Rewrite %dr6 handling

I've found even more PV debug bugs.

1) The MSR leak fix stopped MSR_DEBUGCTL leaking into PV guests, and
even "broke" my XTF test (still not blocking in CI because of bisector
interaction issues).  Really, this was one buggy case swapped for
another, but it needs resolving nevertheless.

2) activate_debugregs() has a misleading dr7 check in it (the caller is
gated on the same condition, making it tautological).  Worse however, it
loads %dr6 which interferes with with the #DB handler trying to get a
clean view of "new bits".  PV guests can't access %dr6 at all, so I'm
reasonably sure it's state we simply don't need to load at all.

3) The comment in paravirt_ctxt_switch_from() about debug regs is
buggy.  The logic is correct, but the reasoning is false.

4) set_debugreg() doesn't account for the dr7 state before loading
dr{0..3} in current context.  This manifests as spuriously loading
breakpoint registers despite not having debugging "active".  I think
it's benign.

5) The order of operations in activate_debugregs() is wrong.  There's a
period of time where we've got the new vCPU's breakpoints active using
the old vCPU's mask registers.  Again, I think it's benign.

~Andrew