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: <fd23ee51-ec47-717d-7cce-1d79db8b6bd3@amd.com>
Date:   Thu, 5 Jan 2023 17:37:20 -0600
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.2.1
Subject: Re: [PATCH RFC v7 37/64] KVM: SVM: Add KVM_SNP_INIT command
Content-Language: en-US
To:     Jarkko Sakkinen <jarkko@kernel.org>,
        Michael Roth <michael.roth@amd.com>
Cc:     kvm@vger.kernel.org, linux-coco@lists.linux.dev,
        linux-mm@kvack.org, linux-crypto@vger.kernel.org, x86@kernel.org,
        linux-kernel@vger.kernel.org, tglx@linutronix.de, mingo@redhat.com,
        jroedel@suse.de, thomas.lendacky@amd.com, hpa@zytor.com,
        ardb@kernel.org, pbonzini@redhat.com, seanjc@google.com,
        vkuznets@redhat.com, wanpengli@tencent.com, jmattson@google.com,
        luto@kernel.org, dave.hansen@linux.intel.com, slp@redhat.com,
        pgonda@google.com, peterz@infradead.org,
        srinivas.pandruvada@linux.intel.com, rientjes@google.com,
        dovmurik@linux.ibm.com, tobin@ibm.com, bp@alien8.de,
        vbabka@suse.cz, kirill@shutemov.name, ak@linux.intel.com,
        tony.luck@intel.com, marcorr@google.com,
        sathyanarayanan.kuppuswamy@linux.intel.com, alpergun@google.com,
        dgilbert@redhat.com, harald@profian.com,
        Brijesh Singh <brijesh.singh@amd.com>,
        Pavan Kumar Paluri <papaluri@amd.com>
References: <20221214194056.161492-1-michael.roth@amd.com>
 <20221214194056.161492-38-michael.roth@amd.com> <Y7BG6pSuoZsBQYrx@kernel.org>
From:   "Kalra, Ashish" <ashish.kalra@amd.com>
In-Reply-To: <Y7BG6pSuoZsBQYrx@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?OWkza25QdzJYWXA0UTJySUdyZDN2RTRSM01VdldxN28reW5oaGtFREdoQkpH?=
 =?utf-8?B?ZjhKb2hhK3BscHR5d0lvT3EyMmU3b0lwbVd3YVVsSFAzTDNWVnhsbzdudy94?=
 =?utf-8?B?OVNoQ2x6RUFmYXRBWUVoRWx5cWRad0g2OWlqY1dqZC9RR3QzeVpmc2RDQVNX?=
 =?utf-8?B?eFNPS25ESFEzRXZYVjVMUTZ3S3FGMzA1OVl4ckQ3QXhXQi9yZzJ0VWRDd2Fm?=
 =?utf-8?B?b1Vib2xWZjdxSVFqT3NjOE1ZUXpIOGZRY3dQemJmOVF1RmJCTVlnd1RHMURi?=
 =?utf-8?B?aURSS2c3VGphVEYvcjZPekR2MWlDb2ZXSmY0eUc2UkhMd0MrOHA4WVQycjUz?=
 =?utf-8?B?Z0VlalhnTE5TeXJVQjVONkhHYStXNTd6SFRrSVVLZUZ1QzB3NTQzWUhHbGZo?=
 =?utf-8?B?NktmZDVzdXE5NThIMnFyU1N1VXM2N2MrKzgydjdGSmhod2xEY2VYZ2JvQUpO?=
 =?utf-8?B?U1I0VkhONjVrd3N1QzExK0FHRUZRMUdTaWozT0Y0WCszMEFmTFhDclBSbUVG?=
 =?utf-8?B?K0VqZFJzSkhOU2ZDTW9TRU1EWEJRcS90Q2RwM2NUZ0sxVEdBWXJlajNaL3Yx?=
 =?utf-8?B?ZEptWU50NmFvY1R0WjJCL0FUc2U2ZHRubXFXVW8rR0pYa2FhdzVuVmtZa3lo?=
 =?utf-8?B?QWQyQzZOdGtnc3B1RWZZMndkdVFzc3UvczFEKzBrZVRkV25NRjl2WG13eXFC?=
 =?utf-8?B?OFhrUHI4NWtsdmZSOHpRQWZkSlUvR2tOeUNOK0p5VXpZS3U1YmMvMXRnSTY2?=
 =?utf-8?B?aTlKYkNGQzAzeXZZSDljUzE4bjltdDBZeE5hdlZFUXEyS25zNzVCYXRJSUta?=
 =?utf-8?B?eW5kdzQ3RHlvUzRSMXQvQ1pVbStBZFBQa3llMUJweWg1akN4RkdVY3ljaWFV?=
 =?utf-8?B?NGxVVCt4TDRvbkREYnl1QUYwZ2loMTdlaFlvMmpkVXBmeGJJRXFXdXh2T3k3?=
 =?utf-8?B?aDFXUUxDSUtVN3R1K1lyZFhJWU9xYlBkcTNSUjlseDVIejJDT2tiSVp5a21U?=
 =?utf-8?B?WnJLeVdMOFVaNnV0Sy9sZTE0OWdTamhMalU5NDZwRVNCSnpXWDczSGRVR0NL?=
 =?utf-8?B?c0tOb05OUXczdWRWL2piVUVibTZtTE5GMlowTE83RUN3dmxGL3ZlanUyYlZ4?=
 =?utf-8?B?L3lFQnpKZGJCRlB2UnljQjJpS2w4eENlZmdtdmNuN3NYUEpEVHh1RU4vb05r?=
 =?utf-8?B?UEpDL29IRERzK0xLQlUyL01MMGJUWGFFd1drMkwydS8rd1J6anlGNDZmYjFZ?=
 =?utf-8?B?bklMQVRGK0c0OHVrT01JbURJQVhwVzFmZTgyWnJJdk5BUUxZZEtxTkowaHM0?=
 =?utf-8?B?dE5sOUFOQVovTUZaQmlaaDdPamxJS0RVVk5WRWowcU4zTi83RkxFZmQvSXhM?=
 =?utf-8?B?YWN2N0lDWWVtYWhyY0NFN1A1RXd2dnNXVUZYR0UwWHd3K0RGbklrdU1LOEZ5?=
 =?utf-8?B?TWZSWldwSEl1L0VkTlRjM005ZW5OQUw5a1lhZmJIbWtLOE5aNGdCbTRRelpQ?=
 =?utf-8?B?OE5ETytleTI0RFI5cE1hVFczdlVQdW1vcW1HUmlJWk12eHJBQlNsSndsRnpn?=
 =?utf-8?B?VXFpRmd3d0o4QlRpRVVXbVBLYm1JdkR3Q2VJSXREWDdIUWwrU2hIR01UcTAr?=
 =?utf-8?B?MHZRSTlILy9BQktLcFNGczhPL3NkbURIUlpGOFhzRTFRRFRqY2FGYkFmWkJj?=
 =?utf-8?B?aFFWZWNSZkQ4SkdYN1Q5MDk4Z1MydkNWVDVreGlhd29HZjl4ajN5N2pRRnlW?=
 =?utf-8?B?Zml4cmJDVCs2VmswYmpaNXNQcDgzeDlIaGZoUXZ6MGtyb0tSeFNlUDcxYXEy?=
 =?utf-8?B?NmdDTitPTVNadkhlRExIM3BDUXdUSGlyeGU3aWxwbFAwNUUyc2wzOUF6VFVk?=
 =?utf-8?B?ekFRNFR4eDdNL3BNT09zbjZyWUtMSHpLK3NnSi9nYldoMFREUTRtRWZlNGhh?=
 =?utf-8?B?Zm0yZjhNYWRJSFVvcXdMWUJVMEpSZzFJbGxSOEpJMkJtVnE4NFJrRXBhQ2Zi?=
 =?utf-8?B?d3QydVN3S09DSkZsZkZ3TjBLZ0RkWDVwbjlvMUcxRFZPYUQvOXlVcUFja0Vy?=
 =?utf-8?B?OTBXTXhjM0Y4WmQ1b1p0bzdObGRvdnQ1aGZCaVZVZzQwZit5Z2lzUVRxMkp6?=
 =?utf-8?Q?wNXKwkXVaUwAJE2TZrbjDcvIv?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 85267bdd-0bdf-4333-40fc-08daef75ce03
X-MS-Exchange-CrossTenant-AuthSource: SN6PR12MB2767.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Jan 2023 23:37:27.4909
 (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: DdbeNcS+0iUuIkkfJ1HpQ2Vpi5eQ9mKfnbAFAVCe8Ab9kef7yBm+/03OsfX4qjBQ78C5nn2eo8uV3BKO3kodRw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR12MB5328
X-Spam-Status: No, score=-5.0 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

Hello Jarkko,

On 12/31/2022 8:27 AM, Jarkko Sakkinen wrote:
> On Wed, Dec 14, 2022 at 01:40:29PM -0600, Michael Roth wrote:
>>   static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
>>   {
>>   	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>> @@ -260,13 +279,23 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
>>   		return ret;
>>   
>>   	sev->active = true;
>> -	sev->es_active = argp->id == KVM_SEV_ES_INIT;
>> +	sev->es_active = (argp->id == KVM_SEV_ES_INIT || argp->id == KVM_SEV_SNP_INIT);
>> +	sev->snp_active = argp->id == KVM_SEV_SNP_INIT;
>>   	asid = sev_asid_new(sev);
>>   	if (asid < 0)
>>   		goto e_no_asid;
>>   	sev->asid = asid;
>>   
>> -	ret = sev_platform_init(&argp->error);
>> +	if (sev->snp_active) {
>> +		ret = verify_snp_init_flags(kvm, argp);
>> +		if (ret)
>> +			goto e_free;
>> +
>> +		ret = sev_snp_init(&argp->error, false);
>> +	} else {
>> +		ret = sev_platform_init(&argp->error);
>> +	}
> 
> Couldn't sev_snp_init() and sev_platform_init() be called unconditionally
> in order?
> 
> Since there is a hardware constraint that SNP init needs to always happen
> before platform init, shouldn't SNP init happen as part of
> __sev_platform_init_locked() instead?
> 

On Genoa there is currently an issue that if we do an SNP_INIT before an 
SEV_INIT and then attempt to launch a SEV guest that may fail, so we 
need to keep SNP INIT and SEV INIT separate.

We need to provide a way to run (existing) SEV guests on a system that 
supports SNP without doing an SNP_INIT at all.

This is done using psp_init_on_probe parameter of the CCP module to 
avoid doing either SNP/SEV firmware initialization during module load 
and then defer the firmware initialization till someone launches a guest 
of one flavor or the other.

And then sev_guest_init() does either SNP or SEV firmware init depending 
on the type of the guest being launched.

> I found these call sites for __sev_platform_init_locked(), none of which
> follow the correct call order:
> 
> * sev_guest_init()

As explained above, this call site is important for deferring the 
firmware initialization to an actual guest launch.

> * sev_ioctl_do_pek_csr
> * sev_ioctl_do_pdh_export()
> * sev_ioctl_do_pek_import()
> * sev_ioctl_do_pek_pdh_gen()
> * sev_pci_init()
> 
> For me it looks like a bit flakky API use to have sev_snp_init() as an API
> call.
> 
> I would suggest to make SNP init internal to the ccp driver and take care
> of the correct orchestration over there.
>

Due to Genoa issue, we may still need SNP init and SEV init to be 
invoked separately outside the CCP driver.

> Also, how it currently works in this patch set, if the firmware did not
> load correctly, SNP init halts the whole system. The version check needs
> to be in all call paths.
> 

Yes, i agree with that.

Thanks,
Ashish