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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Nov 21, 2023 at 06:22:37PM +0100, Jan Beulich wrote:
> 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.

Right, I see now that we do only enable VMFUNC if EPTP switching is
supported, and hence we need to assert it's present on any other
CPUs, so yes, we must keep vmx_vmfunc.

> >> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> > 
> > Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> 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.

OK, IIRC in the past we have switched those kind of attributes as we
changed the code for other reasons I think, but I won't insist.
Coherency of attributes might be more valuable here.

> 
> > 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.

Hm, yes, since we do require PIN_BASED_EXT_INTR_MASK and
PIN_BASED_NMI_EXITING on pin_based_exec_control the setting of the
vmx_ fields is only done on the BSP, or else VMX is not enabled.  It
would be IMO clearer to do the initial setting of the vmx_ fields
based on the function `bsp` parameter.  Anyway, not for this patch.

Thanks, Roger.