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
