Re: [PATCH 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC

[Jane Malalane <Jane.Malalane@xxxxxxxxxx>]

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.

Hi Jan,

After speaking to Andrew he told me of a performance regression that was 
reported some time ago when enabling apicv related to the pass-through 
LAPIC timer of a HVM guest causing Xen to intercept the LAPIC timer MSR, 
making anything that uses the LAPIC timer end up slower than it was 
before. So, adressing your comment here, other than mentioning how being 
able to disable acceleration for apicv can be useful when testing and 
debugging, do you think it's worth mentioning (in more detail) that this 
perf problem exists, in the commit message.

Thanks,

Jane.

> 
>> --- 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
> 
>