Received-SPF: pass (google.com: domain of linux-kernel+bounces-204799-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <688f492d-f999-450f-9ae9-d5dab078d2a8@intel.com>
Date: Thu, 6 Jun 2024 10:27:52 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH V5 4/4] 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>, <kvm@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
References: <cover.1714081725.git.reinette.chatre@intel.com>
 <eac8c5e0431529282e7887aad0ba66506df28e9e.1714081726.git.reinette.chatre@intel.com>
 <Zl38b3lxLpoBj7pZ@google.com>
 <d2d64211-bd70-4212-811f-c039d2d8dabd@intel.com>
 <ZmB0s7UsfSe90kqr@google.com>
Content-Language: en-US
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <ZmB0s7UsfSe90kqr@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?N25pOWJ3VE1zVjMvNUhYZTdpaFBqYUJGMlY0SmxqZHJYdFFwSUMyOUdmZys3?=
 =?utf-8?B?ZkRjbW5mWWQvaEF2VjFwUDZBcXh2ZDFxMGtTdlpGc1JvaEQvTlJNZkVyU3Ny?=
 =?utf-8?B?NE1PRUlnSWVHemcrZ1E5YU5GRjdsVnB1S21OYWJUNDh4T2Z3L1l2Zk0vekNZ?=
 =?utf-8?B?Nkdka1c2di9YRnI1c0JocllxUURvMHhaWDlES3p5UlJIZnFmL3JsbzlEQnZ4?=
 =?utf-8?B?cytiaytiNXlYR0dEaGRpVE5pa0lJZUxsUGVyTWs3NjNFUUxONTlOT0hRb2w5?=
 =?utf-8?B?SmFycVVhdjM4L3l2WklLaXY1R2lwOEtiS0FBeGFFSUxmM3pqZ0V5aEVOdmg2?=
 =?utf-8?B?NzJVbzZOcUFZYTNWNHdzSWtabm5xTmU3TlRTSjl5cU5pZHdxenZzRHozSnJY?=
 =?utf-8?B?anNGNlZnMmxRclVOb2o5ZTFIRldPV20vVWlaZVFML3VxTG5KTlhXaVlQRWdo?=
 =?utf-8?B?S0hJRThQbk5YdWk0YzdERXY5ZG0ybVhuY1F1QjM5Rzl4anVwK21VRGd2LytZ?=
 =?utf-8?B?MTNiZzNaK0dpUG0waWRnQVVsRVMwMDNOek9hMktlYXlmc3JBalIxcWtrSnA1?=
 =?utf-8?B?RnFxODE0cWEvZEpicGNtUWQrVjF0amlwOSs1eEVWcFJoRVpNRTdET1YvNXVt?=
 =?utf-8?B?dmgxVi9rc0ZVbXdhditkbU8wSVFyeUdYU2hzbkFWeXlraWtIR3d2Ym9vb013?=
 =?utf-8?B?NjVBVmtmQUFwazVvaFVOZlVjVlZPbS9iMnJLMk5CMG52TEZFK2VpY2pkamNW?=
 =?utf-8?B?T1VPeUJWcUhybExmWnd3OXJ0OC9QbkdKVDhtTmV5VHJaT0tkRnQzSGlFWmg1?=
 =?utf-8?B?YVZMY3UxemYrL3Ircy9JV0tPbFRSd1BVWXVxRjZJTGUvTkVOT0YybHJxT1VE?=
 =?utf-8?B?bVRnNGZXcXhVWE1YRDdoeDB4UzJ0UUFGbnNYbEZRWkk1RG5OSkFrVXdyRnBl?=
 =?utf-8?B?Qi9UU2NjaGR5cEh1ME9Yelh1V2c4K1BZNDNvYlk2U3JUcUFDL21STlZBVDBx?=
 =?utf-8?B?RWM1cWFBTHg1UlcrYXRETXluelQwaTFSSE54aTJCNDdxTlFVRVBld2NXNlB5?=
 =?utf-8?B?aVNvQWRqalA1aWQwNHBSeEFlUnI1dEVCSU1OS0sxZXhBN3hmZndTUURDOStX?=
 =?utf-8?B?ektFMHo4S3QvbGh4d3JzTWJsVE1FSEJaYnpaZXFOTHFDaFZva1NyVXhHcmpT?=
 =?utf-8?B?N0lxU0xTMHY5c1B1WmlrRURrOVhpSXRKWkNMSWVUeGcxNm9wWWRtL2J4UTZK?=
 =?utf-8?B?YitDT1pYS3J3TC9wQjh0RHFVLzZEcVJDOEpnRGh6eG1QZWI0dHcvK2RTQ3Na?=
 =?utf-8?B?cXVYMmxTTStYQjJDZWw4M1YvdWN4T2svd0x0ZlF3My8zRlpIWEtqRUhiWm9Z?=
 =?utf-8?B?VU03dlVYZHE0Nm9iTTl0S3YyREZKc25OckhKUmdNMUZhbHNiN2ErYm5sQmdu?=
 =?utf-8?B?OFRETi9qcGlpSnpWbWFSR3cxWFZCVlpjb2czamZGWnFMMVlIQzNodGozNzV2?=
 =?utf-8?B?d0lmSzNac1dmbnBLRVR4WlFjSU9TcVRUSjk2azVrQVpidmlGR1RXUU0yMDRx?=
 =?utf-8?B?UlVpZERZNVFQb01vZUlZL3A4cWliVmt5TEplQ01Tb2hLSGQ1UVEwOGp5UHVF?=
 =?utf-8?B?a0kyYmY2bXlrbG1aU0UvamlZVlJaeVVZdFNOSm1uaE43MHg3VXJMdy83SFBa?=
 =?utf-8?B?MHI2OUp5OU5tSkJDY3VTU1hsUUpRS2owZTNOVzF6ajRRRm4wYjRwYm1tY05h?=
 =?utf-8?B?dDIrMTVNaVlWM29iOVRKTUMyVlJ0Vnpndm5TREVhQUpvdDBqSmhSc3FtRVhE?=
 =?utf-8?B?VU8ySXhCUFlUOXMvemR0S20vVWVRdHVyQjI4QVN4MU5pejN0Ym1iVFR0NjBW?=
 =?utf-8?B?Mzl1dDUxeEpzVEN0N2lpREJMQTlkUHp1UHVFQ0orMURoYzVlSm5udEFMQkFy?=
 =?utf-8?B?NWVhMFo4ZHpvWE5NSXk5MXZ3aTRoK1BtZnUvQ1FPZ00wVlNCMzUwK0Z0Wjlr?=
 =?utf-8?B?djBvall6ZXZ5U3ZoUWo2UHZkS21Yc2JHZjRLTGU1TWRZWklkbkcxV0crT1Rt?=
 =?utf-8?B?bFl3dEhER0NKSHo1dzlrNjVKV1BYVlJOcEppdTBIMm9teVVlTXVLNitvczZS?=
 =?utf-8?B?by80ZGJFS1gxZ09zOUFUWUxxR05TSE1ISG51RXdnY0NpMzVucm82VUF0ejQ0?=
 =?utf-8?B?Y0E9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 779dee6a-0fb9-486a-6f20-08dc864e0017
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Jun 2024 17:27:54.8093
 (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: 4oPjBiLCxBSSKfhfZiBcVbvhbG+6rbj6mjXMB1riTjZ6CZy9+rLY4OQl3xd6ZXtN2B3M8KdUrokwGdi4c8qesEQAzfaAqVG+T4vfSxDfqeU=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB7506
X-OriginatorOrg: intel.com

Hi Sean,

On 6/5/24 7:22 AM, Sean Christopherson wrote:
> On Tue, Jun 04, 2024, Reinette Chatre wrote:
>>>> +/*
>>>> + * Pick one convenient value, 1.5GHz. No special meaning and different from
>>>> + * the default value, 1GHz.
>>>
>>> I have no idea where the 1GHz comes from.  KVM doesn't force a default TSC, KVM
>>> uses the underlying CPU's frequency.  Peeking further ahead, I don't understand
>>> why this test sets KVM_SET_TSC_KHZ.  That brings in a whole different set of
>>> behavior, and that behavior is already verified by tsc_scaling_sync.c.
>>>
>>> I suspect/assume this test forces a frequency so that it can hardcode the math,
>>> but (a) that's odd and (b) x86 selftests really should provide a udelay() so that
>>> goofy stuff like this doesn't end up in random tests.
>>
>> I believe the "default 1GHz" actually refers to the default APIC bus frequency and
>> the goal was indeed to (a) make the TSC frequency different from APIC bus frequency,
>> and (b) make math easier.
>>
>> Yes, there is no need to use KVM_SET_TSC_KHZ. An implementation of udelay() would
>> require calibration and to make this simple for KVM I think we can just use
>> KVM_GET_TSC_KHZ. For now I continue to open code this (see later) since I did not
>> notice similar patterns in existing tests that may need a utility. I'd be happy
>> to add a utility if the needed usage pattern is clear since the closest candidate
>> I could find was xapic_ipi_test.c that does not have a nop loop.
> 
> Please add a utility.  ARM and RISC-V already implement udelay(), and this isn't
> the first test that's wanted udelay() functionality.  At the very least, it'd be
> nice to have for debug, e.g. to be able to insert artificial delay if a test is
> failing due to a suspected race suspected.  I.e. this is likely an "if you build
> it, they will come" situations.

Will do.

> 
>>> Unless I'm misremembering, the timer still counts when the LVT entry is masked
>>> so just mask the IRQ in the LVT. Or rather, keep the entry masked in the LVT.
>>
>> hmmm ... I do not think this is specific to LVT entry but instead an attempt
>> to ignore all maskable external interrupt that may interfere with the test.
> 
> I doubt it.  And if that really is the motiviation, that's a fools errand.  This
> is _guest_ code.  Disabling IRQs in the guest doesn't prevent host IRQs from being
> delivered, it only blocks virtual IRQs.  And KVM selftests guests should never
> receive virtual IRQs unless the test itself explicitly sends them.

Thank you for clarifying.

> 
>> LVT entry is prevented from triggering because if the large configuration value.
> 
> Yes and no.  A large configuration value _should_ avoid a timer IRQ, but the
> entire point of this test is to verify KVM correctly emulates the timer.  If this
> test does its job and finds a KVM bug that causes the timer to prematurely expire,
> then unmasking the LVT entry will generate an unexpected IRQ.
> 
> Of course, the test doesn't configure a legal vector so the IRQ will never be
> delivered.  We could fix that problem, but then a test failure would manifest as
> a hard-to-triage unexpected event, compared to an explicit TEST_ASSERT() on the
> timer value.
> 
> That said, I'm not totally pposed to letting the guest die if KVM unexpectedly
> injects a timer IRQ, e.g. if all is well, it's a cheap way to provide a bit of
> extra coverage.  On the other hand, masking the interrupt is even simpler, and
> the odds of false pass are low.

Understood. Will mask the interrupt in LVT entry as you suggested initially. While
checking which bits to set I realized that the existing test was enabling oneshot
mode in an entry where those bits are actually reserved. This is now fixed also.

I plan to send the changes as next version of this work, with merged patches
dropped from series.

Reinette