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

[Sean Christopherson <seanjc@google.com>]

In the shortlog, please capture that code is being moved out of KVM.  This isn't
"adding" a helper, it's moving and renaming an existing helper.   The misleading
shortlog is quite literally why I didn't look at this for a ~month, I thought it
was entirely outside of my normal scope.

On Thu, Jun 04, 2026, Kiryl Shutsemau (Meta) wrote:
> 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.
> 
> Rewrite the body using arithmetic instead of pointer punning so the
> helper does not depend on -fno-strict-aliasing 

Meh, building with -fno-strict-aliasing is 100% mandatory for the kernel.  IMO,
even so much as suggesting that strict aliasing is something worth shooting for
is a misleading and wrong.

https://lore.kernel.org/all/CAHk-=wh921g_+TJ33JRxRGFM2uruMdqG-SONfFOD9UOsLQJ_uw@mail.gmail.com

> or little-endian byte order,

Oh come on, this is low level x86 code used to write registers.

> and 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 <kas@kernel.org>
> ---
>  arch/x86/include/asm/insn-eval.h | 25 +++++++++++++++++++++++++
>  arch/x86/kvm/emulate.c           | 26 ++++----------------------
>  2 files changed, 29 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/x86/include/asm/insn-eval.h b/arch/x86/include/asm/insn-eval.h
> index 4733e9064ee5..85251e718a77 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,28 @@ 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

Careful with the "upper bits" wording.  As Sashiko pointed out, this is used for
{A,B,C,D}H sub-registers as well, in which case the *lower* bits are also left
untouched.

> + * 4-byte write zero-extends to 64 bits (matching IN[BWL], MOV[BWL]
> + * etc.); an 8-byte write overwrites the full register.
> + */
> +static inline void insn_assign_reg(unsigned long *reg, u64 val, int bytes)
> +{
> +	switch (bytes) {
> +	case 1:
> +		*reg = (*reg & ~0xfful)   | (val & 0xff);

Sashiko's feedback aside, I strongly prefer KVM's approach as I find it much more
intuitive.  And its far, far more consistent with respect to the 4-byte and 8-byte
cases.

> +		break;
> +	case 2:
> +		*reg = (*reg & ~0xfffful) | (val & 0xffff);
> +		break;
> +	case 4:
> +		*reg = (u32)val;
> +		break;
> +	case 8:
> +		*reg = val;
> +		break;
> +	}
> +}