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
