|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 6/9] x86/vmx: move declations used only by vmx code from vmx.h to private header
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.
Jan
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |