Re: [PATCH] KVM: PPC: Book3S HV: add smp_mb() in kvmppc_set_host_ipi()

Quoting Michael Roth (2019-09-05 18:21:22)

Quoting Michael Ellerman (2019-09-04 22:04:48)

quoted

That raises the question of whether this needs to be a full barrier or
just a write barrier, and where is the matching barrier on the reading
side?

For this particular case I think the same barrier orders it on the
read-side via kvmppc_set_host_ipi(42, 0) above, but I'm not sure that
work as a general solution, unless maybe we make that sort of usage
(clear-before-processing) part of the protocol of using
kvmppc_set_host_ipi()... it makes sense given we already need to take
care to not miss clearing them else we get issues like what was fixed
in 755563bc79c7, which introduced the clear in doorbell_exception(). So
then it's a matter of additionally making sure we do it prior to
processing host_ipi state. I haven't looked too closely at the other
users of kvmppc_set_host_ipi() yet though.

<snip>

As far as using rw barriers, I can't think of any reason we couldn't, but
I wouldn't say I'm at all confident in declaring that safe atm...

I think we need a full barrier after all. The following seems possible
otherwise:

      CPU
        X: smp_mb()
        X: ipi_message[RESCHEDULE] = 1
        X: kvmppc_set_host_ipi(42, 1)
        X: smp_mb()
        X: doorbell/msgsnd -> 42
       42: doorbell_exception() (from CPU X)
       42: msgsync
       42: kvmppc_set_host_ipi(42, 0) // STORE DEFERRED DUE TO RE-ORDERING
       42: smp_ipi_demux_relaxed()
      105: smb_mb()
      105: ipi_message[CALL_FUNCTION] = 1
      105: smp_mb()
      105: kvmppc_set_host_ipi(42, 1)
       42: kvmppc_set_host_ipi(42, 0) // RE-ORDERED STORE COMPLETES
       42: // returns to executing guest
      105: doorbell/msgsnd -> 42
       42: local_paca->kvm_hstate.host_ipi == 0 // IPI ignored
      105: // hangs waiting on 42 to process messages/call_single_queue

However that also means the current patch is insufficient, since the
barrier for preventing this scenario needs to come *after* setting
paca_ptrs[cpu]->kvm_hstate.host_ipi to 0.

So I think the right interface is for this is to split
kvmppc_set_host_ipi out into:

static inline void kvmppc_set_host_ipi(int cpu)
{
       smp_mb();
       paca_ptrs[cpu]->kvm_hstate.host_ipi = 1;
}

static inline void kvmppc_clear_host_ipi(int cpu)
{
       paca_ptrs[cpu]->kvm_hstate.host_ipi = 0;
       smp_mb();
}