diff mbox series

[4/4] x86/vmx: cleanup vmx.h

Message ID 20230217184814.1243046-5-burzalodowa@gmail.com (mailing list archive)
State Superseded
Headers show
Series x86/hvm: {svm,vmx}.{c,h} cleanup | expand

Commit Message

Xenia Ragiadakou Feb. 17, 2023, 6:48 p.m. UTC
Do not include the headers:
  asm/i387.h
  asm/hvm/trace.h
  asm/processor.h
  asm/regs.h
because none of the declarations and macro definitions in them is used in
this file. Sort alphabetically the rest of the headers.
Fix build by including asm/i387.h in vmx.c, needed for vcpu_restore_fpu_lazy().

Move the definition of GAS_VMX_OP just above the functions that use it and
undefine it after its usage.

Move in vmcs.c the definitions of:
  ept_sync_all()
  __vmxoff()
  __vmxon()
because they are used only in this file. Take the opportunity to remove a
trailing white space.

Move in vmx.c the definitions of:
  pi_test_and_set_pir()
  pi_test_pir()
  pi_test_and_set_on()
  pi_set_on()
  pi_test_and_clear_on()
  pi_test_on()
  pi_get_pir()
  pi_test_sn()
  pi_set_sn()
  pi_clear_sn()
  vpid_sync_vcpu_gva()
because they are used only in this file.

Move in vmx.c the declarations of:
  ve_info_t
  ept_qual_t
  idt_or_gdt_instr_info_t
  ldt_or_tr_instr_info_t
because they are used only in this file.

No functional change intended.

Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
---
 xen/arch/x86/hvm/vmx/vmcs.c            |  31 ++++
 xen/arch/x86/hvm/vmx/vmx.c             | 141 ++++++++++++++++++
 xen/arch/x86/include/asm/hvm/vmx/vmx.h | 193 ++-----------------------
 3 files changed, 182 insertions(+), 183 deletions(-)

Comments

Jan Beulich Feb. 21, 2023, 11:23 a.m. UTC | #1
On 17.02.2023 19:48, Xenia Ragiadakou wrote:
> Do not include the headers:
>   asm/i387.h
>   asm/hvm/trace.h
>   asm/processor.h
>   asm/regs.h
> because none of the declarations and macro definitions in them is used in
> this file. Sort alphabetically the rest of the headers.
> Fix build by including asm/i387.h in vmx.c, needed for vcpu_restore_fpu_lazy().
> 
> Move the definition of GAS_VMX_OP just above the functions that use it and
> undefine it after its usage.
> 
> Move in vmcs.c the definitions of:
>   ept_sync_all()
>   __vmxoff()
>   __vmxon()
> because they are used only in this file. Take the opportunity to remove a
> trailing white space.

While the latter two are probably fine to be moved, I think the first one
wants to remain where it is, as new uses might appear.

> Move in vmx.c the definitions of:
>   pi_test_and_set_pir()
>   pi_test_pir()
>   pi_test_and_set_on()
>   pi_set_on()
>   pi_test_and_clear_on()
>   pi_test_on()
>   pi_get_pir()
>   pi_test_sn()
>   pi_set_sn()
>   pi_clear_sn()
>   vpid_sync_vcpu_gva()
> because they are used only in this file.

I'm not fully convinced of such movement. As a general remark: We typically
avoid "inline" for functions in .c files. Yet most if not all of the above
are pretty good candidates for being explicitly marked "inline".

> Move in vmx.c the declarations of:
>   ve_info_t
>   ept_qual_t
>   idt_or_gdt_instr_info_t
>   ldt_or_tr_instr_info_t
> because they are used only in this file.

I disagree with the movement of such types. Sooner or later they may want
using by vvmx.c as well. If you want to move them, then to a private header
under xen/arch/x86/hvm/vmx/.

Finally I think at least the individual groups of what is being moved or
adjusted want splitting into separate patches.

Jan
Xenia Ragiadakou Feb. 21, 2023, 11:39 a.m. UTC | #2
On 2/21/23 13:23, Jan Beulich wrote:
> On 17.02.2023 19:48, Xenia Ragiadakou wrote:
>> Do not include the headers:
>>    asm/i387.h
>>    asm/hvm/trace.h
>>    asm/processor.h
>>    asm/regs.h
>> because none of the declarations and macro definitions in them is used in
>> this file. Sort alphabetically the rest of the headers.
>> Fix build by including asm/i387.h in vmx.c, needed for vcpu_restore_fpu_lazy().
>>
>> Move the definition of GAS_VMX_OP just above the functions that use it and
>> undefine it after its usage.
>>
>> Move in vmcs.c the definitions of:
>>    ept_sync_all()
>>    __vmxoff()
>>    __vmxon()
>> because they are used only in this file. Take the opportunity to remove a
>> trailing white space.
> 
> While the latter two are probably fine to be moved, I think the first one
> wants to remain where it is, as new uses might appear.

Ok I will leave it where it is.

> 
>> Move in vmx.c the definitions of:
>>    pi_test_and_set_pir()
>>    pi_test_pir()
>>    pi_test_and_set_on()
>>    pi_set_on()
>>    pi_test_and_clear_on()
>>    pi_test_on()
>>    pi_get_pir()
>>    pi_test_sn()
>>    pi_set_sn()
>>    pi_clear_sn()
>>    vpid_sync_vcpu_gva()
>> because they are used only in this file.
> 
> I'm not fully convinced of such movement. As a general remark: We typically
> avoid "inline" for functions in .c files. Yet most if not all of the above
> are pretty good candidates for being explicitly marked "inline".

I could create a private header.

> 
>> Move in vmx.c the declarations of:
>>    ve_info_t
>>    ept_qual_t
>>    idt_or_gdt_instr_info_t
>>    ldt_or_tr_instr_info_t
>> because they are used only in this file.
> 
> I disagree with the movement of such types. Sooner or later they may want
> using by vvmx.c as well. If you want to move them, then to a private header
> under xen/arch/x86/hvm/vmx/.

Ok will do.

> 
> Finally I think at least the individual groups of what is being moved or
> adjusted want splitting into separate patches.

Ok.

> 
> Jan
diff mbox series

Patch

diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index c46bb55f05..a4de21d5dc 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -691,6 +691,37 @@  void cf_check vmx_cpu_dead(unsigned int cpu)
     vmx_pi_desc_fixup(cpu);
 }
 
+static inline void __vmxoff(void)
+{
+    asm volatile (
+        VMXOFF_OPCODE
+        : : : "memory" );
+}
+
+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");
+
+    return rc;
+}
+
+static inline void ept_sync_all(void)
+{
+    __invept(INVEPT_ALL_CONTEXT, 0);
+}
+
 static int _vmx_cpu_up(bool bsp)
 {
     u32 eax, edx;
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index d02ad01b9b..4eade64fd6 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -43,6 +43,7 @@ 
 #include <asm/hvm/vmx/vmcs.h>
 #include <asm/hvm/vmx/vmx.h>
 #include <asm/iocap.h>
+#include <asm/i387.h>
 #include <asm/monitor.h>
 #include <asm/msr.h>
 #include <asm/processor.h>
@@ -245,6 +246,58 @@  struct vmx_pi_blocking_vcpu {
  */
 static DEFINE_PER_CPU(struct vmx_pi_blocking_vcpu, vmx_pi_blocking);
 
+#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)
+{
+    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);
+}
+
 static void vmx_pi_switch_from(struct vcpu *v)
 {
     struct pi_desc *pi_desc = &v->arch.hvm.vmx.pi_desc;
@@ -1329,6 +1382,30 @@  gp_fault:
     return X86EMUL_EXCEPTION;
 }
 
+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:
+    __invvpid(type, v->arch.hvm.n1asid.asid, (u64)gva);
+}
+
 static void cf_check vmx_invlpg(struct vcpu *v, unsigned long linear)
 {
     if ( cpu_has_vmx_vpid )
@@ -2941,6 +3018,16 @@  static int cf_check vmx_vcpu_emulate_vmfunc(const struct cpu_user_regs *regs)
     return rc;
 }
 
+/* #VE information page */
+typedef struct {
+    u32 exit_reason;
+    u32 semaphore;
+    u64 exit_qualification;
+    u64 gla;
+    u64 gpa;
+    u16 eptp_index;
+} ve_info_t;
+
 static bool cf_check vmx_vcpu_emulate_ve(struct vcpu *v)
 {
     const struct page_info *pg = vcpu_altp2m(v).veinfo_pg;
@@ -3700,6 +3787,18 @@  static void vmx_do_extint(struct cpu_user_regs *regs)
     do_IRQ(regs);
 }
 
+/* EPT violation qualifications definitions */
+typedef union ept_qual {
+    unsigned long raw;
+    struct {
+        bool read:1, write:1, fetch:1,
+            eff_read:1, eff_write:1, eff_exec:1, /* eff_user_exec */:1,
+            gla_valid:1,
+            gla_fault:1; /* Valid iff gla_valid. */
+        unsigned long /* pad */:55;
+    };
+} __transparent__ ept_qual_t;
+
 static void ept_handle_violation(ept_qual_t q, paddr_t gpa)
 {
     unsigned long gla, gfn = gpa >> PAGE_SHIFT;
@@ -3956,6 +4055,48 @@  static void vmx_handle_xrstors(void)
     domain_crash(current->domain);
 }
 
+/* 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;
+
 static void vmx_handle_descriptor_access(uint32_t exit_reason)
 {
     uint64_t instr_info;
diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmx.h b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
index 5c748d5b09..2520be0824 100644
--- a/xen/arch/x86/include/asm/hvm/vmx/vmx.h
+++ b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
@@ -19,14 +19,10 @@ 
 #define __ASM_X86_HVM_VMX_VMX_H__
 
 #include <xen/sched.h>
-#include <asm/types.h>
-#include <asm/regs.h>
 #include <asm/asm_defns.h>
-#include <asm/processor.h>
-#include <asm/p2m.h>
-#include <asm/i387.h>
-#include <asm/hvm/trace.h>
 #include <asm/hvm/vmx/vmcs.h>
+#include <asm/p2m.h>
+#include <asm/types.h>
 
 extern int8_t opt_ept_exec_sp;
 
@@ -93,58 +89,6 @@  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)
-{
-    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);
-}
-
 /*
  * Exit Reasons
  */
@@ -321,12 +265,6 @@  extern uint8_t posted_intr_vector;
 #define INVVPID_ALL_CONTEXT                     2
 #define INVVPID_SINGLE_CONTEXT_RETAINING_GLOBAL 3
 
-#ifdef HAVE_AS_VMX
-# define GAS_VMX_OP(yes, no) yes
-#else
-# define GAS_VMX_OP(yes, no) no
-#endif
-
 static always_inline void __vmptrld(u64 addr)
 {
     asm volatile (
@@ -416,6 +354,12 @@  static always_inline void __vmwrite(unsigned long field, unsigned long value)
         );
 }
 
+#ifdef HAVE_AS_VMX
+# 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)
 {
@@ -462,6 +406,8 @@  static inline enum vmx_insn_errno vmwrite_safe(unsigned long field,
     return ret;
 }
 
+#undef GAS_VMX_OP
+
 static always_inline void __invept(unsigned long type, uint64_t eptp)
 {
     struct {
@@ -527,68 +473,13 @@  static always_inline void __invvpid(unsigned long type, u16 vpid, u64 gva)
                    : "memory" );
 }
 
-static inline void ept_sync_all(void)
-{
-    __invept(INVEPT_ALL_CONTEXT, 0);
-}
-
 void ept_sync_domain(struct p2m_domain *p2m);
 
-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:
-    __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" );
-}
-
-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");
-
-    return rc;
-}
-
 int cf_check vmx_guest_x86_mode(struct vcpu *v);
 unsigned int vmx_get_cpl(void);
 
@@ -620,71 +511,7 @@  static inline void vmx_pi_hooks_deassign(struct domain *d) {}
 
 #define APIC_INVALID_DEST           0xffffffff
 
-/* EPT violation qualifications definitions */
-typedef union ept_qual {
-    unsigned long raw;
-    struct {
-        bool read:1, write:1, fetch:1,
-            eff_read:1, eff_write:1, eff_exec:1, /* eff_user_exec */:1,
-            gla_valid:1,
-            gla_fault:1; /* Valid iff gla_valid. */
-        unsigned long /* pad */:55;
-    };
-} __transparent__ ept_qual_t;
-
 #define EPT_L4_PAGETABLE_SHIFT      39
 #define EPT_PAGETABLE_ENTRIES       512
 
-/* #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;
-
 #endif /* __ASM_X86_HVM_VMX_VMX_H__ */