Hi, On 31/01/2022 12:05, Jan Beulich wrote: > 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. Makes sense. > >> --- 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. Indeed, that would be better. Here I think the choice has been to only announce harwdare assisted x2apic support in CPUID if both read and write accesses are virtualized. I could either keep the x2apic_available check in Patch 1 as is and use the per-domain field here, and keep vmx_vlapic_msr_changed as is or vv. > > Jan > > Thank you,
Jane.