On 22.02.2023 13:00, Xenia Ragiadakou wrote:
--- /dev/null
+++ b/xen/arch/x86/hvm/vmx/vmx.h
@@ -0,0 +1,459 @@
+#include <xen/sched.h>
+
+#include <asm/asm_defns.h>
+#include <asm/hvm/vmx/vmcs.h>
+#include <asm/hvm/vmx/vmx.h>
+#include <asm/types.h>
As in the earlier patch, please use xen/types.h right away.
+extern int8_t opt_ept_exec_sp;
+
+#define PI_xAPIC_NDST_MASK 0xFF00
+
+void vmx_asm_vmexit_handler(struct cpu_user_regs);
Even if it was like this originally, this is bogus. This not-really-a-
function doesn't take any parameters. It's declaration also looks to be
missing cf_check - Andrew, was this deliberate?
+void vmx_intr_assist(void);
+void noreturn cf_check vmx_do_resume(void);
+void cf_check vmx_vlapic_msr_changed(struct vcpu *v);
+void vmx_realmode(struct cpu_user_regs *regs);
+void vmx_update_debug_state(struct vcpu *v);
+void vmx_update_exception_bitmap(struct vcpu *v);
+void vmx_update_cpu_exec_control(struct vcpu *v);
+void vmx_update_secondary_exec_control(struct vcpu *v);
+
+#define POSTED_INTR_ON 0
+#define POSTED_INTR_SN 1
+static inline int pi_test_and_set_pir(uint8_t vector, struct pi_desc *pi_desc)
Nit: Blank line please after the #define-s.
+{
+ return test_and_set_bit(vector, pi_desc->pir);
+}
+
+static inline int pi_test_pir(uint8_t vector, const struct pi_desc *pi_desc)
+{
+ return test_bit(vector, pi_desc->pir);
+}
+
+static inline int pi_test_and_set_on(struct pi_desc *pi_desc)
+{
+ return test_and_set_bit(POSTED_INTR_ON, &pi_desc->control);
+}
+
+static inline void pi_set_on(struct pi_desc *pi_desc)
+{
+ set_bit(POSTED_INTR_ON, &pi_desc->control);
+}
+
+static inline int pi_test_and_clear_on(struct pi_desc *pi_desc)
+{
+ return test_and_clear_bit(POSTED_INTR_ON, &pi_desc->control);
+}
+
+static inline int pi_test_on(struct pi_desc *pi_desc)
+{
+ return pi_desc->on;
+}
+
+static inline unsigned long pi_get_pir(struct pi_desc *pi_desc, int group)
+{
+ return xchg(&pi_desc->pir[group], 0);
+}
+
+static inline int pi_test_sn(struct pi_desc *pi_desc)
+{
+ return pi_desc->sn;
+}
+
+static inline void pi_set_sn(struct pi_desc *pi_desc)
+{
+ set_bit(POSTED_INTR_SN, &pi_desc->control);
+}
+
+static inline void pi_clear_sn(struct pi_desc *pi_desc)
+{
+ clear_bit(POSTED_INTR_SN, &pi_desc->control);
+}
Considering all of these are currently used by vmx.c only I wonder whether
we shouldn't go a step further and put the posted-interrupt stuff in its
own private header.
+/*
+ * Interruption-information format
+ *
+ * Note INTR_INFO_NMI_UNBLOCKED_BY_IRET is also used with Exit Qualification
+ * field for EPT violations, PML full and SPP-related event vmexits.
+ */
+#define INTR_INFO_VECTOR_MASK 0xff /* 7:0 */
+#define INTR_INFO_INTR_TYPE_MASK 0x700 /* 10:8 */
+#define INTR_INFO_DELIVER_CODE_MASK 0x800 /* 11 */
+#define INTR_INFO_NMI_UNBLOCKED_BY_IRET 0x1000 /* 12 */
+#define INTR_INFO_VALID_MASK 0x80000000 /* 31 */
+#define INTR_INFO_RESVD_BITS_MASK 0x7ffff000
+
+/*
+ * Exit Qualifications for NOTIFY VM EXIT
+ */
+#define NOTIFY_VM_CONTEXT_INVALID 1u
+
+/*
+ * Exit Qualifications for MOV for Control Register Access
+ */
+enum {
+ VMX_CR_ACCESS_TYPE_MOV_TO_CR,
+ VMX_CR_ACCESS_TYPE_MOV_FROM_CR,
+ VMX_CR_ACCESS_TYPE_CLTS,
+ VMX_CR_ACCESS_TYPE_LMSW,
+};
+typedef union cr_access_qual {
Nit: Blank line please again above here.
+ unsigned long raw;
+ struct {
+ uint16_t cr:4,
+ access_type:2, /* VMX_CR_ACCESS_TYPE_* */
+ lmsw_op_type:1, /* 0 => reg, 1 => mem */
+ :1,
+ gpr:4,
+ :4;
+ uint16_t lmsw_data;
+ uint32_t :32;
+ };
+} __transparent__ cr_access_qual_t;
+
+/*
+ * Access Rights
+ */
+#define X86_SEG_AR_SEG_TYPE 0xf /* 3:0, segment type */
+#define X86_SEG_AR_DESC_TYPE (1u << 4) /* 4, descriptor type */
+#define X86_SEG_AR_DPL 0x60 /* 6:5, descriptor privilege level
*/
+#define X86_SEG_AR_SEG_PRESENT (1u << 7) /* 7, segment present */
+#define X86_SEG_AR_AVL (1u << 12) /* 12, available for system
software */
+#define X86_SEG_AR_CS_LM_ACTIVE (1u << 13) /* 13, long mode active (CS only) */
+#define X86_SEG_AR_DEF_OP_SIZE (1u << 14) /* 14, default operation size */
+#define X86_SEG_AR_GRANULARITY (1u << 15) /* 15, granularity */
+#define X86_SEG_AR_SEG_UNUSABLE (1u << 16) /* 16, segment unusable */
+
+extern uint8_t posted_intr_vector;
+
+#define INVEPT_SINGLE_CONTEXT 1
+#define INVEPT_ALL_CONTEXT 2
+
+#define INVVPID_INDIVIDUAL_ADDR 0
+#define INVVPID_SINGLE_CONTEXT 1
+#define INVVPID_ALL_CONTEXT 2
+#define INVVPID_SINGLE_CONTEXT_RETAINING_GLOBAL 3
+
+static always_inline void __vmptrld(u64 addr)
+{
+ asm volatile (
+#ifdef HAVE_AS_VMX
+ "vmptrld %0\n"
+#else
+ VMPTRLD_OPCODE MODRM_EAX_06
+#endif
+ /* CF==1 or ZF==1 --> BUG() */
+ UNLIKELY_START(be, vmptrld)
+ _ASM_BUGFRAME_TEXT(0)
+ UNLIKELY_END_SECTION
+ :
+#ifdef HAVE_AS_VMX
+ : "m" (addr),
+#else
+ : "a" (&addr),
+#endif
+ _ASM_BUGFRAME_INFO(BUGFRAME_bug, __LINE__, __FILE__, 0)
+ : "memory");
+}
+
+static always_inline void __vmpclear(u64 addr)
+{
+ asm volatile (
+#ifdef HAVE_AS_VMX
+ "vmclear %0\n"
+#else
+ VMCLEAR_OPCODE MODRM_EAX_06
+#endif
+ /* CF==1 or ZF==1 --> BUG() */
+ UNLIKELY_START(be, vmclear)
+ _ASM_BUGFRAME_TEXT(0)
+ UNLIKELY_END_SECTION
+ :
+#ifdef HAVE_AS_VMX
+ : "m" (addr),
+#else
+ : "a" (&addr),
+#endif
+ _ASM_BUGFRAME_INFO(BUGFRAME_bug, __LINE__, __FILE__, 0)
+ : "memory");
+}
+
+static always_inline void __vmwrite(unsigned long field, unsigned long value)
+{
+ asm volatile (
+#ifdef HAVE_AS_VMX
+ "vmwrite %1, %0\n"
+#else
+ VMWRITE_OPCODE MODRM_EAX_ECX
+#endif
+ /* CF==1 or ZF==1 --> BUG() */
+ UNLIKELY_START(be, vmwrite)
+ _ASM_BUGFRAME_TEXT(0)
+ UNLIKELY_END_SECTION
+ :
+#ifdef HAVE_AS_VMX
+ : "r" (field) , "rm" (value),
+#else
+ : "a" (field) , "c" (value),
+#endif
+ _ASM_BUGFRAME_INFO(BUGFRAME_bug, __LINE__, __FILE__, 0)
+ );
+}
+
+
+#ifdef HAVE_AS_VMX
Nit: No double blank lines please (there's at least one more instance
further down).
+# define GAS_VMX_OP(yes, no) yes
+#else
+# define GAS_VMX_OP(yes, no) no
+#endif
+
+static inline enum vmx_insn_errno vmread_safe(unsigned long field,
+ unsigned long *value)
+{
+ unsigned long ret = VMX_INSN_SUCCEED;
+ bool fail_invalid, fail_valid;
+
+ asm volatile ( GAS_VMX_OP("vmread %[field], %[value]\n\t",
+ VMREAD_OPCODE MODRM_EAX_ECX)
+ ASM_FLAG_OUT(, "setc %[invalid]\n\t")
+ ASM_FLAG_OUT(, "setz %[valid]\n\t")
+ : ASM_FLAG_OUT("=@ccc", [invalid] "=rm") (fail_invalid),
+ ASM_FLAG_OUT("=@ccz", [valid] "=rm") (fail_valid),
+ [value] GAS_VMX_OP("=rm", "=c") (*value)
+ : [field] GAS_VMX_OP("r", "a") (field));
+
+ if ( unlikely(fail_invalid) )
+ ret = VMX_INSN_FAIL_INVALID;
+ else if ( unlikely(fail_valid) )
+ __vmread(VM_INSTRUCTION_ERROR, &ret);
+
+ return ret;
+}
+
+static inline enum vmx_insn_errno vmwrite_safe(unsigned long field,
+ unsigned long value)
+{
+ unsigned long ret = VMX_INSN_SUCCEED;
+ bool fail_invalid, fail_valid;
+
+ asm volatile ( GAS_VMX_OP("vmwrite %[value], %[field]\n\t",
+ VMWRITE_OPCODE MODRM_EAX_ECX)
+ ASM_FLAG_OUT(, "setc %[invalid]\n\t")
+ ASM_FLAG_OUT(, "setz %[valid]\n\t")
+ : ASM_FLAG_OUT("=@ccc", [invalid] "=rm") (fail_invalid),
+ ASM_FLAG_OUT("=@ccz", [valid] "=rm") (fail_valid)
+ : [field] GAS_VMX_OP("r", "a") (field),
+ [value] GAS_VMX_OP("rm", "c") (value));
+
+ if ( unlikely(fail_invalid) )
+ ret = VMX_INSN_FAIL_INVALID;
+ else if ( unlikely(fail_valid) )
+ __vmread(VM_INSTRUCTION_ERROR, &ret);
+
+ return ret;
+}
+
+#undef GAS_VMX_OP
+
+
+static always_inline void __invept(unsigned long type, uint64_t eptp)
+{
+ struct {
+ uint64_t eptp, rsvd;
+ } operand = { eptp };
+
+ /*
+ * If single context invalidation is not supported, we escalate to
+ * use all context invalidation.
+ */
+ if ( (type == INVEPT_SINGLE_CONTEXT) &&
+ !cpu_has_vmx_ept_invept_single_context )
+ type = INVEPT_ALL_CONTEXT;
+
+ asm volatile (
+#ifdef HAVE_AS_EPT
+ "invept %0, %1\n"
+#else
+ INVEPT_OPCODE MODRM_EAX_08
+#endif
+ /* CF==1 or ZF==1 --> BUG() */
+ UNLIKELY_START(be, invept)
+ _ASM_BUGFRAME_TEXT(0)
+ UNLIKELY_END_SECTION
+ :
+#ifdef HAVE_AS_EPT
+ : "m" (operand), "r" (type),
+#else
+ : "a" (&operand), "c" (type),
+#endif
+ _ASM_BUGFRAME_INFO(BUGFRAME_bug, __LINE__, __FILE__, 0)
+ : "memory" );
+}
+
+static always_inline void __invvpid(unsigned long type, u16 vpid, u64 gva)
+{
+ struct __packed {
+ u64 vpid:16;
+ u64 rsvd:48;
+ u64 gva;
+ } operand = {vpid, 0, gva};
I don't think __packed is needed here. I wonder if it could be dropped as
you move the code. In any event, here and elsewhere, u64 -> uint64_t (and
alike) please.
+ /* Fix up #UD exceptions which occur when TLBs are flushed before VMXON. */
+ asm volatile ( "1: "
+#ifdef HAVE_AS_EPT
+ "invvpid %0, %1\n"
+#else
+ INVVPID_OPCODE MODRM_EAX_08
+#endif
+ /* CF==1 or ZF==1 --> BUG() */
+ UNLIKELY_START(be, invvpid)
+ _ASM_BUGFRAME_TEXT(0)
+ UNLIKELY_END_SECTION "\n"
+ "2:"
+ _ASM_EXTABLE(1b, 2b)
+ :
+#ifdef HAVE_AS_EPT
+ : "m" (operand), "r" (type),
+#else
+ : "a" (&operand), "c" (type),
+#endif
+ _ASM_BUGFRAME_INFO(BUGFRAME_bug, __LINE__, __FILE__, 0)
+ : "memory" );
+}
+
+static inline void ept_sync_all(void)
+{
+ __invept(INVEPT_ALL_CONTEXT, 0);
+}
+
+static inline void vpid_sync_vcpu_gva(struct vcpu *v, unsigned long gva)
+{
+ int type = INVVPID_INDIVIDUAL_ADDR;
+
+ /*
+ * If individual address invalidation is not supported, we escalate to
+ * use single context invalidation.
+ */
+ if ( likely(cpu_has_vmx_vpid_invvpid_individual_addr) )
+ goto execute_invvpid;
+
+ type = INVVPID_SINGLE_CONTEXT;
+
+ /*
+ * If single context invalidation is not supported, we escalate to
+ * use all context invalidation.
+ */
+ if ( !cpu_has_vmx_vpid_invvpid_single_context )
+ type = INVVPID_ALL_CONTEXT;
+
+execute_invvpid:
Nit (style): Labels indented by at least one blank please.
+ __invvpid(type, v->arch.hvm.n1asid.asid, (u64)gva);
+}
+
+static inline void vpid_sync_all(void)
+{
+ __invvpid(INVVPID_ALL_CONTEXT, 0, 0);
+}
+
+static inline void __vmxoff(void)
+{
+ asm volatile (
+ VMXOFF_OPCODE
+ : : : "memory" );
All on a single line perhaps?
+}
+
+static inline int __vmxon(u64 addr)
+{
+ int rc;
+
+ asm volatile (
+ "1: " VMXON_OPCODE MODRM_EAX_06 "\n"
+ " setna %b0 ; neg %0\n" /* CF==1 or ZF==1 --> rc = -1 */
+ "2:\n"
+ ".section .fixup,\"ax\"\n"
+ "3: sub $2,%0 ; jmp 2b\n" /* #UD or #GP --> rc = -2 */
+ ".previous\n"
+ _ASM_EXTABLE(1b, 3b)
+ : "=q" (rc)
+ : "0" (0), "a" (&addr)
+ : "memory");
Nit: Missing blank before final parenthesis. Would also be nice if the
comments lined up.
+ return rc;
+}
+
+int cf_check vmx_guest_x86_mode(struct vcpu *v);
+unsigned int vmx_get_cpl(void);
+void vmx_inject_extint(int trap, uint8_t source);
+void vmx_inject_nmi(void);
+
+void update_guest_eip(void);
+
+void vmx_pi_per_cpu_init(unsigned int cpu);
+void vmx_pi_desc_fixup(unsigned int cpu);
+
+void vmx_sync_exit_bitmap(struct vcpu *v);
+
+#define APIC_INVALID_DEST 0xffffffff
+
+/* #VE information page */
+typedef struct {
+ u32 exit_reason;
+ u32 semaphore;
+ u64 exit_qualification;
+ u64 gla;
+ u64 gpa;
+ u16 eptp_index;
+} ve_info_t;
+
+/* VM-Exit instruction info for LIDT, LGDT, SIDT, SGDT */
+typedef union idt_or_gdt_instr_info {
+ unsigned long raw;
+ struct {
+ unsigned long scaling :2, /* bits 0:1 - Scaling */
+ :5, /* bits 6:2 - Undefined */
+ addr_size :3, /* bits 9:7 - Address size */
+ :1, /* bit 10 - Cleared to 0 */
+ operand_size :1, /* bit 11 - Operand size */
+ :3, /* bits 14:12 - Undefined */
+ segment_reg :3, /* bits 17:15 - Segment register */
+ index_reg :4, /* bits 21:18 - Index register */
+ index_reg_invalid :1, /* bit 22 - Index register invalid */
+ base_reg :4, /* bits 26:23 - Base register */
+ base_reg_invalid :1, /* bit 27 - Base register invalid */
+ instr_identity :1, /* bit 28 - 0:GDT, 1:IDT */
+ instr_write :1, /* bit 29 - 0:store, 1:load */
+ :34; /* bits 30:63 - Undefined */
+ };
+} idt_or_gdt_instr_info_t;
+
+/* VM-Exit instruction info for LLDT, LTR, SLDT, STR */
+typedef union ldt_or_tr_instr_info {
+ unsigned long raw;
+ struct {
+ unsigned long scaling :2, /* bits 0:1 - Scaling */
+ :1, /* bit 2 - Undefined */
+ reg1 :4, /* bits 6:3 - Reg1 */
+ addr_size :3, /* bits 9:7 - Address size */
+ mem_reg :1, /* bit 10 - Mem/Reg */
+ :4, /* bits 14:11 - Undefined */
+ segment_reg :3, /* bits 17:15 - Segment register */
+ index_reg :4, /* bits 21:18 - Index register */
+ index_reg_invalid :1, /* bit 22 - Index register invalid */
+ base_reg :4, /* bits 26:23 - Base register */
+ base_reg_invalid :1, /* bit 27 - Base register invalid */
+ instr_identity :1, /* bit 28 - 0:LDT, 1:TR */
+ instr_write :1, /* bit 29 - 0:store, 1:load */
+ :34; /* bits 31:63 - Undefined */
+ };
+} ldt_or_tr_instr_info_t;
One file wide remark: I assume you've put things here in the order they
were in originally. I think it would be nice if some re-arrangement was
done, e.g. all structures first (unless there's a reason speaking
against doing so, and of course not including any which are local to
specific inline functions), then all variable decalarations, all function
delarations, and finally all inline functions.
