| Message ID | 20240206012051.3564035-2-george.dunlap@cloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | AMD Nested Virt Preparation | expand |
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 <george.dunlap@cloud.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> > --- > 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
On Mon, Feb 19, 2024 at 9:36 PM Jan Beulich <jbeulich@suse.com> wrote: > > 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 <george.dunlap@cloud.com> > > Reviewed-by: Jan Beulich <jbeulich@suse.com> > > > --- > > 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. Basically there are two stances to take: 1. hap_superpage_2m always has meaning. If there's no HAP, then there are no HAP superpages, so we should say that there are no superpages. 2. hap_superpage_2m only has meaning if hap is enabled. If there's no HAP, then the question "are there superpages" is moot; nobody should be paying attention to it. The second is not without precedent, but is it really the best stance? It means that before reading hap_superpage_2m, you have to first check hap; and it introduces a risk (no matter how small) that someone will forget to check, and end up doing the wrong thing. Not a huge deal, but overall my vote would be #1. I may send a patch at some point in the future. > > * 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. Right, forgot to actually delete this para after adding a patch to address it. > > --- 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".) So you mean basically that the checks & error message are poorly factored, because there's only a single sub-option? (i.e., if there were options which didn't rely on superpages, the check would be incorrect?) Let me know if there's something concrete you'd like me to do here. > > @@ -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. Done. -George
On 21.02.2024 08:02, George Dunlap wrote: > On Mon, Feb 19, 2024 at 9:36 PM Jan Beulich <jbeulich@suse.com> wrote: >> On 06.02.2024 02:20, George Dunlap wrote: >>> --- 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".) > > So you mean basically that the checks & error message are poorly > factored, because there's only a single sub-option? (i.e., if there > were options which didn't rely on superpages, the check would be > incorrect?) Right. > Let me know if there's something concrete you'd like me to do here. Nothing. I meant to express this by starting with "Just to mention it". If you were eager, you might deal with this right away, but there's no expectation to this effect. If and when it becomes a problem, it'll then need sorting. And the message being potentially misleading is, well, just an annoyance - as with many other things, finding it confusing will likely lead to looking up where it's issued, then realizing that what it says isn't what it means. Jan
On Wed, Feb 21, 2024 at 3:23 PM Jan Beulich <jbeulich@suse.com> wrote: > > On 21.02.2024 08:02, George Dunlap wrote: > > On Mon, Feb 19, 2024 at 9:36 PM Jan Beulich <jbeulich@suse.com> wrote: > >> On 06.02.2024 02:20, George Dunlap wrote: > >>> --- 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".) > > > > So you mean basically that the checks & error message are poorly > > factored, because there's only a single sub-option? (i.e., if there > > were options which didn't rely on superpages, the check would be > > incorrect?) > > Right. > > > Let me know if there's something concrete you'd like me to do here. > > Nothing. I meant to express this by starting with "Just to mention it". Right, and when I said "let me know" I meant, "I'm going to ignore this unless you say something, feel free to say nothing". :-D I understood that you weren't asking for anything, but maybe coming back to this after a few days you'd've had a simple fix. I wouldn't mind changing the text of the message, but I didn't feel like finding a better text. Reorganizing the checks (which seems closer to the Right Thing) is off-topic for this patch of course. -George
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index e8deeb0222..ae9d4c4756 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -174,17 +174,17 @@ static int __init cf_check hvm_enable(void) { printk("HVM: Hardware Assisted Paging (HAP) detected\n"); printk("HVM: HAP page sizes: 4kB"); - if ( fns->hap_capabilities & HVM_HAP_SUPERPAGE_2MB ) + if ( fns->caps.hap_superpage_2mb ) { printk(", 2MB%s", opt_hap_2mb ? "" : " [disabled]"); if ( !opt_hap_2mb ) - hvm_funcs.hap_capabilities &= ~HVM_HAP_SUPERPAGE_2MB; + hvm_funcs.caps.hap_superpage_2mb = false; } - if ( fns->hap_capabilities & HVM_HAP_SUPERPAGE_1GB ) + if ( fns->caps.hap_superpage_1gb ) { printk(", 1GB%s", opt_hap_1gb ? "" : " [disabled]"); if ( !opt_hap_1gb ) - hvm_funcs.hap_capabilities &= ~HVM_HAP_SUPERPAGE_1GB; + hvm_funcs.caps.hap_superpage_1gb = false; } printk("\n"); } diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c index 65f437e958..5741287355 100644 --- a/xen/arch/x86/hvm/svm/svm.c +++ b/xen/arch/x86/hvm/svm/svm.c @@ -2581,8 +2581,8 @@ const struct hvm_function_table * __init start_svm(void) printk(" - none\n"); svm_function_table.hap_supported = !!cpu_has_svm_npt; - svm_function_table.hap_capabilities = HVM_HAP_SUPERPAGE_2MB | - (cpu_has_page1gb ? HVM_HAP_SUPERPAGE_1GB : 0); + svm_function_table.caps.hap_superpage_2mb = true; + svm_function_table.caps.hap_superpage_1gb = cpu_has_page1gb; return &svm_function_table; } diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c index 4fe1213855..53f9d81aa9 100644 --- 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"); return 0; diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index 1500dca603..9cfc0140b4 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -2989,12 +2989,8 @@ const struct hvm_function_table * __init start_vmx(void) vmx_function_table.hap_supported = 1; vmx_function_table.altp2m_supported = 1; - vmx_function_table.hap_capabilities = 0; - - if ( cpu_has_vmx_ept_2mb ) - vmx_function_table.hap_capabilities |= HVM_HAP_SUPERPAGE_2MB; - if ( cpu_has_vmx_ept_1gb ) - vmx_function_table.hap_capabilities |= HVM_HAP_SUPERPAGE_1GB; + vmx_function_table.caps.hap_superpage_2mb = cpu_has_vmx_ept_2mb; + vmx_function_table.caps.hap_superpage_1gb = cpu_has_vmx_ept_1gb; setup_ept_dump(); } diff --git a/xen/arch/x86/include/asm/hvm/hvm.h b/xen/arch/x86/include/asm/hvm/hvm.h index 985c1c14c6..f50476f50f 100644 --- a/xen/arch/x86/include/asm/hvm/hvm.h +++ b/xen/arch/x86/include/asm/hvm/hvm.h @@ -61,14 +61,6 @@ enum hvm_intblk { #define HVM_INTR_SHADOW_SMI 0x00000004 #define HVM_INTR_SHADOW_NMI 0x00000008 -/* - * HAP super page capabilities: - * bit0: if 2MB super page is allowed? - * bit1: if 1GB super page is allowed? - */ -#define HVM_HAP_SUPERPAGE_2MB 0x00000001 -#define HVM_HAP_SUPERPAGE_1GB 0x00000002 - #define HVM_EVENT_VECTOR_UNSET (-1) #define HVM_EVENT_VECTOR_UPDATING (-2) @@ -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; + } caps; /* * Initialise/destroy HVM domain/vcpu resources @@ -402,8 +397,8 @@ int hvm_get_param(struct domain *d, uint32_t index, uint64_t *value); (hvm_paging_enabled(v) && ((v)->arch.hvm.guest_cr[4] & X86_CR4_PKS)) /* Can we use superpages in the HAP p2m table? */ -#define hap_has_1gb (!!(hvm_funcs.hap_capabilities & HVM_HAP_SUPERPAGE_1GB)) -#define hap_has_2mb (!!(hvm_funcs.hap_capabilities & HVM_HAP_SUPERPAGE_2MB)) +#define hap_has_1gb hvm_funcs.caps.hap_superpage_1gb +#define hap_has_2mb hvm_funcs.caps.hap_superpage_2mb #define hvm_long_mode_active(v) (!!((v)->arch.hvm.guest_efer & EFER_LMA))
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 <george.dunlap@cloud.com> --- Questions: * Should hap_superpage_2m really be set unconditionally, or should we condition it on cpu_has_svm_npt? * 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? CC: Jan Beulich <jbeulich@suse.com> CC: Andrew Cooper <andrew.cooper3@citrix.com> CC: "Roger Pau Monné" <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> CC: Jun Nakajima <jun.nakajima@intel.com> CC: Kevin Tian <kevin.tian@intel.com> --- xen/arch/x86/hvm/hvm.c | 8 ++++---- xen/arch/x86/hvm/svm/svm.c | 4 ++-- xen/arch/x86/hvm/vmx/vmcs.c | 4 ++-- xen/arch/x86/hvm/vmx/vmx.c | 8 ++------ xen/arch/x86/include/asm/hvm/hvm.h | 19 +++++++------------ 5 files changed, 17 insertions(+), 26 deletions(-)