Received-SPF: pass (google.com: domain of linux-kernel+bounces-212025-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <bb9d3836-8765-4c4b-9966-6842c8cf25e2@intel.com>
Date: Wed, 12 Jun 2024 10:49:17 -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>
 <88c65b89-5174-4076-82cd-7852c8c25b66@intel.com>
 <Zmj2nVhtVoGflaiG@google.com>
From: Reinette Chatre <reinette.chatre@intel.com>
Content-Language: en-US
In-Reply-To: <Zmj2nVhtVoGflaiG@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?eHVscCtuZk5Ud205K1dhZGJQKzZQQ0RvOVVJYW9wS3NVV29IWUJoWWxtUk8r?=
 =?utf-8?B?OTY1UEtNeFZHK2NHeXRiNGRkMXlzUHlhVldrVEZ6YlV5c2JEdFVrSDh5U0NK?=
 =?utf-8?B?eXlsbi8rQnNmVlpwYy83T2JLemRDNzZBV1VJYTR3V3dxS1phMjY4TSthRFdz?=
 =?utf-8?B?V3dEbUNvNFdpdFYvQ2FCR1JtUnFZNGVMSVJVWXdjUTkrSnNCWDFwR1NQMTBU?=
 =?utf-8?B?NzByNFFkVjhmWlViVjM5dkVnZ2V6cFo1SjNNb1lDWkMxVGhTQlVyUis5Y3dE?=
 =?utf-8?B?QVBwYkx0WTdEM0VVNXIzM21CaFJTSldqMm95bWcyaHd0eTdzbHJSVnhoVXk1?=
 =?utf-8?B?UHl1SjRqZ2Uwb2MwdFhtVWNodDBzRXVDa0Y5VkJtVkFwNUhHZkZ1WHZObFJD?=
 =?utf-8?B?L2tDaGRoYTdEaVhsYUZtaWhHV1VXRUJaR0ZBVVNYMXg0R1BhakhnSlhZejds?=
 =?utf-8?B?Rk5mNnRraHBHbmFUaG4wOWRwUVc2YmlYYlY1R3ZwRmg2TWpLa2lFV284TUFI?=
 =?utf-8?B?NzBBOEdpSE9NeHQwaDM0eDlYZTJTQ3VNQ25ndm5EWk5KTnkvcUFEZGdsK1Fy?=
 =?utf-8?B?blczVWJVbEdxTUNzcDYxVzZteUwyNnNXeE4yNGhEQlY3SjloMnkreFdTWmVW?=
 =?utf-8?B?TUE1YzR1bDExdFN3WnVvZCtvYmlaNzJTcktZTEw2YktGYk1ybUVOa3lQeXRH?=
 =?utf-8?B?NkorVlNMc1Z4QnhSU2VDbFhGUW5pZUZub1F6alNTME55cmZSdFlNQzBXZ0x0?=
 =?utf-8?B?SWdhcndmM1VTUm1Xa0FKQXNmeG9VZ2RMYys0ZWI1cXNhdnNPNy9aVGhubTRP?=
 =?utf-8?B?QWZwM0N0cjhodFhlYXluL09XY3doMGpRMUFhTTFKa3pDMWlTb1FyZVhOTk5T?=
 =?utf-8?B?bkhPMmZBc1dlZUJTZG5jWGJ2Um5SaEJQNjREYzFvUFJ4WjgxT0grbHF3aEtu?=
 =?utf-8?B?YW1JbUpPdDVnVlExQTh6Q2R3QiswQ1NVMFB6MGlOMHdYblZqTU1rdDdQVmF6?=
 =?utf-8?B?dVpwMms0RGpveW9FSm1HellWdld6ZFVRRTdGaDR0TnFQRDZVM0Z6RGMxc2RF?=
 =?utf-8?B?b0lNQ1gvRGRNd1dQWFltNGVlZFFxbXFjNVpmNmZoSlA1UjlNdllBYm0rOXJC?=
 =?utf-8?B?Uk44Y2wvSWVFYVd3Q0dVbE9zNkhZL1MwT0RTWm1ZZHYyQmpLVXNlM21tbjBV?=
 =?utf-8?B?RjVlSXJnRnppRGJHN3UyT0FmL3RveHhWZkE5bTBtdktuay9KSVhHUmdWbGRU?=
 =?utf-8?B?K1RXWlA5UUpEa0VJdHJ1S0RsUWZvNkFYa0trK20xWldWM1czSWpHZDFCSzhU?=
 =?utf-8?B?M3NWNkZxSFZFZW80VXJjUEczbEpkM3pWR1E5UXV3TWJjd0Uzd3ZsQmNDYk9o?=
 =?utf-8?B?NTZudHpSajlwU25NR01OY1ArSHJibTd4TXZmMElhZi8yUlJ1MDIvbWJoOXpE?=
 =?utf-8?B?Mm9Xb3FjYk94ODBkUkZDZmo2NUg1UHdTTk5haTF4bkd3OE02blF0MjV4eVAz?=
 =?utf-8?B?TC8yZW5DWlE4QWZKQjZWQzZtblZJMkVFZHBWMTVoOExSNW9lclNvVjhqSzMr?=
 =?utf-8?B?d0JKMWlRMGxGaUwyczRMc0dpVkFRNGpGZDJmVkUzTy9rSHVJdWtkWkQwM0JK?=
 =?utf-8?B?SzVmZjBhYXZhYUs5Y0c1OTZFZ0xvMTlwR21zNllEWVJsbDVsNEdrQVBlSUo0?=
 =?utf-8?B?eE9IaWZrSjlZVnd2MzJkR0JTYlE4TU5OdGpiZy9GaTJObEtRYmg2OFRBYzdX?=
 =?utf-8?B?VmRpa3F5ZkNJMzdUalNMcXIwN1JuVEVMY3g3b0ppUDZQeC9DM2hReHBhS3gr?=
 =?utf-8?B?WlFVb08rTE1iMWhETTJhdVh5QmVCSjJTa21BNExmNWQrcmJKUVFTNCs4aHBV?=
 =?utf-8?B?ZEI4aEwzY1Q1dFNjOGtIdlhRUXQ1dUxLNnhvSHFiLzZEejZGaEh5YTlFQWJD?=
 =?utf-8?B?NERDRzZNdmNDZGdJZmtWYm40UUZSMUZ2Sm9DNUVHdGRCNmptdjVsWXlKc2JI?=
 =?utf-8?B?RjdyNml6OHBROVZFclJaQ0dRSDJJZXZYOWlNMWkzcDVDZ2JFWngwcWhPenlv?=
 =?utf-8?B?c0x2c2dIaVM0Nit3MmwwRGFyNTAwVWl4NTQ3QWc2ODc1dzlXWkRrTXhnYUN6?=
 =?utf-8?B?bTJTRzZiaDFzdUxFV0VSZ3IzYURnMjBPMHBtREc1SXJCdXZ6dzdvc3RtcjZ5?=
 =?utf-8?B?SHc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 52d070b8-275b-4bba-643a-08dc8b07fc92
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Jun 2024 17:49:20.0159
 (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: Hkgo6B0H0NWnESDF+CbLjqcoBlMMhxTZF3wuOXoO7GnLVTJsAAOyCmqs2llkBJimn8x4Kuo2mDG/O7epdlcXp7SP3euIzltNmDSPHmSVVhc=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB8123
X-OriginatorOrg: intel.com

Hi Sean,

On 6/11/24 6:15 PM, Sean Christopherson wrote:
> On Tue, Jun 11, 2024, Reinette Chatre wrote:
>>> 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
> 
> /facepalm
> 
> No, it won't work, e.g. relies on get_ucall() being invoked.  I'm also being
> unnecessarily clever, and missing the obvious, simple solution.
> 
> The only reason tests manually handle UCALL_ABORT is because back when it was
> added, there was no sprintf support in the guest, i.e. the guest could only spit
> out raw information, it couldn't format a human-readable error message.  And so
> tests manually handled UCALL_ABORT with a custom message.
> 
> When we added sprintf support, (almost) all tests moved formatting to the guest
> and converged on using REPORT_GUEST_ASSERT(), but we never completed the cleanup
> by moving REPORT_GUEST_ASSERT() to common code.
> 
> Even more ridiculous is that assert_on_unhandled_exception() is still a thing.
> That code exists _literally_ to handle this scenario, where common guest library
> code needs to signal a failure.
> 
> In short, the right way to resolve this is to have _vcpu_run() (or maybe even
> __vcpu_run()) handle UCALL_ABORT.  The the bajillion REPORT_GUEST_ASSERT() calls
> can be removed, as can UCALL_UNHANDLED and assert_on_unhandled_exception() since
> they can and should use a normal GUEST_ASSERT() now that guest code can provide
> the formating, and library code will ensure the assert is reported.
> 
> For this series, just ignore the GUEST_ASSERT() wonkiness.  If someone develops
> a test that uses udelay(), doesn't handle ucalls, _and_ runs on funky hardware,
> then so be it, they can come yell at me :-)
> 
> And I'll work on a series to handle UCALL_ABORT in _vcpu_run() (and poke around
> a bit more to see if there's other low hanging cleanup fruit).

Thank you very much for explaining these details. Next version intends to address
all your feedback and I will send that shortly.

Reinette