by Tom Lendacky

[permalink] [raw]

Subject: Re: [PATCH v1 6/8] crypto: ccp - Add vdata for platform device

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 <[email protected]>
>>>>> ---
>>>>> 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