Re: [RFC v1 5/7] x86/hvm: Move INSTR_* constants to hvm.h

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.05.2026 15:14, Ross Lagerwall wrote:
> These aren't specific to SVM and will be used for emulator fast path so
> move them to hvm.h.

Some are SVM-specific, e.g. ...

> --- a/xen/arch/x86/hvm/svm/svm.h
> +++ b/xen/arch/x86/hvm/svm/svm.h
> @@ -36,35 +36,6 @@ static inline void svm_invlpga(unsigned long linear, 
> uint32_t asid)
>      asm volatile ( "invlpga" :: "a" (linear), "c" (asid) );
>  }
>  
> -/*
> - * Encoding for svm_get_insn_len().  We take X86EMUL_OPC() for the main
> - * opcode, shifted left to make room for the ModRM byte.
> - *
> - * The Grp7 instructions have their ModRM byte expressed in octal for easier
> - * cross referencing with the opcode extension table.
> - */
> -#define INSTR_ENC(opc, modrm) (((opc) << 8) | (modrm))
> -
> -#define INSTR_PAUSE      INSTR_ENC(X86EMUL_OPC_F3(0, 0x90), 0)
> -#define INSTR_INT3       INSTR_ENC(X86EMUL_OPC(   0, 0xcc), 0)
> -#define INSTR_ICEBP      INSTR_ENC(X86EMUL_OPC(   0, 0xf1), 0)
> -#define INSTR_HLT        INSTR_ENC(X86EMUL_OPC(   0, 0xf4), 0)
> -#define INSTR_XSETBV     INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0321) /* 
> octal-ok */
> -#define INSTR_VMRUN      INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0330) /* 
> octal-ok */
> -#define INSTR_VMCALL     INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0331) /* 
> octal-ok */
> -#define INSTR_VMLOAD     INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0332) /* 
> octal-ok */
> -#define INSTR_VMSAVE     INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0333) /* 
> octal-ok */
> -#define INSTR_STGI       INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0334) /* 
> octal-ok */
> -#define INSTR_CLGI       INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0335) /* 
> octal-ok */
> -#define INSTR_INVLPGA    INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0337) /* 
> octal-ok */

... the 7 ones above.

> --- a/xen/arch/x86/include/asm/hvm/hvm.h
> +++ b/xen/arch/x86/include/asm/hvm/hvm.h
> @@ -851,6 +851,35 @@ static inline void hvm_sync_pir_to_irr(struct vcpu *v)
>          alternative_vcall(hvm_funcs.sync_pir_to_irr, v);
>  }
>  
> +/*
> + * Encoding for svm_get_insn_len().  We take X86EMUL_OPC() for the main
> + * opcode, shifted left to make room for the ModRM byte.

With all of this moved, the comment wants adjusting, at the very least by
putting "e.g." in front of the function name.

> + * The Grp7 instructions have their ModRM byte expressed in octal for easier
> + * cross referencing with the opcode extension table.
> + */
> +#define INSTR_ENC(opc, modrm) (((opc) << 8) | (modrm))
> +
> +#define INSTR_PAUSE      INSTR_ENC(X86EMUL_OPC_F3(0, 0x90), 0)
> +#define INSTR_INT3       INSTR_ENC(X86EMUL_OPC(   0, 0xcc), 0)
> +#define INSTR_ICEBP      INSTR_ENC(X86EMUL_OPC(   0, 0xf1), 0)
> +#define INSTR_HLT        INSTR_ENC(X86EMUL_OPC(   0, 0xf4), 0)
> +#define INSTR_XSETBV     INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0321) /* 
> octal-ok */
> +#define INSTR_VMRUN      INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0330) /* 
> octal-ok */
> +#define INSTR_VMCALL     INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0331) /* 
> octal-ok */
> +#define INSTR_VMLOAD     INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0332) /* 
> octal-ok */
> +#define INSTR_VMSAVE     INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0333) /* 
> octal-ok */
> +#define INSTR_STGI       INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0334) /* 
> octal-ok */
> +#define INSTR_CLGI       INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0335) /* 
> octal-ok */
> +#define INSTR_INVLPGA    INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0337) /* 
> octal-ok */
> +#define INSTR_RDTSCP     INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0371) /* 
> octal-ok */
> +#define INSTR_INVD       INSTR_ENC(X86EMUL_OPC(0x0f, 0x08), 0)
> +#define INSTR_WBINVD     INSTR_ENC(X86EMUL_OPC(0x0f, 0x09), 0)
> +#define INSTR_WRMSR      INSTR_ENC(X86EMUL_OPC(0x0f, 0x30), 0)
> +#define INSTR_RDTSC      INSTR_ENC(X86EMUL_OPC(0x0f, 0x31), 0)
> +#define INSTR_RDMSR      INSTR_ENC(X86EMUL_OPC(0x0f, 0x32), 0)
> +#define INSTR_CPUID      INSTR_ENC(X86EMUL_OPC(0x0f, 0xa2), 0)
> +
>  #else  /* CONFIG_HVM */

I further wonder whether putting this in hvm.h is a good idea. Is there
anything wrong with using a brand new header, e.g. instr-enc.h?

Jan