Messages in this thread | | | Date | Wed, 8 Feb 2023 11:23:20 -0600 | Subject | Re: [PATCH v1 6/8] crypto: ccp - Add vdata for platform device | From | Tom Lendacky <> |
| |
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
| |