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

[Jan Beulich <jbeulich@suse.com>]

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.

Jan