On 06/06/18 15:50, Andrew Cooper 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 appropriate to express how
Apologies - I meant "simply not appropriate". ~Andrew > 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 > [email protected] > https://lists.xenproject.org/mailman/listinfo/xen-devel _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
