Received-SPF: pass (google.com: domain of linux-kernel+bounces-210692-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <88c65b89-5174-4076-82cd-7852c8c25b66@intel.com>
Date: Tue, 11 Jun 2024 16:07:12 -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>
 <a44d4534-3ba1-4bee-b06d-bb2a77fe3856@intel.com>
 <ZmjJnzBkOe58fFL6@google.com>
From: Reinette Chatre <reinette.chatre@intel.com>
Content-Language: en-US
In-Reply-To: <ZmjJnzBkOe58fFL6@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?M1gxUnc4SjNWREpRM2s1Q3Q0RWhxYmY3VU1WcHlHenNiQ2hSMGl3RHdsRmI4?=
 =?utf-8?B?MFJRWU1XSVFjTVFkZEJkV2EvYlFOZUZQNHA2ZFIrSnJBb1pwajhkSlVyN3JL?=
 =?utf-8?B?bHNBSjl4am16NmVaeCtqeGVlTVVVVEJzZ1BSMkprVmNrbU1HTnVXWmtEQlk5?=
 =?utf-8?B?dWxsbUtmbjAvcW1RODRZcFRPZkw2MG5KWk9BazlQeFdMdzN3QTBoTVQ2SlJP?=
 =?utf-8?B?RXJqTG9DMFRrYXlFenlWcUJXaFB5bFp2Ny9jTUJoNjN4dHhNbUZ5NFJEMitE?=
 =?utf-8?B?SS9TY2pqZVNUcnFkd3Nub3FBOW5sMlRRaVQrc3VlOWcyUzRjNW44UDlJc3Zw?=
 =?utf-8?B?emhIZWZhU0t5RW5OWG9uSXpOaHZwMHJoanN3Yi9yZWJNUEx1ZlUyTmZOMnBF?=
 =?utf-8?B?U0hNRm5jWkJtQm5sSmNxbDJqT0tXMjYrdndTb1N1eXBuS3Fyd3RtT0JXQWtZ?=
 =?utf-8?B?dDJvYnVYWUhMUElkZ0J6bDFXK1lHUm93NDF0WWFSZ2l1RGZKZXp0YlVROFls?=
 =?utf-8?B?NVFsWk9RVDJnRGZreGt5MkhkVEcyZ01hUXF6SmFwZEUyQ0hzQi96SExLSzNu?=
 =?utf-8?B?cUc1Zi92bzV6Uld3amk3NkdUbVV4ZjJURlNYek5uUy8yaEVBTkVRTWsxVUcw?=
 =?utf-8?B?ZXdoVG9HSWY3M05KU2wrc3R3a0VxOXp3eUpPaVlWUWxXcFIyY3VUMERBNm9M?=
 =?utf-8?B?eGtETVVDODRqeFdvLzAwQUwvNldSSnowaDVFTUwyZXBidWlacFhwbkZ4R1d2?=
 =?utf-8?B?TFVzbk5ZUHJPZFprNHc5R2lLVkRYSklSSE1MTWRxNkNnbWY3cU9MaEFEdU51?=
 =?utf-8?B?M2RKUGFMM3haQ2t6ZW5tZWtKM2JVY1dQL0NCSjg5SENUNVdEWXZKWTA0OFRq?=
 =?utf-8?B?UXJVUGFmeEV1RFEzZy84cmt1WDc1UThhNm44MUJiT0c5a1dxbGQ5elZoaTZr?=
 =?utf-8?B?c05ZTWpyNkMyb2dWNVJiOXNOU1ZuM0RXV1VUNFZLR1c1NnRsQ2tsOXdXMGdl?=
 =?utf-8?B?eFl3YUFyalNhUWlxSTZKSkhCaHVzejlxYWFpNFhXVnJqbUhIZWNpUmx4bjIv?=
 =?utf-8?B?NnhON0NuQ2ZWYTVpcVI2TDlVK2FkNVhYRDVJcklJTU5kekRTbFdnNUgyVTNI?=
 =?utf-8?B?U2k5QWFKczg0TUQ3NmxFV3NvS2ZLRmF5bm1mRUJLa0VWNThDZDdXTkIvNUdz?=
 =?utf-8?B?Uko2cStxRmlacG9icWJCZDk3MjFab1lxMHhRMEI3TGlhSk5WeU41eEg1eW9I?=
 =?utf-8?B?NTZ1anpNWFl4KzBRUk1WeEl4bUhFY3BhYXE2RHp6ckthbDc4QWFkcTZSOUlU?=
 =?utf-8?B?QkUzQ3pzZm8vT0dWZGt2WWdGNHR5cnNJMGR1LzlCcUtoRWlMYVJTNi9wSUEr?=
 =?utf-8?B?Rml0Wi9QU1BObk53WjZzbU5KWllPdUZtczJISTJCTkVNQm1GSXN6NmRNUGN3?=
 =?utf-8?B?aHpXV1FMVTdrYmpFYW1RVlhZZDFNSlNZeS9mNWNlMnZnUkNaT0tqVm9FSlhj?=
 =?utf-8?B?TVpYMHduMnpxMERpb1VRNS9SdnAxd29aZE13V2JETmlRajNKZFh2alczUDZT?=
 =?utf-8?B?a2doZXF6Z042RUo0djBrT05NM2xTeVRaclVIM29NSmRiQVA3c2QybGxRZC9J?=
 =?utf-8?B?ZW4xbVBYdzZ6SWVTOFFad29xODBTZHB4eXdBTGNRcmJZT01UVkM4VHdXNjdt?=
 =?utf-8?B?QXJ0TVQvbzJPNTdPMm5xNDg0eGIvbXAwRzUyczU2ODdrRk1GT3ArMHh0aFh5?=
 =?utf-8?B?OEJkdkJzUkZrL0FhM2c0WFE5L2lpZ0lYbnF0T0NNYXgzOE1CV3lwQmlJS1Nl?=
 =?utf-8?B?SkZJUjZDd0Z4NTBuSm95T2N2bDZwWC9zL0xVdEt3Z1c1bzd2N1Rwa3B1UDNw?=
 =?utf-8?B?VytpVm9OYWlScCs2aFFNdVcybll0M21IVVdoSC8zRG44bVZyNWlaNVlGZ1dm?=
 =?utf-8?B?ZDk5NGJPemFXQ2pCU3paNlhKT0VwT3RRWk9peXVtNENQd21icENuYTc0QWpj?=
 =?utf-8?B?VzY4c2NrV1pFQmM5WXZiK0Y3UkZnU2Rzck5qd2V3UDdrNHR3Zlh0ajBtTEpw?=
 =?utf-8?B?b3BsSW9EbHp6QkdoelBGMFVpUE5MUFpVQ3dyUXV3YzBMSnc1YjYvd0UwZWJB?=
 =?utf-8?B?bW1zdUJBU0FQVHV5b2J1NFE0SWREL0VtNGZpSG9YYytmOVQ3ellycktQTkxj?=
 =?utf-8?B?N2c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 02233e75-a19d-4fd4-68b2-08dc8a6b3bc6
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Jun 2024 23:07:15.0157
 (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: EGTWfweXis5oJ7uGFHUaeMw5HRIThagCt7ks6NZJgJYsMteYgaXJkNmGPxHaE4q3Ydp8YhzL6qTsoL8Qack2lR2/Lbft2WdC0c8TCZQIwgc=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB6470
X-OriginatorOrg: intel.com

Hi Sean,

On 6/11/24 3:03 PM, Sean Christopherson wrote:
> On Tue, Jun 11, 2024, Reinette Chatre wrote:
>>> 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".
> 
> Yeah, we really need to add a bit more infrastructure, there is way, way, waaaay
> too much boilerplate needed just to run a guest and handle the basic ucalls.
> Reporting guest asserts should Just Work for 99.9% of tests.
> 
> Anyways, is it any less cryptic if ucall_assert() forces a failure?  I forget if
> the problem with an unhandled GUEST_ASSERT() is that the test re-enters the guest,
> or if it's something else.
> 
> I don't think we need a perfect solution _now_, as tsc_khz really should never
> be 0, just something to not make life completely miserable for future developers.
> 
> diff --git a/tools/testing/selftests/kvm/lib/ucall_common.c b/tools/testing/selftests/kvm/lib/ucall_common.c
> index 42151e571953..1116bce5cdbf 100644
> --- a/tools/testing/selftests/kvm/lib/ucall_common.c
> +++ b/tools/testing/selftests/kvm/lib/ucall_common.c
> @@ -98,6 +98,8 @@ void ucall_assert(uint64_t cmd, const char *exp, const char *file,
>   
>          ucall_arch_do_ucall((vm_vaddr_t)uc->hva);
>   
> +       ucall_arch_do_ucall(GUEST_UCALL_FAILED);
> +
>          ucall_free(uc);
>   }
> 

Thank you very much.

With your suggestion an example unhandled GUEST_ASSERT() looks as below.
It does not guide on what (beyond vcpu_run()) triggered the assert but it
indeed provides a hint that adding ucall handling may be needed.

[SNIP]
==== Test Assertion Failure ====
   lib/ucall_common.c:154: addr != (void *)GUEST_UCALL_FAILED
   pid=16002 tid=16002 errno=4 - Interrupted system call
      1  0x000000000040da91: get_ucall at ucall_common.c:154
      2  0x0000000000410142: assert_on_unhandled_exception at processor.c:614
      3  0x0000000000406590: _vcpu_run at kvm_util.c:1718
      4   (inlined by) vcpu_run at kvm_util.c:1729
      5  0x00000000004026cf: test_apic_bus_clock at apic_bus_clock_test.c:115
      6   (inlined by) run_apic_bus_clock_test at apic_bus_clock_test.c:164
      7   (inlined by) main at apic_bus_clock_test.c:201
      8  0x00007fb1d8429d8f: ?? ??:0
      9  0x00007fb1d8429e3f: ?? ??:0
     10  0x00000000004027a4: _start at ??:?
   Guest failed to allocate ucall struct
[SNIP]

Is this acceptable? I can add a new preparatory patch with your
suggestion that has as its goal to provide slightly better error message
when there is an unhandled ucall.

Reinette