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. Jan
