[PATCH v5 0/3] x86/tdx: Fix port I/O handling bugs

[PATCH v5 0/3] x86/tdx: Fix port I/O handling bugs

[Kiryl Shutsemau]

From: "Kiryl Shutsemau (Meta)" <kas@kernel.org>

Three fixes for emulated port I/O in the TDX guest #VE handler.

Patch 1 fixes an off-by-one in the GENMASK() used by handle_in() and
handle_out(): the mask was one bit too wide for every I/O size.

Patch 3 fixes 32-bit port IN to zero-extend into RAX, per x86
semantics, instead of preserving the upper 32 bits. To avoid
open-coding the partial-register-write rules, patch 2 first lifts KVM's
assign_register() helper into <asm/insn-eval.h> as insn_assign_reg() so
both KVM and the #VE handler can share it.

Patch 2 touches arch/x86/kvm/emulate.c (it removes assign_register() and
routes the callers through the new helper), so an ack from the KVM
maintainers would be appreciated before this goes through the x86 tree.

Changes since v4:
  - Rebase onto v7.2-rc1.
  - insn_assign_reg(): drop the arithmetic read-modify-write body from
    v4 and lift KVM's assign_register() verbatim (typed-pointer writes).
    This fixes the unaligned access / adjacent-register clobber for
    high-byte registers (AH/CH/DH/BH) that Sashiko flagged on v4, where
    the emulator hands the helper a pointer offset by one byte. Update
    the changelog to match.
  - Cc: stable on the insn_assign_reg() patch, as it is a prerequisite
    for the patch 3 fix.
  - Collect Reviewed-by from Binbin Wu and Rick Edgecombe.

v4: https://lore.kernel.org/all/cover.1780584300.git.kas@kernel.org/

Kiryl Shutsemau (Meta) (3):
  x86/tdx: Fix off-by-one in port I/O handling
  x86/insn-eval: Add insn_assign_reg() helper
  x86/tdx: Fix zero-extension for 32-bit port I/O

 arch/x86/coco/tdx/tdx.c          | 10 ++++------
 arch/x86/include/asm/insn-eval.h | 30 ++++++++++++++++++++++++++++++
 arch/x86/kvm/emulate.c           | 26 ++++----------------------
 3 files changed, 38 insertions(+), 28 deletions(-)


base-commit: dc59e4fea9d83f03bad6bddf3fa2e52491777482
-- 
2.54.0

[PATCH v5 1/3] x86/tdx: Fix off-by-one in port I/O handling

[Kiryl Shutsemau]

From: "Kiryl Shutsemau (Meta)" <kas@kernel.org>

handle_in() and handle_out() in arch/x86/coco/tdx/tdx.c use:

    u64 mask = GENMASK(BITS_PER_BYTE * size, 0);

GENMASK(h, l) includes bit h. For size=1 (INB), this produces
GENMASK(8, 0) = 0x1FF (9 bits) instead of GENMASK(7, 0) = 0xFF (8
bits). The mask is one bit too wide for all I/O sizes.

Fix the mask calculation.

Fixes: 03149948832a ("x86/tdx: Port I/O: Add runtime hypercalls")
Reported-by: Borys Tsyrulnikov <tsyrulnikov.borys@gmail.com>
Link: https://lore.kernel.org/all/CAKw_Dz96rfSQc6Rn+9QBcUFHhmkK+9zu+P=bxowfZwxrATCBRg@mail.gmail.com/
Signed-off-by: Kiryl Shutsemau (Meta) <kas@kernel.org>
Reviewed-by: Kai Huang <kai.huang@intel.com>
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Reviewed-by: Binbin Wu <binbin.wu@linux.intel.com>
Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
Cc: stable@vger.kernel.org
---
 arch/x86/coco/tdx/tdx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index 29b6f1ed59ec..b8bbd715fb62 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c
@@ -694,7 +694,7 @@ static bool handle_in(struct pt_regs *regs, int size, int port)
 		.r13 = PORT_READ,
 		.r14 = port,
 	};
-	u64 mask = GENMASK(BITS_PER_BYTE * size, 0);
+	u64 mask = GENMASK(BITS_PER_BYTE * size - 1, 0);
 	bool success;
 
 	/*
@@ -714,7 +714,7 @@ static bool handle_in(struct pt_regs *regs, int size, int port)
 
 static bool handle_out(struct pt_regs *regs, int size, int port)
 {
-	u64 mask = GENMASK(BITS_PER_BYTE * size, 0);
+	u64 mask = GENMASK(BITS_PER_BYTE * size - 1, 0);
 
 	/*
 	 * Emulate the I/O write via hypercall. More info about ABI can be found
-- 
2.54.0

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

[Kiryl Shutsemau]

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]
+ * 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
+ * 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;
+		break;
+	case 8:
+		*reg = val;
+		break;
+	}
+}
+
 #endif /* _ASM_X86_INSN_EVAL_H */
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index b566ab5c7515..c6dcb5ac48af 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -24,6 +24,7 @@
 #include "kvm_emulate.h"
 #include <linux/stringify.h>
 #include <asm/debugreg.h>
+#include <asm/insn-eval.h>
 #include <asm/nospec-branch.h>
 #include <asm/ibt.h>
 #include <asm/text-patching.h>
@@ -439,25 +440,6 @@ static void assign_masked(ulong *dest, ulong src, ulong mask)
 	*dest = (*dest & ~mask) | (src & mask);
 }
 
-static void assign_register(unsigned long *reg, u64 val, int bytes)
-{
-	/* The 4-byte case *is* correct: in 64-bit mode we zero-extend. */
-	switch (bytes) {
-	case 1:
-		*(u8 *)reg = (u8)val;
-		break;
-	case 2:
-		*(u16 *)reg = (u16)val;
-		break;
-	case 4:
-		*reg = (u32)val;
-		break;	/* 64b: zero-extend */
-	case 8:
-		*reg = val;
-		break;
-	}
-}
-
 static inline unsigned long ad_mask(struct x86_emulate_ctxt *ctxt)
 {
 	return (1UL << (ctxt->ad_bytes << 3)) - 1;
@@ -505,7 +487,7 @@ register_address_increment(struct x86_emulate_ctxt *ctxt, int reg, int inc)
 {
 	ulong *preg = reg_rmw(ctxt, reg);
 
-	assign_register(preg, *preg + inc, ctxt->ad_bytes);
+	insn_assign_reg(preg, *preg + inc, ctxt->ad_bytes);
 }
 
 static void rsp_increment(struct x86_emulate_ctxt *ctxt, int inc)
@@ -1767,7 +1749,7 @@ static int load_segment_descriptor(struct x86_emulate_ctxt *ctxt,
 
 static void write_register_operand(struct operand *op)
 {
-	return assign_register(op->addr.reg, op->val, op->bytes);
+	return insn_assign_reg(op->addr.reg, op->val, op->bytes);
 }
 
 static int writeback(struct x86_emulate_ctxt *ctxt, struct operand *op)
@@ -2008,7 +1990,7 @@ static int em_popa(struct x86_emulate_ctxt *ctxt)
 		rc = emulate_pop(ctxt, &val, ctxt->op_bytes);
 		if (rc != X86EMUL_CONTINUE)
 			break;
-		assign_register(reg_rmw(ctxt, reg), val, ctxt->op_bytes);
+		insn_assign_reg(reg_rmw(ctxt, reg), val, ctxt->op_bytes);
 		--reg;
 	}
 	return rc;
-- 
2.54.0

[PATCH v5 3/3] x86/tdx: Fix zero-extension for 32-bit port I/O

[Kiryl Shutsemau]

From: "Kiryl Shutsemau (Meta)" <kas@kernel.org>

According to x86 architecture rules, 32-bit operations zero-extend the
result to 64 bits. The current implementation of handle_in() only masks
the lower 32 bits, which preserves the upper 32 bits of RAX when a
32-bit port IN instruction is emulated.

Use insn_assign_reg() to write the result back into RAX with proper
partial-register-write semantics: 1- and 2-byte forms leave the upper
bits untouched, the 4-byte form zero-extends to the full register.

Fixes: 03149948832a ("x86/tdx: Port I/O: Add runtime hypercalls")
Reported-by: Borys Tsyrulnikov <tsyrulnikov.borys@gmail.com>
Link: https://lore.kernel.org/all/CAKw_Dz96rfSQc6Rn+9QBcUFHhmkK+9zu+P=bxowfZwxrATCBRg@mail.gmail.com/
Signed-off-by: Kiryl Shutsemau (Meta) <kas@kernel.org>
Reviewed-by: Binbin Wu <binbin.wu@linux.intel.com>
Cc: stable@vger.kernel.org
---
 arch/x86/coco/tdx/tdx.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index b8bbd715fb62..f904a636d449 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c
@@ -694,8 +694,8 @@ static bool handle_in(struct pt_regs *regs, int size, int port)
 		.r13 = PORT_READ,
 		.r14 = port,
 	};
-	u64 mask = GENMASK(BITS_PER_BYTE * size - 1, 0);
 	bool success;
+	u64 val;
 
 	/*
 	 * Emulate the I/O read via hypercall. More info about ABI can be found
@@ -703,11 +703,9 @@ static bool handle_in(struct pt_regs *regs, int size, int port)
 	 * "TDG.VP.VMCALL<Instruction.IO>".
 	 */
 	success = !__tdx_hypercall(&args);
+	val = success ? args.r11 : 0;
 
-	/* Update part of the register affected by the emulated instruction */
-	regs->ax &= ~mask;
-	if (success)
-		regs->ax |= args.r11 & mask;
+	insn_assign_reg(&regs->ax, val, size);
 
 	return success;
 }
-- 
2.54.0

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

[Sean Christopherson]

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
> + * 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;

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

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

[David Laight]

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;
> > +	}
> > +}  

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

[Kiryl Shutsemau]

On Wed, Jul 01, 2026 at 06:00:33PM +0100, David Laight wrote:
> 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.

Agreed.  Dropped the IN[BWL]/MOV[BWL] examples and reworded the comment
to describe architectural GPR-write behaviour with a pointer to the SDM
section instead.  I also spelled out that @bytes is the width of the
write, not a property of the instruction, to cover the sign-extending
MOV case you raised.

> > 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'.

Reworded to "offsets the pointer by one byte".

> > > + * 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;

Added on the 4-byte case, slightly reworded.

> 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.

I'd rather keep the body exactly as KVM has it today.  This is now a
straight move + rename with no functional change, and the v4 attempt to
rewrite it with arithmetic is precisely what introduced the AH/CH/DH/BH
clobber Sashiko flagged.  Tidying the casts turns it back into a rewrite
and diverges from the form KVM has shipped for years.  Feel free to
submit a separate cleanup on top if you feel strongly.

Updated patch below; I'll fold it into v6.

-- >8 --
Subject: [PATCH] x86/insn-eval: Move assign_register() out of KVM as insn_assign_reg()

KVM's instruction emulator has a small helper, assign_register(), that
writes a value into a register following the x86 rules for writes to
general-purpose registers: an 8- or 16-bit write leaves the rest of the
register untouched, a 32-bit write zero-extends the result to 64 bits,
and a 64-bit write replaces the whole register.

The TDX guest #VE handler needs the same logic for port I/O emulation
to get 32-bit zero-extension right.  Rather than add a third copy of
the same switch, move the helper verbatim to <asm/insn-eval.h>, rename
it to insn_assign_reg(), and route KVM's callers through 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 | 36 ++++++++++++++++++++++++++++++++
 arch/x86/kvm/emulate.c           | 26 ++++-------------------
 2 files changed, 40 insertions(+), 22 deletions(-)

diff --git a/arch/x86/include/asm/insn-eval.h b/arch/x86/include/asm/insn-eval.h
index 4733e9064ee5..ae05647a0afb 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,39 @@ enum insn_mmio_type insn_decode_mmio(struct insn *insn, int *bytes);

 bool insn_is_nop(struct insn *insn);

+/*
+ * Write @val into *@reg following the x86 rules for writes to
+ * general-purpose registers (Intel SDM Vol. 1, "General-Purpose
+ * Registers in 64-Bit Mode"): an 8- or 16-bit write leaves the rest of
+ * the register untouched, a 32-bit write zero-extends the result into
+ * the upper 32 bits, and a 64-bit write replaces the whole register.
+ *
+ * @bytes is the width of the write, not a property of the instruction:
+ * an instruction that, say, sign-extends a 32-bit immediate into a
+ * 64-bit register does a 64-bit write here.
+ *
+ * @reg need not be 8-byte aligned: KVM's instruction emulator offsets
+ * the pointer by one byte to address the high-byte registers (AH, CH,
+ * DH, BH).  Use narrow stores for the sub-word cases so the access
+ * width matches @bytes and the adjacent bytes are left alone.
+ */
+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:
+		/* A 32-bit write zero-extends into the upper 32 bits. */
+		*reg = (u32)val;
+		break;
+	case 8:
+		*reg = val;
+		break;
+	}
+}
+
 #endif /* _ASM_X86_INSN_EVAL_H */
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index b566ab5c7515..c6dcb5ac48af 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -24,6 +24,7 @@
 #include "kvm_emulate.h"
 #include <linux/stringify.h>
 #include <asm/debugreg.h>
+#include <asm/insn-eval.h>
 #include <asm/nospec-branch.h>
 #include <asm/ibt.h>
 #include <asm/text-patching.h>
@@ -439,25 +440,6 @@ static void assign_masked(ulong *dest, ulong src, ulong mask)
 	*dest = (*dest & ~mask) | (src & mask);
 }

-static void assign_register(unsigned long *reg, u64 val, int bytes)
-{
-	/* The 4-byte case *is* correct: in 64-bit mode we zero-extend. */
-	switch (bytes) {
-	case 1:
-		*(u8 *)reg = (u8)val;
-		break;
-	case 2:
-		*(u16 *)reg = (u16)val;
-		break;
-	case 4:
-		*reg = (u32)val;
-		break;	/* 64b: zero-extend */
-	case 8:
-		*reg = val;
-		break;
-	}
-}
-
 static inline unsigned long ad_mask(struct x86_emulate_ctxt *ctxt)
 {
 	return (1UL << (ctxt->ad_bytes << 3)) - 1;
@@ -505,7 +487,7 @@ register_address_increment(struct x86_emulate_ctxt *ctxt, int reg, int inc)
 {
 	ulong *preg = reg_rmw(ctxt, reg);

-	assign_register(preg, *preg + inc, ctxt->ad_bytes);
+	insn_assign_reg(preg, *preg + inc, ctxt->ad_bytes);
 }

 static void rsp_increment(struct x86_emulate_ctxt *ctxt, int inc)
@@ -1767,7 +1749,7 @@ static int load_segment_descriptor(struct x86_emulate_ctxt *ctxt,

 static void write_register_operand(struct operand *op)
 {
-	return assign_register(op->addr.reg, op->val, op->bytes);
+	return insn_assign_reg(op->addr.reg, op->val, op->bytes);
 }

 static int writeback(struct x86_emulate_ctxt *ctxt, struct operand *op)
@@ -2008,7 +1990,7 @@ static int em_popa(struct x86_emulate_ctxt *ctxt)
 		rc = emulate_pop(ctxt, &val, ctxt->op_bytes);
 		if (rc != X86EMUL_CONTINUE)
 			break;
-		assign_register(reg_rmw(ctxt, reg), val, ctxt->op_bytes);
+		insn_assign_reg(reg_rmw(ctxt, reg), val, ctxt->op_bytes);
 		--reg;
 	}
 	return rc;

-- 
  Kiryl Shutsemau / Kirill A. Shutemov