On Wed, 2022-11-23 at 18:21 +0000, Sean Christopherson wrote: > On Sat, Nov 19, 2022, David Woodhouse wrote: ... /* When invoked from kvm_sched_out() we cannot sleep */ if (atomic) return; ... > > + /* > > + * Use kvm_gpc_activate() here because if the runstate > > + * area was configured in 32-bit mode and only extends > > + * to the second page now because the guest changed to > > + * 64-bit mode, the second GPC won't have been set up. > > + */ > > + if (kvm_gpc_activate(v->kvm, gpc2, NULL, KVM_HOST_USES_PFN, > > + gpc1->gpa + user_len1, user_len2)) > > I believe kvm_gpc_activate() needs to be converted from write_lock_irq() to > write_lock_irqsave() for this to be safe. Hm, not sure I concur. You're only permitted to call kvm_gpc_activate() in a context where you can sleep. Interrupts had better not be disabled already, and it had better not need write_lock_irqsave(). In this particular context, we do drop all locks before calling kvm_gpc_activate(), and we don't call it at all in the scheduling-out case when we aren't permitted to sleep. > Side topic, why do all of these flows disable IRQs? The gpc->lock is permitted to be taken in IRQ context by the users of the GPC. For example we do this for the shared_info and vcpu_info when passing interrupts directly through to the guest via kvm_arch_set_irq_inatomic() → kvm_xen_set_evtchn_fast(). The code path is fairly convoluted but I do believe we get there directly from the VFIO IRQ handler, via the custom wakeup handler on the irqfd's waitqueue (irqfd_wakeup). Now, perhaps a GPC user which knows that *it* is not going to use its GPC from IRQ context, could refrain from bothering to disable interrupts when taking its own gpc->lock. And that's probably true for the runstate area. The generic GPC code still needs to disable interrupts when taking a gpc->lock though, unless we want to add yet another flag to control that behaviour.
Attachment:
smime.p7s
Description: S/MIME cryptographic signature
