Re: [PATCH v3 10/14] x86/vmx: move declarations used only by vmx code from vmx.h to private headers
On 28.02.2023 08:36, Xenia Ragiadakou wrote: > > On 2/27/23 17:25, Jan Beulich wrote: >> On 24.02.2023 19:50, Xenia Ragiadakou wrote: >>> --- /dev/null >>> +++ b/xen/arch/x86/hvm/vmx/pi.h >>> @@ -0,0 +1,78 @@ >>> +/* SPDX-License-Identifier: GPL-2.0 */ >>> +/* >>> + * pi.h: VMX Posted Interrupts >>> + * >>> + * Copyright (c) 2004, Intel Corporation. >>> + */ >>> + >>> +#ifndef __X86_HVM_VMX_PI_PRIV_H__ >>> +#define __X86_HVM_VMX_PI_PRIV_H__ >> >> I can see that you need something to disambiguate the two vmx.h. But >> here you don't need the PRIV infix, do you? Even in the private vmx.h >> I'd like to ask to consider e.g. __VMX_H__ as the guard (and then >> __PI_H__ here), rather than one which doesn't really match the >> filename. > > I do agree that adding _PRIV_ is off target. > For the purpose of disambiguation, the header guards need to conform to > a well specified pattern guaranteed not to be used for anything else. > For that reason I would suggest the guard to include the path, not just > the file name. But as we see the path can be ambiguous, unless all non-private headers had an "INCLUDE" infix. Hence my proposal would be no path at all for private headers, and path (as we frequently but perhaps not consistently have it) for non-private ones. > In any case, the pattern that is used to generate the header guards > should be mentioned in the coding style doc. Perhaps. Jan
Re: [PATCH v3 10/14] x86/vmx: move declarations used only by vmx code from vmx.h to private headers
On 27.02.2023 17:26, Andrew Cooper wrote: > On 24/02/2023 6:50 pm, Xenia Ragiadakou wrote: >> Create two new private headers in arch/x86/hvm/vmx called vmx.h and pi.h. >> Move all the definitions and declarations that are used solely by vmx code >> into the private vmx.h, apart from the ones related to posted interrupts that >> are moved into pi.h. >> >> 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. > > Every time I read the vpmu code, I get increasingly sad. > > That is dangerously unsafe, and comes with a chance of exploding completely. > > That __vmread() is in NMI context, which means `current` isn't safe to > deference (we might hit in the middle of a context switch), and more > generally there's no guarantee that the loaded VMCS is the one > associated with `current` (we might hit in the middle of a remote VMCS > access). Are you mixing up oprofile (using NMI) and vPMU (using an ordinary vectored interrupt)? Or am I overlooking a vPMU mode of operation where NMI could be used (i.e. other than apic_intr_init()'s calling of set_direct_apic_vector() and other than pmu_interrupt() invoking vpmu_do_interrupt() /after/ acking the IRQ at the LAPIC)? Jan > vpmu is generally not supported, and BTS needs further custom enablement > because it is only useable with a custom bus analyser. > > > The __vmread() needs deleting - its absolutely not safe to say. > > I'm tempted to hardwire the return 0, and punt the problem to whomever > next uses BTS. > > Alternatively, MSR_DBGCTL needs wiring into the hvm_get_reg() > infrastructure, but I'm not convinced this will actually work in either > of the two problem cases above, hence preferring the previous option. > > Thoughts? > > ~Andrew
Re: [PATCH v3 10/14] x86/vmx: move declarations used only by vmx code from vmx.h to private headers
On 2/27/23 17:25, Jan Beulich wrote: On 24.02.2023 19:50, Xenia Ragiadakou wrote: --- /dev/null +++ b/xen/arch/x86/hvm/vmx/pi.h @@ -0,0 +1,78 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * pi.h: VMX Posted Interrupts + * + * Copyright (c) 2004, Intel Corporation. + */ + +#ifndef __X86_HVM_VMX_PI_PRIV_H__ +#define __X86_HVM_VMX_PI_PRIV_H__ I can see that you need something to disambiguate the two vmx.h. But here you don't need the PRIV infix, do you? Even in the private vmx.h I'd like to ask to consider e.g. __VMX_H__ as the guard (and then __PI_H__ here), rather than one which doesn't really match the filename. I do agree that adding _PRIV_ is off target. For the purpose of disambiguation, the header guards need to conform to a well specified pattern guaranteed not to be used for anything else. For that reason I would suggest the guard to include the path, not just the file name. In any case, the pattern that is used to generate the header guards should be mentioned in the coding style doc. Jan -- Xenia
Re: [PATCH v3 10/14] x86/vmx: move declarations used only by vmx code from vmx.h to private headers
On 27/02/2023 4:26 pm, Andrew Cooper wrote: > On 24/02/2023 6:50 pm, Xenia Ragiadakou wrote: >> Create two new private headers in arch/x86/hvm/vmx called vmx.h and pi.h. >> Move all the definitions and declarations that are used solely by vmx code >> into the private vmx.h, apart from the ones related to posted interrupts that >> are moved into pi.h. >> >> 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. > Every time I read the vpmu code, I get increasingly sad. > > That is dangerously unsafe, and comes with a chance of exploding completely. > > That __vmread() is in NMI context, which means `current` isn't safe to > deference (we might hit in the middle of a context switch), and more > generally there's no guarantee that the loaded VMCS is the one > associated with `current` (we might hit in the middle of a remote VMCS > access). > > vpmu is generally not supported, and BTS needs further custom enablement > because it is only useable with a custom bus analyser. > > > The __vmread() needs deleting - its absolutely not safe to say. to stay* > > I'm tempted to hardwire the return 0, and punt the problem to whomever > next uses BTS. > > Alternatively, MSR_DBGCTL needs wiring into the hvm_get_reg() > infrastructure, but I'm not convinced this will actually work in either > of the two problem cases above, hence preferring the previous option. > > Thoughts? > > ~Andrew
Re: [PATCH v3 10/14] x86/vmx: move declarations used only by vmx code from vmx.h to private headers
On 24/02/2023 6:50 pm, Xenia Ragiadakou wrote: > Create two new private headers in arch/x86/hvm/vmx called vmx.h and pi.h. > Move all the definitions and declarations that are used solely by vmx code > into the private vmx.h, apart from the ones related to posted interrupts that > are moved into pi.h. > > 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. Every time I read the vpmu code, I get increasingly sad. That is dangerously unsafe, and comes with a chance of exploding completely. That __vmread() is in NMI context, which means `current` isn't safe to deference (we might hit in the middle of a context switch), and more generally there's no guarantee that the loaded VMCS is the one associated with `current` (we might hit in the middle of a remote VMCS access). vpmu is generally not supported, and BTS needs further custom enablement because it is only useable with a custom bus analyser. The __vmread() needs deleting - its absolutely not safe to say. I'm tempted to hardwire the return 0, and punt the problem to whomever next uses BTS. Alternatively, MSR_DBGCTL needs wiring into the hvm_get_reg() infrastructure, but I'm not convinced this will actually work in either of the two problem cases above, hence preferring the previous option. Thoughts? ~Andrew
Re: [PATCH v3 10/14] x86/vmx: move declarations used only by vmx code from vmx.h to private headers
On 24.02.2023 19:50, Xenia Ragiadakou wrote: > --- /dev/null > +++ b/xen/arch/x86/hvm/vmx/pi.h > @@ -0,0 +1,78 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * pi.h: VMX Posted Interrupts > + * > + * Copyright (c) 2004, Intel Corporation. > + */ > + > +#ifndef __X86_HVM_VMX_PI_PRIV_H__ > +#define __X86_HVM_VMX_PI_PRIV_H__ I can see that you need something to disambiguate the two vmx.h. But here you don't need the PRIV infix, do you? Even in the private vmx.h I'd like to ask to consider e.g. __VMX_H__ as the guard (and then __PI_H__ here), rather than one which doesn't really match the filename. Jan
[PATCH v3 10/14] x86/vmx: move declarations used only by vmx code from vmx.h to private headers
Create two new private headers in arch/x86/hvm/vmx called vmx.h and pi.h.
Move all the definitions and declarations that are used solely by vmx code
into the private vmx.h, apart from the ones related to posted interrupts that
are moved into pi.h.
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 during the movement to replace u* with uint*_t, fix minor
coding style issues and reduce scope of GAS_VMX_OP definition.
Also, rearrange the code in the following way, place all structures first,
then all variable decalarations, all function delarations, and finally all
inline functions.
No functional change intended.
Signed-off-by: Xenia Ragiadakou
---
Changes in v3:
- add SPDX identifier, reported by Andrew
- add #ifndef header guard, reported by Andrew and Jan
- fold patch reducing the scope of GAS_VMX_OP definition into this,
suggested by Jan
- put pi related declarations in a separate priv header, suggested by Jan
- perform minor coding style fixes pointed out by Jan
- replace u* with uint*_t, suggested by Jan
- rearrange the header in the way Jan's proposed
- rebase to the latest staging
xen/arch/x86/hvm/vmx/intr.c| 2 +
xen/arch/x86/hvm/vmx/pi.h | 78 +
xen/arch/x86/hvm/vmx/realmode.c| 2 +
xen/arch/x86/hvm/vmx/vmcs.c| 2 +
xen/arch/x86/hvm/vmx/vmx.c | 3 +
xen/arch/x86/hvm/vmx/vmx.h | 416 +++
xen/arch/x86/hvm/vmx/vvmx.c| 2 +
xen/arch/x86/include/asm/hvm/vmx/vmx.h | 439 -
8 files changed, 505 insertions(+), 439 deletions(-)
create mode 100644 xen/arch/x86/hvm/vmx/pi.h
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
#include
+#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/pi.h b/xen/arch/x86/hvm/vmx/pi.h
new file mode 100644
index 00..c72cc511da
--- /dev/null
+++ b/xen/arch/x86/hvm/vmx/pi.h
@@ -0,0 +1,78 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * pi.h: VMX Posted Interrupts
+ *
+ * Copyright (c) 2004, Intel Corporation.
+ */
+
+#ifndef __X86_HVM_VMX_PI_PRIV_H__
+#define __X86_HVM_VMX_PI_PRIV_H__
+
+#include
+
+#include
+
+#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, _desc->control);
+}
+
+static inline void pi_set_on(struct pi_desc *pi_desc)
+{
+set_bit(POSTED_INTR_ON, _desc->control);
+}
+
+static inline int pi_test_and_clear_on(struct pi_desc *pi_desc)
+{
+return test_and_clear_bit(POSTED_INTR_ON, _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(_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, _desc->control);
+}
+
+static inline void pi_clear_sn(struct pi_desc *pi_desc)
+{
+clear_bit(POSTED_INTR_SN, _desc->control);
+}
+
+#endif /* __X86_HVM_VMX_PI_PRIV_H__ */
+
+/*
+ * 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/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
#include
+#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 d3c75b3803..4eb2571abb 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -43,6 +43,8 @@
#include
#include
+#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 232107bd79..cb8b133ed5 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -62,6 +62,9 @@
#include
#include
