Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <b28f0cb9-b321-cc55-db31-61296e1a494e@amd.com>
Date:   Tue, 6 Dec 2022 18:14:53 +1100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.5.0
Subject: Re: [PATCH kernel 1/3] x86/amd/dr_addr_mask: Cache values in percpu
 variables
Content-Language: en-US
To:     Sean Christopherson <seanjc@google.com>
Cc:     kvm@vger.kernel.org, x86@kernel.org, linux-kernel@vger.kernel.org,
        Venu Busireddy <venu.busireddy@oracle.com>,
        Tony Luck <tony.luck@intel.com>,
        Tom Lendacky <thomas.lendacky@amd.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Peter Zijlstra <peterz@infradead.org>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Michael Sterritt <sterritt@google.com>,
        Michael Roth <michael.roth@amd.com>,
        Mario Limonciello <mario.limonciello@amd.com>,
        Ingo Molnar <mingo@redhat.com>,
        Heiko Carstens <hca@linux.ibm.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Borislav Petkov <bp@alien8.de>,
        Andrew Cooper <andrew.cooper3@citrix.com>,
        "Jason A. Donenfeld" <Jason@zx2c4.com>,
        "H. Peter Anvin" <hpa@zytor.com>
References: <20221201021948.9259-1-aik@amd.com>
 <20221201021948.9259-2-aik@amd.com> <Y4jdIl1elcnL8JUU@google.com>
From:   Alexey Kardashevskiy <aik@amd.com>
In-Reply-To: <Y4jdIl1elcnL8JUU@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?SUtGelJkallOR2hjRUhiSEdDNzJJRkxWeTNJdHZvZER6SlNxZjdodzE5MVFa?=
 =?utf-8?B?dWFJZzc2K3VYdmhLZzRPYk1oZ1I5anJtWmQzS3k1ek5jRTA1RTU0cnJWcUdy?=
 =?utf-8?B?eURYMzZ5WTF6aFdvcGcyTS9velZzVDMzejhENVoyWnNrbDZaRS9OVGZXTzgv?=
 =?utf-8?B?eG1sUjBYSC93MldjTjhEV1BaaFFFOVk4UW5RVDVSbFVrdk5PWFJabGE1U0Ra?=
 =?utf-8?B?Y1B5NlBkdjM0L0VkaVB1RmNxS3JhdTR1UHFObU9FMGRZN3RUUEYwa2F3WDR5?=
 =?utf-8?B?YVpmOW1LVDgxVndBRXVvN1UySmtMY3JBcHI3ME9LZ1huNUVJM0ZsVlZVbnY4?=
 =?utf-8?B?ZEhjc1IyQW1rNGxoTjNFVm05RGxDYklITmFJY1U4M2pLWFFJSVpWVnh3UzB6?=
 =?utf-8?B?eDVnMVFkTFo3T3ZrWC84bFJWcnVqbDh0c3JRNUZSMk50Q3JhWmU2bGc2RVpO?=
 =?utf-8?B?dFFVNUFXYlhGM3RmaUN0MWtxU1Z6N0VIQ2RZZ2FtaktKWFdVdjM0SWlsVTdK?=
 =?utf-8?B?M20zUGJGa0xrZjJQU2l5YmtYcmZMTU9wY3MwcUQxNTM3LzFLK2lVa3JveitL?=
 =?utf-8?B?a3NxaGpIMnRJcXNBT0JoUGlYaS9pNjZqbDduTzNPVmptcHBja2RWM1BsS3hG?=
 =?utf-8?B?NUdHcjA1eWlGemUvNkxUYVpNWGNSSThZQnRtQk5ja2syQ0wzeWw3amFiMGRy?=
 =?utf-8?B?VVlKU0tTMHBSblN4Kzg2czFBbDJXc2FsWWczZi9sZXVzai96aWVHMWJrRnJS?=
 =?utf-8?B?a2twWk45RFB5MmlPMmZ0QktTc3p1dlJORVhqTElnWG5CMmVpbnphMVRCUmFD?=
 =?utf-8?B?UEtOa0lZZ0Q2ckZEelVvczdodDg0ZlMycnBRN3lZMmNoOXBuMFVHTEhYVlhO?=
 =?utf-8?B?OHpXRjQ4V2NjVkFJZmZGS0ZCTUhET2doZ3I3T2cwdGR5MmNpS1dNeGhvWnEw?=
 =?utf-8?B?dTZvUmloOVY4WG52ZnlqYmlMbytiYXYzWlZ4MTZqZHdxdVpFYkVjU1dnQ3Iy?=
 =?utf-8?B?eG42eTZFamMxU2xxNFI3MytndUpKTHVMRzBpUUU0STdiRmlOa01QdmxhNjVN?=
 =?utf-8?B?cndCNlNlcGlocForK2tpUmVQcGtza0Q2TFR3WGsxRVV2dlhVSmtwZFlBaHJB?=
 =?utf-8?B?bFNOdTlWczhLR29Wd2RLOUpQNE1DSUVCOFA0S0ZycU9jbkd1djhJSm5rWng1?=
 =?utf-8?B?ckdJdjZqWlR4ZE84MUQ1RTBqbmtuMGRiT1dOdmFjOGJUYnZUaVc4cnR0cVFN?=
 =?utf-8?B?M0F3RGxSZTQ2MS83cTd4aDhsU3hmSnp2SmtLU0NZdmt2SlRlN0JjRjhqVmtC?=
 =?utf-8?B?ZUhsNWFMOUN4MlVWSUtQSEhrNzZ2OUFaRmdmdkUzVmRBRjFGVWZUenQxSTg1?=
 =?utf-8?B?ZWozN2xFWXFIQnBIUWRJdG9qZWt0K2Y0S1dYaHdQMU1zeXFOS2RWZTM1MDZr?=
 =?utf-8?B?RE5XN2M4MG5zU3haaWVPcDJhV1paTFFrYjdCQWJsbnoyb1Z5VzUyelV3RHdR?=
 =?utf-8?B?MlpsblNreG1PaUxyQW8zQlN4c3VLbjVGY0E3ek8zcG5jTXN2V2xKNzdoOGVz?=
 =?utf-8?B?ODRuZVdyejFMMXNQeUp4R0Rja3BDMmxJMlE4cUphWExvTGNVSEFiUERQVWt4?=
 =?utf-8?B?dEZXenFvNlZpcW5hdWhqc2lGUGtLWVJ4L1hMYWxkNWlSL3Q0Tnk2eHkwT01P?=
 =?utf-8?B?RU95U29Ccks5TWhtNWlkMjY5Uml0c1JtTzk3eDVIRTJWd2NiQ1RVZjNWdk9N?=
 =?utf-8?B?ZDA1YmF5YmRDSEZPVnNVcTZtY3VGang4MWQyaVlEV2c1bkdPcVpDa1dlamcv?=
 =?utf-8?B?aXBFMFFVNzlpaTdjVHdlVXNQNnpIY3VzcEtmbVMzMnFTdmVMVVV0Y3BETUdi?=
 =?utf-8?B?amZkYnJzRUNTZGpKQ0JjRFFLbk5SNVRQQkwzQlF5d3JRUmFKR0w5Rlh5aW1F?=
 =?utf-8?B?cWpkWDRvcDdFZ2R6clYzMzFYcmhRaTZsL1RsOUNXcDNUTFdWUEpjTWNCSXJM?=
 =?utf-8?B?YTVZMW1kKzRVOWN5d3RlMlFBY3RFbjZZMTA0ZUxUWXBjUHdDOG1VRnJ6RGpH?=
 =?utf-8?B?VkZJTVVvL05MUnpjK1A3eXdLa1ByRWV5ZUNpbysrSEU5NkpUWlk2ZjByRkl0?=
 =?utf-8?Q?Z09YWQv3kvKqf5BDHMrRRGT/b?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 784b7f9e-bfe9-4b27-5742-08dad75998a7
X-MS-Exchange-CrossTenant-AuthSource: DM6PR12MB2843.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Dec 2022 07:15:04.3261
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: AffIVR2zg4AHiTNmMDYWG69LxMp3bP5G2ncBws04ciV6iDC6seYV8SF+UpVPoMbSJI3JLc4XZvNbIKzh9cpDhw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR12MB6638
X-Spam-Status: No, score=-2.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 2/12/22 03:58, Sean Christopherson wrote:
> On Thu, Dec 01, 2022, Alexey Kardashevskiy wrote:
>> Reading DR[0-3]_ADDR_MASK MSRs takes about 250 cycles which is going to
>> be noticeable when the AMD KVM SEV-ES's DebugSwap feature is enabled and
>> KVM needs to store these before switching to a guest; the DebugSwitch
>> hardware support restores them as type B swap.
>>
>> This stores MSR values from set_dr_addr_mask() in percpu values and
>> returns them via new get_dr_addr_mask(). The gain here is about 10x.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@amd.com>
>> ---
>>   arch/x86/include/asm/debugreg.h |  1 +
>>   arch/x86/kernel/cpu/amd.c       | 32 ++++++++++++++++++++
>>   2 files changed, 33 insertions(+)
>>
>> diff --git a/arch/x86/include/asm/debugreg.h b/arch/x86/include/asm/debugreg.h
>> index cfdf307ddc01..c4324d0205b5 100644
>> --- a/arch/x86/include/asm/debugreg.h
>> +++ b/arch/x86/include/asm/debugreg.h
>> @@ -127,6 +127,7 @@ static __always_inline void local_db_restore(unsigned long dr7)
>>   
>>   #ifdef CONFIG_CPU_SUP_AMD
>>   extern void set_dr_addr_mask(unsigned long mask, int dr);
>> +extern unsigned long get_dr_addr_mask(int dr);
>>   #else
>>   static inline void set_dr_addr_mask(unsigned long mask, int dr) { }
> 
> KVM_AMD doesn't depend on CPU_SUP_AMD, i.e. this needs a stub.  Or we need to add
> a dependency.

The new symbol is under CONFIG_CPU_SUP_AMD and so is its only user 
arch/x86/kernel/cpu/amd.c:

arch/x86/kernel/cpu/Makefile:
obj-$(CONFIG_CPU_SUP_AMD)               += amd.o


Is this enough dependency or we need something else? (if enough, I'll 
put it into the next commit log).


>> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
>> index c75d75b9f11a..ec7efcef4e14 100644
>> --- a/arch/x86/kernel/cpu/amd.c
>> +++ b/arch/x86/kernel/cpu/amd.c
>> @@ -1158,6 +1158,11 @@ static bool cpu_has_amd_erratum(struct cpuinfo_x86 *cpu, const int *erratum)
>>   	return false;
>>   }
>>   
>> +DEFINE_PER_CPU_READ_MOSTLY(unsigned long, dr0_addr_mask);
>> +DEFINE_PER_CPU_READ_MOSTLY(unsigned long, dr1_addr_mask);
>> +DEFINE_PER_CPU_READ_MOSTLY(unsigned long, dr2_addr_mask);
>> +DEFINE_PER_CPU_READ_MOSTLY(unsigned long, dr3_addr_mask);
>> +
>>   void set_dr_addr_mask(unsigned long mask, int dr)
>>   {
>>   	if (!boot_cpu_has(X86_FEATURE_BPEXT))
>> @@ -1166,17 +1171,44 @@ void set_dr_addr_mask(unsigned long mask, int dr)
>>   	switch (dr) {
>>   	case 0:
>>   		wrmsr(MSR_F16H_DR0_ADDR_MASK, mask, 0);
> 
> LOL, I'd love to hear how MSR_F16H_DR0_ADDR_MASK ended up with a completely
> different MSR index.
>> +		per_cpu(dr0_addr_mask, smp_processor_id()) = mask;
> 
> Use an array to avoid the copy+paste?  And if you're going to add a cache, might
> as well use it to avoid unnecessary writes.

I'll do this, I did not realize DEFINE_PER_CPU_READ_MOSTLY can do 
arrays. Thanks,

> 
>>   		break;
>>   	case 1:

-- 
Alexey