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
