On Thu, Feb 06, 2020 at 11:47:02AM +0100, Vitaly Kuznetsov wrote: > linmiaohe <linmiaohe@xxxxxxxxxx> writes: > > > From: Miaohe Lin <linmiaohe@xxxxxxxxxx> > > > > There is already an smp_mb() barrier in kvm_make_request(). We reuse it > > here. > > > > Signed-off-by: Miaohe Lin <linmiaohe@xxxxxxxxxx> > > --- > > arch/x86/kvm/lapic.c | 9 ++++++--- > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > > index eafc631d305c..ea871206a370 100644 > > --- a/arch/x86/kvm/lapic.c > > +++ b/arch/x86/kvm/lapic.c > > @@ -1080,9 +1080,12 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode, > > result = 1; > > /* assumes that there are only KVM_APIC_INIT/SIPI */ > > apic->pending_events = (1UL << KVM_APIC_INIT); > > - /* make sure pending_events is visible before sending > > - * the request */ > > - smp_wmb(); > > + /* > > + * Make sure pending_events is visible before sending > > + * the request. > > + * There is already an smp_wmb() in kvm_make_request(), > > + * we reuse that barrier here. > > + */ > > Let me suggest an alternative wording, > > "kvm_make_request() provides smp_wmb() so pending_events changes are > guaranteed to be visible" > > But there is nothing wrong with yours, it's just longer than it could be > :-) I usually lean in favor of more comments, but in thise case I'd vote to drop the comment altogether. There are lots of places that rely on the smp_wmb() in kvm_make_request() without a comment, e.g. the cases for APIC_DM_STARTUP and APIC_DM_REMRD in this same switch, kvm_inject_nmi(), etc... One might wonder what makes INIT special. And on the flip side, APIC_DM_STARTUP is a good example of when a smp_wmb()/smp_rmb() is needed and commented correctly (though calling out the exactly location of the other half would be helpful). > Reviewed-by: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx> > > > kvm_make_request(KVM_REQ_EVENT, vcpu); > > kvm_vcpu_kick(vcpu); > > } > > -- > Vitaly >
