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

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Xenia Ragiadakou <burzalodowa@gmail.com>, xen-devel@lists.xenproject.org
Cc: "Jun Nakajima" <jun.nakajima@intel.com>,
	"Kevin Tian" <kevin.tian@intel.com>,
	"Jan Beulich" <jbeulich@suse.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>
Subject: Re: [PATCH v3 10/14] x86/vmx: move declarations used only by vmx code from vmx.h to private headers
Date: Mon, 27 Feb 2023 16:41:40 +0000	[thread overview]
Message-ID: <a76615be-99db-90ba-292f-1d96c6844e1b@citrix.com> (raw)
In-Reply-To: <12849cea-58af-e13f-89ab-a7e9d4200df8@citrix.com>

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