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.
