| Message ID | 20230206125804.950528-1-burzalodowa@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/vlapic: call vmx_vlapic_msr_changed through an hvm_function callback | expand |
On 06/02/2023 12:58 pm, Xenia Ragiadakou wrote: > diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c > index 270bc98195..6138dc0885 100644 > --- a/xen/arch/x86/hvm/vmx/vmx.c > +++ b/xen/arch/x86/hvm/vmx/vmx.c > @@ -3011,6 +3011,10 @@ const struct hvm_function_table * __init start_vmx(void) > setup_ept_dump(); > } > > + if ( cpu_has_vmx_virtualize_apic_accesses || > + cpu_has_vmx_virtualize_x2apic_mode ) x2apic_mode is definitely wrong here, but I think apic_accesses is too. The top of vmx_vlapic_msr_changed() is buggy too. Right now, the hook is called unconditionally. Given no adjustment in vmx_vlapic_msr_changed(), the new form (using an alternative) needs calling unconditionally too. Naming wise, Linux is fairly bogus too. This should be hvm_update_vlapic_mode(), but I suspect the hook will disappear in due course. > diff --git a/xen/arch/x86/include/asm/hvm/hvm.h b/xen/arch/x86/include/asm/hvm/hvm.h > index 80e4565bd2..b690e2924c 100644 > --- a/xen/arch/x86/include/asm/hvm/hvm.h > +++ b/xen/arch/x86/include/asm/hvm/hvm.h > @@ -786,6 +787,11 @@ static inline int hvm_pi_update_irte(const struct vcpu *v, > return alternative_call(hvm_funcs.pi_update_irte, v, pirq, gvec); > } > > +static inline void hvm_set_virtual_apic_mode(struct vcpu *v) > +{ > + alternative_vcall(hvm_funcs.set_virtual_apic_mode, v); This has to be something like: if ( hvm_funcs.set_virtual_apic_mode ) alternative_vcall(...) Otherwise, Xen will BUG() every time an SVM guest modifies MSR_APIC_BASE. ~Andrew
On 2/6/23 15:32, Andrew Cooper wrote: > On 06/02/2023 12:58 pm, Xenia Ragiadakou wrote: >> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c >> index 270bc98195..6138dc0885 100644 >> --- a/xen/arch/x86/hvm/vmx/vmx.c >> +++ b/xen/arch/x86/hvm/vmx/vmx.c >> @@ -3011,6 +3011,10 @@ const struct hvm_function_table * __init start_vmx(void) >> setup_ept_dump(); >> } >> >> + if ( cpu_has_vmx_virtualize_apic_accesses || >> + cpu_has_vmx_virtualize_x2apic_mode ) > > x2apic_mode is definitely wrong here, but I think apic_accesses is too. Why? > The top of vmx_vlapic_msr_changed() is buggy too. > > Right now, the hook is called unconditionally. Given no adjustment in > vmx_vlapic_msr_changed(), the new form (using an alternative) needs > calling unconditionally too. Ok, I will initialize it unconditionally when the vmx_function_table is defined. > > Naming wise, Linux is fairly bogus too. This should be > hvm_update_vlapic_mode(), but I suspect the hook will disappear in due > course. I can change the name. I gave the same name for cross reference purposes. > >> diff --git a/xen/arch/x86/include/asm/hvm/hvm.h b/xen/arch/x86/include/asm/hvm/hvm.h >> index 80e4565bd2..b690e2924c 100644 >> --- a/xen/arch/x86/include/asm/hvm/hvm.h >> +++ b/xen/arch/x86/include/asm/hvm/hvm.h >> @@ -786,6 +787,11 @@ static inline int hvm_pi_update_irte(const struct vcpu *v, >> return alternative_call(hvm_funcs.pi_update_irte, v, pirq, gvec); >> } >> >> +static inline void hvm_set_virtual_apic_mode(struct vcpu *v) >> +{ >> + alternative_vcall(hvm_funcs.set_virtual_apic_mode, v); > > This has to be something like: > > if ( hvm_funcs.set_virtual_apic_mode ) > alternative_vcall(...) > > Otherwise, Xen will BUG() every time an SVM guest modifies MSR_APIC_BASE. Yes, that's true. I will fix it. > > ~Andrew
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c index eb32f12e2d..34a7eea3a1 100644 --- a/xen/arch/x86/hvm/vlapic.c +++ b/xen/arch/x86/hvm/vlapic.c @@ -37,8 +37,8 @@ #include <asm/hvm/hvm.h> #include <asm/hvm/io.h> #include <asm/hvm/support.h> -#include <asm/hvm/vmx/vmx.h> #include <asm/hvm/nestedhvm.h> +#include <asm/hvm/trace.h> #include <asm/hvm/viridian.h> #include <public/hvm/ioreq.h> #include <public/hvm/params.h> @@ -1165,7 +1165,7 @@ int guest_wrmsr_apic_base(struct vcpu *v, uint64_t value) if ( vlapic_x2apic_mode(vlapic) ) set_x2apic_id(vlapic); - vmx_vlapic_msr_changed(vlapic_vcpu(vlapic)); + hvm_set_virtual_apic_mode(vlapic_vcpu(vlapic)); HVM_DBG_LOG(DBG_LEVEL_VLAPIC, "apic base msr is 0x%016"PRIx64, vlapic->hw.apic_base_msr); @@ -1561,7 +1561,7 @@ static int cf_check lapic_load_hidden(struct domain *d, hvm_domain_context_t *h) unlikely(vlapic_x2apic_mode(s)) ) return -EINVAL; - vmx_vlapic_msr_changed(v); + hvm_set_virtual_apic_mode(v); return 0; } diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index 270bc98195..6138dc0885 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -3011,6 +3011,10 @@ const struct hvm_function_table * __init start_vmx(void) setup_ept_dump(); } + if ( cpu_has_vmx_virtualize_apic_accesses || + cpu_has_vmx_virtualize_x2apic_mode ) + vmx_function_table.set_virtual_apic_mode = vmx_vlapic_msr_changed; + if ( cpu_has_vmx_virtual_intr_delivery ) { vmx_function_table.update_eoi_exit_bitmap = vmx_update_eoi_exit_bitmap; diff --git a/xen/arch/x86/include/asm/hvm/hvm.h b/xen/arch/x86/include/asm/hvm/hvm.h index 80e4565bd2..b690e2924c 100644 --- a/xen/arch/x86/include/asm/hvm/hvm.h +++ b/xen/arch/x86/include/asm/hvm/hvm.h @@ -217,6 +217,7 @@ struct hvm_function_table { void (*handle_eoi)(uint8_t vector, int isr); int (*pi_update_irte)(const struct vcpu *v, const struct pirq *pirq, uint8_t gvec); + void (*set_virtual_apic_mode)(struct vcpu *v); /*Walk nested p2m */ int (*nhvm_hap_walk_L1_p2m)(struct vcpu *v, paddr_t L2_gpa, @@ -786,6 +787,11 @@ static inline int hvm_pi_update_irte(const struct vcpu *v, return alternative_call(hvm_funcs.pi_update_irte, v, pirq, gvec); } +static inline void hvm_set_virtual_apic_mode(struct vcpu *v) +{ + alternative_vcall(hvm_funcs.set_virtual_apic_mode, v); +} + #else /* CONFIG_HVM */ #define hvm_enabled false
APIC virtualization support is currently implemented only for Intel VT-x. To aid future work on separating AMD-V from Intel VT-x code, instead of calling directly vmx_vlapic_msr_changed() from common hvm code, add a stub to the hvm_function_table, named set_virtual_apic_mode (to match the name of the corresponding function in Linux for cross reference). Then, create a wrapper function, called hvm_set_virtual_apic_mode(), to be used by common hvm code. For Intel VT-x, initialize the stub only when either virtual xAPIC mode or virtual x2APIC mode is supported. After the change above, do not include header asm/hvm/vmx/vmx.h as it is not required anymore and resolve subsequent build errors for implicit declaration of functions ‘TRACE_2_LONG_3D’ and ‘TRC_PAR_LONG’ by including missing asm/hvm/trace.h header. No functional change intended. Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com> --- xen/arch/x86/hvm/vlapic.c | 6 +++--- xen/arch/x86/hvm/vmx/vmx.c | 4 ++++ xen/arch/x86/include/asm/hvm/hvm.h | 6 ++++++ 3 files changed, 13 insertions(+), 3 deletions(-)