Re: [PATCH 8/8] Support APX JMPABS

[Jan Beulich <jbeulich@suse.com>]

On 24.11.2023 06:40, Hu, Lin1 wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Tuesday, November 14, 2023 7:15 PM
>>
>> On 14.11.2023 04:26, Hu, Lin1 wrote:
>>>  > On 02.11.2023 12:29, Cui, Lili wrote:
>>>>> @@ -8939,6 +8940,9 @@ process_operands (void)
>>>>>  	}
>>>>>      }
>>>>>
>>>>> +  if (i.tm.mnem_off == MN_jmpabs)
>>>>> +    i.rex2_encoding = true;
>>>>
>>>> Please see my earlier remarks wrt "rex2" vs "{rex2}". What you do
>>>> here is effect the latter. Yet as indicated, the pseudo-prefix isn't
>>>> really an indication of "must have REX2 prefix", but only a weak
>>>> request to do so if possible. I think you want to set i.rex2 here
>>>> instead, requiring a way to express that an empty REX2 prefix is wanted.
>>>>
>>>
>>> But in terms of encoding, i.rex2 should be 0. Can I do special handling in
>> build_rex2_prefix?
>>
>> build_rex2_prefix() wants to remain generic. What I was trying to hint at though
>> is that it ought to be possible to set bits in i.rex2 (to make it non-zero) which
>> then aren't encoded into the REX2 payload byte (leveraging that only the low
>> three bits are actually contributing to the final encoding). The important point is
>> that both i.rex2 and i.rex2_encoding retain the specific meaning they are
>> intended to have.
>>
> 
> I have set i.rex2 = 16;

But hopefully not exactly this way, but using a self-descriptive #define for
the integer literal.

>>>>> --- /dev/null
>>>>> +++ b/gas/testsuite/gas/i386/x86-64-apx-jmpabs-inval.s
>>>>> @@ -0,0 +1,17 @@
>>>>> +# Check bytecode of APX_F jmpabs instructions with illegal encode.
>>>>> +
>>>>> +	.text
>>>>> +# With 66 prefix
>>>>> +	.byte
>>>> 0x66,0x64,0xd5,0x00,0xa1,0x01,0x00,0x00,0x00,0x00,0x00,0x00,0x00
>>>>> +	.byte 0x66,0xd5,0x00,0xa1,0x01,0x00,0x00,0x00,0x00,0x00,0x00,0x00
>>>>> +# With 67 prefix
>>>>> +	.byte
>>>> 0x67,0x64,0xd5,0x00,0xa1,0x01,0x00,0x00,0x00,0x00,0x00,0x00,0x00
>>>>> +	.byte 0x67,0xd5,0x00,0xa1,0x01,0x00,0x00,0x00,0x00,0x00,0x00,0x00
>>>>> +# With F2 prefix
>>>>> +	.byte
>>>> 0xf2,0x64,0xd5,0x00,0xa1,0x01,0x00,0x00,0x00,0x00,0x00,0x00,0x00
>>>>> +	.byte 0xf2,0xd5,0x00,0xa1,0x01,0x00,0x00,0x00,0x00,0x00,0x00,0x00
>>>>> +# With F3 prefix
>>>>> +	.byte
>>>> 0xf3,0x64,0xd5,0x00,0xa1,0x01,0x00,0x00,0x00,0x00,0x00,0x00,0x00
>>>>> +	.byte 0xf3,0xd5,0x00,0xa1,0x01,0x00,0x00,0x00,0x00,0x00,0x00,0x00
>>>>> +# REX2.M0 = 0 REX2.W = 1
>>>>> +	.byte 0xd5,0x08,0xa1,0x01,0x00,0x00,0x00,0x00,0x00,0x00,0x00
>>>>
>>>> As per earlier comments: This wants expressing via .insn, to yield
>>>> input to gas human-readable (even if, as it looks, two .insn are
>>>> going to be required per resulting construct). Further in the last
>>>> comment, why is
>>>> REX2.M0 mentioned there, but not elsewhere? Also what purpose serve
>>>> the
>>>> 0x64 bytes here? The encodings are invalid irrespective of them.
>>>> Instead I'd kind have expected LOCK to also be covered.
>>>>
>>>
>>> Because this error line is only for the special case where M0 == 0, and
>> base_opcode == 0xa1, W should be 0, other than 1. If M0 = 1, W = 1,
>> base_opcode == 0xa1, I think it could decoding as mov rax, moffs or ( some
>> future insn). Elsewhere it's just excluding invalid prefixes.
>>
>> Yet REX2.M == 0 is as relevant there (until such time where some of those
>> prefixes used is assigned meaning).
>>
> 
> I don't think so. the original result with W = 1 was MOV RAX, moffs, but the insn can't support REX2. So If someone input .byte REX2.M = 0 && REX2.W = 1, it should be a Bad_Opcode. 

I fully agree with this. But that doesn't invalidate my original comment:
REX2.M is relevant in all of these tests, and hence comments end up
inconsistent. At the risk of repeating myself - especially when you use
.byte for encoding an instruction, what that (unreadable) byte sequence
is supposed to encode wants to be properly stated in a comment then.
When using .insn, some parts may become self-descriptive, hence why
generally .insn ought to be preferred over .byte.

>>>> Also a spec question as we're talking of what is or is not valid (i.e.
>>>> causing #UD) here: Why would XCR0.APX=0 need to cause #UD? There's no
>>>> use of eGPR-s here.
>>>>
>>>
>>> Sorry, what is XCR0.APX?
>>
>> Bit 19 of the XCR0 register. It is mentioned in exactly this way in the APX-
>> LEGACY-JMPABS exception class description.
>>
> 
> I think XCR0.APX is a state bit to control if support APX instruction set not if support eGPR-s.

No, XCR0 very certainly is a set of controls affecting register use. If
JMPABS is also controlled by it, then I'd view this as an erratum if it
appeared like that in silicon; that erratum may well be a design one then,
or one "justified" by simplifying the implementation in some way, but it
would still be wrong from a conceptual pov.

Jan