From: David Gwynne <david@gwynne.id.au>
Subject: Re: psp(4): Avoid sleep/wakeup race by raising SPL
To: Hans-Jörg Höxer <hshoexer@genua.de>
Cc: OpenBSD Tech <tech@openbsd.org>
Date: Thu, 7 Nov 2024 07:51:02 +1000



> On 7 Nov 2024, at 00:31, Hans-Jörg Höxer <hshoexer@genua.de> wrote:
> 
> Hi,
> 
> got some more feedback, thanks!  Updated diff below.

looks good, ok by me.

> 
> Take care,
> HJ.
> ----------------------------------------------------------------
> commit a2d38daeedc1921e1d1a5befd13d3438f7d277c8
> 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 72978ca5d4e..8814a13e7a0 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>
> 
> @@ -56,6 +57,7 @@ struct psp_softc {
> caddr_t sc_tmr_kva;
> 
> struct rwlock sc_lock;
> + struct mutex psp_lock;
> 
> uint32_t sc_flags;
> #define PSPF_INITIALIZED 0x1
> @@ -90,8 +92,10 @@ psp_sev_intr(void *arg)
> struct psp_softc *sc = (struct psp_softc *)csc->sc_psp;
> uint32_t status;
> 
> + mtx_enter(&sc->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);
> + mtx_leave(&sc->psp_lock);
> 
> if (!(status & PSP_CMDRESP_COMPLETE))
> return (0);
> @@ -137,6 +141,7 @@ psp_attach(struct device *parent, struct device *self, void *aux)
> printf(" vers %d,", arg->version);
> 
> rw_init(&sc->sc_lock, "psp_lock");
> + mtx_init(&sc->psp_lock, IPL_BIO);
> 
> /* create and map SEV command buffer */
> sc->sc_cmd_size = size = PAGE_SIZE;
> @@ -209,7 +214,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, &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 +229,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 +237,14 @@ ccp_docmd(struct psp_softc *sc, int cmd, uint64_t paddr)
> if (!cold)
> cmdword |= PSP_CMDRESP_IOC;
> 
> + mtx_enter(&sc->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(&sc->psp_lock);
> + if (ret)
> return (1);
> 
> /* Did PSP sent a response code? */
> @@ -783,6 +793,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 +852,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);
> }