On 15.09.2022 16:01, Tamas K Lengyel wrote:
While experimenting with the vPMU subsystem an ASSERT failure was
observed in vmx_find_msr because the vcpu_runnable state was true.
The root cause of the bug appears to be the fact that the vPMU subsystem
doesn't save its state on context_switch. The vpmu_load function will attempt
to gather the PMU state if its still loaded two different ways:
1. if the current pcpu is not where the vcpu ran before doing a remote save
2. if the current pcpu had another vcpu active before doing a local save
However, in case the prev vcpu is being rescheduled on another pcpu its state
has already changed and vcpu_runnable is returning true, thus #2 will trip the
ASSERT. The only way to avoid this race condition is to make sure the
prev vcpu is paused while being checked and its context saved. Once the prev
vcpu is resumed and does #1 it will find its state already saved.
While I consider this explanation plausible, I'm worried:
--- a/xen/arch/x86/cpu/vpmu.c
+++ b/xen/arch/x86/cpu/vpmu.c
@@ -419,8 +419,10 @@ int vpmu_load(struct vcpu *v, bool_t from_guest)
vpmu = vcpu_vpmu(prev);
/* Someone ran here before us */
+ vcpu_pause(prev);
vpmu_save_force(prev);
vpmu_reset(vpmu, VPMU_CONTEXT_LOADED);
+ vcpu_unpause(prev);
vpmu = vcpu_vpmu(v);
}
We're running with IRQs off here, yet vcpu_pause() waits for the vcpu
to actually be de-scheduled. Even with IRQs on this is already a
relatively heavy operation (also including its impact on the remote
side). Additionally the function is called from context_switch(), and
I'm unsure of the usability of vcpu_pause() on such a path. In
particular: Is there a risk of two CPUs doing this mutually to one
another? If so, is deadlocking excluded?
Hence at the very least I think the description wants extending, to
discuss the safety of the change.
Boris - any chance you could comment here? Iirc that's code you did
introduce.
