* [PATCH v2 0/9] x86/hvm: {svm,vmx}.{c,h} cleanup
@ 2023-02-22 12:00 Xenia Ragiadakou
2023-02-22 12:00 ` [PATCH v2 1/9] x86/svm: remove unused forward declaration of struct vcpu from svm.h Xenia Ragiadakou
` (8 more replies)
0 siblings, 9 replies; 28+ messages in thread
From: Xenia Ragiadakou @ 2023-02-22 12:00 UTC (permalink / raw)
To: xen-devel
Cc: Jan Beulich, Andrew Cooper, Roger Pau Monné, Wei Liu,
Jun Nakajima, Kevin Tian
This patch series attempts a cleanup of files {svm,vmx}.{c,h} by removing
redundant headers and sorting the rest, reducing the scope of declarations
and definitions, moving functions used only by internal {svm,vmx} code to
private headers.
This series aims to address the comments made on the first version.
Main changes from the v1 series:
- split the changes into smaller reviewable patches
- drop the huge code rearrangment done in vmx.c
- instead of spreading arround the declarations in c files to hide them,
use private headers
There are more detailed per-patch changesets.
Xenia Ragiadakou (9):
x86/svm: remove unused forward declaration of struct vcpu from svm.h
x86/svm: opencode SVM_PAUSE{FILTER,THRESH}_INIT
x86/svm: move declarations used only by svm code from svm.h to private
header
x86/vmx: remove unused included headers from vmx.h
x86/vmx: reduce scope of GAS_VMX_OP definition
x86/vmx: move declations used only by vmx code from vmx.h to private
header
x86/vmx: remove unused included headers from vmx.c
x86/vmx: declare nvmx_enqueue_n2_exceptions() static
x86/vmx: move vmx_update_debug_state() in vmcs.c and declare it static
xen/arch/x86/hvm/svm/nestedsvm.c | 1 +
xen/arch/x86/hvm/svm/svm.c | 2 +
xen/arch/x86/hvm/svm/svm.h | 40 +++
xen/arch/x86/hvm/svm/vmcb.c | 4 +-
xen/arch/x86/hvm/vmx/intr.c | 2 +
xen/arch/x86/hvm/vmx/realmode.c | 2 +
xen/arch/x86/hvm/vmx/vmcs.c | 14 +
xen/arch/x86/hvm/vmx/vmx.c | 73 ++--
xen/arch/x86/hvm/vmx/vmx.h | 458 ++++++++++++++++++++++++
xen/arch/x86/hvm/vmx/vvmx.c | 2 +
xen/arch/x86/include/asm/hvm/svm/svm.h | 33 --
xen/arch/x86/include/asm/hvm/vmx/vmx.h | 468 +------------------------
12 files changed, 564 insertions(+), 535 deletions(-)
create mode 100644 xen/arch/x86/hvm/svm/svm.h
create mode 100644 xen/arch/x86/hvm/vmx/vmx.h
--
2.37.2
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 1/9] x86/svm: remove unused forward declaration of struct vcpu from svm.h
2023-02-22 12:00 [PATCH v2 0/9] x86/hvm: {svm,vmx}.{c,h} cleanup Xenia Ragiadakou
@ 2023-02-22 12:00 ` Xenia Ragiadakou
2023-02-22 12:59 ` Jan Beulich
2023-02-22 12:00 ` [PATCH v2 2/9] x86/svm: opencode SVM_PAUSE{FILTER,THRESH}_INIT Xenia Ragiadakou
` (7 subsequent siblings)
8 siblings, 1 reply; 28+ messages in thread
From: Xenia Ragiadakou @ 2023-02-22 12:00 UTC (permalink / raw)
To: xen-devel; +Cc: Jan Beulich, Andrew Cooper, Roger Pau Monné, Wei Liu
Remove forward declaration of struct vcpu, that is a leftover since
the removal of svm_update_guest_cr()'s declaration from svm.h.
Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
Fixes: b158e72abe30 ("x86/hvm: CFI hardening for hvm_funcs")
---
Changes in v2:
- leave the forward declaration of struct cpu_user_regs in place,
suggested by Andrew
- add a fixes tag based on Andrew's comment
- update commit message
xen/arch/x86/include/asm/hvm/svm/svm.h | 1 -
1 file changed, 1 deletion(-)
diff --git a/xen/arch/x86/include/asm/hvm/svm/svm.h b/xen/arch/x86/include/asm/hvm/svm/svm.h
index 65e35a4f59..fa39d4d76a 100644
--- a/xen/arch/x86/include/asm/hvm/svm/svm.h
+++ b/xen/arch/x86/include/asm/hvm/svm/svm.h
@@ -46,7 +46,6 @@ static inline void svm_invlpga(unsigned long linear, uint32_t asid)
}
struct cpu_user_regs;
-struct vcpu;
unsigned long *svm_msrbit(unsigned long *msr_bitmap, uint32_t msr);
void __update_guest_eip(struct cpu_user_regs *regs, unsigned int inst_len);
--
2.37.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v2 2/9] x86/svm: opencode SVM_PAUSE{FILTER,THRESH}_INIT
2023-02-22 12:00 [PATCH v2 0/9] x86/hvm: {svm,vmx}.{c,h} cleanup Xenia Ragiadakou
2023-02-22 12:00 ` [PATCH v2 1/9] x86/svm: remove unused forward declaration of struct vcpu from svm.h Xenia Ragiadakou
@ 2023-02-22 12:00 ` Xenia Ragiadakou
2023-02-23 10:19 ` Jan Beulich
2023-02-22 12:00 ` [PATCH v2 3/9] x86/svm: move declarations used only by svm code from svm.h to private header Xenia Ragiadakou
` (6 subsequent siblings)
8 siblings, 1 reply; 28+ messages in thread
From: Xenia Ragiadakou @ 2023-02-22 12:00 UTC (permalink / raw)
To: xen-devel; +Cc: Jan Beulich, Andrew Cooper, Roger Pau Monné, Wei Liu
Delete the macros SVM_PAUSE{FILTER,THRESH}_INIT from svm.h and opencode
their values, since they are used in a single place and using macros is
just unnecessary obfuscation.
Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
---
Changes in v2:
- opencode instead of moving the macros in vmcs.c, suggested by Andrew
xen/arch/x86/hvm/svm/vmcb.c | 4 ++--
xen/arch/x86/include/asm/hvm/svm/svm.h | 3 ---
2 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
index 305d4767e3..ba93375e87 100644
--- a/xen/arch/x86/hvm/svm/vmcb.c
+++ b/xen/arch/x86/hvm/svm/vmcb.c
@@ -169,11 +169,11 @@ static int construct_vmcb(struct vcpu *v)
if ( cpu_has_pause_filter )
{
- vmcb->_pause_filter_count = SVM_PAUSEFILTER_INIT;
+ vmcb->_pause_filter_count = 4000;
vmcb->_general1_intercepts |= GENERAL1_INTERCEPT_PAUSE;
if ( cpu_has_pause_thresh )
- vmcb->_pause_filter_thresh = SVM_PAUSETHRESH_INIT;
+ vmcb->_pause_filter_thresh = 1000;
}
/*
diff --git a/xen/arch/x86/include/asm/hvm/svm/svm.h b/xen/arch/x86/include/asm/hvm/svm/svm.h
index fa39d4d76a..c62d0caa32 100644
--- a/xen/arch/x86/include/asm/hvm/svm/svm.h
+++ b/xen/arch/x86/include/asm/hvm/svm/svm.h
@@ -95,9 +95,6 @@ extern u32 svm_feature_flags;
#define cpu_has_svm_sss cpu_has_svm_feature(SVM_FEATURE_SSS)
#define cpu_has_svm_spec_ctrl cpu_has_svm_feature(SVM_FEATURE_SPEC_CTRL)
-#define SVM_PAUSEFILTER_INIT 4000
-#define SVM_PAUSETHRESH_INIT 1000
-
/* TSC rate */
#define DEFAULT_TSC_RATIO 0x0000000100000000ULL
#define TSC_RATIO_RSVD_BITS 0xffffff0000000000ULL
--
2.37.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v2 3/9] x86/svm: move declarations used only by svm code from svm.h to private header
2023-02-22 12:00 [PATCH v2 0/9] x86/hvm: {svm,vmx}.{c,h} cleanup Xenia Ragiadakou
2023-02-22 12:00 ` [PATCH v2 1/9] x86/svm: remove unused forward declaration of struct vcpu from svm.h Xenia Ragiadakou
2023-02-22 12:00 ` [PATCH v2 2/9] x86/svm: opencode SVM_PAUSE{FILTER,THRESH}_INIT Xenia Ragiadakou
@ 2023-02-22 12:00 ` Xenia Ragiadakou
2023-02-23 10:24 ` Jan Beulich
2023-02-23 11:16 ` Andrew Cooper
2023-02-22 12:00 ` [PATCH v2 4/9] x86/vmx: remove unused included headers from vmx.h Xenia Ragiadakou
` (5 subsequent siblings)
8 siblings, 2 replies; 28+ messages in thread
From: Xenia Ragiadakou @ 2023-02-22 12:00 UTC (permalink / raw)
To: xen-devel; +Cc: Jan Beulich, Andrew Cooper, Roger Pau Monné, Wei Liu
Create a new private header in arch/x86/hvm/svm called svm.h and move there
all definitions and declarations that are used solely by svm code.
The function svm_invlpga() stays in arch/x86/hvm/svm/svm.h because it is used
by arch/x86/hvm/svm/asid.h.
Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
---
Changes in v2:
- new patch, the creation of a private header was suggested by Andrew and Jan
I have not added #ifndef guards as it is a private and it should not be
included by other headers. However, this is considered a MISRA-C violation
... I 'm not sure what to do.
xen/arch/x86/hvm/svm/nestedsvm.c | 1 +
xen/arch/x86/hvm/svm/svm.c | 2 ++
xen/arch/x86/hvm/svm/svm.h | 40 ++++++++++++++++++++++++++
xen/arch/x86/include/asm/hvm/svm/svm.h | 29 -------------------
4 files changed, 43 insertions(+), 29 deletions(-)
create mode 100644 xen/arch/x86/hvm/svm/svm.h
diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
index 77f7547360..a341ccc876 100644
--- a/xen/arch/x86/hvm/svm/nestedsvm.c
+++ b/xen/arch/x86/hvm/svm/nestedsvm.c
@@ -27,6 +27,7 @@
#include <asm/event.h> /* for local_event_delivery_(en|dis)able */
#include <asm/p2m.h> /* p2m_get_pagetable, p2m_get_nestedp2m */
+#include "svm.h"
#define NSVM_ERROR_VVMCB 1
#define NSVM_ERROR_VMENTRY 2
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 9c43227b76..6d394e4fe3 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -55,6 +55,8 @@
#include <public/sched.h>
+#include "svm.h"
+
void noreturn svm_asm_do_resume(void);
u32 svm_feature_flags;
diff --git a/xen/arch/x86/hvm/svm/svm.h b/xen/arch/x86/hvm/svm/svm.h
new file mode 100644
index 0000000000..b2ec293078
--- /dev/null
+++ b/xen/arch/x86/hvm/svm/svm.h
@@ -0,0 +1,40 @@
+#include <xen/types.h>
+
+static inline void svm_vmload_pa(paddr_t vmcb)
+{
+ asm volatile (
+ ".byte 0x0f,0x01,0xda" /* vmload */
+ : : "a" (vmcb) : "memory" );
+}
+
+static inline void svm_vmsave_pa(paddr_t vmcb)
+{
+ asm volatile (
+ ".byte 0x0f,0x01,0xdb" /* vmsave */
+ : : "a" (vmcb) : "memory" );
+}
+
+struct cpu_user_regs;
+
+unsigned long *svm_msrbit(unsigned long *msr_bitmap, uint32_t msr);
+void __update_guest_eip(struct cpu_user_regs *regs, unsigned int inst_len);
+
+/* TSC rate */
+#define DEFAULT_TSC_RATIO 0x0000000100000000ULL
+#define TSC_RATIO_RSVD_BITS 0xffffff0000000000ULL
+
+/* EXITINFO1 fields on NPT faults */
+#define _NPT_PFEC_with_gla 32
+#define NPT_PFEC_with_gla (1UL<<_NPT_PFEC_with_gla)
+#define _NPT_PFEC_in_gpt 33
+#define NPT_PFEC_in_gpt (1UL<<_NPT_PFEC_in_gpt)
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/x86/include/asm/hvm/svm/svm.h b/xen/arch/x86/include/asm/hvm/svm/svm.h
index c62d0caa32..254de55ee9 100644
--- a/xen/arch/x86/include/asm/hvm/svm/svm.h
+++ b/xen/arch/x86/include/asm/hvm/svm/svm.h
@@ -22,20 +22,6 @@
#include <xen/types.h>
-static inline void svm_vmload_pa(paddr_t vmcb)
-{
- asm volatile (
- ".byte 0x0f,0x01,0xda" /* vmload */
- : : "a" (vmcb) : "memory" );
-}
-
-static inline void svm_vmsave_pa(paddr_t vmcb)
-{
- asm volatile (
- ".byte 0x0f,0x01,0xdb" /* vmsave */
- : : "a" (vmcb) : "memory" );
-}
-
static inline void svm_invlpga(unsigned long linear, uint32_t asid)
{
asm volatile (
@@ -45,11 +31,6 @@ static inline void svm_invlpga(unsigned long linear, uint32_t asid)
"a" (linear), "c" (asid));
}
-struct cpu_user_regs;
-
-unsigned long *svm_msrbit(unsigned long *msr_bitmap, uint32_t msr);
-void __update_guest_eip(struct cpu_user_regs *regs, unsigned int inst_len);
-
/*
* PV context switch helpers. Prefetching the VMCB area itself has been shown
* to be useful for performance.
@@ -95,14 +76,4 @@ extern u32 svm_feature_flags;
#define cpu_has_svm_sss cpu_has_svm_feature(SVM_FEATURE_SSS)
#define cpu_has_svm_spec_ctrl cpu_has_svm_feature(SVM_FEATURE_SPEC_CTRL)
-/* TSC rate */
-#define DEFAULT_TSC_RATIO 0x0000000100000000ULL
-#define TSC_RATIO_RSVD_BITS 0xffffff0000000000ULL
-
-/* EXITINFO1 fields on NPT faults */
-#define _NPT_PFEC_with_gla 32
-#define NPT_PFEC_with_gla (1UL<<_NPT_PFEC_with_gla)
-#define _NPT_PFEC_in_gpt 33
-#define NPT_PFEC_in_gpt (1UL<<_NPT_PFEC_in_gpt)
-
#endif /* __ASM_X86_HVM_SVM_H__ */
--
2.37.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v2 4/9] x86/vmx: remove unused included headers from vmx.h
2023-02-22 12:00 [PATCH v2 0/9] x86/hvm: {svm,vmx}.{c,h} cleanup Xenia Ragiadakou
` (2 preceding siblings ...)
2023-02-22 12:00 ` [PATCH v2 3/9] x86/svm: move declarations used only by svm code from svm.h to private header Xenia Ragiadakou
@ 2023-02-22 12:00 ` Xenia Ragiadakou
2023-02-23 10:29 ` Jan Beulich
2023-02-22 12:00 ` [PATCH v2 5/9] x86/vmx: reduce scope of GAS_VMX_OP definition Xenia Ragiadakou
` (4 subsequent siblings)
8 siblings, 1 reply; 28+ messages in thread
From: Xenia Ragiadakou @ 2023-02-22 12:00 UTC (permalink / raw)
To: xen-devel
Cc: Jun Nakajima, Kevin Tian, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Wei Liu
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 the rest of the headers alphabetically.
Fix build by including asm/i387.h in vmx.c, needed for vcpu_restore_fpu_lazy().
Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
---
Changes in v2:
-add a blank line between different types of headers, reported by Andrew
-fix english in commit message, reported by Andrew
xen/arch/x86/hvm/vmx/vmx.c | 1 +
xen/arch/x86/include/asm/hvm/vmx/vmx.h | 9 +++------
2 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 0ec33bcc18..a0297e8c6c 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 <public/sched.h>
#include <public/hvm/ioreq.h>
+#include <asm/i387.h>
#include <asm/hvm/vpic.h>
#include <asm/hvm/vlapic.h>
#include <asm/x86_emulate.h>
diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmx.h b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
index 97d6b810ec..c0ca6d10e3 100644
--- a/xen/arch/x86/include/asm/hvm/vmx/vmx.h
+++ b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
@@ -19,14 +19,11 @@
#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;
--
2.37.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v2 5/9] x86/vmx: reduce scope of GAS_VMX_OP definition
2023-02-22 12:00 [PATCH v2 0/9] x86/hvm: {svm,vmx}.{c,h} cleanup Xenia Ragiadakou
` (3 preceding siblings ...)
2023-02-22 12:00 ` [PATCH v2 4/9] x86/vmx: remove unused included headers from vmx.h Xenia Ragiadakou
@ 2023-02-22 12:00 ` Xenia Ragiadakou
2023-02-23 10:31 ` Jan Beulich
2023-02-22 12:00 ` [PATCH v2 6/9] x86/vmx: move declations used only by vmx code from vmx.h to private header Xenia Ragiadakou
` (3 subsequent siblings)
8 siblings, 1 reply; 28+ messages in thread
From: Xenia Ragiadakou @ 2023-02-22 12:00 UTC (permalink / raw)
To: xen-devel
Cc: Jun Nakajima, Kevin Tian, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Wei Liu
Since GAS_VMX_OP is used only by vm{read,write}_safe, move its definition
just above those functions and undefine it after use.
Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
---
Changes in v2:
- none
xen/arch/x86/include/asm/hvm/vmx/vmx.h | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmx.h b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
index c0ca6d10e3..1ba2937c23 100644
--- a/xen/arch/x86/include/asm/hvm/vmx/vmx.h
+++ b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
@@ -319,12 +319,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 (
@@ -414,6 +408,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)
{
@@ -460,6 +460,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 {
--
2.37.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v2 6/9] x86/vmx: move declations used only by vmx code from vmx.h to private header
2023-02-22 12:00 [PATCH v2 0/9] x86/hvm: {svm,vmx}.{c,h} cleanup Xenia Ragiadakou
` (4 preceding siblings ...)
2023-02-22 12:00 ` [PATCH v2 5/9] x86/vmx: reduce scope of GAS_VMX_OP definition Xenia Ragiadakou
@ 2023-02-22 12:00 ` Xenia Ragiadakou
2023-02-23 10:47 ` Jan Beulich
2023-02-22 12:00 ` [PATCH v2 7/9] x86/vmx: remove unused included headers from vmx.c Xenia Ragiadakou
` (2 subsequent siblings)
8 siblings, 1 reply; 28+ messages in thread
From: Xenia Ragiadakou @ 2023-02-22 12:00 UTC (permalink / raw)
To: xen-devel
Cc: Jun Nakajima, Kevin Tian, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Wei Liu
Create a new private header in arch/x86/hvm/vmx/ called vmx.h and move there
all definitions and declarations that are used solely by vmx code.
EPT related declarations and definitions stay in asm/hvm/vmx/vmx.h because
they are used in arch/x86/mm and drivers/passthrough/vtd.
Also, __vmread(), used in arch/x86/cpu, and consequently the opcodes stay in
asm/hvm/vmx/vmx.h.
Take the opportunity to remove a trailing white space from __vmxon().
Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
---
Changes in v2:
- new patch, creation of private header for holding declarations and
definitions consumed only by vmx code, suggested by Andrew and Jan
I have not added #ifndef guards as it is a private and it should not be
included by other headers. However, this is considered a MISRA-C violation
... I 'm not sure what to do.
xen/arch/x86/hvm/vmx/intr.c | 2 +
xen/arch/x86/hvm/vmx/realmode.c | 2 +
xen/arch/x86/hvm/vmx/vmcs.c | 2 +
xen/arch/x86/hvm/vmx/vmx.c | 2 +
xen/arch/x86/hvm/vmx/vmx.h | 459 ++++++++++++++++++++++++
xen/arch/x86/hvm/vmx/vvmx.c | 2 +
xen/arch/x86/include/asm/hvm/vmx/vmx.h | 463 +------------------------
7 files changed, 479 insertions(+), 453 deletions(-)
create mode 100644 xen/arch/x86/hvm/vmx/vmx.h
diff --git a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c
index 6a8316de0e..c8db501759 100644
--- a/xen/arch/x86/hvm/vmx/intr.c
+++ b/xen/arch/x86/hvm/vmx/intr.c
@@ -38,6 +38,8 @@
#include <asm/hvm/trace.h>
#include <asm/vm_event.h>
+#include "vmx.h"
+
/*
* A few notes on virtual NMI and INTR delivery, and interactions with
* interruptibility states:
diff --git a/xen/arch/x86/hvm/vmx/realmode.c b/xen/arch/x86/hvm/vmx/realmode.c
index 4ac93e0810..5591660230 100644
--- a/xen/arch/x86/hvm/vmx/realmode.c
+++ b/xen/arch/x86/hvm/vmx/realmode.c
@@ -23,6 +23,8 @@
#include <asm/hvm/vmx/vmx.h>
#include <asm/hvm/vmx/vmcs.h>
+#include "vmx.h"
+
static void realmode_deliver_exception(
unsigned int vector,
unsigned int insn_len,
diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index e1c268789e..bc2acd3063 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -43,6 +43,8 @@
#include <asm/tboot.h>
#include <asm/apic.h>
+#include "vmx.h"
+
static bool_t __read_mostly opt_vpid_enabled = 1;
boolean_param("vpid", opt_vpid_enabled);
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index a0297e8c6c..a19ece90fc 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -62,6 +62,8 @@
#include <asm/prot-key.h>
#include <public/arch-x86/cpuid.h>
+#include "vmx.h"
+
static bool_t __initdata opt_force_ept;
boolean_param("force-ept", opt_force_ept);
diff --git a/xen/arch/x86/hvm/vmx/vmx.h b/xen/arch/x86/hvm/vmx/vmx.h
new file mode 100644
index 0000000000..ffc19539bc
--- /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>
+
+extern int8_t opt_ept_exec_sp;
+
+#define PI_xAPIC_NDST_MASK 0xFF00
+
+void vmx_asm_vmexit_handler(struct cpu_user_regs);
+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)
+{
+ 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);
+}
+
+/*
+ * 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 {
+ 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
+# 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};
+
+ /* 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:
+ __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);
+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;
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 674cdabb07..0bda8430b9 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -29,6 +29,8 @@
#include <asm/hvm/vmx/vvmx.h>
#include <asm/hvm/nestedhvm.h>
+#include "vmx.h"
+
static DEFINE_PER_CPU(u64 *, vvmcs_buf);
static void nvmx_purge_vvmcs(struct vcpu *v);
diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmx.h b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
index 1ba2937c23..c81d76e4eb 100644
--- a/xen/arch/x86/include/asm/hvm/vmx/vmx.h
+++ b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
@@ -20,13 +20,9 @@
#include <xen/sched.h>
-#include <asm/asm_defns.h>
-#include <asm/hvm/vmx/vmcs.h>
#include <asm/p2m.h>
#include <asm/types.h>
-extern int8_t opt_ept_exec_sp;
-
typedef union {
struct {
u64 r : 1, /* bit 0 - Read permission */
@@ -77,71 +73,8 @@ typedef enum {
#define EPTE_RWX_MASK 0x7
#define EPTE_FLAG_MASK 0x7f
-#define PI_xAPIC_NDST_MASK 0xFF00
-
-void vmx_asm_vmexit_handler(struct cpu_user_regs);
-void vmx_intr_assist(void);
-void noreturn cf_check vmx_do_resume(void);
-void cf_check vmx_vlapic_msr_changed(struct vcpu *v);
struct hvm_emulate_ctxt;
void vmx_realmode_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt);
-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)
-{
- 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
@@ -212,60 +145,6 @@ static inline void pi_clear_sn(struct pi_desc *pi_desc)
#define EXIT_REASON_NOTIFY 75
/* Remember to also update VMX_PERF_EXIT_REASON_SIZE! */
-/*
- * 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 {
- 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 */
-
#define VMCALL_OPCODE ".byte 0x0f,0x01,0xc1\n"
#define VMCLEAR_OPCODE ".byte 0x66,0x0f,0xc7\n" /* reg/opcode: /6 */
#define VMLAUNCH_OPCODE ".byte 0x0f,0x01,0xc2\n"
@@ -284,8 +163,6 @@ typedef union cr_access_qual {
#define MODRM_EAX_07 ".byte 0x38\n" /* [EAX], with reg/opcode: /7 */
#define MODRM_EAX_ECX ".byte 0xc1\n" /* EAX, ECX */
-extern uint8_t posted_intr_vector;
-
#define cpu_has_vmx_ept_exec_only_supported \
(vmx_ept_vpid_cap & VMX_EPT_EXEC_ONLY_SUPPORTED)
@@ -299,70 +176,6 @@ extern uint8_t posted_intr_vector;
#define cpu_has_vmx_ept_invept_single_context \
(vmx_ept_vpid_cap & VMX_EPT_INVEPT_SINGLE_CONTEXT)
-#define EPT_2MB_SHIFT 16
-#define EPT_1GB_SHIFT 17
-#define ept_has_2mb(c) ((c >> EPT_2MB_SHIFT) & 1)
-#define ept_has_1gb(c) ((c >> EPT_1GB_SHIFT) & 1)
-
-#define INVEPT_SINGLE_CONTEXT 1
-#define INVEPT_ALL_CONTEXT 2
-
-#define cpu_has_vmx_vpid_invvpid_individual_addr \
- (vmx_ept_vpid_cap & VMX_VPID_INVVPID_INDIVIDUAL_ADDR)
-#define cpu_has_vmx_vpid_invvpid_single_context \
- (vmx_ept_vpid_cap & VMX_VPID_INVVPID_SINGLE_CONTEXT)
-#define cpu_has_vmx_vpid_invvpid_single_context_retaining_global \
- (vmx_ept_vpid_cap & VMX_VPID_INVVPID_SINGLE_CONTEXT_RETAINING_GLOBAL)
-
-#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 __vmread(unsigned long field, unsigned long *value)
{
asm volatile (
@@ -386,215 +199,20 @@ static always_inline void __vmread(unsigned long field, unsigned long *value)
);
}
-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
-# 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};
-
- /* 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" );
-}
+#define EPT_2MB_SHIFT 16
+#define EPT_1GB_SHIFT 17
+#define ept_has_2mb(c) ((c >> EPT_2MB_SHIFT) & 1)
+#define ept_has_1gb(c) ((c >> EPT_1GB_SHIFT) & 1)
-static inline void ept_sync_all(void)
-{
- __invept(INVEPT_ALL_CONTEXT, 0);
-}
+#define cpu_has_vmx_vpid_invvpid_individual_addr \
+ (vmx_ept_vpid_cap & VMX_VPID_INVVPID_INDIVIDUAL_ADDR)
+#define cpu_has_vmx_vpid_invvpid_single_context \
+ (vmx_ept_vpid_cap & VMX_VPID_INVVPID_SINGLE_CONTEXT)
+#define cpu_has_vmx_vpid_invvpid_single_context_retaining_global \
+ (vmx_ept_vpid_cap & VMX_VPID_INVVPID_SINGLE_CONTEXT_RETAINING_GLOBAL)
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);
-
-void vmx_inject_extint(int trap, uint8_t source);
-void vmx_inject_nmi(void);
-
void ept_walk_table(struct domain *d, unsigned long gfn);
bool_t ept_handle_misconfig(uint64_t gpa);
int epte_get_entry_emt(struct domain *d, gfn_t gfn, mfn_t mfn,
@@ -603,13 +221,6 @@ void setup_ept_dump(void);
/* Locate an alternate p2m by its EPTP */
unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp);
-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);
-
#ifdef CONFIG_HVM
void vmx_pi_hooks_assign(struct domain *d);
void vmx_pi_hooks_deassign(struct domain *d);
@@ -618,8 +229,6 @@ static inline void vmx_pi_hooks_assign(struct domain *d) {}
static inline void vmx_pi_hooks_deassign(struct domain *d) {}
#endif
-#define APIC_INVALID_DEST 0xffffffff
-
/* EPT violation qualifications definitions */
typedef union ept_qual {
unsigned long raw;
@@ -635,56 +244,4 @@ typedef union ept_qual {
#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__ */
--
2.37.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v2 7/9] x86/vmx: remove unused included headers from vmx.c
2023-02-22 12:00 [PATCH v2 0/9] x86/hvm: {svm,vmx}.{c,h} cleanup Xenia Ragiadakou
` (5 preceding siblings ...)
2023-02-22 12:00 ` [PATCH v2 6/9] x86/vmx: move declations used only by vmx code from vmx.h to private header Xenia Ragiadakou
@ 2023-02-22 12:00 ` Xenia Ragiadakou
2023-02-23 11:16 ` Jan Beulich
2023-02-22 12:00 ` [PATCH v2 8/9] x86/vmx: declare nvmx_enqueue_n2_exceptions() static Xenia Ragiadakou
2023-02-22 12:00 ` [PATCH v2 9/9] x86/vmx: move vmx_update_debug_state() in vmcs.c and declare it static Xenia Ragiadakou
8 siblings, 1 reply; 28+ messages in thread
From: Xenia Ragiadakou @ 2023-02-22 12:00 UTC (permalink / raw)
To: xen-devel
Cc: Jun Nakajima, Kevin Tian, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Wei Liu
Do not include the headers:
asm/hvm/vpic.h
asm/hvm/vpt.h
asm/io.h
asm/mce.h
asm/mem_sharing.h
asm/regs.h
public/arch-x86/cpuid.h
public/hvm/save.h
because none of the declarations and macro definitions in them is used.
Sort the rest of the headers alphabetically.
Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
---
Changes in v2:
-add a blank line between different types of hesders
xen/arch/x86/hvm/vmx/vmx.c | 56 +++++++++++++++++---------------------
1 file changed, 25 insertions(+), 31 deletions(-)
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index a19ece90fc..a6ec0a11fb 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -15,52 +15,46 @@
* this program; If not, see <http://www.gnu.org/licenses/>.
*/
+#include <xen/domain_page.h>
#include <xen/guest_access.h>
+#include <xen/hypercall.h>
#include <xen/init.h>
+#include <xen/irq.h>
#include <xen/lib.h>
#include <xen/param.h>
-#include <xen/trace.h>
+#include <xen/perfc.h>
#include <xen/sched.h>
-#include <xen/irq.h>
#include <xen/softirq.h>
-#include <xen/domain_page.h>
-#include <xen/hypercall.h>
-#include <xen/perfc.h>
-#include <asm/current.h>
-#include <asm/io.h>
-#include <asm/iocap.h>
-#include <asm/regs.h>
+#include <xen/trace.h>
+
+#include <asm/altp2m.h>
+#include <asm/apic.h>
#include <asm/cpufeature.h>
-#include <asm/processor.h>
+#include <asm/current.h>
+#include <asm/gdbsx.h>
#include <asm/debugreg.h>
-#include <asm/msr.h>
-#include <asm/p2m.h>
-#include <asm/mem_sharing.h>
+#include <asm/event.h>
#include <asm/hvm/emulate.h>
#include <asm/hvm/hvm.h>
+#include <asm/hvm/monitor.h>
+#include <asm/hvm/nestedhvm.h>
#include <asm/hvm/support.h>
-#include <asm/hvm/vmx/vmx.h>
+#include <asm/hvm/trace.h>
+#include <asm/hvm/vlapic.h>
#include <asm/hvm/vmx/vmcs.h>
-#include <public/sched.h>
-#include <public/hvm/ioreq.h>
+#include <asm/hvm/vmx/vmx.h>
+#include <asm/iocap.h>
#include <asm/i387.h>
-#include <asm/hvm/vpic.h>
-#include <asm/hvm/vlapic.h>
-#include <asm/x86_emulate.h>
-#include <asm/hvm/vpt.h>
-#include <public/hvm/save.h>
-#include <asm/hvm/trace.h>
-#include <asm/hvm/monitor.h>
-#include <asm/xenoprof.h>
-#include <asm/gdbsx.h>
-#include <asm/apic.h>
-#include <asm/hvm/nestedhvm.h>
-#include <asm/altp2m.h>
-#include <asm/event.h>
-#include <asm/mce.h>
#include <asm/monitor.h>
+#include <asm/msr.h>
+#include <asm/processor.h>
#include <asm/prot-key.h>
-#include <public/arch-x86/cpuid.h>
+#include <asm/p2m.h>
+#include <asm/xenoprof.h>
+#include <asm/x86_emulate.h>
+
+#include <public/sched.h>
+#include <public/hvm/ioreq.h>
#include "vmx.h"
--
2.37.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v2 8/9] x86/vmx: declare nvmx_enqueue_n2_exceptions() static
2023-02-22 12:00 [PATCH v2 0/9] x86/hvm: {svm,vmx}.{c,h} cleanup Xenia Ragiadakou
` (6 preceding siblings ...)
2023-02-22 12:00 ` [PATCH v2 7/9] x86/vmx: remove unused included headers from vmx.c Xenia Ragiadakou
@ 2023-02-22 12:00 ` Xenia Ragiadakou
2023-02-23 11:19 ` Jan Beulich
2023-02-22 12:00 ` [PATCH v2 9/9] x86/vmx: move vmx_update_debug_state() in vmcs.c and declare it static Xenia Ragiadakou
8 siblings, 1 reply; 28+ messages in thread
From: Xenia Ragiadakou @ 2023-02-22 12:00 UTC (permalink / raw)
To: xen-devel
Cc: Jun Nakajima, Kevin Tian, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Wei Liu
Reduce the scope of nvmx_enqueue_n2_exceptions() to static because it is used
only in this file.
Take the opportunity to remove a trailing space.
Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
---
Changes in v2:
- none
xen/arch/x86/hvm/vmx/vmx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index a6ec0a11fb..5b210e0db4 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1908,7 +1908,7 @@ static void cf_check vmx_update_guest_efer(struct vcpu *v)
vmx_set_msr_intercept(v, MSR_EFER, VMX_MSR_R);
}
-void nvmx_enqueue_n2_exceptions(struct vcpu *v,
+static void nvmx_enqueue_n2_exceptions(struct vcpu *v,
unsigned long intr_fields, int error_code, uint8_t source)
{
struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
--
2.37.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v2 9/9] x86/vmx: move vmx_update_debug_state() in vmcs.c and declare it static
2023-02-22 12:00 [PATCH v2 0/9] x86/hvm: {svm,vmx}.{c,h} cleanup Xenia Ragiadakou
` (7 preceding siblings ...)
2023-02-22 12:00 ` [PATCH v2 8/9] x86/vmx: declare nvmx_enqueue_n2_exceptions() static Xenia Ragiadakou
@ 2023-02-22 12:00 ` Xenia Ragiadakou
2023-02-23 11:21 ` Jan Beulich
8 siblings, 1 reply; 28+ messages in thread
From: Xenia Ragiadakou @ 2023-02-22 12:00 UTC (permalink / raw)
To: xen-devel
Cc: Jun Nakajima, Kevin Tian, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Wei Liu
Move vmx_update_debug_state() in vmcs.c because it is used only in this
file and limit its scope to this file by declaring it static and removing
its declaration from private vmx.h
Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
---
Changes in v2:
- remove it from the private vmx.h header instead of the global one
xen/arch/x86/hvm/vmx/vmcs.c | 12 ++++++++++++
xen/arch/x86/hvm/vmx/vmx.c | 12 ------------
xen/arch/x86/hvm/vmx/vmx.h | 1 -
3 files changed, 12 insertions(+), 13 deletions(-)
diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index bc2acd3063..1492f85213 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -1868,6 +1868,18 @@ void vmx_vmentry_failure(void)
void noreturn vmx_asm_do_vmentry(void);
+static void vmx_update_debug_state(struct vcpu *v)
+{
+ if ( v->arch.hvm.debug_state_latch )
+ v->arch.hvm.vmx.exception_bitmap |= 1U << TRAP_int3;
+ else
+ v->arch.hvm.vmx.exception_bitmap &= ~(1U << TRAP_int3);
+
+ vmx_vmcs_enter(v);
+ vmx_update_exception_bitmap(v);
+ vmx_vmcs_exit(v);
+}
+
void cf_check vmx_do_resume(void)
{
struct vcpu *v = current;
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 5b210e0db4..ada36d6923 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1610,18 +1610,6 @@ static void cf_check vmx_update_host_cr3(struct vcpu *v)
vmx_vmcs_exit(v);
}
-void vmx_update_debug_state(struct vcpu *v)
-{
- if ( v->arch.hvm.debug_state_latch )
- v->arch.hvm.vmx.exception_bitmap |= 1U << TRAP_int3;
- else
- v->arch.hvm.vmx.exception_bitmap &= ~(1U << TRAP_int3);
-
- vmx_vmcs_enter(v);
- vmx_update_exception_bitmap(v);
- vmx_vmcs_exit(v);
-}
-
static void cf_check vmx_update_guest_cr(
struct vcpu *v, unsigned int cr, unsigned int flags)
{
diff --git a/xen/arch/x86/hvm/vmx/vmx.h b/xen/arch/x86/hvm/vmx/vmx.h
index ffc19539bc..7d4ecf6d59 100644
--- a/xen/arch/x86/hvm/vmx/vmx.h
+++ b/xen/arch/x86/hvm/vmx/vmx.h
@@ -14,7 +14,6 @@ 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);
--
2.37.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH v2 1/9] x86/svm: remove unused forward declaration of struct vcpu from svm.h
2023-02-22 12:00 ` [PATCH v2 1/9] x86/svm: remove unused forward declaration of struct vcpu from svm.h Xenia Ragiadakou
@ 2023-02-22 12:59 ` Jan Beulich
2023-02-23 10:50 ` Andrew Cooper
0 siblings, 1 reply; 28+ messages in thread
From: Jan Beulich @ 2023-02-22 12:59 UTC (permalink / raw)
To: Xenia Ragiadakou; +Cc: Andrew Cooper, Roger Pau Monné, Wei Liu, xen-devel
On 22.02.2023 13:00, Xenia Ragiadakou wrote:
> Remove forward declaration of struct vcpu, that is a leftover since
> the removal of svm_update_guest_cr()'s declaration from svm.h.
>
> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
> Fixes: b158e72abe30 ("x86/hvm: CFI hardening for hvm_funcs")
I'm a little puzzled by this (a stray forward decl of a struct isn't
really a bug imo), but ...
> ---
>
> Changes in v2:
> - leave the forward declaration of struct cpu_user_regs in place,
> suggested by Andrew
> - add a fixes tag based on Andrew's comment
... I realize you were asked to add it. (As a minor remark, more
commonly the Fixes: tag would come ahead of the S-o-b: one, I think.)
Jan
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/9] x86/svm: opencode SVM_PAUSE{FILTER,THRESH}_INIT
2023-02-22 12:00 ` [PATCH v2 2/9] x86/svm: opencode SVM_PAUSE{FILTER,THRESH}_INIT Xenia Ragiadakou
@ 2023-02-23 10:19 ` Jan Beulich
0 siblings, 0 replies; 28+ messages in thread
From: Jan Beulich @ 2023-02-23 10:19 UTC (permalink / raw)
To: Xenia Ragiadakou; +Cc: Andrew Cooper, Roger Pau Monné, Wei Liu, xen-devel
On 22.02.2023 13:00, Xenia Ragiadakou wrote:
> Delete the macros SVM_PAUSE{FILTER,THRESH}_INIT from svm.h and opencode
> their values, since they are used in a single place and using macros is
> just unnecessary obfuscation.
>
> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
> Changes in v2:
> - opencode instead of moving the macros in vmcs.c, suggested by Andrew
Which hence probably wants expressing via a Suggested-by: tag. I guess this
can be transformed while committing.
Jan
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 3/9] x86/svm: move declarations used only by svm code from svm.h to private header
2023-02-22 12:00 ` [PATCH v2 3/9] x86/svm: move declarations used only by svm code from svm.h to private header Xenia Ragiadakou
@ 2023-02-23 10:24 ` Jan Beulich
2023-02-23 11:16 ` Andrew Cooper
1 sibling, 0 replies; 28+ messages in thread
From: Jan Beulich @ 2023-02-23 10:24 UTC (permalink / raw)
To: Xenia Ragiadakou; +Cc: Andrew Cooper, Roger Pau Monné, Wei Liu, xen-devel
On 22.02.2023 13:00, Xenia Ragiadakou wrote:
> Create a new private header in arch/x86/hvm/svm called svm.h and move there
> all definitions and declarations that are used solely by svm code.
>
> The function svm_invlpga() stays in arch/x86/hvm/svm/svm.h because it is used
> by arch/x86/hvm/svm/asid.h.
>
> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
> Changes in v2:
> - new patch, the creation of a private header was suggested by Andrew and Jan
Same remark here as on patch 2.
> I have not added #ifndef guards as it is a private and it should not be
> included by other headers. However, this is considered a MISRA-C violation
> ... I 'm not sure what to do.
Personally I prefer the guard-less form, but since Misra insists, we may
better add them from the start. Besides being a little bit of clutter they're
harmless, after all. My ack stands with or without the addition, but I
wouldn't want to add them "while committing". So I guess we want to wait a
little for further views, and then either commit as is or wait for a v3 with
the guards added.
Jan
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 4/9] x86/vmx: remove unused included headers from vmx.h
2023-02-22 12:00 ` [PATCH v2 4/9] x86/vmx: remove unused included headers from vmx.h Xenia Ragiadakou
@ 2023-02-23 10:29 ` Jan Beulich
2023-02-23 10:42 ` Xenia Ragiadakou
0 siblings, 1 reply; 28+ messages in thread
From: Jan Beulich @ 2023-02-23 10:29 UTC (permalink / raw)
To: Xenia Ragiadakou
Cc: Jun Nakajima, Kevin Tian, Andrew Cooper, Roger Pau Monné,
Wei Liu, xen-devel
On 22.02.2023 13:00, 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 the rest of the headers alphabetically.
> Fix build by including asm/i387.h in vmx.c, needed for vcpu_restore_fpu_lazy().
Nit: You don't really "fix" the build, you keep it working.
> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
In principle
Reviewed-by: Jan Beulich <jbeulich@suse.com>
but ...
> --- a/xen/arch/x86/include/asm/hvm/vmx/vmx.h
> +++ b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
> @@ -19,14 +19,11 @@
> #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>
... can this please become xen/types.h (and move up accordingly), so
things won't break here when my further work in that area lands?
Jan
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 5/9] x86/vmx: reduce scope of GAS_VMX_OP definition
2023-02-22 12:00 ` [PATCH v2 5/9] x86/vmx: reduce scope of GAS_VMX_OP definition Xenia Ragiadakou
@ 2023-02-23 10:31 ` Jan Beulich
2023-02-23 10:45 ` Xenia Ragiadakou
0 siblings, 1 reply; 28+ messages in thread
From: Jan Beulich @ 2023-02-23 10:31 UTC (permalink / raw)
To: Xenia Ragiadakou
Cc: Jun Nakajima, Kevin Tian, Andrew Cooper, Roger Pau Monné,
Wei Liu, xen-devel
On 22.02.2023 13:00, Xenia Ragiadakou wrote:
> Since GAS_VMX_OP is used only by vm{read,write}_safe, move its definition
> just above those functions and undefine it after use.
>
> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
This can easily be done as part of the next patch, with less code churn
overall.
Jan
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 4/9] x86/vmx: remove unused included headers from vmx.h
2023-02-23 10:29 ` Jan Beulich
@ 2023-02-23 10:42 ` Xenia Ragiadakou
0 siblings, 0 replies; 28+ messages in thread
From: Xenia Ragiadakou @ 2023-02-23 10:42 UTC (permalink / raw)
To: Jan Beulich
Cc: Jun Nakajima, Kevin Tian, Andrew Cooper, Roger Pau Monné,
Wei Liu, xen-devel
On 2/23/23 12:29, Jan Beulich wrote:
> On 22.02.2023 13:00, 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 the rest of the headers alphabetically.
>> Fix build by including asm/i387.h in vmx.c, needed for vcpu_restore_fpu_lazy().
>
> Nit: You don't really "fix" the build, you keep it working.
Ok I will rephrase it in v3.
>
>> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
>
> In principle
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> but ...
>
>> --- a/xen/arch/x86/include/asm/hvm/vmx/vmx.h
>> +++ b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
>> @@ -19,14 +19,11 @@
>> #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>
>
> ... can this please become xen/types.h (and move up accordingly), so
> things won't break here when my further work in that area lands?
Sure.
>
> Jan
--
Xenia
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 5/9] x86/vmx: reduce scope of GAS_VMX_OP definition
2023-02-23 10:31 ` Jan Beulich
@ 2023-02-23 10:45 ` Xenia Ragiadakou
0 siblings, 0 replies; 28+ messages in thread
From: Xenia Ragiadakou @ 2023-02-23 10:45 UTC (permalink / raw)
To: Jan Beulich
Cc: Jun Nakajima, Kevin Tian, Andrew Cooper, Roger Pau Monné,
Wei Liu, xen-devel
On 2/23/23 12:31, Jan Beulich wrote:
> On 22.02.2023 13:00, Xenia Ragiadakou wrote:
>> Since GAS_VMX_OP is used only by vm{read,write}_safe, move its definition
>> just above those functions and undefine it after use.
>>
>> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
>
> This can easily be done as part of the next patch, with less code churn
> overall.
Sure. I ve spotted a bug anyway in the next patch (I should not have
deleted the #include <asm/hvm/vmx/vmcs.h>) so I will squeeze them in v3.
>
> Jan
--
Xenia
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 6/9] x86/vmx: move declations used only by vmx code from vmx.h to private header
2023-02-22 12:00 ` [PATCH v2 6/9] x86/vmx: move declations used only by vmx code from vmx.h to private header Xenia Ragiadakou
@ 2023-02-23 10:47 ` Jan Beulich
2023-02-23 11:56 ` Andrew Cooper
2023-02-24 7:49 ` Xenia Ragiadakou
0 siblings, 2 replies; 28+ messages in thread
From: Jan Beulich @ 2023-02-23 10:47 UTC (permalink / raw)
To: Xenia Ragiadakou, Andrew Cooper
Cc: Jun Nakajima, Kevin Tian, Roger Pau Monné, Wei Liu,
xen-devel
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 1/9] x86/svm: remove unused forward declaration of struct vcpu from svm.h
2023-02-22 12:59 ` Jan Beulich
@ 2023-02-23 10:50 ` Andrew Cooper
2023-02-23 11:00 ` Xenia Ragiadakou
0 siblings, 1 reply; 28+ messages in thread
From: Andrew Cooper @ 2023-02-23 10:50 UTC (permalink / raw)
To: Jan Beulich, Xenia Ragiadakou; +Cc: Roger Pau Monné, Wei Liu, xen-devel
On 22/02/2023 12:59 pm, Jan Beulich wrote:
> On 22.02.2023 13:00, Xenia Ragiadakou wrote:
>> Remove forward declaration of struct vcpu, that is a leftover since
>> the removal of svm_update_guest_cr()'s declaration from svm.h.
>>
>> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>
>
>> Fixes: b158e72abe30 ("x86/hvm: CFI hardening for hvm_funcs")
> I'm a little puzzled by this (a stray forward decl of a struct isn't
> really a bug imo), but ...
>> ---
>>
>> Changes in v2:
>> - leave the forward declaration of struct cpu_user_regs in place,
>> suggested by Andrew
>> - add a fixes tag based on Andrew's comment
> ... I realize you were asked to add it. (As a minor remark, more
> commonly the Fixes: tag would come ahead of the S-o-b: one, I think.)
I didn't intend my reply to mean "put in a fixes tag". I was just
trying to make an observation. But it doesn't hurt either.
But I do agree that a Fixes tag ought to be ahead of a SoB tag. Where
possible, we put tags in chronological order.
I can fix that on commit.
~Andrew
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 1/9] x86/svm: remove unused forward declaration of struct vcpu from svm.h
2023-02-23 10:50 ` Andrew Cooper
@ 2023-02-23 11:00 ` Xenia Ragiadakou
0 siblings, 0 replies; 28+ messages in thread
From: Xenia Ragiadakou @ 2023-02-23 11:00 UTC (permalink / raw)
To: Andrew Cooper, Jan Beulich; +Cc: Roger Pau Monné, Wei Liu, xen-devel
On 2/23/23 12:50, Andrew Cooper wrote:
> On 22/02/2023 12:59 pm, Jan Beulich wrote:
>> On 22.02.2023 13:00, Xenia Ragiadakou wrote:
>>> Remove forward declaration of struct vcpu, that is a leftover since
>>> the removal of svm_update_guest_cr()'s declaration from svm.h.
>>>
>>> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>
>>> Fixes: b158e72abe30 ("x86/hvm: CFI hardening for hvm_funcs")
>> I'm a little puzzled by this (a stray forward decl of a struct isn't
>> really a bug imo), but ...
>>> ---
>>>
>>> Changes in v2:
>>> - leave the forward declaration of struct cpu_user_regs in place,
>>> suggested by Andrew
>>> - add a fixes tag based on Andrew's comment
>> ... I realize you were asked to add it. (As a minor remark, more
>> commonly the Fixes: tag would come ahead of the S-o-b: one, I think.)
>
> I didn't intend my reply to mean "put in a fixes tag". I was just
> trying to make an observation. But it doesn't hurt either.
>
> But I do agree that a Fixes tag ought to be ahead of a SoB tag. Where
> possible, we put tags in chronological order.
>
> I can fix that on commit.
Thanks. I ve added it just for reference. It s ok to remove it.
>
> ~Andrew
--
Xenia
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 3/9] x86/svm: move declarations used only by svm code from svm.h to private header
2023-02-22 12:00 ` [PATCH v2 3/9] x86/svm: move declarations used only by svm code from svm.h to private header Xenia Ragiadakou
2023-02-23 10:24 ` Jan Beulich
@ 2023-02-23 11:16 ` Andrew Cooper
2023-02-23 14:40 ` Xenia Ragiadakou
1 sibling, 1 reply; 28+ messages in thread
From: Andrew Cooper @ 2023-02-23 11:16 UTC (permalink / raw)
To: Xenia Ragiadakou, xen-devel; +Cc: Jan Beulich, Roger Pau Monné, Wei Liu
On 22/02/2023 12:00 pm, Xenia Ragiadakou wrote:
> Create a new private header in arch/x86/hvm/svm called svm.h and move there
> all definitions and declarations that are used solely by svm code.
>
> The function svm_invlpga() stays in arch/x86/hvm/svm/svm.h because it is used
> by arch/x86/hvm/svm/asid.h.
I'm reasonably sure that all headers in arch/x86/hvm/svm/ other than
svm.h can move to being private easily.
>
> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
> ---
>
> Changes in v2:
> - new patch, the creation of a private header was suggested by Andrew and Jan
>
> I have not added #ifndef guards as it is a private and it should not be
> included by other headers. However, this is considered a MISRA-C violation
> ... I 'm not sure what to do.
Always have guards. Firstly because that is the decision taken by the
MISRA working group.
But more importantly, because life is too short to deal with the
shooting yourself in the foot which will occur from trying to take
shortcuts like these.
> diff --git a/xen/arch/x86/hvm/svm/svm.h b/xen/arch/x86/hvm/svm/svm.h
> new file mode 100644
> index 0000000000..b2ec293078
> --- /dev/null
> +++ b/xen/arch/x86/hvm/svm/svm.h
> @@ -0,0 +1,40 @@
> +#include <xen/types.h>
Elsewhere, we're retrofitting SPDX tags to all source files, but we're
already putting tags in new source files.
This one needs to be /* SPDX-License-Identifier: GPL-2.0 */ I think.
> +
> +static inline void svm_vmload_pa(paddr_t vmcb)
> +{
> + asm volatile (
> + ".byte 0x0f,0x01,0xda" /* vmload */
> + : : "a" (vmcb) : "memory" );
> +}
> +
> +static inline void svm_vmsave_pa(paddr_t vmcb)
> +{
> + asm volatile (
> + ".byte 0x0f,0x01,0xdb" /* vmsave */
> + : : "a" (vmcb) : "memory" );
> +}
> +
> +struct cpu_user_regs;
Looking at this, you could fold patch 1 into this patch and reduce the
net churn. It would be fine to say "delete used forward declaration" in
the commit message, seeing as you're deleting that block of code anyway
from svm.h
If you want to do this, then I'll skip committing patch 1.
~Andrew
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 7/9] x86/vmx: remove unused included headers from vmx.c
2023-02-22 12:00 ` [PATCH v2 7/9] x86/vmx: remove unused included headers from vmx.c Xenia Ragiadakou
@ 2023-02-23 11:16 ` Jan Beulich
0 siblings, 0 replies; 28+ messages in thread
From: Jan Beulich @ 2023-02-23 11:16 UTC (permalink / raw)
To: Xenia Ragiadakou
Cc: Jun Nakajima, Kevin Tian, Andrew Cooper, Roger Pau Monné,
Wei Liu, xen-devel
On 22.02.2023 13:00, Xenia Ragiadakou wrote:
> Do not include the headers:
> asm/hvm/vpic.h
> asm/hvm/vpt.h
> asm/io.h
> asm/mce.h
> asm/mem_sharing.h
This one puzzled me, so I've looked up its origin. It's entirely unclear
to me why 29317cfbf36d ("HAP fault handling for shared pages") added it
both here and in svm.c
> asm/regs.h
> public/arch-x86/cpuid.h
> public/hvm/save.h
> because none of the declarations and macro definitions in them is used.
> Sort the rest of the headers alphabetically.
>
> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
> ---
>
> Changes in v2:
> -add a blank line between different types of hesders
>
> xen/arch/x86/hvm/vmx/vmx.c | 56 +++++++++++++++++---------------------
> 1 file changed, 25 insertions(+), 31 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index a19ece90fc..a6ec0a11fb 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -15,52 +15,46 @@
> * this program; If not, see <http://www.gnu.org/licenses/>.
> */
>
> +#include <xen/domain_page.h>
> #include <xen/guest_access.h>
> +#include <xen/hypercall.h>
> #include <xen/init.h>
> +#include <xen/irq.h>
> #include <xen/lib.h>
> #include <xen/param.h>
> -#include <xen/trace.h>
> +#include <xen/perfc.h>
> #include <xen/sched.h>
> -#include <xen/irq.h>
> #include <xen/softirq.h>
> -#include <xen/domain_page.h>
> -#include <xen/hypercall.h>
> -#include <xen/perfc.h>
> -#include <asm/current.h>
> -#include <asm/io.h>
> -#include <asm/iocap.h>
> -#include <asm/regs.h>
> +#include <xen/trace.h>
> +
> +#include <asm/altp2m.h>
> +#include <asm/apic.h>
> #include <asm/cpufeature.h>
> -#include <asm/processor.h>
> +#include <asm/current.h>
> +#include <asm/gdbsx.h>
> #include <asm/debugreg.h>
> -#include <asm/msr.h>
> -#include <asm/p2m.h>
> -#include <asm/mem_sharing.h>
> +#include <asm/event.h>
> #include <asm/hvm/emulate.h>
> #include <asm/hvm/hvm.h>
> +#include <asm/hvm/monitor.h>
> +#include <asm/hvm/nestedhvm.h>
> #include <asm/hvm/support.h>
> -#include <asm/hvm/vmx/vmx.h>
> +#include <asm/hvm/trace.h>
> +#include <asm/hvm/vlapic.h>
> #include <asm/hvm/vmx/vmcs.h>
> -#include <public/sched.h>
> -#include <public/hvm/ioreq.h>
> +#include <asm/hvm/vmx/vmx.h>
> +#include <asm/iocap.h>
> #include <asm/i387.h>
> -#include <asm/hvm/vpic.h>
> -#include <asm/hvm/vlapic.h>
> -#include <asm/x86_emulate.h>
> -#include <asm/hvm/vpt.h>
> -#include <public/hvm/save.h>
> -#include <asm/hvm/trace.h>
> -#include <asm/hvm/monitor.h>
> -#include <asm/xenoprof.h>
> -#include <asm/gdbsx.h>
> -#include <asm/apic.h>
> -#include <asm/hvm/nestedhvm.h>
> -#include <asm/altp2m.h>
> -#include <asm/event.h>
> -#include <asm/mce.h>
> #include <asm/monitor.h>
> +#include <asm/msr.h>
> +#include <asm/processor.h>
> #include <asm/prot-key.h>
> -#include <public/arch-x86/cpuid.h>
> +#include <asm/p2m.h>
> +#include <asm/xenoprof.h>
> +#include <asm/x86_emulate.h>
> +
> +#include <public/sched.h>
> +#include <public/hvm/ioreq.h>
It's a shame this one's needed - handle_pio() really should have a "bool"
last parameter.
Anyway, patch looks okay to me, but will need a maintainer ack.
Jan
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 8/9] x86/vmx: declare nvmx_enqueue_n2_exceptions() static
2023-02-22 12:00 ` [PATCH v2 8/9] x86/vmx: declare nvmx_enqueue_n2_exceptions() static Xenia Ragiadakou
@ 2023-02-23 11:19 ` Jan Beulich
0 siblings, 0 replies; 28+ messages in thread
From: Jan Beulich @ 2023-02-23 11:19 UTC (permalink / raw)
To: Xenia Ragiadakou
Cc: Jun Nakajima, Kevin Tian, Andrew Cooper, Roger Pau Monné,
Wei Liu, xen-devel
On 22.02.2023 13:00, Xenia Ragiadakou wrote:
> Reduce the scope of nvmx_enqueue_n2_exceptions() to static because it is used
> only in this file.
>
> Take the opportunity to remove a trailing space.
>
> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 9/9] x86/vmx: move vmx_update_debug_state() in vmcs.c and declare it static
2023-02-22 12:00 ` [PATCH v2 9/9] x86/vmx: move vmx_update_debug_state() in vmcs.c and declare it static Xenia Ragiadakou
@ 2023-02-23 11:21 ` Jan Beulich
0 siblings, 0 replies; 28+ messages in thread
From: Jan Beulich @ 2023-02-23 11:21 UTC (permalink / raw)
To: Xenia Ragiadakou
Cc: Jun Nakajima, Kevin Tian, Andrew Cooper, Roger Pau Monné,
Wei Liu, xen-devel
On 22.02.2023 13:00, Xenia Ragiadakou wrote:
> Move vmx_update_debug_state() in vmcs.c because it is used only in this
> file and limit its scope to this file by declaring it static and removing
> its declaration from private vmx.h
>
> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
Perhaps move this ahead in the series, so you don't need to touch ...
> --- a/xen/arch/x86/hvm/vmx/vmx.h
> +++ b/xen/arch/x86/hvm/vmx/vmx.h
> @@ -14,7 +14,6 @@ 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);
... this header again right away?
Jan
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 6/9] x86/vmx: move declations used only by vmx code from vmx.h to private header
2023-02-23 10:47 ` Jan Beulich
@ 2023-02-23 11:56 ` Andrew Cooper
2023-02-24 7:49 ` Xenia Ragiadakou
1 sibling, 0 replies; 28+ messages in thread
From: Andrew Cooper @ 2023-02-23 11:56 UTC (permalink / raw)
To: Jan Beulich, Xenia Ragiadakou
Cc: Jun Nakajima, Kevin Tian, Roger Pau Monné, Wei Liu,
xen-devel
On 23/02/2023 10:47 am, Jan Beulich wrote:
> 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>
Like svm.h, SPDX tag and guards please.
>
>> +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?
Yes, it's deliberate. As you identified, it's never called, and
therefore doesn't undergo any CFI typechecking.
It's an address written into the VMCS's HOST_RIP field, which also
explains why it has a bogus type and no-one's noticed in 18 years. (Yup
really - c/s 9c3118a825, December 2004)
But it does bring us back to a general question which has come up a
couple of times recently - how to represent asm symbols in C.
This ought to be
void nocall vmx_asm_vmexit_handler(void);
to identify that it is a function-like thing without a normal calling
convention. IMO i could live in vmcs.c rather than being declared publicly.
I see nothing in MISRA that objects to this. Rule 8.5 states that an
external object or function shall be declared once, and only in one
file. There is no mandate that this single file is a header file.
The point of moving this to a .c file is to indicate that the asm symbol
isn't legitimate to reference anywhere else.
Such a change probably wants backporting. Let me do a specific fix, to
separate it from the cleanup.
~Andrew
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 3/9] x86/svm: move declarations used only by svm code from svm.h to private header
2023-02-23 11:16 ` Andrew Cooper
@ 2023-02-23 14:40 ` Xenia Ragiadakou
0 siblings, 0 replies; 28+ messages in thread
From: Xenia Ragiadakou @ 2023-02-23 14:40 UTC (permalink / raw)
To: Andrew Cooper, xen-devel; +Cc: Jan Beulich, Roger Pau Monné, Wei Liu
On 2/23/23 13:16, Andrew Cooper wrote:
> On 22/02/2023 12:00 pm, Xenia Ragiadakou wrote:
>> Create a new private header in arch/x86/hvm/svm called svm.h and move there
>> all definitions and declarations that are used solely by svm code.
>>
>> The function svm_invlpga() stays in arch/x86/hvm/svm/svm.h because it is used
>> by arch/x86/hvm/svm/asid.h.
>
> I'm reasonably sure that all headers in arch/x86/hvm/svm/ other than
> svm.h can move to being private easily.
>
>>
>> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
>> ---
>>
>> Changes in v2:
>> - new patch, the creation of a private header was suggested by Andrew and Jan
>>
>> I have not added #ifndef guards as it is a private and it should not be
>> included by other headers. However, this is considered a MISRA-C violation
>> ... I 'm not sure what to do.
>
> Always have guards. Firstly because that is the decision taken by the
> MISRA working group.
>
> But more importantly, because life is too short to deal with the
> shooting yourself in the foot which will occur from trying to take
> shortcuts like these.
>
>
>> diff --git a/xen/arch/x86/hvm/svm/svm.h b/xen/arch/x86/hvm/svm/svm.h
>> new file mode 100644
>> index 0000000000..b2ec293078
>> --- /dev/null
>> +++ b/xen/arch/x86/hvm/svm/svm.h
>> @@ -0,0 +1,40 @@
>> +#include <xen/types.h>
>
> Elsewhere, we're retrofitting SPDX tags to all source files, but we're
> already putting tags in new source files.
>
> This one needs to be /* SPDX-License-Identifier: GPL-2.0 */ I think.
>
>> +
>> +static inline void svm_vmload_pa(paddr_t vmcb)
>> +{
>> + asm volatile (
>> + ".byte 0x0f,0x01,0xda" /* vmload */
>> + : : "a" (vmcb) : "memory" );
>> +}
>> +
>> +static inline void svm_vmsave_pa(paddr_t vmcb)
>> +{
>> + asm volatile (
>> + ".byte 0x0f,0x01,0xdb" /* vmsave */
>> + : : "a" (vmcb) : "memory" );
>> +}
>> +
>> +struct cpu_user_regs;
>
> Looking at this, you could fold patch 1 into this patch and reduce the
> net churn. It would be fine to say "delete used forward declaration" in
> the commit message, seeing as you're deleting that block of code anyway
> from svm.h
>
> If you want to do this, then I'll skip committing patch 1.
Yes I will do it.
>
> ~Andrew
--
Xenia
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 6/9] x86/vmx: move declations used only by vmx code from vmx.h to private header
2023-02-23 10:47 ` Jan Beulich
2023-02-23 11:56 ` Andrew Cooper
@ 2023-02-24 7:49 ` Xenia Ragiadakou
2023-02-24 8:26 ` Jan Beulich
1 sibling, 1 reply; 28+ messages in thread
From: Xenia Ragiadakou @ 2023-02-24 7:49 UTC (permalink / raw)
To: Jan Beulich, Andrew Cooper
Cc: Jun Nakajima, Kevin Tian, Roger Pau Monné, Wei Liu,
xen-devel
Hi Jan,
On 2/23/23 12:47, Jan Beulich wrote:
> 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.
Yes I put everything as it was. Thanks for the review I will address all
your comments above in v3.
I have a question regarding the rearrangement, where are the definitions
placed (if there is not a semantic relation with some other part)?
>
> Jan
--
Xenia
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 6/9] x86/vmx: move declations used only by vmx code from vmx.h to private header
2023-02-24 7:49 ` Xenia Ragiadakou
@ 2023-02-24 8:26 ` Jan Beulich
0 siblings, 0 replies; 28+ messages in thread
From: Jan Beulich @ 2023-02-24 8:26 UTC (permalink / raw)
To: Xenia Ragiadakou
Cc: Jun Nakajima, Kevin Tian, Roger Pau Monné, Wei Liu,
xen-devel, Andrew Cooper
On 24.02.2023 08:49, Xenia Ragiadakou wrote:
> On 2/23/23 12:47, Jan Beulich wrote:
>> 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.
>
> Yes I put everything as it was. Thanks for the review I will address all
> your comments above in v3.
>
> I have a question regarding the rearrangement, where are the definitions
> placed (if there is not a semantic relation with some other part)?
If there really are any which aren't associated with other items (struct
fields first of all), then I'd say near the top. But I'm less concerned
there; putting struct-s ahead of function decls is mainly because the
latter may use (some of) the former. Similarly var decls before or after
func ones is pretty benign (sometimes it may even be more logical to
intermix them to keep related things together).
One other general request: Please trim replies. I had to scroll through
the entire 500-line mail just to find a single remark at the end, which
clearly didn't require all the context to be kept.
Jan
^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2023-02-24 8:26 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-02-22 12:00 [PATCH v2 0/9] x86/hvm: {svm,vmx}.{c,h} cleanup Xenia Ragiadakou
2023-02-22 12:00 ` [PATCH v2 1/9] x86/svm: remove unused forward declaration of struct vcpu from svm.h Xenia Ragiadakou
2023-02-22 12:59 ` Jan Beulich
2023-02-23 10:50 ` Andrew Cooper
2023-02-23 11:00 ` Xenia Ragiadakou
2023-02-22 12:00 ` [PATCH v2 2/9] x86/svm: opencode SVM_PAUSE{FILTER,THRESH}_INIT Xenia Ragiadakou
2023-02-23 10:19 ` Jan Beulich
2023-02-22 12:00 ` [PATCH v2 3/9] x86/svm: move declarations used only by svm code from svm.h to private header Xenia Ragiadakou
2023-02-23 10:24 ` Jan Beulich
2023-02-23 11:16 ` Andrew Cooper
2023-02-23 14:40 ` Xenia Ragiadakou
2023-02-22 12:00 ` [PATCH v2 4/9] x86/vmx: remove unused included headers from vmx.h Xenia Ragiadakou
2023-02-23 10:29 ` Jan Beulich
2023-02-23 10:42 ` Xenia Ragiadakou
2023-02-22 12:00 ` [PATCH v2 5/9] x86/vmx: reduce scope of GAS_VMX_OP definition Xenia Ragiadakou
2023-02-23 10:31 ` Jan Beulich
2023-02-23 10:45 ` Xenia Ragiadakou
2023-02-22 12:00 ` [PATCH v2 6/9] x86/vmx: move declations used only by vmx code from vmx.h to private header Xenia Ragiadakou
2023-02-23 10:47 ` Jan Beulich
2023-02-23 11:56 ` Andrew Cooper
2023-02-24 7:49 ` Xenia Ragiadakou
2023-02-24 8:26 ` Jan Beulich
2023-02-22 12:00 ` [PATCH v2 7/9] x86/vmx: remove unused included headers from vmx.c Xenia Ragiadakou
2023-02-23 11:16 ` Jan Beulich
2023-02-22 12:00 ` [PATCH v2 8/9] x86/vmx: declare nvmx_enqueue_n2_exceptions() static Xenia Ragiadakou
2023-02-23 11:19 ` Jan Beulich
2023-02-22 12:00 ` [PATCH v2 9/9] x86/vmx: move vmx_update_debug_state() in vmcs.c and declare it static Xenia Ragiadakou
2023-02-23 11:21 ` Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.