Re: [PATCH v3 10/14] x86/vmx: move declarations used only by vmx code from vmx.h to private headers

[Jan Beulich <jbeulich@xxxxxxxx>]

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