Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <77cbdadb-b86c-f8ab-4992-f1963a9fa1e9@amd.com>
Date:   Thu, 4 Aug 2022 09:59:44 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH] crypto: ccp: Load the firmware twice when SEV API version
 < 1.43
Content-Language: en-US
To:     Jarkko Sakkinen <jarkko@kernel.org>,
        Paolo Bonzini <pbonzini@redhat.com>
Cc:     Jarkko Sakkinen <jarkko@profian.com>,
        Harald Hoyer <harald@profian.com>,
        Brijesh Singh <brijesh.singh@amd.com>,
        John Allen <john.allen@amd.com>,
        Herbert Xu <herbert@gondor.apana.org.au>,
        "David S. Miller" <davem@davemloft.net>,
        "open list:AMD CRYPTOGRAPHIC COPROCESSOR (CCP) DRIVER - SE..." 
        <linux-crypto@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>
References: <20220804010223.3619-1-jarkko@kernel.org>
From:   Tom Lendacky <thomas.lendacky@amd.com>
In-Reply-To: <20220804010223.3619-1-jarkko@kernel.org>
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?Y24xdUcwMS9MMlNGbmUzV3AwZkI0VXpuSWttcVlBa0UvN3ByKzR0YU0zbTZ4?=
 =?utf-8?B?cENBM1dRdmU3MXFaSExPM2dYWTBOQmRpUFQzeUsrblBxZXdKMWN5QVd5aWNT?=
 =?utf-8?B?UnZjVWROeUVMbUJ4Vlk0WlBGYitTeVJIUmRIMnJJTXRDTFJXTTRrQ0NvU2NU?=
 =?utf-8?B?RUhMVllyS1Jnbzl3UTJTRTdEblJwd0IrU25GK3NvR3lpYk5zMHIwVzlDNVJJ?=
 =?utf-8?B?ZDJRRlRpd3libWhIbnB5ZnMzUDJVRnpQVlJLSzI5bEhCMFRFOE5GaU8vcXky?=
 =?utf-8?B?YU84cnMwNjFsbzFlQ3RjSS9TS0s5b2VIdEFBWGx2bmIvd295WHBMMVNTM2hk?=
 =?utf-8?B?Mi80MVRRUkwraVpOd2tHdlo4NFR5SUwrU0ozeG1yTUhLM0F0bTFBMmxzbjJN?=
 =?utf-8?B?YVJYZEtqTnpCVG1icUwrTHlkK2drdC9NS2FNMnBUaVRUMVZDcUp0SHBKUnFW?=
 =?utf-8?B?MGRsbjhPU25NWUcvNklZVTFhZFhLTnNTblJLa1VSL1l4UnZqNlVneWRaWmZY?=
 =?utf-8?B?aFJKSXN6akIrQnJNaFJYcHZIL3hKay8raHBETHBHSnlvL01WNmh0Y0V2YVNU?=
 =?utf-8?B?cnp3UjN6dnR4VTJZcytKUDNOWWR6QXJVczlrUkpITVlKTkpPWWR1RWc1aXlQ?=
 =?utf-8?B?dC9xYUI1T3p4akduMWxFTnVtRWZHYUlCRUZXL29TSG54cnQ0MXNuUTR5TENI?=
 =?utf-8?B?UGo4dHZBK2xkSmFmazczb2RUbHBVdTBXNlZBMnN3THNLb09tWnhGSEw0cGhX?=
 =?utf-8?B?MHhTK3preGtkd1c2U1dNYWI4M3NNM2JaZHZ3eVVwTkVrMHFBbHgwMWdiUDg1?=
 =?utf-8?B?dThyVVFpWm5xTnJQTllJMThFcHh0RUhYMHRjbEt6M0ZOdVd4eU5MR0FNZU50?=
 =?utf-8?B?bVNWVFhJTzRDTzErOXM2NklNeS9TTWNFa0JZeVNwbWdQODVlYjBtWmt3K2sx?=
 =?utf-8?B?TWNiQ3BiOEhabktPTkZMVVlVcFVnY2FROGJZb1M2UWp5VXYySUFzWTVXQjNI?=
 =?utf-8?B?NVIxRjkyejZITm5FdUgzbklHbTJOTW9qWTdkK2tqWG5Eb3U3NU4vUG15bGFE?=
 =?utf-8?B?bGJoWkpBUkxmSkNMc3d1MTdMUmlMM3RSeUNKTGJTR011QzFuMGhEK01IOUFa?=
 =?utf-8?B?bDdvUmNjZGxockV1S3JHS3VicU0xZmd4czhydEpuUzlFZmNaL1V3ckhzQXl6?=
 =?utf-8?B?Qm1TYU5TNWNMaXdjZ0hrSUoySU8xakNqUFFDbS9lZXhjUDRCQzROOTdqUWI4?=
 =?utf-8?B?RVp4anZZQjQrMThnNmlveXZHL2UwcXo5SHNocDlmaElzeTFaemRHd1dyb2Yw?=
 =?utf-8?B?MDdkelJPaE5BczdEV3h6YzYyYlFtR1RpQk9YTS9RTGF2Zy9MYTJKZjdmeWxr?=
 =?utf-8?B?VnFyaDAvN1dMbUhhNzdwRk03UDBNcHFSNDFpZ3U3cEJxbVFMK0RoMkQxUnJR?=
 =?utf-8?B?WWVsajIwSVhST1I5YXdrWHFrcnV5MWxBWFBVeEtSQ25NcWtvMm9KZDdGdFNS?=
 =?utf-8?B?SUhEdGpoSUFXRWNDYzRZc2JYQkhBWjhZbnIvN2UxU3REMEhhbmZyNVZEVmlh?=
 =?utf-8?B?NDVLSDgwTHZYdFlMNHRlWnZVYThZQUw4eEdnMElRQS9uWU12MWRBdUNkcElT?=
 =?utf-8?B?blZmMjU1ZWpETy8wczlDbzZXVlJId2padVhEMFBZUnFPalE2VUVXQTYrcVVS?=
 =?utf-8?B?dXl4NVpuRzJOU1BqcnpnRGlaUnd0VDZxZVUxR21tODRualdaOVo2WEdVSnJm?=
 =?utf-8?B?dmt2WEpyNG40d3BIdktKZlBJUFNpQUtLZ0ZLTnFCc3VPNU1zZTYxbHplQ05M?=
 =?utf-8?B?Qk9yeXVMMGlBVEIralFHZWhlZ1Q1WlcxQyt1K29BTSt4bU5qcVZJRFdueGYr?=
 =?utf-8?B?WVllQlE3bUVYM2JoTVpEUVhab0xEZmd0aDVWbVp6a3B1bWhqMXNZUy9DVVZq?=
 =?utf-8?B?NHBkbVdTVFlHVkpkR0ZuUFE0OTdialAzZ1lhUllCUmord0VQNmh6aGkrcFlK?=
 =?utf-8?B?NC9JSnRHZTUwUmEwNUplV1VZM2ViaURGdldhS2didzJTN2txdTdTQitwSW1M?=
 =?utf-8?B?ZXBuMkM5S3Z4NzB0R1krZTRxWFZEekNEVTZHNlFvRUpYYzZ0a2dUSmpmTXo2?=
 =?utf-8?Q?Hu1ayRDJKvKiLnVk+kKA1KLnI?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 35679454-85d6-43b9-b781-08da7629f8e9
X-MS-Exchange-CrossTenant-AuthSource: DM4PR12MB5229.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Aug 2022 14:59:46.9399
 (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: ghSgqSaEQp9B1Lq51RHGBFDS7vgQOX5OMB5uxJP+snzAfm3re2WNUsDfNPrggjQlo50VeGZSMONjR/kFyUJs5Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR12MB4385
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-crypto.vger.kernel.org>
X-Mailing-List: linux-crypto@vger.kernel.org

On 8/3/22 20:02, Jarkko Sakkinen wrote:
> From: Jarkko Sakkinen <jarkko@profian.com>
> 
> SEV-SNP does not initialize to a legit state, unless the firmware is
> loaded twice, when SEP API version < 1.43, and the firmware is updated
> to a later version. Because of this user space needs to work around
> this with "rmmod && modprobe" combo. Fix this by implementing the
> workaround to the driver.
> 
> Reported-by: Harald Hoyer <harald@profian.com>
> Signed-off-by: Jarkko Sakkinen <jarkko@profian.com>
> ---
>   drivers/crypto/ccp/sev-dev.c | 22 +++++++++++++++++++---
>   1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 799b476fc3e8..f2abb7439dde 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c

> @@ -1274,6 +1285,7 @@ void sev_pci_init(void)
>   {
>   	struct sev_device *sev = psp_master->sev_data;
>   	int error, rc;
> +	int i;
>   
>   	if (!sev)
>   		return;
> @@ -1283,9 +1295,13 @@ void sev_pci_init(void)
>   	if (sev_get_api_version())
>   		goto err;
>   
> -	if (sev_version_greater_or_equal(0, 15) &&
> -	    sev_update_firmware(sev->dev) == 0)
> -		sev_get_api_version();
> +	/*
> +	 * SEV-SNP does not work properly before loading the FW twice in the API
> +	 * versions older than SEV 1.43.
> +	 */
> +	for (i = 0; i < sev_version_greater_or_equal(0, 15) + sev_version_less(1, 43); i++)
> +		if (sev_update_firmware(sev->dev) == 0)
> +			sev_get_api_version();

I prefer to have this logic in the sev_update_firmware() function.

And while the loop is correct, it isn't obvious. Something in
sev_update_firmware() that just does:

	ret = sev_do_cmd(SEV_CMD_DOWNLOAD_FIRMWARE, data, &error);

	if (!ret && sev_version_greater_or_equal(0, 15) && sev_version_less(1, 43))
		ret = sev_do_cmd(SEV_CMD_DOWNLOAD_FIRMWARE, data, &error);

And place a comment before the second command that references the reason
for the second download.

Thanks,
Tom

>