Re: [Xen-devel] [PATCH 04/11] x86: Fix calculation of %dr6/7 reserved bits

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.06.18 at 15:59, <andrew.cooper3@xxxxxxxxxx> wrote:
> The reserved bit calculations for %dr6 and %dr7 depend on whether the VM has
> the Restricted Transnational Memory feature available.

Transactional, I think.

> --- a/xen/include/asm-x86/debugreg.h
> +++ b/xen/include/asm-x86/debugreg.h
> @@ -10,9 +10,18 @@
>  #define DR_STATUS    6
>  #define DR_CONTROL   7
>  
> -/* Define a few things for the status register.  We can use this to determine
> -   which debugging register was responsible for the trap.  The other bits
> -   are either reserved or not of interest to us. */
> +/*
> + * DR6 status bits.
> + *   N.B. For backwards compatibility, X86_DR6_RTM has inverted polarity.
> + */
> +#define X86_DR6_B0              (1u <<  0)  /* Breakpoint 0 triggered  */
> +#define X86_DR6_B1              (1u <<  1)  /* Breakpoint 1 triggered  */
> +#define X86_DR6_B2              (1u <<  2)  /* Breakpoint 2 triggered  */
> +#define X86_DR6_B3              (1u <<  3)  /* Breakpoint 3 triggered  */
> +#define X86_DR6_BD              (1u << 13)  /* Debug register accessed */
> +#define X86_DR6_BS              (1u << 14)  /* Single step             */
> +#define X86_DR6_BT              (1u << 15)  /* Task switch             */
> +#define X86_DR6_RTM             (1u << 16)  /* #DB/#BP in RTM region   */

Could I talk you into introducing these into x86-defs.h instead, for the
emulator to use them eventually too?

> @@ -84,4 +90,30 @@
>  long set_debugreg(struct vcpu *, unsigned int reg, unsigned long value);
>  void activate_debugregs(const struct vcpu *);
>  
> +static inline unsigned long adjust_dr6_rsvd(unsigned long dr6, bool rtm)
> +{
> +    /*
> +     * DR6: Bits 4-11,17-31 reserved (set to 1).
> +     *      Bit  16 reserved (set to 1) if RTM unavailable.
> +     *      Bit  12 reserved (set to 0).
> +     */

Please also mention bits 32-63 (also for DR7).

> +    dr6 |= 0xfffe0ff0 | (rtm ? 0 : X86_DR6_RTM);
> +    dr6 &= 0xffffefff;

I'm not overly happy with this move to literal numbers. Could we
at least meet in the middle and adjust the first line to

    dr6 |= X86_DR6_DEFAULT & ~(rtm ? 0 : X86_DR6_RTM);

? Even the second line, it doesn't look unreasonable to me to
accompany the other X86_DR6_* values you introduce with
X86_DR6_MBZ (or some such, if you dislike this name). Of course
then for the first line using X86_DR6_MBS would also be an option,
allowing you to retain the | (which documentation-wise might be
slightly better than the & ~()).

> +    return dr6;
> +}
> +
> +static inline unsigned long adjust_dr7_rsvd(unsigned long dr7, bool rtm)
> +{
> +    /*
> +     * DR7: Bit  10 reserved (set to 1).
> +     *      Bit  11 reserved (set to 0) if RTM unavailable.
> +     *      Bits 12,14-15 reserved (set to 0).
> +     */
> +    dr7 |= 0x00000400;
> +    dr7 &= 0xffff23ff & (rtm ? 0 : ~DR_RTM_ENABLE);

Along those lines here then.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel