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

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

On Wed, Dec 7, 2022 at 11:38 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 08.12.2022 00:57, H.J. Lu wrote:
> > On Wed, Dec 7, 2022 at 2:47 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 06.12.2022 23:32, H.J. Lu wrote:
> >>> On Tue, Dec 6, 2022 at 8:36 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>
> >>>> On 06.12.2022 17:11, H.J. Lu wrote:
> >>>>> 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.
> >>>>
> >>>> And a flaw in disassembly of move-to-sreg (and maybe other insns)?
> >>>> Is the spec then also wrong with {,v}pinsr{b,w}? Also note that
> >>>> while the assembler wants to provide backwards compatibility, the
> >>>> same is rarely necessary for the disassembler. Hence what it may
> >>>> or may not have done for over 30 years doesn't really matter.
> >>>>
> >>>> Please can we avoid introducing further inconsistencies, and rather
> >>>> work towards more consistency (and not by then also corrupting
> >>>> move-to-sreg and possible other insns)?
> >>>>
> >>>
> >>> I have a different view on "inconsistencies".  For LAR/LSL, they
> >>> are different:
> >>>
> >>> 1.  All operands are integer registers.
> >>> 2. The operand size prefix doesn't apply to the source.
> >>> 3. Only the 16 bits of the source are used.
> >>
> >> 2 and 3 are true for move-to-sreg and {,v}pinsr{b,w} as well.
> >>
> >>> 4. 16 bit source has been in use for more than 30 years.
> >>>
> >>> What counts are how the processor behaves
> >>
> >> Which means we have further things to fix: For example LTR and LLDT,
> >> like move-to-sreg, also accept 32- or 64-bit register operands,
> >> silently ignoring the upper bits. The assembler wants to accept such
> >> for consistency (even if the SDM doesn't name these variants), and
> >> the disassembler wants to express operand size by picking the correct
> >> register name (didn't check if it maybe already does).
> >>
> >> This "picking the correct register name" applies to both operands of
> >> LAR and LSL just as much.
> >
> > We have different opinions on what "the correct register name"
> > should be.
>
> Indeed - hence why the patch should never have been committed. Recall
> you did approve of the earlier patch, which now you've partially
> reverted without even saying so in your commit? Which is made even
> worse by the fact that to address PR gas/29844 there was no need at
> all to also touch the disassembler, i.e. you've mixed in a single
> commit two entirely separate changes (one controversial, the other
> not).
>
> >   When there are more than one integer registers
> > and the operand size prefix is ignored on one, the register
> > name with the proper size is the correct register name.
>
> Quite differently: Except where necessary (MOVSX, MOVZX, CRC32),
> register sizes ought to match so there are as few anomalies as possible
> throughout the entire ISA. With non-matching register sizes it'll be
> easily ambiguous what an insn's operand size actually is.
>

I don't think we should change disassembler for them after
more than 30 years.

-- 
H.J.