| Message ID | fd9d6043-4877-4156-97d9-4bdf7cc8fe04@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/HVM: misc tidying | expand |
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). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Roger Pau Monné <roger.pau@citrix.com> > --- > In how far the sole vmx_vmfunc use is actually meaningful (on its own) > I'm not really sure. > > --- 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(). 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. Thanks, Roger.
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
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@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. 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.
--- 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; static DEFINE_PER_CPU_READ_MOSTLY(paddr_t, vmxon_region); static DEFINE_PER_CPU(paddr_t, current_vmcs); @@ -475,8 +474,7 @@ static int vmx_init_vmcs_config(bool bsp vmx_basic_msr = ((u64)vmx_basic_msr_high << 32) | vmx_basic_msr_low; vmx_vmfunc = _vmx_vmfunc; - vmx_virt_exception = !!(_vmx_secondary_exec_control & - SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS); + vmx_display_features(); /* IA-32 SDM Vol 3B: VMCS size is never greater than 4kB. */
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). Signed-off-by: Jan Beulich <jbeulich@suse.com> --- In how far the sole vmx_vmfunc use is actually meaningful (on its own) I'm not really sure.