Re: [PATCH v2 04/16] KVM: VMX: Introduce unified instruction info structure

[Sean Christopherson <seanjc@google.com>]

On Mon, Jan 12, 2026, Chang S. Bae wrote:
> Define a unified data structure that can represent both the legacy and
> extended VMX instruction information formats.
> 
> VMX provides per-instruction metadata for VM exits to help decode the
> attributes of the instruction that triggered the exit. The legacy format,
> however, only supports up to 16 GPRs and thus cannot represent EGPRs. To
> support these new registers, VMX introduces an extended 64-bit layout.
> 
> Instead of maintaining separate storage for each format, a single
> union structure makes the overall handling simple. The field names are
> consistent across both layouts. While the presence of certain fields
> depends on the instruction type, the offsets remain fixed within each
> format.
> 
> Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
> ---
>  arch/x86/kvm/vmx/vmx.h | 61 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
> 
> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> index bc3ed3145d7e..567320115a5a 100644
> --- a/arch/x86/kvm/vmx/vmx.h
> +++ b/arch/x86/kvm/vmx/vmx.h
> @@ -311,6 +311,67 @@ struct kvm_vmx {
>  	u64 *pid_table;
>  };
>  
> +/*
> + * 32-bit layout of the legacy instruction information field. This format
> + * supports the 16 legacy GPRs.
> + */
> +struct base_insn_info {
> +	u32 scale		: 2;	/* Scaling factor */
> +	u32 reserved1		: 1;
> +	u32 reg1		: 4;	/* First register index */
> +	u32 asize		: 3;	/* Address size */
> +	u32 is_reg		: 1;	/* 0: memory, 1: register */
> +	u32 osize		: 2;	/* Operand size */
> +	u32 reserved2		: 2;
> +	u32 seg			: 3;	/* Segment register index */
> +	u32 index		: 4;	/* Index register index */
> +	u32 index_invalid	: 1;	/* 0: valid, 1: invalid */
> +	u32 base		: 4;	/* Base register index */
> +	u32 base_invalid	: 1;	/* 0: valid, 1: invalid */
> +	u32 reg2		: 4;	/* Second register index */
> +};
> +
> +/*
> + * 64-bit layout of the extended instruction information field, which
> + * supports EGPRs.
> + */
> +struct ext_insn_info {
> +	u64 scale		: 2;	/* Scaling factor */
> +	u64 asize		: 2;	/* Address size */
> +	u64 is_reg		: 1;	/* 0: memory, 1: register */
> +	u64 osize		: 2;	/* Operand size */
> +	u64 seg			: 3;	/* Segment register index */
> +	u64 index_invalid	: 1;	/* 0: valid, 1: invalid */
> +	u64 base_invalid	: 1;	/* 0: valid, 1: invalid */
> +	u64 reserved1		: 4;
> +	u64 reg1		: 5;	/* First register index */
> +	u64 reserved2		: 3;
> +	u64 index		: 5;	/* Index register index */
> +	u64 reserved3		: 3;
> +	u64 base		: 5;	/* Base register index */
> +	u64 reserved4		: 3;
> +	u64 reg2		: 5;	/* Second register index */
> +	u64 reserved5		: 19;
> +};
> +
> +/* Union for accessing either the legacy or extended format. */
> +union insn_info {
> +	struct base_insn_info base;
> +	struct ext_insn_info  ext;
> +	u32 word;
> +	u64 dword;

word is 16 bits, dword is 32 bits, qword is 64 bits.

> +};
> +
> +/*
> + * Wrapper structure combining the instruction info and a flag indicating
> + * whether the extended layout is in use.
> + */
> +struct vmx_insn_info {
> +	/* true if using the extended layout */
> +	bool extended;
> +	union insn_info info;
> +};

Absolutely not.  I despise bit fields, as they're extremely difficult to review,
don't help developers/debuggers understand the expected layout (finding flags and
whatnot in .h files is almost always faster than searching the SDM), and they
often generate suboptimal code.

This is also infrastructure overkill.  Two bitfields, a union, and another struct,
just to track a 64-bit value.  And the macros added later on only add to the
obfuscation.

Even worse, saving the "extended" flag on the stack and passing it around turns
a static branch into a dynamic branch.

I don't see any reason to do anything more complicated than:

static inline u64 vmx_get_insn_info(void)
{
	if (vmx_insn_info_extended())
		return vmcs_read64(EXTENDED_INSTRUCTION_INFO);

	return vmcs_read32(VMX_INSTRUCTION_INFO);
}

static inline int vmx_get_insn_info_reg(u64 insn_info)
{
	return vmx_insn_info_extended() ? (insn_info >> ??) & 0x1f :
					  (insn_info >> 3) & 0xf;
}