From: Tom Lendacky <thomas.lendacky@amd.com>
To: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>,
linux-kernel@vger.kernel.org
Cc: Brijesh Singh <brijesh.singh@amd.com>,
"Kalra, Ashish" <ashish.kalra@amd.com>,
linux-crypto@vger.kernel.org
Subject: Re: [PATCH v1 6/8] crypto: ccp - Add vdata for platform device
Date: Tue, 31 Jan 2023 14:36:01 -0600 [thread overview]
Message-ID: <6f76fe2b-63ea-8c45-87d8-3de30d3d76c2@amd.com> (raw)
In-Reply-To: <20230123152250.26413-7-jpiotrowski@linux.microsoft.com>
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");
next prev parent reply other threads:[~2023-01-31 20:36 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-23 15:22 [PATCH v1 0/8] Support ACPI PSP on Hyper-V Jeremi Piotrowski
2023-01-23 15:22 ` [PATCH v1 1/8] include/acpi: add definition of ASPT table Jeremi Piotrowski
2023-01-23 19:56 ` Rafael J. Wysocki
2023-01-24 16:05 ` Jeremi Piotrowski
2023-01-23 15:22 ` [PATCH v1 2/8] ACPI: ASPT: Add helper to parse table Jeremi Piotrowski
2023-01-23 15:22 ` [PATCH v1 3/8] x86/psp: Register PSP platform device when ASP table is present Jeremi Piotrowski
2023-01-31 18:49 ` Tom Lendacky
2023-02-01 14:09 ` Jeremi Piotrowski
2023-02-01 14:57 ` Tom Lendacky
2023-01-23 15:22 ` [PATCH v1 4/8] x86/psp: Add IRQ support Jeremi Piotrowski
2023-01-31 19:45 ` Tom Lendacky
2023-01-23 15:22 ` [PATCH v1 5/8] crypto: cpp - Bind to psp platform device on x86 Jeremi Piotrowski
2023-01-31 19:51 ` Tom Lendacky
2023-02-08 12:48 ` Jeremi Piotrowski
2023-01-23 15:22 ` [PATCH v1 6/8] crypto: ccp - Add vdata for platform device Jeremi Piotrowski
2023-01-31 20:36 ` Tom Lendacky [this message]
2023-02-01 19:24 ` Jeremi Piotrowski
2023-02-06 19:13 ` Tom Lendacky
2023-02-08 12:45 ` Jeremi Piotrowski
2023-02-08 17:23 ` Tom Lendacky
2023-01-23 15:22 ` [PATCH v1 7/8] crypto: ccp - Skip DMA coherency check for platform psp Jeremi Piotrowski
2023-01-31 20:42 ` Tom Lendacky
2023-02-08 12:56 ` Jeremi Piotrowski
2023-01-23 15:22 ` [PATCH v1 8/8] crypto: ccp - Allow platform device to be psp master device Jeremi Piotrowski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6f76fe2b-63ea-8c45-87d8-3de30d3d76c2@amd.com \
--to=thomas.lendacky@amd.com \
--cc=ashish.kalra@amd.com \
--cc=brijesh.singh@amd.com \
--cc=jpiotrowski@linux.microsoft.com \
--cc=linux-crypto@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox