Re: [PATCH 3/3] x86: Fix calculation of %dr6/dr7 reserved bits

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 29/08/2023 3:21 pm, Jan Beulich wrote:
> On 29.08.2023 15:43, Andrew Cooper wrote:
>> --- a/xen/arch/x86/include/asm/debugreg.h
>> +++ b/xen/arch/x86/include/asm/debugreg.h
>> @@ -1,6 +1,7 @@
>>  #ifndef _X86_DEBUGREG_H
>>  #define _X86_DEBUGREG_H
>>  
>> +#include <asm/x86-defns.h>
>>  
>>  /* Indicate the register numbers for a number of the specific
>>     debug registers.  Registers 0-3 contain the addresses we wish to trap on */
>> @@ -21,7 +22,6 @@
>>  #define DR_STEP         (0x4000)        /* single-step */
>>  #define DR_SWITCH       (0x8000)        /* task switch */
>>  #define DR_NOT_RTM      (0x10000)       /* clear: #BP inside RTM region */
>> -#define DR_STATUS_RESERVED_ZERO (~0xffffefffUL) /* Reserved, read as zero */
> As you're dropping constants from here, how about the others? Especially
> DR_NOT_RTM would be nice to go away as well (I don't really like its name),
> yet DR_SWITCH looks to also be unused.

That's dealt with later in the series.  None of these DR_* constants
survive, but I think it's better to leave deleting them to the patch
that converts all.

>
>> --- a/xen/arch/x86/include/asm/x86-defns.h
>> +++ b/xen/arch/x86/include/asm/x86-defns.h
>> @@ -102,13 +102,30 @@
>>  
>>  /*
>>   * Debug status flags in DR6.
>> + *
>> + * For backwards compatibility, status flags which overlap with
>> + * X86_DR6_DEFAULT have inverted polarity.
>>   */
>> -#define X86_DR6_DEFAULT         0xffff0ff0  /* Default %dr6 value. */
>> +#define X86_DR6_B0              (_AC(1, UL) <<  0)   /* Breakpoint 0                */
>> +#define X86_DR6_B1              (_AC(1, UL) <<  1)   /* Breakpoint 1                */
>> +#define X86_DR6_B2              (_AC(1, UL) <<  2)   /* Breakpoint 2                */
>> +#define X86_DR6_B3              (_AC(1, UL) <<  3)   /* Breakpoint 3                */
>> +#define X86_DR6_BLD             (_AC(1, UL) << 11)   /* BusLock detect (INV)        */
>> +#define X86_DR6_BD              (_AC(1, UL) << 13)   /* %dr access                  */
>> +#define X86_DR6_BS              (_AC(1, UL) << 14)   /* Single step                 */
>> +#define X86_DR6_BT              (_AC(1, UL) << 15)   /* Task switch                 */
>> +#define X86_DR6_RTM             (_AC(1, UL) << 16)   /* #DB/#BP in RTM region (INV) */
>> +
>> +#define X86_DR6_ZEROS           _AC(0x00010000, UL)  /* %dr6 bits forced to 0       */
> 0x00001000?

Bah yes - serves me right for a last minute refactor.

~Andrew