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

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

On 06/06/18 15:16, Jan Beulich wrote:
>> +    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 & ~()).

At the moment, every single use of DR_*_RESERVED_* in the entire
codebase is buggy, although this is admittedly a side effect of the
introduction of RTM (and that the advice given the Intel manual has
caused software not to be forwards compatible to the introduction of
RTM.  I've also arranged for that advice to be fixed.)

Irrespective, the constants are now simply appropriate to express how
the reserved bit behaviour works, which is why I'm removing them and
introducing these functions instead.

As for naked numbers, they are like this because its the clearest I
could make the code.  Hiding these behind defines makes it harder to
cross reference with the comment.

An alternative might be to go for an ARM approach using GENMASK()/BIT(),
but I'm not a fan of those because it introduces more confusion as to
where the boundaries of the mask lie.

Like you, I'm not a fan of naked numbers, but in this instance, I chose
them because I think they are the least bad option available.

~Andrew

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