Re: [Xen-devel] [PATCH] x86/debug: Avoid crashing in early boot because of debugger_trap_entry()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 04/08/16 08:05, Jan Beulich wrote:
>>>> On 03.08.16 at 19:08, <andrew.cooper3@xxxxxxxxxx> wrote:
>> debugger_trap_entry() is not safe to use during early boot, as it follows
>> current before it is necesserily safe to do so.  Futhermore it does this
>> unconditionally, despite most callsites turning into no-ops because of the
>> vector test.
>>
>> Inline debugger_trap_entry() into the two callers where doesn't become a
>> no-op,
> Implied from that you delete all other invocations. While from a
> (current) functionality pov that's no functional change, to me these
> invocations have always served as an indication that some action
> _could_ be taken there as well. I'm not sure it's a good idea to
> remove that.

I considered that point specifically, but I disagree.

The current callsites require debugger_trap_entry() to double up all the
safety checks the main, which isn't reasonable IMO.

This code hasn't changed in years and don't think it is likely to change
in the forseeable future.  If people did want something a little like
how it currently is, the current code is not a good starting point.

OTOH, I do have an alternative patch, but would prefer not to use it.

>
>> @@ -3814,6 +3825,13 @@ void do_debug(struct cpu_user_regs *regs)
>>      /* Save debug status register where guest OS can peek at it */
>>      v->arch.debugreg[6] = read_debugreg(6);
>>  
>> +    if ( guest_kernel_mode(v, regs) && v->domain->debugger_attached )
>> +    {
>> +        v->arch.gdbsx_vcpu_event = TRAP_debug;
> Note how in the original code this write didn't happen. I can't tell why,
> but it's a behavioral change which certainly would need explaining

This was mentioned in the commit message.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel