Re: [Xen-devel] [PATCH 1/6] x86/msr: Clean up the MSR_EFER constants

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.09.18 at 16:47, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 28/06/18 14:56, Jan Beulich wrote:
>>>>> On 28.06.18 at 15:36, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 28/06/18 14:00, Jan Beulich wrote:
>>>>>>> On 26.06.18 at 15:18, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>>> @@ -49,6 +28,18 @@
>>>>>  #define ARCH_CAPS_RSBA                   (_AC(1, ULL) << 2)
>>>>>  #define ARCH_CAPS_SSB_NO         (_AC(1, ULL) << 4)
>>>>>  
>>>>> +#define MSR_EFER                        0xc0000080 /* Extended Feature 
>>>>> Enable Register */
>>>>> +#define EFER_SCE                        (_AC(1, ULL) <<  0) /* SYSCALL 
>>>>> Enable */
>>>>> +#define EFER_LME                        (_AC(1, ULL) <<  8) /* Long Mode 
>>>>> Enable */
>>>>> +#define EFER_LMA                        (_AC(1, ULL) << 10) /* Long Mode 
>>>>> Active */
>>>>> +#define EFER_NXE                        (_AC(1, ULL) << 11) /* No 
>>>>> Execute Enable */
>>>>> +#define EFER_SVME                       (_AC(1, ULL) << 12) /* Secure 
>>>>> Virtual Machine Enable */
>>>>> +#define EFER_LMSLE                      (_AC(1, ULL) << 13) /* Long Mode 
>>>>> Segment Limit Enable */
>>>>> +#define EFER_FFXSE                      (_AC(1, ULL) << 14) /* Fast 
>>>>> FXSAVE/FXRSTOR */
>>>>> +
>>>>> +#define EFER_KNOWN_MASK (EFER_SCE | EFER_LME | EFER_LMA | EFER_NXE | \
>>>>> +                         EFER_SVME | EFER_LMSLE | EFER_FFXSE)
>>>> When meaning to clean up and consolidate these and others, why
>>>> don't we switch to architectural MSR names at the same time? While
>>>> this will increase source size a little, it'll
>>>> - allow grep-ing for the MSRs' uses by their SDM names,
>>>> - significantly reduce the risk of name clashes with something on e.g.
>>>>   the arm side (EFER may not be the most risky one here, but some
>>>>   of the subsequent patches certainly seem to incur such a risk).
>>>>
>>>> I.e. here MSR_IA32_EFER and IA32_EFER_SCE etc.
>>>>
>>>> Other than this I'm certainly fine in general with this cleanup.
>>> Removing IA32 is a deliberate and intended properly.  The
>>> non-architectural vs architectural nature of MSRs changes over time
>>> meaning the names here get stale.
>> But I don't think they've ever changed from IA32 to no IA32. I.e.
>> once an MSR becomes architectural, we could rename it once and
>> be done.
> 
> In some cases, use of the IA32 prefix is different per vendor.

I guess as soon as one calls it architectural, so could we do in
our naming. (I'm btw unsure I've ever seen AMD introduce an
MSR with IA32 infix.)

>>> As for grepability, most MSRs can't currently be located like that, and
>>> (naming instability aside) I believe the reduction in code volume is
>>> more important property to have.
>> The fact that "most MSRs can't currently be located like that" is
>> what I've long hoped we could overcome.
> 
> Why? Its not like you can grep the docs themselves.  If you are trying
> to cross reference, going by number is far easier.

I can surely use the reader's "search" function.

>>> There is no chance of clashing with ARM, as these are all arch-specific
>>> constants.  Any common code referencing them should be fixed by becoming
>>> arch-specific code.
>> The risk is for this to go unnoticed for a while.
> 
> You can make that argument about any choice of naming.  Having an
> unnecessarily verbose constant name doesn't alter the risk.

Just like we use XEN_ / xen_ to separate public interface name space,
prefixing x86_ or infixing _IA32_ seems quite reasonable a thing to me
to separate x86 from ARM and common name spaces.

Jan



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