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:
make the feature bit visible from /proc/cpuinfo by removing the leading "" from the comment following the bit definition in patch 1
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