Message-ID: <6f76fe2b-63ea-8c45-87d8-3de30d3d76c2@amd.com>
Date:   Tue, 31 Jan 2023 14:36:01 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: [PATCH v1 6/8] crypto: ccp - Add vdata for platform device
Content-Language: en-US
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
References: <20230123152250.26413-1-jpiotrowski@linux.microsoft.com>
 <20230123152250.26413-7-jpiotrowski@linux.microsoft.com>
From:   Tom Lendacky <thomas.lendacky@amd.com>
In-Reply-To: <20230123152250.26413-7-jpiotrowski@linux.microsoft.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?NVBsM0hHYXM2eUtNaGdtTzB3eWxqKzNHQ2NEbHZwcy8zeU9KRklFdFNvVnpo?=
 =?utf-8?B?c0lsditCUzZLb2dwUVF4NW9mbkRyVTRDcmtOak5MNUJVY1Yram9CUWgvSnZq?=
 =?utf-8?B?U0MxTTJxaUQ0anlyVHArdkZPTTRNaVJSQ1dnejg4KzlZZ2dnRFZnT0NGUlBp?=
 =?utf-8?B?RTg1WE5DbGNwMXB5SVllak41UFNqbUJadTZoeXUrOG5ieFhCaDJycHBUNWZY?=
 =?utf-8?B?SGxJaTRRSmcvVWhYRTZVQzVIMjFtTXNWcmxBdVRFVkdYTDV4ajB0Si9XN3BZ?=
 =?utf-8?B?RmtRVUZCR3JjRkpwZkt0Zk5RWE1BQlpIbkZrbFlmTE9JR051LzNjQ29Vbjhp?=
 =?utf-8?B?Rkl1dUIyOUgwS3NqTGxxWUUxQzZPWXk4bC85WndTMzE1a2tmZ21FVVdseWRK?=
 =?utf-8?B?ZDR1MGxaL2FDSnBqMjZSRnNXQWd5SDlyQ1dndUFjZzZlM1JmdXgvN3A0ZnBr?=
 =?utf-8?B?Z3FZd3ZleWxxdnp0Y0NDVGVNNGZkalJ4Z052bWUrMkV2OFRMYytWSUlZaTk0?=
 =?utf-8?B?azVaZktrOXQydXRDOGdhVjBGZHR0QVJ0bzdCVkJ0TWhzOWtBeXgzWmNqaE9v?=
 =?utf-8?B?RFRsVllnRndtRVJTOFRqcVlVV0xrL1pBbkw5NHY2NjhCQ2NzNXEva3hYRmdv?=
 =?utf-8?B?dllFRlNCMmJhL0dRL1JLdnFWekJJQzFsM1hkVG82YWJ2ZEJvYTFrWkJDTVpj?=
 =?utf-8?B?YWF4dmt4ODQrRnhuVE8reXZtSFJhb09CVVdUY0t1T2cvVGM4YWw3NDY5NHVK?=
 =?utf-8?B?SDRVK2xuZncyQUdRNnNjTUJHUEZlRm1ha21MTGtXQllmbWgrLzZCRGhpQWRP?=
 =?utf-8?B?K2dpczdLZ211YzJhcnlpUm1PQlNBTEM5b2szdGlQNzdJUjF0QlJ5ZDNKK0xZ?=
 =?utf-8?B?VHdvazdGTHVpcm1TZmhLMllDQnorQ1hYV1YvUVkrWkdycHN4V1FwSG1hRkwv?=
 =?utf-8?B?cFBWY0pVTHp0Qmd4emRiZmE2bVpYMmlXbERrWjY4RnR6OVNzdEwyVFlNWHR1?=
 =?utf-8?B?M2NrTVJJL1FlUkloaWNjWC9OMVJmb01XbU91SVF0UkRFakcyeFpqejR1TXla?=
 =?utf-8?B?N0R3aG11WEJUMkhzN2tJeFZDUXVkRldNUnJMTHIvMVMzcmErWmw5WHkzTk9S?=
 =?utf-8?B?bjdzK0RGQzJlaEVqM0dsQ0U1aTZtUG9hMjdmNy9sS3FvZWlIZjd4WVFsUk1y?=
 =?utf-8?B?N3pmLyt5UnF2Zm8zUGpDcGZONEp4dUFBRzFhVTRKUFF1eEw2VVpCQ1lBOTVU?=
 =?utf-8?B?TENBcmYzeDFCeENQLzJUSGNlU2RkNGxsNTlSQjBqeUNxWm5RTjdlQnZsWUlE?=
 =?utf-8?B?eUJkeExuM241Y0krQ1RmcEpiMFBpekNjdUJqNmZjRjBOZjROYUVyaXQwZHJ3?=
 =?utf-8?B?Q2VPcHhic09ncTlsN29ubUNtMitHMWluQzFnaTh4TWxCckxQTTY3ZWxKbkQ1?=
 =?utf-8?B?M1BjQWRsMG9OQ1hFbUZmQ21DV0lVeFkrOGhPZ0VYd0tJeUhJenNmdnlWQ0Mr?=
 =?utf-8?B?WXdXOFpZcHFTL3VLdmxoK21RT2xBVi9zUmgwNWF6d0Nud2N5eTFSdy9BZVNN?=
 =?utf-8?B?dmNvL3dPRklHc1RQWEYwdUNvWjFDTHpPR05mMHVlc3lQTVlTUU9uWjBISFUv?=
 =?utf-8?B?V2g5VHVWNFZnbG0xenJuSjlFRjg3S2VDVk9id3RrZTA3dnZEd3VvM0VnVDRl?=
 =?utf-8?B?amRPdTdYekxvdnp4aDloWWxSd1g4eVZlTldiRUFIUVpZMmQxZGR2QldaMlN2?=
 =?utf-8?B?VHZWaEVudUFaRENNRnlXZDJYcHFFdno3a0J3OWx5RVRSdlpUM3RxMjQ0M0RG?=
 =?utf-8?B?dUV1N2VNR1dDS2MvTHJrUVY4UEEwcTNzRkRERzNDRS8wUEVUMUFDZGQ5MlBG?=
 =?utf-8?B?MlRPSE1QWXk1aHcwT2pJRll3aEFOYkpYYWtueUlROGtKMHdHS05sN0VNUFhu?=
 =?utf-8?B?dUw0REV4UVJOYkxJakRJYWxOODMzYnp1UnJ1cDNvTHVCZnJGWlBqRWxhMU5F?=
 =?utf-8?B?UHVZaklQWEVJWjhiY29aZWlvQzBWd2JKZmdsa1kyRjkvNHd0akNWK003ZWg4?=
 =?utf-8?B?VHg2eXdWUHZnWlFGZlVNL09ZbXFGdEYydXRWTUMvS1lVejQ0VDNKWTBZWlRh?=
 =?utf-8?Q?ZNHPUWXIxj9OPLfgviZwBAQHR?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 9a7983cd-e4fb-4a12-dbb4-08db03cac53f
X-MS-Exchange-CrossTenant-AuthSource: DM4PR12MB5229.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 Jan 2023 20:36:03.2505
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: VT6h/q7ShEp+y2cp2KUsxF8ET6ZPIZ5MD7suh7/iJ+Sv2Ptt0bQmjRlvPo6fogRUqUxfsUue4JwutDJ0XFfW3w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: LV2PR12MB6016
Precedence: bulk
List-ID: <linux-crypto.vger.kernel.org>
X-Mailing-List: linux-crypto@vger.kernel.org

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");