On Tue, Jan 31, 2023 at 02:36:01PM -0600, Tom Lendacky wrote: > On 1/23/23 09:22, Jeremi Piotrowski wrote: > >When matching the "psp" platform_device, determine the register offsets > >at runtime from the ASP ACPI table. Pass the parsed register offsets > >from the ASPT through platdata. > > > >To support this scenario, mark the members of 'struct sev_vdata' and > >'struct psp_vdata' non-const so that the probe function can write the > >values. This does not affect the other users of sev_vdata/psp_vdata as > >they define the whole struct const and the pointer in struct > >sp_dev_vdata stays const too. > > > >Signed-off-by: Jeremi Piotrowski <jpiotrowski@xxxxxxxxxxxxxxxxxxx> > >--- > > arch/x86/kernel/psp.c | 3 ++ > > drivers/crypto/ccp/sp-dev.h | 12 +++---- > > drivers/crypto/ccp/sp-platform.c | 57 +++++++++++++++++++++++++++++++- > > 3 files changed, 65 insertions(+), 7 deletions(-) > > > >diff --git a/arch/x86/kernel/psp.c b/arch/x86/kernel/psp.c > >index 24181d132bae..68511a14df63 100644 > >--- a/arch/x86/kernel/psp.c > >+++ b/arch/x86/kernel/psp.c > >@@ -199,6 +199,9 @@ static int __init psp_init_platform_device(void) > > if (err) > > return err; > > err = platform_device_add_resources(&psp_device, res, 2); > >+ if (err) > >+ return err; > >+ err = platform_device_add_data(&psp_device, &pdata, sizeof(pdata)); > > if (err) > > return err; > >diff --git a/drivers/crypto/ccp/sp-dev.h b/drivers/crypto/ccp/sp-dev.h > >index 20377e67f65d..aaa651364425 100644 > >--- a/drivers/crypto/ccp/sp-dev.h > >+++ b/drivers/crypto/ccp/sp-dev.h > >@@ -40,9 +40,9 @@ struct ccp_vdata { > > }; > > struct sev_vdata { > >- const unsigned int cmdresp_reg; > >- const unsigned int cmdbuff_addr_lo_reg; > >- const unsigned int cmdbuff_addr_hi_reg; > >+ unsigned int cmdresp_reg; > >+ unsigned int cmdbuff_addr_lo_reg; > >+ unsigned int cmdbuff_addr_hi_reg; > > }; > > struct tee_vdata { > >@@ -56,9 +56,9 @@ struct tee_vdata { > > struct psp_vdata { > > const struct sev_vdata *sev; > > const struct tee_vdata *tee; > >- const unsigned int feature_reg; > >- const unsigned int inten_reg; > >- const unsigned int intsts_reg; > >+ unsigned int feature_reg; > >+ unsigned int inten_reg; > >+ unsigned int intsts_reg; > > }; > > /* Structure to hold SP device data */ > >diff --git a/drivers/crypto/ccp/sp-platform.c b/drivers/crypto/ccp/sp-platform.c > >index ea8926e87981..281dbf6b150c 100644 > >--- a/drivers/crypto/ccp/sp-platform.c > >+++ b/drivers/crypto/ccp/sp-platform.c > >@@ -22,6 +22,7 @@ > > #include <linux/of.h> > > #include <linux/of_address.h> > > #include <linux/acpi.h> > >+#include <linux/platform_data/psp.h> > > #include "ccp-dev.h" > >@@ -30,11 +31,31 @@ struct sp_platform { > > unsigned int irq_count; > > }; > >+#ifdef CONFIG_CRYPTO_DEV_SP_PSP > >+static struct sev_vdata sev_platform = { > >+ .cmdresp_reg = -1, > >+ .cmdbuff_addr_lo_reg = -1, > >+ .cmdbuff_addr_hi_reg = -1, > >+}; > >+static struct psp_vdata psp_platform = { > >+ .sev = &sev_platform, > >+ .feature_reg = -1, > >+ .inten_reg = -1, > >+ .intsts_reg = -1, > >+}; > >+#endif > >+ > > static const struct sp_dev_vdata dev_vdata[] = { > > { > > .bar = 0, > > #ifdef CONFIG_CRYPTO_DEV_SP_CCP > > .ccp_vdata = &ccpv3_platform, > >+#endif > >+ }, > >+ { > >+ .bar = 0, > >+#ifdef CONFIG_CRYPTO_DEV_SP_PSP > >+ .psp_vdata = &psp_platform, > > #endif > > }, > > }; > >@@ -57,7 +78,7 @@ MODULE_DEVICE_TABLE(of, sp_of_match); > > #endif > > static const struct platform_device_id sp_plat_match[] = { > >- { "psp" }, > >+ { "psp", (kernel_ulong_t)&dev_vdata[1] }, > > { }, > > }; > > MODULE_DEVICE_TABLE(platform, sp_plat_match); > >@@ -86,6 +107,38 @@ static struct sp_dev_vdata *sp_get_acpi_version(struct platform_device *pdev) > > return NULL; > > } > >+static struct sp_dev_vdata *sp_get_plat_version(struct platform_device *pdev) > >+{ > >+ struct sp_dev_vdata *drvdata = (struct sp_dev_vdata *)pdev->id_entry->driver_data; > > s/drvdata/vdata/ > ok > >+ struct device *dev = &pdev->dev; > >+ > > Should check for null vdata and return NULL, e.g.: > > if (!vdata) > return NULL; > ok > >+ if (drvdata == &dev_vdata[1]) { > > This should be a check for vdata->psp_vdata being non-NULL and > vdata->psp_vdata->sev being non-NULL, e.g.: > > if (vdata->psp_vdata && vdata->psp_vdata->sev) { > > >+ struct psp_platform_data *pdata = dev_get_platdata(dev); > >+ > >+ if (!pdata) { > >+ dev_err(dev, "missing platform data\n"); > >+ return NULL; > >+ } > >+#ifdef CONFIG_CRYPTO_DEV_SP_PSP > > No need for this with the above checks > > >+ psp_platform.feature_reg = pdata->feature_reg; > > These should then be: > > vdata->psp_vdata->inten_reg = pdata->feature_reg; > ... I see where you're going with this and the above suggestions, but the psp_vdata pointer is const in struct sp_dev_vdata and so is the sev pointer in struct psp_vdata. I find these consts to be important and doing it this way would require casting away the const. I don't think that's worth doing. > > >+ psp_platform.inten_reg = pdata->irq_en_reg; > >+ psp_platform.intsts_reg = pdata->irq_st_reg; > >+ sev_platform.cmdresp_reg = pdata->sev_cmd_resp_reg; > > And this should be: > > vdata->psp_vdata->sev->cmdbuff_addr_lo = ... > > >+ sev_platform.cmdbuff_addr_lo_reg = pdata->sev_cmd_buf_lo_reg; > >+ sev_platform.cmdbuff_addr_hi_reg = pdata->sev_cmd_buf_hi_reg; > >+ dev_dbg(dev, "GLBL feature:\t%x\n", pdata->feature_reg); > > s/GLBL feature/PSP feature register/ > > >+ dev_dbg(dev, "GLBL irq en:\t%x\n", pdata->irq_en_reg); > > s/GLBL irq en/PSP IRQ enable register/ > > >+ dev_dbg(dev, "GLBL irq st:\t%x\n", pdata->irq_st_reg); > > s/GLBL irq st/PSP IRQ status register/ > > >+ dev_dbg(dev, "SEV cmdresp:\t%x\n", pdata->sev_cmd_resp_reg); > > s/SEV cmdresp/SEV cmdresp register/ > > >+ dev_dbg(dev, "SEV cmdbuf lo:\t%x\n", pdata->sev_cmd_buf_lo_reg); > > s/SEV cmdbuf lo/SEV cmdbuf lo register/ > > >+ dev_dbg(dev, "SEV cmdbuf hi:\t%x\n", pdata->sev_cmd_buf_hi_reg); > > s/SEV cmdbuf hi/SEV cmdbuf hi register/ > > >+ dev_dbg(dev, "SEV mbox:\t%x\n", pdata->mbox_irq_id); > > s/SEV mbox/SEV cmdresp IRQ/ > ok to all the above rewordings > > >+ dev_dbg(dev, "ACPI cmdresp:\t%x\n", pdata->acpi_cmd_resp_reg); > > Duplicate entry I don't see it. This is the ACPI register (the one used for the IRQ config). Here's how these prints look when the module is loaded with dyndbg=+p: ccp psp: GLBL feature: 0 ccp psp: GLBL irq en: 4 ccp psp: GLBL irq st: 8 ccp psp: SEV cmdresp: 10 ccp psp: SEV cmdbuf lo: 14 ccp psp: SEV cmdbuf hi: 18 ccp psp: SEV mbox: 1 ccp psp: ACPI cmdresp: 20 > > >+#endif > >+ } > >+ return drvdata; > >+} > >+ > > static int sp_get_irqs(struct sp_device *sp) > > { > > struct sp_platform *sp_platform = sp->dev_specific; > >@@ -137,6 +190,8 @@ static int sp_platform_probe(struct platform_device *pdev) > > sp->dev_specific = sp_platform; > > sp->dev_vdata = pdev->dev.of_node ? sp_get_of_version(pdev) > > : sp_get_acpi_version(pdev); > >+ if (!sp->dev_vdata && pdev->id_entry) > > Move this pdev->id_entry check into sp_get_plat_version(), returning > NULL if not set. > ok > Thanks, > Tom > > >+ sp->dev_vdata = sp_get_plat_version(pdev); > > if (!sp->dev_vdata) { > > ret = -ENODEV; > > dev_err(dev, "missing driver data\n");