[PATCH] x86: allow 32-bit reg to be used with U{RD,WR}MSR

[Jan Beulich]

On 13.12.2023 03:23, Hu, Lin1 wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Tuesday, December 12, 2023 5:21 PM
>> To: Hu, Lin1 <lin1.hu@intel.com>
>> Cc: H.J. Lu <hjl.tools@gmail.com>; Binutils <binutils@sourceware.org>
>> Subject: Re: [PATCH] x86: allow 32-bit reg to be used with U{RD,WR}MSR
>>
>> On 12.12.2023 09:51, Hu, Lin1 wrote:
>>>> -----Original Message-----
>>>> From: Jan Beulich <jbeulich@suse.com>
>>>> Sent: Tuesday, December 12, 2023 4:13 PM
>>>> To: Hu, Lin1 <lin1.hu@intel.com>
>>>> Cc: H.J. Lu <hjl.tools@gmail.com>; Binutils <binutils@sourceware.org>
>>>> Subject: Re: [PATCH] x86: allow 32-bit reg to be used with
>>>> U{RD,WR}MSR
>>>>
>>>> On 12.12.2023 08:13, Hu, Lin1 wrote:
>>>>>> -----Original Message-----
>>>>>> From: Jan Beulich <jbeulich@suse.com>
>>>>>> Sent: Monday, December 4, 2023 3:18 PM
>>>>>>
>>>>>> On 04.12.2023 02:45, Hu, Lin1 wrote:
>>>>>>> I talked to the people involved. They found the eds description
>>>>>>> about "ignoring h32 bits of the MSR address for register variants"
>>>>>>> is a documentation
>>>>>> bug. The right version is USER_MSR have a GP fault on h32 of the
>>>>>> MSR address for the register version. So they update the
>>>>>> description from
>>>>>> #GP(0) If MSR_address[63:0] & 0x0000_0000_FFFF_C000 != 0 to #GP(0)
>>>>>> If MSR_address[63:0] & 0xFFFF_FFFF_FFFF_C000 != 0. And the source
>>>>>> register will still be r64 in the spec.
>>>>>>
>>>>>> What an unhelpful behavior. I'll need to revert the patch below
>>>>>> then, assuming they're not willing to re-think.
>>>>>>
>>>>>
>>>>> I think they don't what to change it. Because, if they change the
>>>>> spec, it would
>>>> involve a lot of places, and  r64 make sense at the moment.
>>>>
>>>> So this last part is what I don't understand: MSR space is 32 bits.
>>>> Why does
>>>> "r64 make sense at the moment"?
>>>>
>>>
>>> Without ignoring the high 32 bits, I think r64 and r32 are equivalent. Because
>> the GP fault is "If MSR_address[63:0] &  0xFFFF_FFFF_FFFF_C000", other than "If
>> MSR_address[63:0] & 0xFFFF_FFFF_0000_0000". When they use "urdmsr" or
>> "uwrmsr", Users need to know the MSR name. R32 also can't tell them which
>> name is right or wrong.
>>
>> Feels like you're not getting my point. Imo operand size should reflect what part
>> of a register is actually used. If the check was as presently documented, only the
>> low 32 bits of the operand could ever be used (as MSR space isn't wider),
>> irrespective of the initial form of the insn limiting things even further (to the low
>> 14 bits). With the wider mask, you're opening a path to a future where MSR
>> space is 64 bits. That may indeed be intended, yet I have severe doubts that
>> going beyond 32 bits is ever going to be necessary. You'd at least need to come
>> close to the order or a billion MSRs to warrant widening space, I would say.
>>
>> Furthermore while in the vast majority of cases I expect it won't make a
>> difference to users simply because whatever operations are needed to establish
>> the intended MSR index can be carried out with 32-bit operations (thus zeroing
>> the upper half anyway), someone using 64-bit operations for such calculations
>> (for whatever reason, perhaps even by
>> mistake) would then be running into an issue in case such an operation ended up
>> overflowing into the high 32 bits. Personally I see no reason to create such a
>> pitfall, even if it is just a minor one.
> 
> In my opinion, MSR's name is a key. The user doesn't need to add or subtract to it, it's the OS's job to calculate the address. About R64 is telling the user that the MSR space may be wider. I don't think it is a problem. Because, this has no impact on user usage or coding. rdmsr/wrmsr only uses 32-bit registers because they only had 32-bit registers at the time, and the designers stated that if they were designing this instruction now it would use 64-bit registers.
> 
> Actually, we're talking about USER_MSR. USER_MSR's actual address is calculated by a_function(base_address, MSR_address[13:0]). Overflow is not a problem, the user just needs to make sure that what is in the lower 14 bits is valid. But I still can't understand why a user would take a key and calculate it.

Even now there are already ranges of similar MSRs (they aren't user mode
accessible, but such ranges could show up). Calculating the actual MSR
address then may indeed involve arithmetic, and how exactly one chooses
to carry out the arithmetic would imo better be left entirely to the
programmer, without the architecture dictating anything more than the
bare minimum of constraints.

Anyway, I see this doesn't lead anywhere. I'll revert said patch, perhaps
not today but later this week.

Jan