Re: [PATCH v3 5/5] KVM: selftests: KVM: SVM: Add Idle HLT intercept test

[Manali Shukla <manali.shukla@amd.com>]

Hi Chao,

Thanks for reviewing the patches.

On 5/31/2024 12:19 PM, Chao Gao wrote:
> On Thu, May 30, 2024 at 06:49:56PM +0530, Manali Shukla wrote:
>> Hi Chao,
>> Thank you for reviewing my patches.
>>
>> On 5/28/2024 1:16 PM, Chao Gao wrote:
>>>> +static void guest_code(void)
>>>> +{
>>>> +	uint32_t icr_val;
>>>> +	int i;
>>>> +
>>>> +	xapic_enable();
>>>> +
>>>> +	icr_val = (APIC_DEST_SELF | APIC_INT_ASSERT | VINTR_VECTOR);
>>>> +
>>>> +	for (i = 0; i < NUM_ITERATIONS; i++) {
>>>> +		cli();
>>>> +		xapic_write_reg(APIC_ICR, icr_val);
>>>> +		safe_halt();
>>>> +		GUEST_ASSERT(READ_ONCE(irq_received));
>>>> +		WRITE_ONCE(irq_received, false);
>>>
>>> any reason to use READ/WRITE_ONCE here?
>>
>> This is done to ensure that irq is already received at this point,
>> as irq_received is set to true in guest_vintr_handler.
> 
> OK. so, READ_ONCE() is to ensure that irq_received is always read directly
> from memory. Otherwise, the compiler might assume it remains false (in the
> 2nd and subsequent iterations) and apply some optimizations.
> 
> However, I don't understand why WRITE_ONCE() is necessary here. Is it to
> prevent the compiler from merging all writes to irq_received across
> iterations into a single write (e.g., simply drop writes in the 2nd
> and subsequent iterations)? I'm not sure.
> 

Compiler optimizing this out is one case. If WRITE_ONCE to irq_received is
not called, the test will not be able to figure out that whether 
irq_received has a stale "true" from the previous iteration (maybe the vintr
interrupt handler did not get invoked) or a fresh "true" from the current
iteration. 


> I suggest adding one comment here because it isn't obvious to everyone.
>
Sure I will add the comment in V4.
 
>>
>>>
>>>> +	}
>>>> +	GUEST_DONE();
>>>> +}
>>>> +
>>>> +static void guest_vintr_handler(struct ex_regs *regs)
>>>> +{
>>>> +	WRITE_ONCE(irq_received, true);
>>>> +	xapic_write_reg(APIC_EOI, 0x00);
>>>> +}
>>>> +
>>>> +int main(int argc, char *argv[])
>>>> +{
>>>> +	struct kvm_vm *vm;
>>>> +	struct kvm_vcpu *vcpu;
>>>> +	struct ucall uc;
>>>> +	uint64_t  halt_exits, vintr_exits;
>>>> +
>>>> +	/* Check the extension for binary stats */
>>>> +	TEST_REQUIRE(this_cpu_has(X86_FEATURE_IDLE_HLT));
>>>
>>> IIUC, this test assumes that the IDLE_HLT feature is enabled for guests if it
>>> is supported by the CPU. But this isn't true in some cases:
>>>
>> I understand you are intending to create a capability for IDLE HLT intercept feature, but in my
>> opinion, the IDLE Halt intercept feature doesn't require user space to do anything for the feature
>> itself.
> 
> Yes, I agree. Actually, I was thinking about:
> 
> 1. make the feature bit visible from /proc/cpuinfo by removing the leading ""
>    from the comment following the bit definition in patch 1
> 
> 2. parse /proc/cpuinfo to determine if this IDLE_HLT feature is supported by the
>    kernel
> 
> But I am not sure if it's worth it. I'll defer to maintainers.

Ack.

-Manali