On 24.08.2023 17:26, Jinoh Kang wrote:
> From: Andrew Cooper <andrew.coop...@citrix.com>
>
> 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.coop...@citrix.com>
> Reviewed-by: Roger Pau Monné <roger....@citrix.com>
> Reviewed-by: Jan Beulich <jbeul...@suse.com>
> [ jinoh: Rebase onto staging, forward declare struct vcpu ]
> Signed-off-by: Jinoh Kang <jinoh.kang...@gmail.com>
> ---
> CC: Andrew Cooper <andrew.coop...@citrix.com>
> CC: Jan Beulich <jbeul...@suse.com>
> CC: Wei Liu <w...@xen.org>
> CC: Roger Pau Monné <roger....@citrix.com>
> CC: Jun Nakajima <jun.nakaj...@intel.com>
> CC: Kevin Tian <kevin.t...@intel.com>
> CC: Tim Deegan <t...@xen.org>
> CC: Tamas K Lengyel <ta...@tklengyel.com>
> CC: Alexandru Isaila <aisa...@bitdefender.com>
> CC: Petre Pircalabu <ppircal...@bitdefender.com>
>
> v1 -> v2: [S-o-b fixes. More details below.]
>
> - Update DR6 for gdbsx when trapped in PV guest kernel mode
I take it that this refers to ...
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -1989,17 +1989,17 @@ 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);
> -
> if ( guest_kernel_mode(v, regs) && v->domain->debugger_attached )
> {
> + /* Save debug status register where gdbsx can peek at it */
> + v->arch.dr6 |= (dr6 & ~X86_DR6_DEFAULT);
> + v->arch.dr6 &= (dr6 | ~X86_DR6_DEFAULT);
> +
> domain_pause_for_debugger();
> return;
> }
... this code movement. I'm afraid this should have resulted in you
dropping the earlier R-b, and I'm further afraid I'm not convinced
this is correct, despite seeing why you would want to do this. The
issue is that this way you also alter guest-visible state, when the
intention is that such now happen via ...
> - pv_inject_hw_exception(X86_EXC_DB, X86_EVENT_NO_EC);
> + pv_inject_debug_exn(dr6 ^ X86_DR6_DEFAULT);
> }
... this call alone. I fear I can't currently see how to get both
aspects right, other than by breaking up
Jan
