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 [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
