Re: [Xen-devel] [PATCH] x86/vmx: fix dr6 handling issue

["Zhang, Yang Z" <yang.z.zhang@xxxxxxxxx>]

Jan Beulich wrote on 2014-05-13:
>>>> On 13.05.14 at 10:00, <yang.z.zhang@xxxxxxxxx> wrote:
>> @@ -1178,16 +1189,10 @@ static void vmx_update_host_cr3(struct vcpu
>> *v)
>> 
>>  void vmx_update_debug_state(struct vcpu *v)  {
>> -    unsigned long mask;
>> -
>> -    mask = 1u << TRAP_int3;
>> -    if ( !cpu_has_monitor_trap_flag )
>> -        mask |= 1u << TRAP_debug;
> 
> So how are things going to work now when !cpu_has_monitor_trap_flag?

Umm.. it seems wrong in such case.

> 
>> @@ -2717,10 +2722,18 @@ void vmx_vmexit_handler(struct cpu_user_regs
> *regs)
>>               */
>>              __vmread(EXIT_QUALIFICATION, &exit_qualification);
>>              HVMTRACE_1D(TRAP_DEBUG, exit_qualification);
>> -            write_debugreg(6, exit_qualification | 0xffff0ff0); -     
>>       if ( !v->domain->debugger_attached || cpu_has_monitor_trap_flag )
>> -                goto exit_and_crash; -           
>> domain_pause_for_debugger(); +            exit_qualification |=
>> DR_STATUS_RESERVED_ONE; +            if ( v->domain->debugger_attached
>> ) +            { +                write_debugreg(6,
>> exit_qualification); +                domain_pause_for_debugger(); +   
>>         } +            else +            { +               
>> v->arch.debugreg[6] |= exit_qualification; +               
>> __restore_debug_registers(v); +               
>> hvm_inject_hw_exception(TRAP_debug, HVM_DELIVER_NO_ERROR_CODE); +      
>>      }
> 
> I.e. is that really the only thing that needs doing when there' no MTF?

Previously, I think it is enough. But after more thoughts, I am not sure 
whether it will cause any problem if guest is also using debugging mechanism 
while host is debugging the guest itself. Need more investigations.

> 
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -3763,8 +3763,8 @@ long set_debugreg(struct vcpu *v, unsigned int
>> reg,
> unsigned long value)
>>           * DR6: Bits 4-11,16-31 reserved (set to 1).
>>           *      Bit 12 reserved (set to 0).
>>           */
>> -        value &= 0xffffefff; /* reserved bits => 0 */
>> -        value |= 0xffff0ff0; /* reserved bits => 1 */
>> +        value &= ~DR_STATUS_RESERVED_ZERO;  /* reserved bits => 0
> */
> 
> This is not equivalent to the old code, ...
> 
>> --- a/xen/include/asm-x86/debugreg.h
>> +++ b/xen/include/asm-x86/debugreg.h
>> @@ -76,4 +76,7 @@
>>  long set_debugreg(struct vcpu *, unsigned int reg, unsigned long
>> value);  void activate_debugregs(const struct vcpu *);
>> 
>> +#define DR_STATUS_RESERVED_ZERO  (0x00001000ul) /* Reserved, read as
>> +zero */
> 
> ... because here you're introducing a problem similar to the one which
> was the subject of XSA-12. This needs to be
> 
> #define DR_STATUS_RESERVED_ZERO  (~0xffffeffful) /* Reserved, read as
> zero */
> 
> matching DR_CONTROL_RESERVED_ZERO.
> 
> Jan

Good point. 

Best regards,
Yang



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