Download raw body.
On Mon, Oct 21, 2024 at 01:27:47PM +0200, Mark Kettenis wrote:
> > Also I made the attach code more verbose to see where it fails.
> > The final step to make it work was a BIOS update which included a
> > newer firmware for the psp.
>
> What happened with the older firmware? I suppose it prints one of
> those messages you've added? That suggests people might actually see
> those messages. So maybe we need a little bit more care in what gets
> printed. I mean:
>
> psp0 at ccp0: vers 1, uninitiazed state failed
>
> doesn't make a lot of sense.
This is the output before the BIOS update.
ccp0 at pci4 dev 0 function 2 "AMD 17h Crypto" rev 0x00: msix
psp0 at ccp0: vers 1, api 0.16, fw 12, init failed
I think we should try to attach and we should print an error. So
the user sees that it might work, but there is a problem.
hshoexer@ is working on a ioctl(2) to update firmware on the fly.
For that we have to attach. Maybe we will move the psp init from
attach code to vmd device open to do it after firmware update.
But for now I think being verbose about what goes wrong helps.
> > The ccp_wait() now has 2 seconds timeout, both for polling and
> > interrupt. And I moved to code a bit to prevent a useless
> > bus_space_read_4().
> >
> > Dmesg shows:
> > cpu0: AMD EPYC 3151 4-Core Processor, 2700.00 MHz, 17-01-02, patch 0800126f
> > cpu0: cpuid 8000001F eax=f<SME,SEV,PFLUSH_MSR,SEVES> ecx=f edx=1
> > ccp0 at pci4 dev 0 function 2 "AMD 17h Crypto" rev 0x00: msix
> > psp0 at ccp0: vers 1, api 0.17, build 48, SEV, SEV-ES
> > ccp1 at pci5 dev 0 function 1 "AMD 17h Crypto" rev 0x00
> >
> > Note that ccp1 has a different PCI Id and does not support SEV.
> > This prevents attach of a non existent psp1.
> >
> > ok?
>
> Apart from the error messages, the naming of the #defines bother me a
> bit. We typically use all-caps from macros, although we do
> occasionally use lowercase letters in them. So maybe that is fine.
> But if there is v1 and v2 hardware it makes sense to make this
> explicit and use v2 for the existing defines.
According to Linux driver there are psp v1, v2, v3, v4, v5, v6. We
have seen hardware with v1, v2, v4 and only implement these for
now. v2, v3, v4 have the same register numbers. So I kept the
generic name. I have changed v1 to V1 in the defines.
As hshoexer@ has tested it I added version 4 PCI Id.
bluhm
Index: dev/ic/psp.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/dev/ic/psp.c,v
diff -u -p -r1.5 psp.c
--- dev/ic/psp.c 4 Oct 2024 16:58:26 -0000 1.5
+++ dev/ic/psp.c 21 Oct 2024 11:48:12 -0000
@@ -37,7 +37,12 @@ struct psp_softc {
bus_space_handle_t sc_ioh;
bus_dma_tag_t sc_dmat;
- uint32_t sc_capabilities;
+
+ bus_size_t sc_reg_inten;
+ bus_size_t sc_reg_intsts;
+ bus_size_t sc_reg_cmdresp;
+ bus_size_t sc_reg_addrlo;
+ bus_size_t sc_reg_addrhi;
bus_dmamap_t sc_cmd_map;
bus_dma_segment_t sc_cmd_seg;
@@ -74,8 +79,8 @@ psp_sev_intr(void *arg)
struct psp_softc *sc = (struct psp_softc *)csc->sc_psp;
uint32_t status;
- status = bus_space_read_4(sc->sc_iot, sc->sc_ioh, PSP_REG_INTSTS);
- bus_space_write_4(sc->sc_iot, sc->sc_ioh, PSP_REG_INTSTS, status);
+ 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);
if (!(status & PSP_CMDRESP_COMPLETE))
return (0);
@@ -101,10 +106,25 @@ psp_attach(struct device *parent, struct
size_t size;
int nsegs;
+ printf(":");
sc->sc_iot = arg->iot;
sc->sc_ioh = arg->ioh;
sc->sc_dmat = arg->dmat;
- sc->sc_capabilities = arg->capabilities;
+ if (arg->version == 1) {
+ sc->sc_reg_inten = PSPV1_REG_INTEN;
+ sc->sc_reg_intsts = PSPV1_REG_INTSTS;
+ sc->sc_reg_cmdresp = PSPV1_REG_CMDRESP;
+ sc->sc_reg_addrlo = PSPV1_REG_ADDRLO;
+ sc->sc_reg_addrhi = PSPV1_REG_ADDRHI;
+ } else {
+ sc->sc_reg_inten = PSP_REG_INTEN;
+ sc->sc_reg_intsts = PSP_REG_INTSTS;
+ sc->sc_reg_cmdresp = PSP_REG_CMDRESP;
+ sc->sc_reg_addrlo = PSP_REG_ADDRLO;
+ sc->sc_reg_addrhi = PSP_REG_ADDRHI;
+ }
+ if (arg->version)
+ printf(" vers %d,", arg->version);
rw_init(&sc->sc_lock, "psp_lock");
@@ -127,8 +147,16 @@ psp_attach(struct device *parent, struct
size, NULL, BUS_DMA_WAITOK) != 0)
goto fail_2;
- if (psp_get_pstatus(sc, &pst) || pst.state != 0)
+ if (psp_get_pstatus(sc, &pst)) {
+ printf(" platform status");
goto fail_3;
+ }
+ if (pst.state != PSP_PSTATE_UNINIT) {
+ printf(" uninitialized state");
+ goto fail_3;
+ }
+ printf(" api %u.%u, build %u,",
+ pst.api_major, pst.api_minor, pst.cfges_build >> 24);
/*
* create and map Trusted Memory Region (TMR); size 1 Mbyte,
@@ -156,15 +184,20 @@ psp_attach(struct device *parent, struct
init.enable_es = 1;
init.tmr_length = PSP_TMR_SIZE;
init.tmr_paddr = sc->sc_tmr_map->dm_segs[0].ds_addr;
- if (psp_init(sc, &init))
+ if (psp_init(sc, &init)) {
+ printf(" init");
goto fail_7;
+ }
- printf(": SEV");
+ printf(" SEV");
psp_get_pstatus(sc, &pst);
- if ((pst.state == 1) && (pst.cfges_build & 0x1))
+ if ((pst.state == PSP_PSTATE_INIT) && (pst.cfges_build & 0x1))
printf(", SEV-ES");
+ /* enable interrupts */
+ bus_space_write_4(sc->sc_iot, sc->sc_ioh, sc->sc_reg_inten, -1);
+
printf("\n");
return;
@@ -186,7 +219,7 @@ fail_1:
fail_0:
bus_dmamap_destroy(sc->sc_dmat, sc->sc_cmd_map);
- printf("\n");
+ printf(" failed\n");
return;
}
@@ -199,9 +232,9 @@ ccp_wait(struct psp_softc *sc, uint32_t
if (poll) {
count = 0;
- while (count++ < 100) {
+ while (count++ < 400) {
cmdword = bus_space_read_4(sc->sc_iot, sc->sc_ioh,
- PSP_REG_CMDRESP);
+ sc->sc_reg_cmdresp);
if (cmdword & PSP_CMDRESP_RESPONSE)
goto done;
delay(5000);
@@ -214,12 +247,10 @@ ccp_wait(struct psp_softc *sc, uint32_t
if (tsleep_nsec(sc, PWAIT, "psp", SEC_TO_NSEC(2)) == EWOULDBLOCK)
return (1);
+ cmdword = bus_space_read_4(sc->sc_iot, sc->sc_ioh, sc->sc_reg_cmdresp);
done:
- if (status) {
- *status = bus_space_read_4(sc->sc_iot, sc->sc_ioh,
- PSP_REG_CMDRESP);
- }
-
+ if (status != NULL)
+ *status = cmdword;
return (0);
}
@@ -234,9 +265,9 @@ ccp_docmd(struct psp_softc *sc, int cmd,
if (!cold)
cmdword |= PSP_CMDRESP_IOC;
- bus_space_write_4(sc->sc_iot, sc->sc_ioh, PSP_REG_ADDRLO, plo);
- bus_space_write_4(sc->sc_iot, sc->sc_ioh, PSP_REG_ADDRHI, phi);
- bus_space_write_4(sc->sc_iot, sc->sc_ioh, PSP_REG_CMDRESP, cmdword);
+ 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))
return (1);
Index: dev/ic/pspvar.h
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/dev/ic/pspvar.h,v
diff -u -p -r1.2 pspvar.h
--- dev/ic/pspvar.h 4 Sep 2024 07:45:08 -0000 1.2
+++ dev/ic/pspvar.h 21 Oct 2024 11:48:03 -0000
@@ -19,6 +19,13 @@
#include <sys/ioctl.h>
/* AMD 17h */
+#define PSPV1_REG_INTEN 0x10610
+#define PSPV1_REG_INTSTS 0x10614
+#define PSPV1_REG_CMDRESP 0x10580
+#define PSPV1_REG_ADDRLO 0x105e0
+#define PSPV1_REG_ADDRHI 0x105e4
+#define PSPV1_REG_CAPABILITIES 0x105fc
+
#define PSP_REG_INTEN 0x10690
#define PSP_REG_INTSTS 0x10694
#define PSP_REG_CMDRESP 0x10980
@@ -252,10 +259,18 @@ struct psp_attach_args {
bus_dma_tag_t dmat;
uint32_t capabilities;
+ int version;
};
int pspsubmatch(struct device *, void *, void *);
int pspprint(void *aux, const char *pnp);
int psp_sev_intr(void *);
+
+struct ccp_softc;
+struct pci_attach_args;
+
+int psp_pci_match(struct ccp_softc *, struct pci_attach_args *);
+void psp_pci_intr_map(struct ccp_softc *, struct pci_attach_args *);
+void psp_pci_attach(struct ccp_softc *, struct pci_attach_args *);
#endif /* _KERNEL */
Index: dev/pci/ccp_pci.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/dev/pci/ccp_pci.c,v
diff -u -p -r1.13 ccp_pci.c
--- dev/pci/ccp_pci.c 4 Sep 2024 07:45:08 -0000 1.13
+++ dev/pci/ccp_pci.c 21 Oct 2024 11:39:12 -0000
@@ -36,9 +36,6 @@
int ccp_pci_match(struct device *, void *, void *);
void ccp_pci_attach(struct device *, struct device *, void *);
-void ccp_pci_intr_map(struct ccp_softc *, struct pci_attach_args *);
-void ccp_pci_psp_attach(struct ccp_softc *, struct pci_attach_args *);
-
const struct cfattach ccp_pci_ca = {
sizeof(struct ccp_softc),
ccp_pci_match,
@@ -67,6 +64,9 @@ ccp_pci_attach(struct device *parent, st
struct ccp_softc *sc = (struct ccp_softc *)self;
struct pci_attach_args *pa = aux;
pcireg_t memtype;
+#if NPSP > 0
+ int psp_matched;
+#endif
memtype = pci_mapreg_type(pa->pa_pc, pa->pa_tag, CCP_PCI_BAR);
if (PCI_MAPREG_TYPE(memtype) != PCI_MAPREG_TYPE_MEM) {
@@ -80,59 +80,16 @@ ccp_pci_attach(struct device *parent, st
return;
}
- ccp_pci_intr_map(sc, pa);
-
- ccp_attach(sc);
-
- ccp_pci_psp_attach(sc, pa);
-}
-
-void
-ccp_pci_intr_map(struct ccp_softc *sc, struct pci_attach_args *pa)
-{
#if NPSP > 0
- pci_intr_handle_t ih;
- const char *intrstr = NULL;
-
- /* clear and disable interrupts */
- bus_space_write_4(sc->sc_iot, sc->sc_ioh, PSP_REG_INTEN, 0);
- bus_space_write_4(sc->sc_iot, sc->sc_ioh, PSP_REG_INTSTS, -1);
-
- if (pci_intr_map_msix(pa, 0, &ih) != 0 &&
- pci_intr_map_msi(pa, &ih) != 0 && pci_intr_map(pa, &ih) != 0) {
- printf(": couldn't map interrupt\n");
- return;
- }
-
- 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);
- if (sc->sc_irqh != NULL)
- printf(": %s", intrstr);
+ psp_matched = psp_pci_match(sc, aux);
+ if (psp_matched)
+ psp_pci_intr_map(sc, pa);
#endif
-}
-void
-ccp_pci_psp_attach(struct ccp_softc *sc, struct pci_attach_args *pa)
-{
-#if NPSP > 0
- struct psp_attach_args arg;
- struct device *self = (struct device *)sc;
-
- memset(&arg, 0, sizeof(arg));
- arg.iot = sc->sc_iot;
- arg.ioh = sc->sc_ioh;
- arg.dmat = pa->pa_dmat;
- arg.capabilities = bus_space_read_4(sc->sc_iot, sc->sc_ioh,
- PSP_REG_CAPABILITIES);
-
- sc->sc_psp = config_found_sm(self, &arg, pspprint, pspsubmatch);
- if (sc->sc_psp == NULL) {
- pci_intr_disestablish(pa->pa_pc, sc->sc_irqh);
- return;
- }
+ ccp_attach(sc);
- /* enable interrupts */
- bus_space_write_4(sc->sc_iot, sc->sc_ioh, PSP_REG_INTEN, -1);
+#if NPSP > 0
+ if (psp_matched)
+ psp_pci_attach(sc, pa);
#endif
}
Index: dev/pci/files.pci
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/dev/pci/files.pci,v
diff -u -p -r1.366 files.pci
--- dev/pci/files.pci 14 Aug 2024 14:40:46 -0000 1.366
+++ dev/pci/files.pci 21 Oct 2024 11:39:12 -0000
@@ -830,6 +830,7 @@ file dev/pci/if_bwfm_pci.c bwfm_pci
# AMD Cryptographic Co-processor
attach ccp at pci with ccp_pci
file dev/pci/ccp_pci.c ccp_pci
+file dev/pci/psp_pci.c psp
# Broadcom NetXtreme-C/E
device bnxt: ether, ifnet, ifmedia, intrmap, stoeplitz
Index: dev/pci/psp_pci.c
===================================================================
RCS file: dev/pci/psp_pci.c
diff -N dev/pci/psp_pci.c
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ dev/pci/psp_pci.c 21 Oct 2024 11:47:12 -0000
@@ -0,0 +1,125 @@
+/* $OpenBSD$ */
+
+/*
+ * Copyright (c) 2023-2024 Hans-Joerg Hoexer <hshoexer@genua.de>
+ * Copyright (c) 2024 Alexander Bluhm <bluhm@openbsd.org>
+ *
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#include <sys/param.h>
+#include <sys/systm.h>
+#include <sys/device.h>
+
+#include <machine/bus.h>
+
+#include <dev/pci/pcidevs.h>
+#include <dev/pci/pcivar.h>
+
+#include <dev/ic/ccpvar.h>
+#include <dev/ic/pspvar.h>
+
+static const struct pci_matchid psp_pci_devices[] = {
+ { PCI_VENDOR_AMD, PCI_PRODUCT_AMD_17_CCP_1 },
+ { PCI_VENDOR_AMD, PCI_PRODUCT_AMD_17_3X_CCP },
+};
+
+int
+psp_pci_match(struct ccp_softc *sc, struct pci_attach_args *pa)
+{
+ bus_size_t reg_capabilities;
+ uint32_t capabilities;
+
+ if (!pci_matchbyid(pa, psp_pci_devices, nitems(psp_pci_devices)))
+ return (0);
+
+ if (PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_AMD_17_CCP_1)
+ reg_capabilities = PSPV1_REG_CAPABILITIES;
+ else
+ reg_capabilities = PSP_REG_CAPABILITIES;
+ capabilities = bus_space_read_4(sc->sc_iot, sc->sc_ioh,
+ reg_capabilities);
+ if (!ISSET(capabilities, PSP_CAP_SEV))
+ return (0);
+
+ return (1);
+}
+
+void
+psp_pci_intr_map(struct ccp_softc *sc, struct pci_attach_args *pa)
+{
+ pci_intr_handle_t ih;
+ const char *intrstr = NULL;
+ bus_size_t reg_inten, reg_intsts;
+
+ /* clear and disable interrupts */
+ if (PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_AMD_17_CCP_1) {
+ reg_inten = PSPV1_REG_INTEN;
+ reg_intsts = PSPV1_REG_INTSTS;
+ } else {
+ reg_inten = PSP_REG_INTEN;
+ reg_intsts = PSP_REG_INTSTS;
+ }
+ bus_space_write_4(sc->sc_iot, sc->sc_ioh, reg_inten, 0);
+ bus_space_write_4(sc->sc_iot, sc->sc_ioh, reg_intsts, -1);
+
+ if (pci_intr_map_msix(pa, 0, &ih) != 0 &&
+ pci_intr_map_msi(pa, &ih) != 0 && pci_intr_map(pa, &ih) != 0) {
+ printf(": couldn't map interrupt\n");
+ return;
+ }
+
+ 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);
+ if (sc->sc_irqh != NULL)
+ printf(": %s", intrstr);
+}
+
+void
+psp_pci_attach(struct ccp_softc *sc, struct pci_attach_args *pa)
+{
+ struct psp_attach_args arg;
+ struct device *self = (struct device *)sc;
+ bus_size_t reg_capabilities;
+
+ memset(&arg, 0, sizeof(arg));
+ arg.iot = sc->sc_iot;
+ arg.ioh = sc->sc_ioh;
+ arg.dmat = pa->pa_dmat;
+ switch (PCI_PRODUCT(pa->pa_id)) {
+ case PCI_PRODUCT_AMD_17_CCP_1:
+ arg.version = 1;
+ reg_capabilities = PSPV1_REG_CAPABILITIES;
+ break;
+ case PCI_PRODUCT_AMD_17_3X_CCP:
+ arg.version = 2;
+ reg_capabilities = PSP_REG_CAPABILITIES;
+ break;
+ case PCI_PRODUCT_AMD_19_1X_PSP:
+ arg.version = 4;
+ reg_capabilities = PSP_REG_CAPABILITIES;
+ break;
+ default:
+ reg_capabilities = PSP_REG_CAPABILITIES;
+ break;
+ }
+ arg.capabilities = bus_space_read_4(sc->sc_iot, sc->sc_ioh,
+ reg_capabilities);
+
+ sc->sc_psp = config_found_sm(self, &arg, pspprint, pspsubmatch);
+ if (sc->sc_psp == NULL) {
+ pci_intr_disestablish(pa->pa_pc, sc->sc_irqh);
+ return;
+ }
+}