From: Hans-Jörg Höxer <hshoexer@genua.de>
Subject: Re: psp(4): Avoid sleep/wakeup race by raising SPL
To: <tech@openbsd.org>
Date: Tue, 5 Nov 2024 14:49:51 +0100

Hi,

dlg@ suggested to make the interrupt handler MP safe and unlock
pspioctl().  See diff below for my take on this.

Take care,
HJ.

-----------------------------------------------------------------------
commit d94e8b533d2f73422c234bb5975851855c63ec59
Author: Hans-Joerg Hoexer <hshoexer@genua.de>
Date:   Mon Nov 4 17:09:24 2024 +0100

    psp(4): Avoid sleep/wakeup race; make interrupt MP safe
    
    And therefore pspioctl() can release the kernel lock.

diff --git a/sys/dev/ic/psp.c b/sys/dev/ic/psp.c
index e697b307236..22329aceb5b 100644
--- a/sys/dev/ic/psp.c
+++ b/sys/dev/ic/psp.c
@@ -20,12 +20,13 @@
 #include <sys/systm.h>
 #include <sys/device.h>
 #include <sys/malloc.h>
+#include <sys/mutex.h>
 #include <sys/pledge.h>
+#include <sys/proc.h>
 #include <sys/rwlock.h>
 
 #include <machine/bus.h>
 
-#include <sys/proc.h>
 #include <uvm/uvm.h>
 #include <crypto/xform.h>
 
@@ -66,6 +67,8 @@ struct psp_softc {
 	size_t			sc_ucodelen;
 };
 
+struct mutex psp_lock = MUTEX_INITIALIZER(IPL_BIO);
+
 int	psp_get_pstatus(struct psp_softc *, struct psp_platform_status *);
 int	psp_init(struct psp_softc *, struct psp_init *);
 int	psp_reinit(struct psp_softc *);
@@ -90,6 +93,8 @@ psp_sev_intr(void *arg)
 	struct psp_softc *sc = (struct psp_softc *)csc->sc_psp;
 	uint32_t status;
 
+	mtx_enter(&psp_lock);
+
 	status = bus_space_read_4(sc->sc_iot, sc->sc_ioh, sc->sc_reg_intsts);
 	bus_space_write_4(sc->sc_iot, sc->sc_ioh, sc->sc_reg_intsts, status);
 
@@ -98,6 +103,8 @@ psp_sev_intr(void *arg)
 
 	wakeup(sc);
 
+	mtx_leave(&psp_lock);
+
 	return (1);
 }
 
@@ -209,7 +216,8 @@ ccp_wait(struct psp_softc *sc, uint32_t *status, int poll)
 		return (1);
 	}
 
-	if (tsleep_nsec(sc, PWAIT, "psp", SEC_TO_NSEC(2)) == EWOULDBLOCK)
+	if (msleep_nsec(sc, &psp_lock, PWAIT, "psp", SEC_TO_NSEC(2))
+	    == EWOULDBLOCK)
 		return (1);
 
 	cmdword = bus_space_read_4(sc->sc_iot, sc->sc_ioh, sc->sc_reg_cmdresp);
@@ -223,6 +231,7 @@ static int
 ccp_docmd(struct psp_softc *sc, int cmd, uint64_t paddr)
 {
 	uint32_t	plo, phi, cmdword, status;
+	int		ret;
 
 	plo = ((paddr >> 0) & 0xffffffff);
 	phi = ((paddr >> 32) & 0xffffffff);
@@ -230,11 +239,14 @@ ccp_docmd(struct psp_softc *sc, int cmd, uint64_t paddr)
 	if (!cold)
 		cmdword |= PSP_CMDRESP_IOC;
 
+	mtx_enter(&psp_lock);
 	bus_space_write_4(sc->sc_iot, sc->sc_ioh, sc->sc_reg_addrlo, plo);
 	bus_space_write_4(sc->sc_iot, sc->sc_ioh, sc->sc_reg_addrhi, phi);
 	bus_space_write_4(sc->sc_iot, sc->sc_ioh, sc->sc_reg_cmdresp, cmdword);
 
-	if (ccp_wait(sc, &status, cold))
+	ret = ccp_wait(sc, &status, cold);
+	mtx_leave(&psp_lock);
+	if (ret)
 		return (1);
 
 	/* Did PSP sent a response code? */
@@ -783,6 +795,8 @@ pspioctl(dev_t dev, u_long cmd, caddr_t data, int flag, struct proc *p)
 	if (sc == NULL)
 		return (ENXIO);
 
+	KERNEL_UNLOCK();
+
 	rw_enter_write(&sc->sc_lock);
 
 	switch (cmd) {
@@ -840,6 +854,8 @@ pspioctl(dev_t dev, u_long cmd, caddr_t data, int flag, struct proc *p)
 
 	rw_exit_write(&sc->sc_lock);
 
+	KERNEL_LOCK();
+
 	return (ret);
 }
 
diff --git a/sys/dev/pci/psp_pci.c b/sys/dev/pci/psp_pci.c
index e1356ceeb8a..09864796611 100644
--- a/sys/dev/pci/psp_pci.c
+++ b/sys/dev/pci/psp_pci.c
@@ -81,8 +81,8 @@ psp_pci_intr_map(struct ccp_softc *sc, struct pci_attach_args *pa)
 	}
 
 	intrstr = pci_intr_string(pa->pa_pc, ih);
-	sc->sc_irqh = pci_intr_establish(pa->pa_pc, ih, IPL_BIO, psp_sev_intr,
-	    sc, sc->sc_dev.dv_xname);
+	sc->sc_irqh = pci_intr_establish(pa->pa_pc, ih, IPL_BIO | IPL_MPSAFE,
+	    psp_sev_intr, sc, sc->sc_dev.dv_xname);
 	if (sc->sc_irqh != NULL)
 		printf(": %s", intrstr);
 }