linux-kernel - Re: [PATCH 06/20] x86/msr: Standardize on 'u32' MSR indices in <asm/msr.h>

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3B57B3E5-47C9-4196-AD2B-44916E18B6D0@zytor.com>
Date: Wed, 09 Apr 2025 20:29:17 -0700
From: "H. Peter Anvin" <hpa@...or.com>
To: Xin Li <xin@...or.com>, Ingo Molnar <mingo@...nel.org>,
        linux-kernel@...r.kernel.org
CC: Juergen Gross <jgross@...e.com>, Dave Hansen <dave.hansen@...el.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Peter Zijlstra <peterz@...radead.org>, Borislav Petkov <bp@...en8.de>,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH 06/20] x86/msr: Standardize on 'u32' MSR indices in <asm/msr.h>

On April 9, 2025 8:18:12 PM PDT, Xin Li <xin@...or.com> wrote:
>On 4/9/2025 6:37 PM, H. Peter Anvin wrote:
>> On 4/9/25 14:55, Xin Li wrote:
>>> 
>>> It looks to me that we don't use the "const" qualifier in the code a
>>> lot.  However since the MSR index is usually not expected to change
>>> inside the MSR API implementations, would it be nicer to add the "const"
>>> qualifier?
>>> 
>>> The same to the MSR value of MSR write APIs.
>>> 
>> 
>> "const" on an automatic variable (including function arguments) is usually not very meaningful, unless it is manifest as a memory object (see below.)
>> 
>> Personally I tend to use "const" anyway in more complex functions to make it clear that a variable is not expected to change while in scope (and I also prefer to reduce the scope of a variable as much as possible), but for a simple function like this it is more clutter than anything else.
>
>Good point!
>
>A question NOT related to this patch set, the MSR write API prototype
>defined in struct pv_cpu_ops as:
>    void (*write_msr)(unsigned int msr, unsigned low, unsigned high);
>
>Will it be better to add "const" to its arguments?  I.e.,
>    void (*write_msr)(const u32 msr, const u32 low, const u32 high);
>
>> Now, "const" on a *memory object* (pointer target) is a very different thing and should in general be used where ever writing to an object is not going to happen.
>> 
>> An automatic variable becomes manifest as a memory object if its address is taken anywhere in its scope (using the & operator or an unindexed array) and the address of that pointer stored. The last part means that the compiler (if it is is smart enough) can take a sequence of operations equivalent to *& and eliminate it.
>> 
>> Keep in mind that, for C (not necessarily C++):
>> 
>> 1. in *all* cases "foo[x]" is exactly equivalent to "*(foo + x)"
>> 2. *if* "foo" is declared as an array type, then "foo" is exactly equivalent to "&foo[0]".
>> 
>> "const" in C a little less strict than you would like; the only way in C before C23 to declare a "true" constant is using enum or a #define macro (which of course pollutes the global namespace). In block scope it usually doesn't matter for scalar types and const or static const will work just fine, but it is only in C23 than C imported "constexpr" from C++ (which has had it since C++11.)
>Nice to know.
>Thanks!
>    Xin

No, that makes no sense (it would have absolutely no effect.)