| Message ID | 20230213115017.902037-1-burzalodowa@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] x86/vlapic: call vmx_vlapic_msr_changed through an hvm_function callback | expand |
On 13.02.2023 12:50, Xenia Ragiadakou wrote: > 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 update_vlapic_mode, and create a wrapper > function, called hvm_vlapic_mode(), to be used by common hvm code. > > 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> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> From: Xenia Ragiadakou <burzalodowa@gmail.com> > Sent: Monday, February 13, 2023 7:50 PM > > 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 update_vlapic_mode, and create a > wrapper > function, called hvm_vlapic_mode(), to be used by common hvm code. > > 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> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
On 13/02/2023 11:50 am, Xenia Ragiadakou wrote: > diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmx.h b/xen/arch/x86/include/asm/hvm/vmx/vmx.h > index 234da4a7f4..97d6b810ec 100644 > --- a/xen/arch/x86/include/asm/hvm/vmx/vmx.h > +++ b/xen/arch/x86/include/asm/hvm/vmx/vmx.h > @@ -85,7 +85,7 @@ typedef enum { > void vmx_asm_vmexit_handler(struct cpu_user_regs); > void vmx_intr_assist(void); > void noreturn cf_check vmx_do_resume(void); > -void vmx_vlapic_msr_changed(struct vcpu *v); > +void cf_check vmx_vlapic_msr_changed(struct vcpu *v); Hi, I see this patch has been committed, but this public declaration should deleted, and vmx_vlapic_msr_changed() made static now that it's only referenced in vmx.c. It needs a forward declaration in vmx.c because of its position relative to the vmx_function_table, but that's fine - we've got plenty of other examples like this. Could I talk you into doing an incremental fix? Alternatively, we could get better cleanup by forward declaring just {vmx,svm}_function_table, then moving the tables to the very bottom of {vmx,svm}.c at which point we can drop all the forward declarations. Oh top of that, I suspect we have other public function definitions which can turn static, if you happen to spot any while doing this. Thanks, ~Andrew
Hi Andrew, On 2/16/23 12:28, Andrew Cooper wrote: > On 13/02/2023 11:50 am, Xenia Ragiadakou wrote: >> diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmx.h b/xen/arch/x86/include/asm/hvm/vmx/vmx.h >> index 234da4a7f4..97d6b810ec 100644 >> --- a/xen/arch/x86/include/asm/hvm/vmx/vmx.h >> +++ b/xen/arch/x86/include/asm/hvm/vmx/vmx.h >> @@ -85,7 +85,7 @@ typedef enum { >> void vmx_asm_vmexit_handler(struct cpu_user_regs); >> void vmx_intr_assist(void); >> void noreturn cf_check vmx_do_resume(void); >> -void vmx_vlapic_msr_changed(struct vcpu *v); >> +void cf_check vmx_vlapic_msr_changed(struct vcpu *v); > > Hi, > > I see this patch has been committed, but this public declaration should > deleted, and vmx_vlapic_msr_changed() made static now that it's only > referenced in vmx.c. It is also used in vmcs.c > > It needs a forward declaration in vmx.c because of its position relative > to the vmx_function_table, but that's fine - we've got plenty of other > examples like this. > > Could I talk you into doing an incremental fix? > > Alternatively, we could get better cleanup by forward declaring just > {vmx,svm}_function_table, then moving the tables to the very bottom of > {vmx,svm}.c at which point we can drop all the forward declarations. > > Oh top of that, I suspect we have other public function definitions > which can turn static, if you happen to spot any while doing this. Sure I could try to cleanup {svm,vmx}.c and the corresponding headers. > > Thanks, > > ~Andrew
On 16/02/2023 1:36 pm, Xenia Ragiadakou wrote: > Hi Andrew, > > On 2/16/23 12:28, Andrew Cooper wrote: >> On 13/02/2023 11:50 am, Xenia Ragiadakou wrote: >>> diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmx.h >>> b/xen/arch/x86/include/asm/hvm/vmx/vmx.h >>> index 234da4a7f4..97d6b810ec 100644 >>> --- a/xen/arch/x86/include/asm/hvm/vmx/vmx.h >>> +++ b/xen/arch/x86/include/asm/hvm/vmx/vmx.h >>> @@ -85,7 +85,7 @@ typedef enum { >>> void vmx_asm_vmexit_handler(struct cpu_user_regs); >>> void vmx_intr_assist(void); >>> void noreturn cf_check vmx_do_resume(void); >>> -void vmx_vlapic_msr_changed(struct vcpu *v); >>> +void cf_check vmx_vlapic_msr_changed(struct vcpu *v); >> >> Hi, >> >> I see this patch has been committed, but this public declaration should >> deleted, and vmx_vlapic_msr_changed() made static now that it's only >> referenced in vmx.c. > > It is also used in vmcs.c Oh, so it is. It just doesn't show up on the patch diff. That use likely won't survive fixing the Intel APIC-V logic, but I guess we're stuck with it for now. Sorry for the noise. > >> >> It needs a forward declaration in vmx.c because of its position relative >> to the vmx_function_table, but that's fine - we've got plenty of other >> examples like this. >> >> Could I talk you into doing an incremental fix? >> >> Alternatively, we could get better cleanup by forward declaring just >> {vmx,svm}_function_table, then moving the tables to the very bottom of >> {vmx,svm}.c at which point we can drop all the forward declarations. >> >> Oh top of that, I suspect we have other public function definitions >> which can turn static, if you happen to spot any while doing this. > > Sure I could try to cleanup {svm,vmx}.c and the corresponding headers. Thankyou. ~Andrew
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c index eb32f12e2d..dc93b5e930 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_update_vlapic_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_update_vlapic_mode(v); return 0; } diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index 270bc98195..0ec33bcc18 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -2763,6 +2763,7 @@ static struct hvm_function_table __initdata_cf_clobber vmx_function_table = { .nhvm_vcpu_vmexit_event = nvmx_vmexit_event, .nhvm_intr_blocked = nvmx_intr_blocked, .nhvm_domain_relinquish_resources = nvmx_domain_relinquish_resources, + .update_vlapic_mode = vmx_vlapic_msr_changed, .nhvm_hap_walk_L1_p2m = nvmx_hap_walk_L1_p2m, .enable_msr_interception = vmx_enable_msr_interception, .altp2m_vcpu_update_p2m = vmx_vcpu_update_eptp, @@ -3472,7 +3473,7 @@ static void vmx_install_vlapic_mapping(struct vcpu *v) vmx_vmcs_exit(v); } -void vmx_vlapic_msr_changed(struct vcpu *v) +void cf_check vmx_vlapic_msr_changed(struct vcpu *v) { int virtualize_x2apic_mode; struct vlapic *vlapic = vcpu_vlapic(v); diff --git a/xen/arch/x86/include/asm/hvm/hvm.h b/xen/arch/x86/include/asm/hvm/hvm.h index 80e4565bd2..43d3fc2498 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 (*update_vlapic_mode)(struct vcpu *v); /*Walk nested p2m */ int (*nhvm_hap_walk_L1_p2m)(struct vcpu *v, paddr_t L2_gpa, @@ -786,6 +787,12 @@ 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_update_vlapic_mode(struct vcpu *v) +{ + if ( hvm_funcs.update_vlapic_mode ) + alternative_vcall(hvm_funcs.update_vlapic_mode, v); +} + #else /* CONFIG_HVM */ #define hvm_enabled false diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmx.h b/xen/arch/x86/include/asm/hvm/vmx/vmx.h index 234da4a7f4..97d6b810ec 100644 --- a/xen/arch/x86/include/asm/hvm/vmx/vmx.h +++ b/xen/arch/x86/include/asm/hvm/vmx/vmx.h @@ -85,7 +85,7 @@ typedef enum { void vmx_asm_vmexit_handler(struct cpu_user_regs); void vmx_intr_assist(void); void noreturn cf_check vmx_do_resume(void); -void vmx_vlapic_msr_changed(struct vcpu *v); +void cf_check vmx_vlapic_msr_changed(struct vcpu *v); struct hvm_emulate_ctxt; void vmx_realmode_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt); void vmx_realmode(struct cpu_user_regs *regs);
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 update_vlapic_mode, and create a wrapper function, called hvm_vlapic_mode(), to be used by common hvm code. 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> --- Changes in v3: - add cf_check attribute in both the function declaration and definition of vmx_vlapic_msr_changed since it is used as callback, bug reported by Jan - initialize update_vlapic_mode in the vmx_function_table initializer to keep unconditionally initialized cbs in a single place, reported by Jan xen/arch/x86/hvm/vlapic.c | 6 +++--- xen/arch/x86/hvm/vmx/vmx.c | 3 ++- xen/arch/x86/include/asm/hvm/hvm.h | 7 +++++++ xen/arch/x86/include/asm/hvm/vmx/vmx.h | 2 +- 4 files changed, 13 insertions(+), 5 deletions(-)