On 9/16/22 8:52 AM, Jan Beulich wrote:
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.
Is the assertion in vmx_find_msr() really needs to be for runnable vcpu or can it be a check on whether vcpu is actually running (e.g. RUNSTATE_running)? I think call chain vpmu_load()->..->*_vpmu_save()->...->vmx_find_msr() is the only one where we are doing it for non-current vcpu. If we can guarantee that run state is set after vpmu_load() call (maybe it is already, I haven't checked) then testing the state may avoid the assertion. -boris
