| Message ID | 20230328151636.1353846-1-jpiotrowski@linux.microsoft.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: ccp - Clear PSP interrupt status register before calling handler | expand |
On 3/28/23 10:16, Jeremi Piotrowski wrote: > The PSP IRQ is edge-triggered (MSI or MSI-X) in all cases supported by > the psp module so clear the interrupt status register early in the > handler to prevent missed interrupts. sev_irq_handler() calls wake_up() > on a wait queue, which can result in a new command being submitted from > a different CPU. This then races with the clearing of isr and can result > in missed interrupts. A missed interrupt results in a command waiting > until it times out, which results in the psp being declared dead. > > This is unlikely on bare metal, but has been observed when running > virtualized. In the cases where this is observed, sev->cmdresp_reg has > PSP_CMDRESP_RESP set which indicates that the command was processed > correctly but no interrupt was asserted. > > The full sequence of events looks like this: > > CPU 1: submits SEV cmd #1 > CPU 1: calls wait_event_timeout() > CPU 0: enters psp_irq_handler() > CPU 0: calls sev_handler()->wake_up() > CPU 1: wakes up; finishes processing cmd #1 > CPU 1: submits SEV cmd #2 > CPU 1: calls wait_event_timeout() > PSP: finishes processing cmd #2; interrupt status is still set; no interrupt > CPU 0: clears intsts > CPU 0: exits psp_irq_handler() > CPU 1: wait_event_timeout() times out; psp_dead=true > > Fixes: 200664d5237f ("crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support") > Cc: stable@vger.kernel.org > Signed-off-by: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com> Acked-by: Tom Lendacky <thomas.lendacky@amd.com> > --- > drivers/crypto/ccp/psp-dev.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c > index c9c741ac8442..949a3fa0b94a 100644 > --- a/drivers/crypto/ccp/psp-dev.c > +++ b/drivers/crypto/ccp/psp-dev.c > @@ -42,6 +42,9 @@ static irqreturn_t psp_irq_handler(int irq, void *data) > /* Read the interrupt status: */ > status = ioread32(psp->io_regs + psp->vdata->intsts_reg); > > + /* Clear the interrupt status by writing the same value we read. */ > + iowrite32(status, psp->io_regs + psp->vdata->intsts_reg); > + > /* invoke subdevice interrupt handlers */ > if (status) { > if (psp->sev_irq_handler) > @@ -51,9 +54,6 @@ static irqreturn_t psp_irq_handler(int irq, void *data) > psp->tee_irq_handler(irq, psp->tee_irq_data, status); > } > > - /* Clear the interrupt status by writing the same value we read. */ > - iowrite32(status, psp->io_regs + psp->vdata->intsts_reg); > - > return IRQ_HANDLED; > } >
On Tue, Mar 28, 2023 at 03:16:36PM +0000, Jeremi Piotrowski wrote: > The PSP IRQ is edge-triggered (MSI or MSI-X) in all cases supported by > the psp module so clear the interrupt status register early in the > handler to prevent missed interrupts. sev_irq_handler() calls wake_up() > on a wait queue, which can result in a new command being submitted from > a different CPU. This then races with the clearing of isr and can result > in missed interrupts. A missed interrupt results in a command waiting > until it times out, which results in the psp being declared dead. > > This is unlikely on bare metal, but has been observed when running > virtualized. In the cases where this is observed, sev->cmdresp_reg has > PSP_CMDRESP_RESP set which indicates that the command was processed > correctly but no interrupt was asserted. > > The full sequence of events looks like this: > > CPU 1: submits SEV cmd #1 > CPU 1: calls wait_event_timeout() > CPU 0: enters psp_irq_handler() > CPU 0: calls sev_handler()->wake_up() > CPU 1: wakes up; finishes processing cmd #1 > CPU 1: submits SEV cmd #2 > CPU 1: calls wait_event_timeout() > PSP: finishes processing cmd #2; interrupt status is still set; no interrupt > CPU 0: clears intsts > CPU 0: exits psp_irq_handler() > CPU 1: wait_event_timeout() times out; psp_dead=true > > Fixes: 200664d5237f ("crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support") > Cc: stable@vger.kernel.org > Signed-off-by: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com> > --- > drivers/crypto/ccp/psp-dev.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) Patch applied. Thanks.
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c index c9c741ac8442..949a3fa0b94a 100644 --- a/drivers/crypto/ccp/psp-dev.c +++ b/drivers/crypto/ccp/psp-dev.c @@ -42,6 +42,9 @@ static irqreturn_t psp_irq_handler(int irq, void *data) /* Read the interrupt status: */ status = ioread32(psp->io_regs + psp->vdata->intsts_reg); + /* Clear the interrupt status by writing the same value we read. */ + iowrite32(status, psp->io_regs + psp->vdata->intsts_reg); + /* invoke subdevice interrupt handlers */ if (status) { if (psp->sev_irq_handler) @@ -51,9 +54,6 @@ static irqreturn_t psp_irq_handler(int irq, void *data) psp->tee_irq_handler(irq, psp->tee_irq_data, status); } - /* Clear the interrupt status by writing the same value we read. */ - iowrite32(status, psp->io_regs + psp->vdata->intsts_reg); - return IRQ_HANDLED; }
The PSP IRQ is edge-triggered (MSI or MSI-X) in all cases supported by the psp module so clear the interrupt status register early in the handler to prevent missed interrupts. sev_irq_handler() calls wake_up() on a wait queue, which can result in a new command being submitted from a different CPU. This then races with the clearing of isr and can result in missed interrupts. A missed interrupt results in a command waiting until it times out, which results in the psp being declared dead. This is unlikely on bare metal, but has been observed when running virtualized. In the cases where this is observed, sev->cmdresp_reg has PSP_CMDRESP_RESP set which indicates that the command was processed correctly but no interrupt was asserted. The full sequence of events looks like this: CPU 1: submits SEV cmd #1 CPU 1: calls wait_event_timeout() CPU 0: enters psp_irq_handler() CPU 0: calls sev_handler()->wake_up() CPU 1: wakes up; finishes processing cmd #1 CPU 1: submits SEV cmd #2 CPU 1: calls wait_event_timeout() PSP: finishes processing cmd #2; interrupt status is still set; no interrupt CPU 0: clears intsts CPU 0: exits psp_irq_handler() CPU 1: wait_event_timeout() times out; psp_dead=true Fixes: 200664d5237f ("crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support") Cc: stable@vger.kernel.org Signed-off-by: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com> --- drivers/crypto/ccp/psp-dev.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)