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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.05.2026 11:14, Ross Lagerwall wrote:
> On 5/21/26 12:57 PM, Jan Beulich wrote:
>> On 21.05.2026 12:12, Ross Lagerwall wrote:
>>> On 5/19/26 10:49 AM, Jan Beulich wrote:
>>>> On 18.05.2026 15:14, Ross Lagerwall wrote:
>>>>> --- 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?
>>>
>>> No objection to that. I do wonder though if using the instruction encoding 
>>> like
>>> this is the best way of passing through the instruction to the fast path in
>>> hvm_emulate_one_ctxt() since I think in some cases the instruction encoding
>>> may not match the actual instruction that triggered the VMEXIT.
>>
>> Do you have an example? If so, that would indeed be at risk of being 
>> misleading
>> or actively confusing. (Of course INSTR_VMCALL wants renaming, as was already
>> suggested.)
>>
> 
> VMEXIT_CR0_READ may be triggered by MOV-from-CR or SMSW. There are probably
> other examples...

Like LMSW, then also relevant to VMX. I think these indeed better wouldn't use
the wrong INSTR_*. On SVM with decode assists, SMSW can be identified, but
LMSW / CLTS are ambiguous without fetching the insn.

Jan