Re: [PATCH v1 6/8] crypto: ccp - Add vdata for platform device
From: Tom Lendacky
Date: Wed Feb 08 2023 - 12:23:31 EST
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@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.
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