[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC
On 27.01.2022 17:01, Jane Malalane wrote: > Introduce a new per-domain creation x86 specific flag to > select whether hardware assisted virtualization should be used for > x{2}APIC. > > A per-domain option is added to xl in order to select the usage of > x{2}APIC hardware assisted vitualization, as well as a global > configuration option. > > Having all APIC interaction exit to Xen for emulation is slow and can > induce much overhead. Hardware can speed up x{2}APIC by running APIC > read/write accesses without taking a VM exit. This is odd to read for a patch which makes it possible to _turn off_ acceleration. Instead it would be interesting to know what problems you have encountered making it desirable to have a way to turn this off. > --- a/xen/arch/x86/hvm/vmx/vmx.c > +++ b/xen/arch/x86/hvm/vmx/vmx.c > @@ -3342,16 +3342,19 @@ static void vmx_install_vlapic_mapping(struct vcpu *v) > > void vmx_vlapic_msr_changed(struct vcpu *v) > { > - int virtualize_x2apic_mode; > + int virtualize_xapic_mode, virtualize_x2apic_mode; Please switch to bool as you touch and extend this. > struct vlapic *vlapic = vcpu_vlapic(v); > unsigned int msr; > > + virtualize_xapic_mode = ( cpu_has_vmx_virtualize_apic_accesses && > + v->domain->arch.hvm.assisted_xapic ); Please don't clone the bad use of blanks immediately inside parentheses here; instead, ... > virtualize_x2apic_mode = ( (cpu_has_vmx_apic_reg_virt || > cpu_has_vmx_virtual_intr_delivery) && > - cpu_has_vmx_virtualize_x2apic_mode ); > + cpu_has_vmx_virtualize_x2apic_mode && > + v->domain->arch.hvm.assisted_x2apic ); ... since you're touching this anyway, please consider correcting the style violation here. However - do you need these expressions anymore? The per-domain fields can only be set if the respective CPU capabilities exit. > --- a/xen/arch/x86/include/asm/hvm/domain.h > +++ b/xen/arch/x86/include/asm/hvm/domain.h > @@ -117,6 +117,12 @@ struct hvm_domain { > > bool is_s3_suspended; > > + /* xAPIC hardware assisted emulation. */ > + bool assisted_xapic; > + > + /* x2APIC hardware assisted emulation. */ > + bool assisted_x2apic; > + > /* hypervisor intercepted msix table */ > struct list_head msixtbl_list; Please follow how adjacent code pads types / names here. > --- a/xen/arch/x86/traps.c > +++ b/xen/arch/x86/traps.c > @@ -1115,7 +1115,8 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, > uint32_t leaf, > if ( !is_hvm_domain(d) || subleaf != 0 ) > break; > > - if ( cpu_has_vmx_apic_reg_virt ) > + if ( cpu_has_vmx_apic_reg_virt && > + v->domain->arch.hvm.assisted_xapic ) > res->a |= XEN_HVM_CPUID_APIC_ACCESS_VIRT; > > /* > @@ -1126,7 +1127,8 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, > uint32_t leaf, > */ > if ( cpu_has_vmx_virtualize_x2apic_mode && > cpu_has_vmx_apic_reg_virt && > - cpu_has_vmx_virtual_intr_delivery ) > + cpu_has_vmx_virtual_intr_delivery && > + v->domain->arch.hvm.assisted_x2apic ) > res->a |= XEN_HVM_CPUID_X2APIC_VIRT; Same remark as above - can't you now use _just_ the per-domain field? In this latter of the two cases this would then also mean bringing the CPU feature checks in line with what vmx_vlapic_msr_changed() does (as also pointed out for patch 1). Albeit it might be best to have a prereq patch fixing the issue, which could then be backported. Jan
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |