Re: [PATCH] x86: Allow 16-bit register source for LAR and LSL

["H.J. Lu" <hjl.tools@gmail.com>]

On Mon, Dec 5, 2022 at 11:51 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 06.12.2022 00:20, H.J. Lu wrote:
> > On Mon, Dec 5, 2022 at 3:11 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 03.12.2022 05:13, H.J. Lu wrote:
> >>> Since LAR and LSL only access 16 bits of the source operand, regardless
> >>> of operand size, allow 16-bit register source for LAR and LSL, and always
> >>> disassemble LAR and LSL with 16-bit source operand.
> >>>
> >>> gas/
> >>>
> >>>       PR gas/29844
> >>>       * testsuite/gas/i386/i386.s: Add tests for LAR and LSL.
> >>>       * testsuite/gas/i386/x86_64.s: Likewise.
> >>>       * testsuite/gas/i386/intelbad.s: Remove "lar/lsl eax, ax".
> >>>       * testsuite/gas/i386/i386-intel.d: Updated.
> >>>       * testsuite/gas/i386/i386.d: Likewise.
> >>>       * testsuite/gas/i386/intel-intel.d: Likewise.
> >>>       * testsuite/gas/i386/intel.d: Likewise.
> >>>       * testsuite/gas/i386/intelbad.l: Likewise.
> >>>       * testsuite/gas/i386/x86_64-intel.d: Likewise.
> >>>       * testsuite/gas/i386/x86_64.d: Likewise.
> >>>
> >>> opcodes/
> >>>
> >>>       PR gas/29844
> >>>       * i386-dis.c (MOD_0F02): Removed.
> >>>       (MOD_0F03): Likewise.
> >>>       (dis386_twobyte): Restore larS and lslS.
> >>>       (mod_table): Remove MOD_0F02 and MOD_0F03.
> >>>       * i386-opc.tbl: Allow 16-bit register source for LAR and LSL.
> >>>       * i386-tbl.h: Regenerated.
> >>
> >> Please can you refrain from immediately committing patches which have
> >> a risk of being controversial.
> >>
> >> In the case here, given there are uses of the 16-bit register operand
> >> form in the Linux kernel, I can accept the assembler part of the change.
> >> The lines in i386-opc.tbl, however, need a comment then, as allowing for
> >> 16-bit registers despite a wider destination is explicitly not in line
> >> with the SDM. (Interestingly AMD's PM is different in this regard.)
> >>
> >> For the disassembler part you're completely undoing what I did, which is
> >> wrong - again with reference to the SDM. If you want to accommodate for
> >> AMD's PM, then you need to vary disassembly according to command line
> >> options specified, with the default being in line with the SDM (I can
> >> dig out a pretty old version of the doc, but I believe it has always
> >> been that way, i.e. even before AMD introduced their clones).
> >>
> >> I will revert this change unless you come forward with an adjustment
> >> within the next couple of days.
> >>
> >
> > Given that the only lower 16 bits are used, the 16-bit register source
> > is more appropriate.  I will raise the issue with the Intel SDM author.
>
> I see no point in changing the documentation when what's there has been
> valid for well over 30 years. There are other cases in newer insns where
> only the low 16 (or 8) bits are used, yet still the 32-bit register name
> is specified ({,v}pinsr{b,w} come to mind immediately). Also what you've
> done brought things out of sync with mov-to-sreg (and no, please don't
> "restore" consistency by also changing disassembly there).

The 16-bit register has been used in both assembler and disassembler
for well over 30 years.  I consider this a flaw in the spec.

> My request stand: Please undo / adjust the disassembler change you've
> done, or I'll have to revert the entire commit as being wrong and having
> been committed prematurely.
>
> Jan



-- 
H.J.