Xen 4.18.5 PV dbregs fail

Xen 4.18.5 PV dbregs fail

[Manuel Bouyer]

Hello,
since I updated to Xen 4.18.5 (from 4.18.4), NetBSD's dbregs-related tests
are failing. Only for PV; PVH and HVM guests are fine. They are
failing for both 32bits and 64bits guests.

I tracked it down to dr6 being 0xffff0ff0 after the trace trap, when at
last one of the lower bits should be 1 (I think it's bit 0, from the code).

I looked at the commit log between 4.18.4 and 4.18.5 but didn't see
anything obvious.

Any idea ?

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Re: Xen 4.18.5 PV dbregs fail

[Andrew Cooper]

On 26/05/2025 6:59 pm, Manuel Bouyer wrote:
> Hello,
> since I updated to Xen 4.18.5 (from 4.18.4), NetBSD's dbregs-related tests
> are failing. Only for PV; PVH and HVM guests are fine. They are
> failing for both 32bits and 64bits guests.
>
> I tracked it down to dr6 being 0xffff0ff0 after the trace trap, when at
> last one of the lower bits should be 1 (I think it's bit 0, from the code).
>
> I looked at the commit log between 4.18.4 and 4.18.5 but didn't see
> anything obvious.
>
> Any idea ?
>

Have you got a link to the test in question?

We've had a couple of bugfixes in this area, although debug handling
isn't helped by the fact that both the SDM and APM are factually
incorrect on how to write a #DB handler (and the vendors are moving at a
glacial pace to correct them).

~Andrew

Re: Xen 4.18.5 PV dbregs fail

[Andrew Cooper]

On 26/05/2025 7:06 pm, Andrew Cooper wrote:
> On 26/05/2025 6:59 pm, Manuel Bouyer wrote:
>> Hello,
>> since I updated to Xen 4.18.5 (from 4.18.4), NetBSD's dbregs-related tests
>> are failing. Only for PV; PVH and HVM guests are fine. They are
>> failing for both 32bits and 64bits guests.
>>
>> I tracked it down to dr6 being 0xffff0ff0 after the trace trap, when at
>> last one of the lower bits should be 1 (I think it's bit 0, from the code).
>>
>> I looked at the commit log between 4.18.4 and 4.18.5 but didn't see
>> anything obvious.
>>
>> Any idea ?
>>
> Have you got a link to the test in question?
>
> We've had a couple of bugfixes in this area, although debug handling
> isn't helped by the fact that both the SDM and APM are factually
> incorrect on how to write a #DB handler (and the vendors are moving at a
> glacial pace to correct them).

But yes, having looked between 4.18.4 and 4.18.5, I can't see anything
relevant to debug handling either.

Judging from your description, it looks like a breakpoint is going missing.

The relevant recent change for that is
https://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=54ef601a66e8d812a6a6a308f02524e81201825e
although it's a bit older than 4.18.4.

I suppose it's possible that we call x86_merge_dr6() more than once when
forwarding #DB to the guest, which might lose the breakpoint bits?

~Andrew

Re: Xen 4.18.5 PV dbregs fail

[Manuel Bouyer]

On Mon, May 26, 2025 at 07:06:04PM +0100, Andrew Cooper wrote:
> On 26/05/2025 6:59 pm, Manuel Bouyer wrote:
> > Hello,
> > since I updated to Xen 4.18.5 (from 4.18.4), NetBSD's dbregs-related tests
> > are failing. Only for PV; PVH and HVM guests are fine. They are
> > failing for both 32bits and 64bits guests.
> >
> > I tracked it down to dr6 being 0xffff0ff0 after the trace trap, when at
> > last one of the lower bits should be 1 (I think it's bit 0, from the code).
> >
> > I looked at the commit log between 4.18.4 and 4.18.5 but didn't see
> > anything obvious.
> >
> > Any idea ?
> >
> 
> Have you got a link to the test in question?

For example, dbregs_dr0_trap_code in 
https://cvsweb.netbsd.org/bsdweb.cgi/src/tests/lib/libc/sys/t_ptrace_x86_wait.h?rev=1.33;content-type=text%2Fplain

What's failing is
	ATF_REQUIRE_EQ(info.psi_siginfo.si_code, TRAP_DBREG);

I added printfs in the kernel to show the debug registers when
the process traps, this is where the 0xffff0ff0 value comes from.

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Re: Xen 4.18.5 PV dbregs fail

[Manuel Bouyer]

On Mon, May 26, 2025 at 07:17:13PM +0100, Andrew Cooper wrote:
> On 26/05/2025 7:06 pm, Andrew Cooper wrote:
> > On 26/05/2025 6:59 pm, Manuel Bouyer wrote:
> >> Hello,
> >> since I updated to Xen 4.18.5 (from 4.18.4), NetBSD's dbregs-related tests
> >> are failing. Only for PV; PVH and HVM guests are fine. They are
> >> failing for both 32bits and 64bits guests.
> >>
> >> I tracked it down to dr6 being 0xffff0ff0 after the trace trap, when at
> >> last one of the lower bits should be 1 (I think it's bit 0, from the code).
> >>
> >> I looked at the commit log between 4.18.4 and 4.18.5 but didn't see
> >> anything obvious.
> >>
> >> Any idea ?
> >>
> > Have you got a link to the test in question?
> >
> > We've had a couple of bugfixes in this area, although debug handling
> > isn't helped by the fact that both the SDM and APM are factually
> > incorrect on how to write a #DB handler (and the vendors are moving at a
> > glacial pace to correct them).
> 
> But yes, having looked between 4.18.4 and 4.18.5, I can't see anything
> relevant to debug handling either.
> 
> Judging from your description, it looks like a breakpoint is going missing.
> 
> The relevant recent change for that is
> https://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=54ef601a66e8d812a6a6a308f02524e81201825e
> although it's a bit older than 4.18.4.

Well, my previous xen-debug.gz kernel is from Oct, 3 so it can't be
4.18.4, it's 4.18.3_20240909 which points to commit
bd51e573a730efc569646379cd59ccba967cde97

looks like I forgot to update this server. So I need to looks deeper in
the commit logs

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Re: Xen 4.18.5 PV dbregs fail

[Andrew Cooper]

On 26/05/2025 7:46 pm, Manuel Bouyer wrote:
> On Mon, May 26, 2025 at 07:17:13PM +0100, Andrew Cooper wrote:
>> On 26/05/2025 7:06 pm, Andrew Cooper wrote:
>>> On 26/05/2025 6:59 pm, Manuel Bouyer wrote:
>>>> Hello,
>>>> since I updated to Xen 4.18.5 (from 4.18.4), NetBSD's dbregs-related tests
>>>> are failing. Only for PV; PVH and HVM guests are fine. They are
>>>> failing for both 32bits and 64bits guests.
>>>>
>>>> I tracked it down to dr6 being 0xffff0ff0 after the trace trap, when at
>>>> last one of the lower bits should be 1 (I think it's bit 0, from the code).
>>>>
>>>> I looked at the commit log between 4.18.4 and 4.18.5 but didn't see
>>>> anything obvious.
>>>>
>>>> Any idea ?
>>>>
>>> Have you got a link to the test in question?
>>>
>>> We've had a couple of bugfixes in this area, although debug handling
>>> isn't helped by the fact that both the SDM and APM are factually
>>> incorrect on how to write a #DB handler (and the vendors are moving at a
>>> glacial pace to correct them).
>> But yes, having looked between 4.18.4 and 4.18.5, I can't see anything
>> relevant to debug handling either.
>>
>> Judging from your description, it looks like a breakpoint is going missing.
>>
>> The relevant recent change for that is
>> https://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=54ef601a66e8d812a6a6a308f02524e81201825e
>> although it's a bit older than 4.18.4.
> Well, my previous xen-debug.gz kernel is from Oct, 3 so it can't be
> 4.18.4, it's 4.18.3_20240909 which points to commit
> bd51e573a730efc569646379cd59ccba967cde97
>
> looks like I forgot to update this server. So I need to looks deeper in
> the commit logs

Well, that range does include the aforementioned commit.

Can you bisect around d32c77f471fb8400b6512c171a14cdd58f04f0a3 which is
the backport of ^ in 4.18 ?

~Andrew

Re: Xen 4.18.5 PV dbregs fail

[Manuel Bouyer]

On Mon, May 26, 2025 at 07:50:14PM +0100, Andrew Cooper wrote:
> [...]
> Well, that range does include the aforementioned commit.
> 
> Can you bisect around d32c77f471fb8400b6512c171a14cdd58f04f0a3 which is
> the backport of ^ in 4.18 ?

Sure,
with 0d5f15e and d32c77f the test pass, with cecee35 it fails.

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Re: Xen 4.18.5 PV dbregs fail

[Andrew Cooper]

On 26/05/2025 9:13 pm, Manuel Bouyer wrote:
> On Mon, May 26, 2025 at 07:50:14PM +0100, Andrew Cooper wrote:
>> [...]
>> Well, that range does include the aforementioned commit.
>>
>> Can you bisect around d32c77f471fb8400b6512c171a14cdd58f04f0a3 which is
>> the backport of ^ in 4.18 ?
> Sure,
> with 0d5f15e and d32c77f the test pass, with cecee35 it fails.
>

Oh interesting, so the basic forwarding of #DB back into a guest
(d32c77f) works fine, but the changes to emulated debug exceptions
(cecee35) break.

Anyway, I think I've spotted a logical error.  We do indeed end up
calling x86_merge_dr6() twice, because of the TODO just out of context. 
Does this help?

~Andrew


diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 6d75b59b1e97..01b8be02b055 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1798,7 +1798,7 @@ void asmlinkage do_debug(struct cpu_user_regs *regs)
         return;
     }
 
-    pv_inject_DB(0 /* N/A, already merged */);
+    pv_inject_DB(dr6 ^ X86_DR6_DEFAULT);
 }
 
 void asmlinkage do_entry_CP(struct cpu_user_regs *regs)

[PATCH] x86/pv: Fix breakpoint reporting

[Andrew Cooper]

x86_merge_dr6() is not a no-op when 0 is passed in; it will discard the
previously latched breakpoint bits.

The combination of do_debug()'s manual call to x86_merge_dr6() for external
debuggers, and pv_inject_DB() calling pv_inject_event(), results in two
x86_merge_dr6() calls.

Feed the same pending_dbg in the second time.  This makes pv_inject_event()'s
update of dr6 effectively a no-op, retaining the correct breakpoint bits.

Fixes: db39fa4b27ea ("x86/pv: Fix merging of new status bits into %dr6")
Reported-by: Manuel Bouyer <bouyer@antioche.eu.org>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Manuel Bouyer <bouyer@antioche.eu.org>

Now to figure out why my testing didn't spot this...
---
 xen/arch/x86/traps.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 22f20629327d..e7b4693415cd 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1823,7 +1823,7 @@ void asmlinkage do_debug(struct cpu_user_regs *regs)
         return;
     }
 
-    pv_inject_DB(0 /* N/A, already merged */);
+    pv_inject_DB(dr6 ^ X86_DR6_DEFAULT);
 }
 
 void asmlinkage do_entry_CP(struct cpu_user_regs *regs)

base-commit: 6cfe3ae346fc84fbd4380fc45c70780935da590a
-- 
2.39.5

Re: Xen 4.18.5 PV dbregs fail

[Manuel Bouyer]

On Mon, May 26, 2025 at 09:30:53PM +0100, Andrew Cooper wrote:
> On 26/05/2025 9:13 pm, Manuel Bouyer wrote:
> > On Mon, May 26, 2025 at 07:50:14PM +0100, Andrew Cooper wrote:
> >> [...]
> >> Well, that range does include the aforementioned commit.
> >>
> >> Can you bisect around d32c77f471fb8400b6512c171a14cdd58f04f0a3 which is
> >> the backport of ^ in 4.18 ?
> > Sure,
> > with 0d5f15e and d32c77f the test pass, with cecee35 it fails.
> >
> 
> Oh interesting, so the basic forwarding of #DB back into a guest
> (d32c77f) works fine, but the changes to emulated debug exceptions
> (cecee35) break.
> 
> Anyway, I think I've spotted a logical error.  We do indeed end up
> calling x86_merge_dr6() twice, because of the TODO just out of context. 
> Does this help?

Yes, it works for cecee35; now testing with 4.18.5

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Re: Xen 4.18.5 PV dbregs fail

[Manuel Bouyer]

On Mon, May 26, 2025 at 10:56:41PM +0200, Manuel Bouyer wrote:
> On Mon, May 26, 2025 at 09:30:53PM +0100, Andrew Cooper wrote:
> > On 26/05/2025 9:13 pm, Manuel Bouyer wrote:
> > > On Mon, May 26, 2025 at 07:50:14PM +0100, Andrew Cooper wrote:
> > >> [...]
> > >> Well, that range does include the aforementioned commit.
> > >>
> > >> Can you bisect around d32c77f471fb8400b6512c171a14cdd58f04f0a3 which is
> > >> the backport of ^ in 4.18 ?
> > > Sure,
> > > with 0d5f15e and d32c77f the test pass, with cecee35 it fails.
> > >
> > 
> > Oh interesting, so the basic forwarding of #DB back into a guest
> > (d32c77f) works fine, but the changes to emulated debug exceptions
> > (cecee35) break.
> > 
> > Anyway, I think I've spotted a logical error.  We do indeed end up
> > calling x86_merge_dr6() twice, because of the TODO just out of context. 
> > Does this help?
> 
> Yes, it works for cecee35; now testing with 4.18.5

yes, it works with 4.18.5 too. All dbreg-related tests now pass again
thanks !

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Re: Xen 4.18.5 PV dbregs fail

[Andrew Cooper]

On 26/05/2025 10:10 pm, Manuel Bouyer wrote:
> On Mon, May 26, 2025 at 10:56:41PM +0200, Manuel Bouyer wrote:
>> On Mon, May 26, 2025 at 09:30:53PM +0100, Andrew Cooper wrote:
>>> On 26/05/2025 9:13 pm, Manuel Bouyer wrote:
>>>> On Mon, May 26, 2025 at 07:50:14PM +0100, Andrew Cooper wrote:
>>>>> [...]
>>>>> Well, that range does include the aforementioned commit.
>>>>>
>>>>> Can you bisect around d32c77f471fb8400b6512c171a14cdd58f04f0a3 which is
>>>>> the backport of ^ in 4.18 ?
>>>> Sure,
>>>> with 0d5f15e and d32c77f the test pass, with cecee35 it fails.
>>>>
>>> Oh interesting, so the basic forwarding of #DB back into a guest
>>> (d32c77f) works fine, but the changes to emulated debug exceptions
>>> (cecee35) break.
>>>
>>> Anyway, I think I've spotted a logical error.  We do indeed end up
>>> calling x86_merge_dr6() twice, because of the TODO just out of context. 
>>> Does this help?
>> Yes, it works for cecee35; now testing with 4.18.5
> yes, it works with 4.18.5 too. All dbreg-related tests now pass again
> thanks !
>

Sorry I screwed it up so badly, and apparently also my own testing...

May I take that as a Tested-by tag on the patch then?

I'll get the fix merged once it's been reviewed, but 4.18 is in
security-only support right now and is unlikely to get this as a backport.

Are you able to take the patch logically for NetBSD?

~Andrew

Re: Xen 4.18.5 PV dbregs fail

[Manuel Bouyer]

On Mon, May 26, 2025 at 10:17:06PM +0100, Andrew Cooper wrote:
> On 26/05/2025 10:10 pm, Manuel Bouyer wrote:
> > On Mon, May 26, 2025 at 10:56:41PM +0200, Manuel Bouyer wrote:
> >> On Mon, May 26, 2025 at 09:30:53PM +0100, Andrew Cooper wrote:
> >>> On 26/05/2025 9:13 pm, Manuel Bouyer wrote:
> >>>> On Mon, May 26, 2025 at 07:50:14PM +0100, Andrew Cooper wrote:
> >>>>> [...]
> >>>>> Well, that range does include the aforementioned commit.
> >>>>>
> >>>>> Can you bisect around d32c77f471fb8400b6512c171a14cdd58f04f0a3 which is
> >>>>> the backport of ^ in 4.18 ?
> >>>> Sure,
> >>>> with 0d5f15e and d32c77f the test pass, with cecee35 it fails.
> >>>>
> >>> Oh interesting, so the basic forwarding of #DB back into a guest
> >>> (d32c77f) works fine, but the changes to emulated debug exceptions
> >>> (cecee35) break.
> >>>
> >>> Anyway, I think I've spotted a logical error.  We do indeed end up
> >>> calling x86_merge_dr6() twice, because of the TODO just out of context. 
> >>> Does this help?
> >> Yes, it works for cecee35; now testing with 4.18.5
> > yes, it works with 4.18.5 too. All dbreg-related tests now pass again
> > thanks !
> >
> 
> Sorry I screwed it up so badly, and apparently also my own testing...
> 
> May I take that as a Tested-by tag on the patch then?

Sure

> 
> I'll get the fix merged once it's been reviewed, but 4.18 is in
> security-only support right now and is unlikely to get this as a backport.
> 
> Are you able to take the patch logically for NetBSD?

Yes, I can add it to pkgsrc, no problem.

I need to package 4.20 too ...

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Re: [PATCH] x86/pv: Fix breakpoint reporting

[Roger Pau Monné]

On Mon, May 26, 2025 at 09:53:02PM +0100, Andrew Cooper wrote:
> x86_merge_dr6() is not a no-op when 0 is passed in; it will discard the
> previously latched breakpoint bits.
> 
> The combination of do_debug()'s manual call to x86_merge_dr6() for external
> debuggers, and pv_inject_DB() calling pv_inject_event(), results in two
> x86_merge_dr6() calls.
> 
> Feed the same pending_dbg in the second time.  This makes pv_inject_event()'s
> update of dr6 effectively a no-op, retaining the correct breakpoint bits.
> 
> Fixes: db39fa4b27ea ("x86/pv: Fix merging of new status bits into %dr6")
> Reported-by: Manuel Bouyer <bouyer@antioche.eu.org>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Thanks, Roger.