Re: [PATCH 1/5] VMX: drop vmx_virt_exception and make vmx_vmfunc static

From: Jan Beulich <jbeulich@suse.com>
To: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	Andrew Cooper <andrew.cooper3@citrix.com>, Wei Liu <wl@xen.org>,
	Kevin Tian <kevin.tian@intel.com>,
	Jun Nakajima <jun.nakajima@intel.com>
Subject: Re: [PATCH 1/5] VMX: drop vmx_virt_exception and make vmx_vmfunc static
Date: Tue, 21 Nov 2023 18:22:37 +0100	[thread overview]
Message-ID: <a058b837-82da-4b93-be5c-b616511d24f9@suse.com> (raw)
In-Reply-To: <ZVzROZu9Afs-mEah@macbook.local>

On 21.11.2023 16:48, Roger Pau Monné wrote:
> On Thu, Nov 16, 2023 at 02:30:41PM +0100, Jan Beulich wrote:
>> The variable was introduced by 69b830e5ffb4 ("VMX: VMFUNC and #VE
>> definitions and detection") without any use and - violating Misra C:2012
>> rule 8.4 - without a declaration. Since no use has appeared, drop it.
>>
>> For vmx_vmfunc the situation is similar, but not identical: It at least
>> has one use. Convert it to be static (and make style adjustments while
>> there).
> 
> I think you could also remove the sole user of vmx_vmfunc, as it's
> just a cap_check() usage (unless there are more hidden usages).

Well, perhaps (and hence my post-commit-message remark in the original
submission). Yet then I thought we permitted vmfunc use for altp2m, at
which point the cap_check() is meaningful.

>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

>> ---
>> In how far the sole vmx_vmfunc use is actually meaningful (on its own)
>> I'm not really sure.

(Here ^^^)

>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>> @@ -167,8 +167,7 @@ u32 vmx_secondary_exec_control __read_mo
>>  u32 vmx_vmexit_control __read_mostly;
>>  u32 vmx_vmentry_control __read_mostly;
>>  u64 vmx_ept_vpid_cap __read_mostly;
>> -u64 vmx_vmfunc __read_mostly;
>> -bool_t vmx_virt_exception __read_mostly;
>> +static uint64_t __read_mostly vmx_vmfunc;
> 
> I'm quite sure this should be __ro_after_init, but I guess we cannot
> be sure give the current code in vmx_init_vmcs_config().

I think we can be sure. But if we were to switch, I think all the
related variables should also be switched at the same time.

> Any CPU hot plugged that has a different set of VMX controls should
> not be onlined, the more that migrating an already running VMCS to
> such CPU will lead to failures if non-supported features are used.

That's the intention of that code, yes.

Jan