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

[Jane Malalane <Jane.Malalane@xxxxxxxxxx>]

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.