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(-)
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 */
--
2.25.1
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
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
© 2016 - 2024 Red Hat, Inc.