On 06.02.2024 02:20, George Dunlap wrote: > hvm_function_table is an internal structure; rather than manually > |-ing and &-ing bits, just make it a boolean bitfield and let the > compiler do all the work. This makes everything easier to read, and > presumably allows the compiler more flexibility in producing efficient > code. > > No functional change intended. > > Signed-off-by: George Dunlap <[email protected]>
Reviewed-by: Jan Beulich <[email protected]> > --- > Questions: > > * Should hap_superpage_2m really be set unconditionally, or should we > condition it on cpu_has_svm_npt? That's HAP capabilities; there's not going to be any use of HAP when there's no NPT (on an AMD system). IOW - all is fine as is, imo. > * Do we really need to "!!cpu_has_svm_npt"? If so, wouldn't it be > better to put the "!!" in the #define, rather than requiring the > user to know that it's needed? Considering that hap_supported is bool now, the !! can simply be dropped. We've been doing so as code was touched anyway, not in a concerted effort. > --- a/xen/arch/x86/hvm/vmx/vmcs.c > +++ b/xen/arch/x86/hvm/vmx/vmcs.c > @@ -113,8 +113,8 @@ static int cf_check parse_ept_param_runtime(const char *s) > int val; > > if ( !cpu_has_vmx_ept || !hvm_funcs.hap_supported || > - !(hvm_funcs.hap_capabilities & > - (HVM_HAP_SUPERPAGE_2MB | HVM_HAP_SUPERPAGE_1GB)) ) > + !(hvm_funcs.caps.hap_superpage_2mb || > + hvm_funcs.caps.hap_superpage_1gb) ) > { > printk("VMX: EPT not available, or not in use - ignoring\n"); Just to mention it: The conditional and the log message don't really fit together. (I was first wondering what the 2mb/1gb checks had to do here at all, but that's immediately clear when seeing that the only sub-option here is "exec-sp".) > @@ -104,8 +96,11 @@ struct hvm_function_table { > /* Hardware virtual interrupt delivery enable? */ > bool virtual_intr_delivery_enabled; > > - /* Indicate HAP capabilities. */ > - unsigned int hap_capabilities; > + struct { > + /* Indicate HAP capabilities. */ > + bool hap_superpage_1gb:1, > + hap_superpage_2mb:1; Nit: Would be nice imo if the two identifiers aligned vertically with one another. Jan
