Re: [PATCH v5 2/3] x86/insn-eval: Add insn_assign_reg() helper

[David Laight <david.laight.linux@gmail.com>]

On Wed, 1 Jul 2026 07:59:05 -0700
Sean Christopherson <seanjc@google.com> wrote:

> On Wed, Jul 01, 2026, Kiryl Shutsemau wrote:
> > From: "Kiryl Shutsemau (Meta)" <kas@kernel.org>
> > 
> > KVM's instruction emulator has a small helper, assign_register(), that
> > writes a value into a sub-register with x86 partial-register-write
> > semantics: 1- and 2-byte writes leave the upper bits of the destination
> > untouched, 4-byte writes zero-extend to 64 bits, 8-byte writes overwrite
> > the full register.
> > 
> > The TDX guest #VE handler needs the same logic for port I/O emulation
> > to get 32-bit zero-extension right.  Rather than copy-pasting the
> > helper, lift it to <asm/insn-eval.h> as insn_assign_reg() so both can
> > use it.
> > 
> > Add <asm/insn.h> to the header's includes so it builds standalone in
> > callers that have not pulled it in transitively.
> > 
> > No functional change.
> > 
> > Signed-off-by: Kiryl Shutsemau (Meta) <kas@kernel.org>
> > Cc: stable@vger.kernel.org # prerequisite for the following 32-bit port I/O zero-extension fix
> > ---
> >  arch/x86/include/asm/insn-eval.h | 30 ++++++++++++++++++++++++++++++
> >  arch/x86/kvm/emulate.c           | 26 ++++----------------------
> >  2 files changed, 34 insertions(+), 22 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/insn-eval.h b/arch/x86/include/asm/insn-eval.h
> > index 4733e9064ee5..0c87759816d3 100644
> > --- a/arch/x86/include/asm/insn-eval.h
> > +++ b/arch/x86/include/asm/insn-eval.h
> > @@ -9,6 +9,7 @@
> >  #include <linux/compiler.h>
> >  #include <linux/bug.h>
> >  #include <linux/err.h>
> > +#include <asm/insn.h>
> >  #include <asm/ptrace.h>
> >  
> >  #define INSN_CODE_SEG_ADDR_SZ(params) ((params >> 4) & 0xf)
> > @@ -46,4 +47,33 @@ enum insn_mmio_type insn_decode_mmio(struct insn *insn, int *bytes);
> >  
> >  bool insn_is_nop(struct insn *insn);
> >  
> > +/*
> > + * Write @val into *@reg with x86 partial-register-write semantics: a 1-
> > + * or 2-byte write leaves the upper bits of the destination untouched; a
> > + * 4-byte write zero-extends to 64 bits (matching IN[BWL], MOV[BWL]  
> 
> The placement of the "(matching IN[BWL], MOV[BWL] etc.)" blurb is confusing.  I
> *think* you're trying to say this behavior matches that of MOVB, MOVW, and MOVL
> instruction mnemonics, but the blurb is buried in the snippet that specifically
> describes the 4-byte write behavior.
> 
> FWIW, I think giving examples does more harm than good, because the behavior isn't
> instruction specific, it's architectural behavior that applies to all writes to
> GPRs, as defined in "3.4.1.1 General-Purpose Registers in 64-Bit Mode".  E.g. for
> a MOV instruction that sign-extends a 32-bit immediate to a 64-bit registers, it's
> not that the instruction is exempt from the normal GPR semenatics, it's that the
> instruction performs a 64-bit write to the destination even though the source is
> only 32 bits.
> 
> And the B/W/L terminology isn't architectural, it's AT&T syntax.  E.g. trying
> to encode "movl" with NASM yields "error: instruction expected, found `movl dword'".
> Yes, the kernel uses AT&T syntax for assembly, but I think this helper should very
> explicitly document that it's emulating architectural behavior.
> 
> > + * etc.); an 8-byte write overwrites the full register.
> > + *
> > + * @reg need not be 8-byte aligned: KVM's instruction emulator points
> > + * into the middle of a register slot to address the high-byte
                 ^ it isn't really the 'middle'.

> > + * registers (AH, CH, DH, BH).  Use narrow stores for the sub-word
> > + * cases so that the access width matches @bytes.
> > + */
> > +static inline void insn_assign_reg(unsigned long *reg, u64 val, int bytes)
> > +{
> > +	switch (bytes) {
> > +	case 1:
> > +		*(u8 *)reg = (u8)val;
> > +		break;
> > +	case 2:
> > +		*(u16 *)reg = (u16)val;
> > +		break;
> > +	case 4:
> > +		*reg = (u32)val;  
> 
> IMO, it's worth keeping a short comment here, because even with the explanation
> above, I suspect most people will think the code is buggy.  E.g.
> 
> 		/* As above, zero-extend 4-byte writes on 64-bit CPUs. */
> 		*reg = (u32)val;

Or be even more specific and use '& 0xffffffff' rather than a cast.
Particularly since the casts of the RHS in the byte/short cases aren't
needed at all.

-- David

> 
> > +		break;
> > +	case 8:
> > +		*reg = val;
> > +		break;
> > +	}
> > +}