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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.06.18 at 16:50, <andrew.cooper3@xxxxxxxxxx> wrote:
> 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 not appropriate to express how
[I've taken the liberty to insert the omitted "not" above, to help someone,
 like me, going through this later (again) to have correct context]
> the reserved bit behaviour works, which is why I'm removing them and
> introducing these functions instead.

Hence my suggestion to introduce (correct) X86_DR6_MBZ and
X86_DR6_MBS.

> 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.

Whether the comment goes next to their definition or next to their
(currently single) use is a secondary aspect.

> 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.

I don't like this option either.

Jan



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