Received-SPF: pass (google.com: domain of linux-kernel+bounces-210615-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <a44d4534-3ba1-4bee-b06d-bb2a77fe3856@intel.com>
Date: Tue, 11 Jun 2024 14:33:50 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH V8 1/2] KVM: selftests: Add x86_64 guest udelay() utility
To: Sean Christopherson <seanjc@google.com>
CC: <isaku.yamahata@intel.com>, <pbonzini@redhat.com>,
	<erdemaktas@google.com>, <vkuznets@redhat.com>, <vannapurve@google.com>,
	<jmattson@google.com>, <mlevitsk@redhat.com>, <xiaoyao.li@intel.com>,
	<chao.gao@intel.com>, <rick.p.edgecombe@intel.com>, <yuan.yao@intel.com>,
	<kvm@vger.kernel.org>, <linux-kernel@vger.kernel.org>
References: <cover.1718043121.git.reinette.chatre@intel.com>
 <ad03cb58323158c1ea14f485f834c5dfb7bf9063.1718043121.git.reinette.chatre@intel.com>
 <ZmeYp8Sornz36ZkO@google.com>
From: Reinette Chatre <reinette.chatre@intel.com>
Content-Language: en-US
In-Reply-To: <ZmeYp8Sornz36ZkO@google.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?a09GdVphMk4zZG1UZ28zcG9FWjVuVmhuWmw2R0NwL3JteGgvQU1Ldk4wNXRa?=
 =?utf-8?B?eG9IODJYWkxGaHJWK2tiRytJbGhVZ2N4L25INExEQUpidW1SMUVSTGRHY1h4?=
 =?utf-8?B?bmxTQ3ZFNVJDNWVycUN4aTlDUVJZNVdtaFB0MDU5T294bTNLblBwV3RmRm4x?=
 =?utf-8?B?NUc2TXdkbTBpc2FWWUNMejdTdStTSGNUQVNMMzBIcEc4NmpXSHB6UmdwN01W?=
 =?utf-8?B?cGtuZXQvb1pTZ2lVczhJNHhrelYzR3BrV3MxeTZnYkZxWC9oZGs1Z09kZHVk?=
 =?utf-8?B?cXdiU08ybmJVOHhET3EzdVo2S1YzTmdHL0xvUkh5ZUNKWURpMnQrYkJkbjhQ?=
 =?utf-8?B?Zm96ck1JK3hsWXdDKzhDM21DdzJmMm8xOWJBOWhKTEl1U25RaDdvZWZTc0RC?=
 =?utf-8?B?UE1qaitibjA2dGdZUFErbmhrQnZCb1E4RElucnJxM0JiWldTS1FZM3dOSHBH?=
 =?utf-8?B?cnBPbWVDSVY2Ynd6MG84NVdTTmxzbkNCdkVsTVNKbVpOSGxqQWZPbkRxTGxT?=
 =?utf-8?B?K1FXcTZVT1BsbUw0SW5WYktYUXA0aVU2QkNDNk1Xb2kyOUFIbk5CS2Z6MGRO?=
 =?utf-8?B?V1lJTEh3eUVwQ1J5S2F2bXlOb3VJK0o2ZHZpTFFIWk9GQVNKNjlyNHNRK1RN?=
 =?utf-8?B?NS93Mm5ERzdrWDVwdU5wWkUrK0lFeVgxOWtWWVQ3WTQ0eCthWExJTDZjbDRB?=
 =?utf-8?B?bDI1SkxxZVp6eS9KUHVVNmNPaGpBcnphR2ZFYUNYOUhJRlZGc0lGZjFib0Vv?=
 =?utf-8?B?NTZ1OVFhZVF2ZG1GRmdFZG0yNmNtY1JjTmNjZk9zMjBEdDlBS3EvSUk0TUJL?=
 =?utf-8?B?ZUl2SVZxdXl1NW9LV2ljbWFZWlMzN09oaHlOd0l5SUYzVkdYMkNTczlPNDMz?=
 =?utf-8?B?aXJObXZ0b2QySzZKWnJxRmkvUGF4OFFFUnBPT3RHQmQvVXJYWVU4L0NzcmtU?=
 =?utf-8?B?a01SbFZ1U2pxbk93YTM4a1VnNFFqS2hGQnRONFRrUzRHak9KM2QwbGU3RXNs?=
 =?utf-8?B?RCttbzlFUmNrNWFIRVFLSUxHZVpSOFc0OXBidURseTJ2YXNLNVB5b2ttRnZR?=
 =?utf-8?B?UlZENkhlWEFXd2dCSEJJWkFIbE4yaGo0bDBmbFl2WkdlWkNRM1dJbnZOYnFF?=
 =?utf-8?B?SmY0RDJ3M09SK04zRnlLd1g4am1nTXNkbnhKaStEYTVZclkxQWp0ZzVmVnFO?=
 =?utf-8?B?SHlOSmpmbFJSekxTV1FTakgrWC94LzJCd2lNbnJZYUQ5OUkvSTkvMTVjeUts?=
 =?utf-8?B?ejdsSlYxS214ODBOK3BzWms5THJnVWNFYzNyMm9Oa2pBTXB4cDJxcVBtTzVG?=
 =?utf-8?B?eFNVaFNzUW50SkxuNFI0d1V6TThkNEpSOENCMjVadGlRT2s5VW9venRBNmtI?=
 =?utf-8?B?RFVoRDUrM3hkSGtrUXQ1ZVhDS2JkRTcrS0FRZlVaNmczT3pjUytjZzFoMjU4?=
 =?utf-8?B?VkR2dVNzVjdKUExacEpJZTMzWFRXTGtZU3FHUHB0V2wxUkpHTk1xOGtTQmFp?=
 =?utf-8?B?TXZzMCtvbmROL1FKOUJxTVFqanFhSlhMbmhsOHZQM2R4UEZJMng3NVJ4aFBl?=
 =?utf-8?B?ZjlXMThzYmpva1pnMTJQMkhrVGFYNGw5eC9IV1VGSXowMFYwY3gwTVo4Yndm?=
 =?utf-8?B?Y3RYVWFBTFN6bFEvcWdVRnRaYVEvdnA5aHNKTG9zemJTSmJFUjdudmZzQm94?=
 =?utf-8?B?dXlGblY0YmRJakE1THdaS2ZhclcvV1FDS2dmTzVXUVZvaW1PdmxpOG03S0hk?=
 =?utf-8?B?M2ZWS1N1WUo1dGl6bVA0Z2U3elBrQVZKTjBsa01VWEU3WnlGeGloUWlLVkRq?=
 =?utf-8?B?YnZEV2ZrWTNEc0Iva3RtRlBCd3JUNVJQL3NZdVN3TGhicDc2YzhyeFc2bzln?=
 =?utf-8?B?OUU1TG92bDdaeU1QcFI2YVZkc0xwbWR2VFpOVFQ1WnVIZVZsWko1SzdzOGF6?=
 =?utf-8?B?bW8vZk9JazIwbmxXb2k5dEdTWTA3QitBQ01qREdrTFJBTjNtem1tZllUN1Yy?=
 =?utf-8?B?RE5Kb2ljUTlqVmlXVGRWWEhHL1ZCS3dXM3FrOUZoN1lhaXpsVkxvRDBlb3VG?=
 =?utf-8?B?ODV1aHpWMk5wKzFkbFduUm9lSWtQVVZJRTJHSWZTWmpoY3hrTVc4S0tDQ0k0?=
 =?utf-8?B?WThXQlJVM0NqMU9TS25JNzR6clY4K3FKWFh4MUF3YnI3MU5wYWViWTVWNUhj?=
 =?utf-8?B?UUE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 5f38d228-bf81-47cb-3c60-08dc8a5e3105
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Jun 2024 21:33:53.5776
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Y3JWHaI1A+/eBdoHuV7R2rJcu6j1dgNivqmgBKfUr2sTblv7cR5HwcbMz2kf1raWEOZ1oSlksS55GcQa4lkNXiUFi9aOvJvgaGukyahV4Lg=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR11MB8025
X-OriginatorOrg: intel.com

Hi Sean,

Thank you very much for your detailed feedback.

On 6/10/24 5:21 PM, Sean Christopherson wrote:
> On Mon, Jun 10, 2024, Reinette Chatre wrote:
>> diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
>> index 8eb57de0b587..b473f210ba6c 100644
>> --- a/tools/testing/selftests/kvm/include/x86_64/processor.h
>> +++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
>> @@ -23,6 +23,7 @@
>>   
>>   extern bool host_cpu_is_intel;
>>   extern bool host_cpu_is_amd;
>> +extern unsigned int tsc_khz;
>>   
>>   /* Forced emulation prefix, used to invoke the emulator unconditionally. */
>>   #define KVM_FEP "ud2; .byte 'k', 'v', 'm';"
>> @@ -815,6 +816,20 @@ static inline void cpu_relax(void)
>>   	asm volatile("rep; nop" ::: "memory");
>>   }
>>   
>> +static inline void udelay(unsigned long usec)
> 
> uint64_t instead of unsigned long?  Practically speaking it doesn't change anything,
> but I don't see any reason to mix "unsigned long" and "uint64_t", e.g. the max
> delay isn't a property of the address space.

I assume that you refer to "cycles" below. Will do.

> 
>> +{
>> +	unsigned long cycles = tsc_khz / 1000 * usec;
>> +	uint64_t start, now;
>> +
>> +	start = rdtsc();
>> +	for (;;) {
>> +		now = rdtsc();
>> +		if (now - start >= cycles)
>> +			break;
>> +		cpu_relax();
>> +	}
>> +}
>> +
>>   #define ud2()			\
>>   	__asm__ __volatile__(	\
>>   		"ud2\n"	\
>> diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
>> index c664e446136b..ff579674032f 100644
>> --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
>> +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
>> @@ -25,6 +25,7 @@ vm_vaddr_t exception_handlers;
>>   bool host_cpu_is_amd;
>>   bool host_cpu_is_intel;
>>   bool is_forced_emulation_enabled;
>> +unsigned int tsc_khz;
> 
> Slight preference for uint32_t, mostly because KVM stores its version as a u32.

Changed it to uint32_t.

> 
>>   static void regs_dump(FILE *stream, struct kvm_regs *regs, uint8_t indent)
>>   {
>> @@ -616,6 +617,8 @@ void assert_on_unhandled_exception(struct kvm_vcpu *vcpu)
>>   
>>   void kvm_arch_vm_post_create(struct kvm_vm *vm)
>>   {
>> +	int r;
>> +
>>   	vm_create_irqchip(vm);
>>   	vm_init_descriptor_tables(vm);
>>   
>> @@ -628,6 +631,15 @@ void kvm_arch_vm_post_create(struct kvm_vm *vm)
>>   
>>   		vm_sev_ioctl(vm, KVM_SEV_INIT2, &init);
>>   	}
>> +
>> +	if (kvm_has_cap(KVM_CAP_GET_TSC_KHZ)) {
> 
> I think we should make this a TEST_REQUIRE(), or maybe even a TEST_ASSERT().
> Support for KVM_GET_TSC_KHZ predates KVM selftests by 7+ years.

Changed it to a TEST_ASSERT() right at the beginning of kvm_arch_vm_post_create().

> 
>> +		r = __vm_ioctl(vm, KVM_GET_TSC_KHZ, NULL);
>> +		if (r < 0)
> 
> Heh, the docs are stale.  KVM hasn't returned an error since commit cc578287e322
> ("KVM: Infrastructure for software and hardware based TSC rate scaling"), which
> again predates selftests by many years (6+ in this case).  To make our lives
> much simpler, I think we should assert that KVM_GET_TSC_KHZ succeeds, and maybe
> throw in a GUEST_ASSERT(thz_khz) in udelay()?

I added the GUEST_ASSERT() but I find that it comes with a caveat (more below).

I plan an assert as below that would end up testing the same as what a
GUEST_ASSERT(tsc_khz) would accomplish:

	r = __vm_ioctl(vm, KVM_GET_TSC_KHZ, NULL);
	TEST_ASSERT(r > 0, "KVM_GET_TSC_KHZ did not provide a valid TSC freq.");
	tsc_khz = r;


Caveat is: the additional GUEST_ASSERT() requires all tests that use udelay() in
the guest to now subtly be required to implement a ucall (UCALL_ABORT) handler.
I did a crude grep check to see and of the 69 x86_64 tests there are 47 that do
indeed have a UCALL_ABORT handler. If any of the other use udelay() then the
GUEST_ASSERT() will of course still trigger, but will be quite cryptic. For
example, "Unhandled exception '0xe' at guest RIP '0x0'" vs. "tsc_khz".
  
> E.g. as is, if KVM_GET_TSC_KHZ is allowed to fail, then we risk having to deal
> with weird failures due to udelay() unexpectedly doing nothing.
> 
> 
>> +			tsc_khz = 0;
>> +		else
>> +			tsc_khz = r;
>> +		sync_global_to_guest(vm, tsc_khz);
>> +	}
>>   }
>>   
>>   void vcpu_arch_set_entry_point(struct kvm_vcpu *vcpu, void *guest_code)
>> -- 
>> 2.34.1
>>

Reinette