[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


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Jane Malalane <Jane.Malalane@xxxxxxxxxx>
  • Date: Mon, 31 Jan 2022 16:02:37 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=none; dmarc=none; dkim=none; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=NeJT0svVHvHkRZA58ohgAZChpht1pjNNvlyn88wgcOY=; b=lOMXr/aBqEUcVu6ipEHmpnda+2TUWThCUJi1oNG2RNaH82lmWHJ5QvSvNGMxik3/uZag4tJL2/2hPg97c1VNF80G0h+GywGwni7eKvqVMioxsR8OcjT4t1h4HUCCvmY/TFtS/AA74qLRw8sy5np5gV/ib610Rnp7K7KxkV7xPPJyVnRF9d12StVpqY3Vy2Y824w6N5WAcD9SWopvGV7w011PjxG35P6orGZMR3bLO6wrVnvdE6VJoXZBElHlOIgZehcAomPJpvcVIQOidiqB8gEMX+fNhg2YKq7bWlopj+IgKEoEfY+L+3+LojH2PCwVeXdBZOUp6BN+6ZuiCK9Qaw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=iBCNN5iBrlKa1Rva6pF7Zf2qfM1Z6N7ZuMOnMbBfyORTV+Mn/acH7XcTHWOV9wrcnXCNm2NOMuwcCqmQt2AZXX4uZhx0kd6YLeqk4BzLPSn73KYVT0XB8+5+6/SEuHpCEqqzvcmT8J2kWxVAKBsSAvebAKBYnXIJ3f09WGjIa+tUHMbaFRt8yogrv1ha1WKLtXRwci9R8KXq+21nWRLz5e3SAlB2v+gzNlINGzqg60j6+hGtVpA1dIdGdRbs5gceQx9r3RzU8QFrcYX2YJ4syms8w4MiNC/9aCDrqdYpB1cKdh68B24mH9clOBBUJ3cxWW43BGLsL/WcnGZ55NWHww==
  • Authentication-results: esa2.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Wei Liu <wl@xxxxxxx>, Anthony Perard <anthony.perard@xxxxxxxxxx>, "Juergen Gross" <jgross@xxxxxxxx>, Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>, "George Dunlap" <George.Dunlap@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, "Stefano Stabellini" <sstabellini@xxxxxxxxxx>, Christian Lindig <christian.lindig@xxxxxxxxxx>, David Scott <dave@xxxxxxxxxx>, "Volodymyr Babchuk" <Volodymyr_Babchuk@xxxxxxxx>, Roger Pau Monne <roger.pau@xxxxxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Mon, 31 Jan 2022 16:02:51 +0000
  • Ironport-data: A9a23:oIsDBKxhXvos8tgTpsZ6t+d/wCrEfRIJ4+MujC+fZmUNrF6WrkVUn 2QYXT+EPqyNN2L0ed5zOd7goRgOuZeDzoRqQVQ5qSAxQypGp/SeCIXCJC8cHc8zwu4v7q5Dx 59DAjUVBJlsFhcwnvopW1TYhSEUOZugH9IQM8aZfHAhLeNYYH1500g7wrVg2tQAbeWRWGthh /uj+6UzB3f9s9JEGjp8B3Wr8U4HUFza4Vv0j3RmDRx5lAa2e0o9VfrzEZqZPXrgKrS4K8bhL wr1IBNVyUuCl/slIovNfr8W6STmSJaKVeSFoiI+t6RPHnGuD8H9u0o2HKN0VKtZt9mGt9tI8 ZYds46ccig0YpSLwbwZXBB5HC4raMWq+JefSZS+mcmazkmAeHrw2fR+SkoxOOX0+M4uXzsIr 6ZBbmlQMFbT3Ipaw5riIgVoruYqKsStGYobs3VpyzzxBvc6W5HTBa7N4Le02R9u3ZkSQaiFO aL1bxJdSQbQUiV/N2xMN5Ujn/WBj3r/Vjdx/Qf9Sa0fvDGIkV0ZPKLWGNbcZNGiX8hemUec4 GXc8AzRGQoGPdaSzT6E9HOEheLVmy7/HoUIG9WQ5vNsxVGe2GEXIBkXTkeg5+m0jFakXNBSI FBS/TAhxYAM80isQsj4TgePineOtR4BWPJdC+Q/rgqKz8L8/AKxFmUCCDlbZ7QOtsU7WDgr3 V+hhM7yCHpkt7j9YXCA8raZqxuiNC5TKnUNDQcGQhEC+MLLu5wog1TESdMLLUKupoSrQ3eqm WnM9XVgweVI5SIW60ml1Vv+unH8oIrldSEoujyQBzqh4QpYa4HwMuRE9mPnxfpHKY+YSHyIs 34Fh9WS4YgyMH2dqMCeaL5TRe/0vp5pJBWZ2AcyRMd5q1xB7lb+Jdg43d1oGKt+3i/okxfNa VSbhw5e7YQ70JCCPf4uONLZ5yjHIMHd+TXZuhL8M4AmjntZLlbvEMRSiai4hTqFfK8EyvlXB HtjWZzwZUv28Iw+pNZMe88T0KUw2gc1zn7JSJbwwnyPiOTCPyPFFu9VbgrSP4jVCZ9oRi2Pq b6z0OPRk31ivBDWOHGLoeb/03hXRZTEOXwGg5MOLbPSSuaXMGogF+XQ0dscl39NxMxoehPz1 ijlACdwkQOn7VWecFniQi09NNvHAMguxVpmbX1EFQv5gBALPNfwhJrzgrNqJ9HLAsQ5k64tJ xTEEu3daslypsPvompFNcel8N09JXxGR2umZkKYXdT2RLY5LyTh8d74ZAr/si4ICyu8r8wlp LO8kAjcRPI+q85KV646sdqjkAG8u2YzguV3UxeaK9VfYhy0ooNrNzbwnrk8JMRVcUfPwT6T1 gC3BxYEpLaS/99poYeR3a3U/Z20F+ZeH1ZBGzWJ57iBKiSHrHGoxpVNUbjUcGmFBn/04qire c5c0+r4bK8chF9PvoclS+RrwKsy6sHBvbhfygg4TnzHY07yUuFrI2Wc3NkJvapIn+cLtQyzU 0OJ299bJbTWZ5+1TA9PfFIoN73R2+sVlz/e6eUODH/7vCInrqCaVUhyPgWXjHAPJrVCL454k /wqv9Qb6lLjh0NyYMqGlC1d60+FMmcED/c8rpgfDYLm1lgrx1VFbcCOAyP6+sjSOdBFM01sK T6InqvSwb9bwxOaIXY0EHHM28tbhIgP508WnANTeQzRl4qXnOIz0T1Q7S8zH1ZcwRhw2u5uP nRmah9uLqKU8jY03MVOUghAwe2a6MF1LqAp92Y0qQ==
  • Ironport-hdrordr: A9a23:2nNNhqB4Ksomz5rlHemo55DYdb4zR+YMi2TDsHoBLiC9E/bo8/ xG+c5x6faaslossR0b9uxoW5PhfZq/z/BICOAqVN/JMTUO01HIEKhSqafk3j38C2nf24dmpM JdmnFFeb7N5I5B/KTH3DU=
  • Ironport-sdr: gDpqPWBIeIHXoxE0tHtWPj+N4EHvbS7c6LTUovmR1MoZCyMAUWRPAvPHeikrh6yc3DzljRA1DO SZcx+jwxr9ZrPGrJb/pR9mEbh5XXEBvhX49pLi7MmJR7eByBs4Lb3TK9Oq2JMI3eQg0RTEiYQm FNt0wSFoLGSDG1hBLHKHJR6FzP0inSpWxOQd/MI3fAciBhKYvqKjlM2QTwyqbYpLhdPqfnDvzI drb/iUuX6W8vVJHAbQKBBcPzKCgPCeZVMWXyzDvOX1Xy80m25uJZIdyK/cz9GJ5OksPDuo2N2p XTO1vUNq44VSYjpNbEvnfkCw
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-index: AQHYE5eVWWLwfSz8JEi35sISHqV9YKx9DnUAgABCRgA=
  • Thread-topic: [PATCH 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC

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.

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.