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

[Chao Gao <chao.gao@intel.com>]

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.

I suggest adding one comment here because it isn't obvious to everyone.

>
>> 
>>> +	}
>>> +	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.