Received-SPF: pass (google.com: domain of linux-kernel+bounces-210616-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: <fba4628f-9786-4e76-84cb-178508d90fd8@intel.com>
Date: Tue, 11 Jun 2024 14:34:58 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH V8 2/2] KVM: selftests: Add test for configure of x86 APIC
 bus frequency
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>
 <09b11d24e957056c621e3bf2d6c9d78bd4f7461b.1718043121.git.reinette.chatre@intel.com>
 <ZmefjsFArRSnC71I@google.com>
From: Reinette Chatre <reinette.chatre@intel.com>
Content-Language: en-US
In-Reply-To: <ZmefjsFArRSnC71I@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?S1dMcEdCLzEvQjZ3U2YvTXhMMExzMUVtQjhORW5leDQ3QWFIcnNGV2xxck9G?=
 =?utf-8?B?UDJqVXhqWDBtSVFuTUZpMVp4ZjBucHFzY085MHNhU2pOdGV4d0dQcVZRUHFv?=
 =?utf-8?B?SE81U3NFaE0xeml3WHZHQlh6eXBldDB5R2F2S1FsZ1RRblhNQlZlc05RRWJo?=
 =?utf-8?B?c1Mwb2liRkFCSWJpN2VKZ2VVeDJqTVJMb3Q3bEo4M2lzVGFvYVJhenVxeUhh?=
 =?utf-8?B?YmV6VEVDdUlnNG4wWEFORFd2NitOTUIwOXFhN1B0WHhqUHFjaTBzdktYT3Jl?=
 =?utf-8?B?dVdBdW5EcmY5c1ZPVHQzVUhKNFgvVlJrRmVMaWVBL1g0QVBCTGpQR1g5TUU1?=
 =?utf-8?B?SUV3SG9aaDl6d3lZK01zalZmb2oreG9oTFljdkZjUnQ3Mk1YTzVEd1puaStQ?=
 =?utf-8?B?aERhQlZpQnZzZHZWVnJIS2VJb0NwWllRSklCOC9IWk9JSkF5M0orTDFYenVL?=
 =?utf-8?B?ai9qN2g0MlJiYTNvRzNYcERjOWRvODRpaUxJaEpESzJFZjAyUTlvTnh6aE9P?=
 =?utf-8?B?dm5WWHJpSUJHRUNBVjVUZU5MalBQY1BwMUdSdzE3NEZXTHBPQkZYUGl3TkhU?=
 =?utf-8?B?VXhTOVA3RFY0YUZibXZPeWxXQWlxRC96QjZBN0VEeHEvVTNtR29HTmNMREdH?=
 =?utf-8?B?ZGV0eEV3ajBySW9sNk1vK3Q5blRYK1dXUjhvUngvLytxeHEwQVprMVpBYlA2?=
 =?utf-8?B?K1d2Y3NwWml2NG5zSjFMbUtsZHdlMGVZK1JwVlhjY2cxRkwrWXJrcGxVSHVo?=
 =?utf-8?B?VFhnbnh1ZHBwT01oWHV0VnRIUTVXb0F4U2lwdEtac0JJUjV4UUV5K29BajMy?=
 =?utf-8?B?ZnVNc0JNN1AxMU1pWnBjckN5RUZVVWxsMDkrei9CdUV1eWFUZGRjQnhjVGxh?=
 =?utf-8?B?VjJPVEhFTHpKZzdjc2l3cUMzQk9vRlNIRzJ5aEJnQ0tPUW9vQkZpdktHNVpo?=
 =?utf-8?B?K3VreERzY3pQbWVqc2xSK05KendrRzFpb3grODQ1c3REUEl4NGZKVHVmVFpP?=
 =?utf-8?B?OTcyMGNzbEpKbnV2N0t6L1ZtdG9kSG54cmxJMUJiMjB5MFNSMWJId1F0V0Nh?=
 =?utf-8?B?Z1JGWGxPSjEzTjBiNjgvWThpelBLQUprVkJuc203Z2RKOXBKUkZiVVloZTVS?=
 =?utf-8?B?ME94T3c2Y0E0M1FTbmpIQmxuYjVLeXpVbXh6WndPVmZPZUkwcENwMytLdHJB?=
 =?utf-8?B?VEdaSWkzOCtVTndYMUNWN2ZxemsrbExXMm9oeGw4c2JYd1pmOXlrQ0w1Rm16?=
 =?utf-8?B?OWtnbFdWMFQvdG90ZW1yWnI3a1R1dzhpVWhESkswM0R1b1dHakl2YnN6b0Q1?=
 =?utf-8?B?UDlHYko5cDU0UjljMFVsTGZlVTArK2tHb3NjQ0VPVjQveUtYQlc3ZjJLT2dM?=
 =?utf-8?B?aXZQcEwrQ29yeTluMnhicTIzdkE3TGVlS04rdloraWkzN2tPb1UyeEp2am5M?=
 =?utf-8?B?dndNUmNPeGVMb290eDcybUQxTXBkWCtHMG9TNjZ3UjN6N3Z2UmoxQ2FweUhz?=
 =?utf-8?B?bFhLeDFHMGFRTlNzQmhIbU85K2VyUmNkK1RLNk9zWHNwdHJwVUNxaWJDT1Vi?=
 =?utf-8?B?dXJBZFJQQm5CTHRBbHpoWTBFTWNZUS93S01qdnV5K2tQMTVkOWlKTFJlRm1J?=
 =?utf-8?B?R2R2YkRUd1A2czNua0c0Nm82empzYlNQcVpxREFQSkwxZ2xpVlR4NGNlMTB1?=
 =?utf-8?B?SlluS2FkbWt4OWU4WkxKK0MrMlpIcFhKeVJkRXNxZDAwS0NmaEhsWVU0Z0NR?=
 =?utf-8?B?VW9rUFQ1K1p0VjE5QXkvditKamxNcFRKdkZXakpEN1lIeGxtZlU3SUwzMllp?=
 =?utf-8?B?MFAybkJwVUp5c2phQ1JiUFNxcTdrNjRYbU5yZ2V5NjVxRGh4YUFDNWREMHcx?=
 =?utf-8?B?eHFWZ3JKbnBaZngxeGdUbVdDU09wUWl3ajlITUxDdS9YRWlQMVVkeUtKSllE?=
 =?utf-8?B?bXRLTm5CSG4yNEkzN3BSTytXMytKYVg1SzdQcEZmbm5mUlU1em05NmdTSGJv?=
 =?utf-8?B?YWNNQ3RJY3E2dnpUZEkwb29KZFhxMmxmM3Nla294SHNqV2x4bjQzMCsrc0Fy?=
 =?utf-8?B?aXpla0ZrUTloeTl5ejJoTnB1TnY2azUycVhKeDI4YlJHRlpKUDlzWVNmNFJw?=
 =?utf-8?B?ODYyNjFSRjF0RzcrWTQ4bHJXYnBMYWJ0NVk3dFIyV2xtd0gwZCtZSGVuTXNr?=
 =?utf-8?B?OFE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 4099f691-036f-47e5-04c5-08dc8a5e5903
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:35:00.5862
 (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: u6bC30/fiqa1dPj3nsypoEQDeX++TNRg/rMoLWYoCAygi7arwwJAD3dGPNL4ugc6ATtPTsfgL07O6aulK4Vm439CRHgFztrvUhYsbz5/wVI=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR11MB8025
X-OriginatorOrg: intel.com

Hi Sean,

On 6/10/24 5:51 PM, Sean Christopherson wrote:
> On Mon, Jun 10, 2024, Reinette Chatre wrote:
>> diff --git a/tools/testing/selftests/kvm/x86_64/apic_bus_clock_test.c b/tools/testing/selftests/kvm/x86_64/apic_bus_clock_test.c
>> new file mode 100644
>> index 000000000000..602cec91d8ee
>> --- /dev/null
>> +++ b/tools/testing/selftests/kvm/x86_64/apic_bus_clock_test.c
>> @@ -0,0 +1,219 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2024 Intel Corporation
>> + *
>> + * Verify KVM correctly emulates the APIC bus frequency when the VMM configures
>> + * the frequency via KVM_CAP_X86_APIC_BUS_CYCLES_NS.  Start the APIC timer by
>> + * programming TMICT (timer initial count) to the largest value possible (so
>> + * that the timer will not expire during the test).  Then, after an arbitrary
>> + * amount of time has elapsed, verify TMCCT (timer current count) is within 1%
>> + * of the expected value based on the time elapsed, the APIC bus frequency, and
>> + * the programmed TDCR (timer divide configuration register).
>> + */
>> +
>> +#include "apic.h"
>> +#include "test_util.h"
>> +
>> +/*
>> + * Pick 25MHz for APIC bus frequency. Different enough from the default 1GHz.
>> + * User can override via command line.
>> + */
>> +unsigned long apic_hz = 25 * 1000 * 1000;
> 
> static, and maybe a uint64_t to match the other stuff?

Sure. Also moved all other globals and functions back to static.

> 
>> +/*
>> + * Possible TDCR values with matching divide count. Used to modify APIC
>> + * timer frequency.
>> + */
>> +struct {
>> +	uint32_t tdcr;
>> +	uint32_t divide_count;
>> +} tdcrs[] = {
>> +	{0x0, 2},
>> +	{0x1, 4},
>> +	{0x2, 8},
>> +	{0x3, 16},
>> +	{0x8, 32},
>> +	{0x9, 64},
>> +	{0xa, 128},
>> +	{0xb, 1},
>> +};
>> +
>> +void guest_verify(uint64_t tsc_cycles, uint32_t apic_cycles, uint32_t divide_count)
> 
> uin64_t for apic_cycles?  And maybe something like guest_check_apic_count(), to
> make it more obvious what is being verified?  Actually, it should be quite easy
> to have the two flavors share the bulk of the code.

I now plan to drop this function and instead just open code the
checks in what has now become a shared function between xAPIC and x2APIC.

> 
>> +{
>> +	unsigned long tsc_hz = tsc_khz * 1000;
>> +	uint64_t freq;
>> +
>> +	GUEST_ASSERT(tsc_cycles > 0);
> 
> Is this necessary?  Won't the "freq < ..." check fail?  I love me some paranoia,
> but this seems unnecessary.

Sure. After needing to field reports from static checkers not able to determine
that a denominator can never be zero I do tend to add these checks just to
pre-emptively placate them. I did run the code through a static checker after making
all planned changes and it had no complaints so it is now gone.

> 
>> +	freq = apic_cycles * divide_count * tsc_hz / tsc_cycles;
>> +	/* Check if measured frequency is within 1% of configured frequency. */
>> +	GUEST_ASSERT(freq < apic_hz * 101 / 100);
>> +	GUEST_ASSERT(freq > apic_hz * 99 / 100);
>> +}
>> +
>> +void x2apic_guest_code(void)
>> +{
>> +	uint32_t tmict, tmcct;
>> +	uint64_t tsc0, tsc1;
>> +	int i;
>> +
>> +	x2apic_enable();
>> +
>> +	/*
>> +	 * Setup one-shot timer.  The vector does not matter because the
>> +	 * interrupt should not fire.
>> +	 */
>> +	x2apic_write_reg(APIC_LVTT, APIC_LVT_TIMER_ONESHOT | APIC_LVT_MASKED);
>> +
>> +	for (i = 0; i < ARRAY_SIZE(tdcrs); i++) {
>> +		x2apic_write_reg(APIC_TDCR, tdcrs[i].tdcr);
>> +
>> +		/* Set the largest value to not trigger the interrupt. */
> 
> Nit, the goal isn't to avoid triggering the interrupt, e.g. the above masking
> takes care of that.  The goal is to prevent the timer from expiring, because if
> the timer expires it stops counting and will trigger a false failure because the
> TSC doesn't stop counting.
> 
> Honestly, I would just delete the comment.  Same with the "busy wait for 100 msec"
> and "Read APIC timer and TSC" comments.  They state the obvious.  Loading the max
> TMICT is mildly interesting, but that's covered by the file-level comment.
> 
>> +		tmict = ~0;
> 
> This really belongs outside of the loop, e.g.
> 
> 	const uint32_t tmict = ~0u;
> 
>> +		x2apic_write_reg(APIC_TMICT, tmict);
>> +
>> +		/* Busy wait for 100 msec. */
> 
> Hmm, should this be configurable?

Will do.

> 
>> +		tsc0 = rdtsc();
>> +		udelay(100000);
>> +		/* Read APIC timer and TSC. */
>> +		tmcct = x2apic_read_reg(APIC_TMCCT);
>> +		tsc1 = rdtsc();
>> +
>> +		/* Stop timer. */
> 
> This comment is a bit more interesting, as readers might not know writing '0'
> stops the timer.  But that's even more interesting is the ordering, e.g. it's
> not at all unreasonable to think that the timer should be stopped _before_ reading
> the current count.  E.g. something like:
> 
> 		/*
> 		 * Stop the timer _after_ reading the current, final count, as
> 		 * writing the initial counter also modifies the current count.
> 		 */
> 
>> +		x2apic_write_reg(APIC_TMICT, 0);
>> +
>> +		guest_verify(tsc1 - tsc0, tmict - tmcct, tdcrs[i].divide_count);
>> +	}
>> +
>> +	GUEST_DONE();
>> +}
>> +
>> +void xapic_guest_code(void)
>> +{
>> +	uint32_t tmict, tmcct;
>> +	uint64_t tsc0, tsc1;
>> +	int i;
>> +
>> +	xapic_enable();
>> +
>> +	/*
>> +	 * Setup one-shot timer.  The vector does not matter because the
>> +	 * interrupt should not fire.
>> +	 */
>> +	xapic_write_reg(APIC_LVTT, APIC_LVT_TIMER_ONESHOT | APIC_LVT_MASKED);
>> +
>> +	for (i = 0; i < ARRAY_SIZE(tdcrs); i++) {
>> +		xapic_write_reg(APIC_TDCR, tdcrs[i].tdcr);
>> +
>> +		/* Set the largest value to not trigger the interrupt. */
>> +		tmict = ~0;
>> +		xapic_write_reg(APIC_TMICT, tmict);
>> +
>> +		/* Busy wait for 100 msec. */
>> +		tsc0 = rdtsc();
>> +		udelay(100000);
>> +		/* Read APIC timer and TSC. */
>> +		tmcct = xapic_read_reg(APIC_TMCCT);
>> +		tsc1 = rdtsc();
>> +
>> +		/* Stop timer. */
>> +		xapic_write_reg(APIC_TMICT, 0);
>> +
>> +		guest_verify(tsc1 - tsc0, tmict - tmcct, tdcrs[i].divide_count);
> 
> That's some nice copy+paste :-)
> 
> This test isn't writing ICR, so the whole 32-bit vs. 64-bit weirdness with xAPIC
> vs X2APIC is irrevelant.  Two tiny helpers, a global flag, and you can avoid a
> pile of copy+paste, and the need to find a better name than guest_verify().

Will do. Thank you very much for your detailed and valuable feedback.

Reinette