| Message ID | 20240206012051.3564035-4-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: > @@ -144,7 +144,7 @@ static bool __init hap_supported(struct hvm_function_table *fns) > > if ( !opt_hap_enabled ) > { > - fns->hap_supported = 0; > + fns->caps.hap = 0; As you touch such, would you mind switching to true/false instead of 1/0 at this occasion? > @@ -3000,7 +3000,7 @@ const struct hvm_function_table * __init start_vmx(void) > vmx_function_table.update_eoi_exit_bitmap = vmx_update_eoi_exit_bitmap; > vmx_function_table.process_isr = vmx_process_isr; > vmx_function_table.handle_eoi = vmx_handle_eoi; > - vmx_function_table.virtual_intr_delivery_enabled = true; > + vmx_function_table.caps.virtual_intr_delivery = true; I'm unsure about this one - it had "enabled" in its name for a good reason. Then again its (somewhat more involved) derivation from other capability bits (and a command line option) isn't fundamentally different from that of, say, hap_supported. Hence I guess with the other item taken care of Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
On 06.02.2024 02:20, George Dunlap wrote: > --- a/xen/arch/x86/include/asm/hvm/hvm.h > +++ b/xen/arch/x86/include/asm/hvm/hvm.h > @@ -86,20 +86,19 @@ struct hvm_vcpu_nonreg_state { > struct hvm_function_table { > const char *name; > > - /* Support Hardware-Assisted Paging? */ > - bool hap_supported; > - > - /* Necessary hardware support for alternate p2m's? */ > - bool altp2m_supported; > - bool singlestep_supported; > - > - /* Hardware virtual interrupt delivery enable? */ > - bool virtual_intr_delivery_enabled; > - > struct { > /* Indicate HAP capabilities. */ > - bool hap_superpage_1gb:1, > - hap_superpage_2mb:1; > + bool hap:1, > + hap_superpage_1gb:1, > + hap_superpage_2mb:1, > + > + /* Altp2m capabilities */ > + altp2m:1, > + singlestep:1, > + > + /* Hardware virtual interrupt delivery enable? */ > + virtual_intr_delivery; > + > } caps; Nit (spotted only while looking at patch 6): You're adding a stray blank line at the end of the structure. Further I expect virtual_intr_delivery would also want to be just a single bit? Jan
On Tue, Feb 20, 2024 at 12:08 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 06.02.2024 02:20, George Dunlap wrote: > > --- a/xen/arch/x86/include/asm/hvm/hvm.h > > +++ b/xen/arch/x86/include/asm/hvm/hvm.h > > @@ -86,20 +86,19 @@ struct hvm_vcpu_nonreg_state { > > struct hvm_function_table { > > const char *name; > > > > - /* Support Hardware-Assisted Paging? */ > > - bool hap_supported; > > - > > - /* Necessary hardware support for alternate p2m's? */ > > - bool altp2m_supported; > > - bool singlestep_supported; > > - > > - /* Hardware virtual interrupt delivery enable? */ > > - bool virtual_intr_delivery_enabled; > > - > > struct { > > /* Indicate HAP capabilities. */ > > - bool hap_superpage_1gb:1, > > - hap_superpage_2mb:1; > > + bool hap:1, > > + hap_superpage_1gb:1, > > + hap_superpage_2mb:1, > > + > > + /* Altp2m capabilities */ > > + altp2m:1, > > + singlestep:1, > > + > > + /* Hardware virtual interrupt delivery enable? */ > > + virtual_intr_delivery; > > + > > } caps; > > Nit (spotted only while looking at patch 6): You're adding a stray blank > line at the end of the structure. Further I expect virtual_intr_delivery > would also want to be just a single bit? Oh yes, good catch. (I kind of feel like ":1" should be the default for bools, but hey...) I'll fix up this and the 0/1 => false/true thing. -George
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index ae9d4c4756..aa2f2d054a 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -136,7 +136,7 @@ static struct notifier_block cpu_nfb = { static bool __init hap_supported(struct hvm_function_table *fns) { - if ( !fns->hap_supported ) + if ( !fns->caps.hap ) { printk("HVM: Hardware Assisted Paging (HAP) not detected\n"); return false; @@ -144,7 +144,7 @@ static bool __init hap_supported(struct hvm_function_table *fns) if ( !opt_hap_enabled ) { - fns->hap_supported = 0; + fns->caps.hap = 0; printk("HVM: Hardware Assisted Paging (HAP) detected but disabled\n"); return false; } @@ -190,7 +190,7 @@ static int __init cf_check hvm_enable(void) } if ( !opt_altp2m_enabled ) - hvm_funcs.altp2m_supported = 0; + hvm_funcs.caps.altp2m = 0; if ( opt_hvm_fep ) warning_add(warning_hvm_fep); diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c index 40bc1ffbc6..b551eac807 100644 --- a/xen/arch/x86/hvm/svm/svm.c +++ b/xen/arch/x86/hvm/svm/svm.c @@ -2580,7 +2580,7 @@ const struct hvm_function_table * __init start_svm(void) if ( !printed ) printk(" - none\n"); - svm_function_table.hap_supported = cpu_has_svm_npt; + svm_function_table.caps.hap = cpu_has_svm_npt; svm_function_table.caps.hap_superpage_2mb = true; svm_function_table.caps.hap_superpage_1gb = cpu_has_page1gb; diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c index 71a4b954b0..dcbcf4a1fe 100644 --- a/xen/arch/x86/hvm/vlapic.c +++ b/xen/arch/x86/hvm/vlapic.c @@ -1326,7 +1326,7 @@ int vlapic_has_pending_irq(struct vcpu *v) if ( irr == -1 ) return -1; - if ( hvm_funcs.virtual_intr_delivery_enabled && + if ( hvm_funcs.caps.virtual_intr_delivery && !nestedhvm_vcpu_in_guestmode(v) ) return irr; @@ -1361,7 +1361,7 @@ int vlapic_ack_pending_irq(struct vcpu *v, int vector, bool force_ack) int isr; if ( !force_ack && - hvm_funcs.virtual_intr_delivery_enabled ) + hvm_funcs.caps.virtual_intr_delivery ) return 1; /* If there's no chance of using APIC assist then bail now. */ diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c index 53f9d81aa9..aff69d5320 100644 --- a/xen/arch/x86/hvm/vmx/vmcs.c +++ b/xen/arch/x86/hvm/vmx/vmcs.c @@ -112,7 +112,7 @@ static int cf_check parse_ept_param_runtime(const char *s) struct domain *d; int val; - if ( !cpu_has_vmx_ept || !hvm_funcs.hap_supported || + if ( !cpu_has_vmx_ept || !hvm_funcs.caps.hap || !(hvm_funcs.caps.hap_superpage_2mb || hvm_funcs.caps.hap_superpage_1gb) ) { diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index 9cfc0140b4..4bcf436d2c 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -2963,7 +2963,7 @@ const struct hvm_function_table * __init start_vmx(void) return NULL; } - vmx_function_table.singlestep_supported = cpu_has_monitor_trap_flag; + vmx_function_table.caps.singlestep = cpu_has_monitor_trap_flag; if ( cpu_has_vmx_dt_exiting ) vmx_function_table.set_descriptor_access_exiting = @@ -2986,8 +2986,8 @@ const struct hvm_function_table * __init start_vmx(void) printk("VMX: Disabling executable EPT superpages due to CVE-2018-12207\n"); } - vmx_function_table.hap_supported = 1; - vmx_function_table.altp2m_supported = 1; + vmx_function_table.caps.hap = 1; + vmx_function_table.caps.altp2m = 1; vmx_function_table.caps.hap_superpage_2mb = cpu_has_vmx_ept_2mb; vmx_function_table.caps.hap_superpage_1gb = cpu_has_vmx_ept_1gb; @@ -3000,7 +3000,7 @@ const struct hvm_function_table * __init start_vmx(void) vmx_function_table.update_eoi_exit_bitmap = vmx_update_eoi_exit_bitmap; vmx_function_table.process_isr = vmx_process_isr; vmx_function_table.handle_eoi = vmx_handle_eoi; - vmx_function_table.virtual_intr_delivery_enabled = true; + vmx_function_table.caps.virtual_intr_delivery = true; } if ( cpu_has_vmx_posted_intr_processing ) diff --git a/xen/arch/x86/include/asm/hvm/hvm.h b/xen/arch/x86/include/asm/hvm/hvm.h index f50476f50f..bbd83a8275 100644 --- a/xen/arch/x86/include/asm/hvm/hvm.h +++ b/xen/arch/x86/include/asm/hvm/hvm.h @@ -86,20 +86,19 @@ struct hvm_vcpu_nonreg_state { struct hvm_function_table { const char *name; - /* Support Hardware-Assisted Paging? */ - bool hap_supported; - - /* Necessary hardware support for alternate p2m's? */ - bool altp2m_supported; - bool singlestep_supported; - - /* Hardware virtual interrupt delivery enable? */ - bool virtual_intr_delivery_enabled; - struct { /* Indicate HAP capabilities. */ - bool hap_superpage_1gb:1, - hap_superpage_2mb:1; + bool hap:1, + hap_superpage_1gb:1, + hap_superpage_2mb:1, + + /* Altp2m capabilities */ + altp2m:1, + singlestep:1, + + /* Hardware virtual interrupt delivery enable? */ + virtual_intr_delivery; + } caps; /* @@ -642,18 +641,18 @@ static inline void hvm_enable_msr_interception(struct domain *d, uint32_t msr) static inline bool hvm_is_singlestep_supported(void) { - return hvm_funcs.singlestep_supported; + return hvm_funcs.caps.singlestep; } static inline bool hvm_hap_supported(void) { - return hvm_funcs.hap_supported; + return hvm_funcs.caps.hap; } /* returns true if hardware supports alternate p2m's */ static inline bool hvm_altp2m_supported(void) { - return hvm_funcs.altp2m_supported; + return hvm_funcs.caps.altp2m; } /* updates the current hardware p2m */
Moving them all together has several advantages: * Collects them all in one part of the struct * The `caps` field means that we can drop the "_supported" suffix, as it's clear what is meant. Signed-off-by: George Dunlap <george.dunlap@cloud.com> --- 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 | 6 +++--- xen/arch/x86/hvm/svm/svm.c | 2 +- xen/arch/x86/hvm/vlapic.c | 4 ++-- xen/arch/x86/hvm/vmx/vmcs.c | 2 +- xen/arch/x86/hvm/vmx/vmx.c | 8 ++++---- xen/arch/x86/include/asm/hvm/hvm.h | 29 ++++++++++++++--------------- 6 files changed, 25 insertions(+), 26 deletions(-)