| Message ID | 20230123152250.26413-7-jpiotrowski@linux.microsoft.com |
|---|---|
| State | New |
| Headers | show |
| Series | Support ACPI PSP on Hyper-V | expand |
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/ > + struct device *dev = &pdev->dev; > + Should check for null vdata and return NULL, e.g.: if (!vdata) return NULL; > + 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; ... > + 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/ > + dev_dbg(dev, "ACPI cmdresp:\t%x\n", pdata->acpi_cmd_resp_reg); Duplicate entry > +#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. Thanks, Tom > + sp->dev_vdata = sp_get_plat_version(pdev); > if (!sp->dev_vdata) { > ret = -ENODEV; > dev_err(dev, "missing driver data\n");
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. > > >+ 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");
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");
On 2/8/23 06:45, Jeremi Piotrowski wrote: > On 06/02/2023 20:13, Tom Lendacky wrote: >> 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. >> > > I can do that and undo the removal of consts from the > struct (sev|psp)_vdata members, but the outcome would look something > like this: > > static void sp_platform_fill_vdata(struct sp_dev_vdata *vdata, > struct psp_vdata *psp, > struct sev_vdata *sev, > const struct psp_platform_data *pdata) > { > struct sev_vdata sevtmp = { > .cmdbuff_addr_hi_reg = pdata->sev_cmd_buf_hi_reg, > .cmdbuff_addr_lo_reg = pdata->sev_cmd_buf_lo_reg, > .cmdresp_reg = pdata->sev_cmd_resp_reg, > }; > struct psp_vdata psptmp = { > .feature_reg = pdata->feature_reg, > .inten_reg = pdata->irq_en_reg, > .intsts_reg = pdata->irq_st_reg, > .sev = sev, > }; > > memcpy(sev, &sevtmp, sizeof(*sev)); > memcpy(psp, &psptmp, sizeof(*psp)); > vdata->psp_vdata = psp; > } > > static struct sp_dev_vdata *sp_get_platform_version(struct sp_device *sp) > { > struct sp_platform *sp_platform = sp->dev_specific; > struct psp_platform_data *pdata; > struct device *dev = sp->dev; > struct sp_dev_vdata *vdata; > struct psp_vdata *psp; > struct sev_vdata *sev; > > pdata = dev_get_platdata(dev); > if (!pdata) { > dev_err(dev, "missing platform data\n"); > return NULL; > } > > sp_platform->is_platform_device = true; > > vdata = devm_kzalloc(dev, sizeof(*vdata) + sizeof(*psp) + sizeof(*sev), GFP_KERNEL); > if (!vdata) > return NULL; > > psp = (void *)vdata + sizeof(*vdata); > sev = (void *)psp + sizeof(*psp); > sp_platform_fill_vdata(vdata, psp, sev, pdata); > > /* elided debug print */ > ... > > return vdata; > } > > with the const fields in the struct it's not possible to assign in any > other way than on initialization, so I need to use the helper function, > tmp structs and memcpy. Yeah, not the prettiest, but I prefer this over altering the static data. > > Could you ack that you like this approach before I post a v2? Yes, please go with this approach. Thanks, Tom
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; + struct device *dev = &pdev->dev; + + if (drvdata == &dev_vdata[1]) { + 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 + psp_platform.feature_reg = pdata->feature_reg; + 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; + 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); + dev_dbg(dev, "GLBL irq en:\t%x\n", pdata->irq_en_reg); + dev_dbg(dev, "GLBL irq st:\t%x\n", pdata->irq_st_reg); + dev_dbg(dev, "SEV cmdresp:\t%x\n", pdata->sev_cmd_resp_reg); + dev_dbg(dev, "SEV cmdbuf lo:\t%x\n", pdata->sev_cmd_buf_lo_reg); + dev_dbg(dev, "SEV cmdbuf hi:\t%x\n", pdata->sev_cmd_buf_hi_reg); + dev_dbg(dev, "SEV mbox:\t%x\n", pdata->mbox_irq_id); + dev_dbg(dev, "ACPI cmdresp:\t%x\n", pdata->acpi_cmd_resp_reg); +#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) + sp->dev_vdata = sp_get_plat_version(pdev); if (!sp->dev_vdata) { ret = -ENODEV; dev_err(dev, "missing driver data\n");
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(-)