Messages in this thread | | | Date | Mon, 6 Feb 2023 13:13:41 -0600 | Subject | Re: [PATCH v1 6/8] crypto: ccp - Add vdata for platform device | From | Tom Lendacky <> |
| |
On 2/1/23 13:24, Jeremi Piotrowski wrote: > 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@linux.microsoft.com> >>> --- >>> 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.
Ok, then maybe it would be better to kmalloc a vdata structure that you fill in and then assign that to dev_vdata field for use. That could eliminate the removal of the "const" notations in one of the previous patches. I just don't think you should be changing the underlying module data that isn't expected to be changed.
> >> >>> + 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:
My bad, misread it as SEV cmdresp.
Thanks, Tom
> > 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");
| |