Received-SPF: pass (google.com: domain of linux-kernel+bounces-193035-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <4f704d19-088a-89d7-0515-144db61b93a5@amd.com>
Date: Tue, 28 May 2024 15:28:28 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Content-Language: en-US
To: Borislav Petkov <bp@alien8.de>
Cc: linux-kernel@vger.kernel.org, x86@kernel.org, linux-coco@lists.linux.dev,
 svsm-devel@coconut-svsm.dev, Thomas Gleixner <tglx@linutronix.de>,
 Ingo Molnar <mingo@redhat.com>, Dave Hansen <dave.hansen@linux.intel.com>,
 "H. Peter Anvin" <hpa@zytor.com>, Andy Lutomirski <luto@kernel.org>,
 Peter Zijlstra <peterz@infradead.org>,
 Dan Williams <dan.j.williams@intel.com>, Michael Roth
 <michael.roth@amd.com>, Ashish Kalra <ashish.kalra@amd.com>
References: <cover.1713974291.git.thomas.lendacky@amd.com>
 <aa7f311d90efb49dfa6f4589854ee43c049b7b88.1713974291.git.thomas.lendacky@amd.com>
 <20240527123304.GBZlR9gBzBIjqEKKo6@fat_crate.local>
From: Tom Lendacky <thomas.lendacky@amd.com>
Subject: Re: [PATCH v4 07/15] x86/sev: Use the SVSM to create a vCPU when not
 in VMPL0
In-Reply-To: <20240527123304.GBZlR9gBzBIjqEKKo6@fat_crate.local>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?dVJHdEJ0K0FMU2JZUitWT2pDMFYyQXVZdm1nVzFVVEJZQ3Y4UEZDbTZFWGFr?=
 =?utf-8?B?bEpOTXBaWU50L1ArS0d6Y1l3Zzk2M25pUTVxZU9IK3dKSGFkUFlXVVVIQUhF?=
 =?utf-8?B?ajR1eHh2UGtqOElMdERIMjV0ejNDT1R6djJkZTQ2azlIdTRjeWxEZkZ5SHI2?=
 =?utf-8?B?N0NxYlhsc0VXMkFjb2NOQ1FJVEFKRUc1cDAxY2g1RjViNXltN2RzWTltdVRF?=
 =?utf-8?B?dzhWV3VBQ2VMbnY4T1RyUTZBNzNBa3kxYjhRZVg5Unk1MHRWbE1VK01wc2I5?=
 =?utf-8?B?WTArZGdOWEZFekt1dWRHdUg1bFFQeEhwSTdYVm1OcFJ2OTNuaWdCb2lyZ3Z0?=
 =?utf-8?B?L2hpM1FNdnNmNFdOYXFWSlhCaTN2YnVxVGQ3T0VJeDV3ekRaa2tMVFZlNVFo?=
 =?utf-8?B?YlJhQTNEbFJyeGlpSTdrMmRONTUvb0FobVozQk0vbk1DUmtXNkJXMVpSTklP?=
 =?utf-8?B?YkRCckdDVDJtaDlNQmpLcm5SNjE0VDRnZGdwemtCSW1CLzZLbTJURnUrSWxM?=
 =?utf-8?B?Qit5VElya1MwWkpaSUtDOG5Tejd2M3pSVTJlUXlZb3R6YzB4TFNNalI3K3BN?=
 =?utf-8?B?OStwTVJ6QTIyRWxOUHpaNzBjcjd1OVdQakpVeVg3ZXI5eUN2azZlazdscmw5?=
 =?utf-8?B?NW00Zms1ODV2QktXbldHcThWWVVTOEFqMFR4dVVhRENIVWhuSGxFYXBKY3k1?=
 =?utf-8?B?OTlzeS96aGJtTS92NERFU3BYWElpR3Y0Yk1ERjdPWjdmanhHUjFEQk5GRlFT?=
 =?utf-8?B?RXJtNnlySnMxWTUzMnQyVkpqL1pHOWJPQTVVUlpodS94VG1uTkJPMXVVa0lS?=
 =?utf-8?B?eWJ1SmRJa001Y2I5ZUNVTVdjU1pOR0MxeEZIcjNJMzQ5NVRZajlBVVQ0aHg2?=
 =?utf-8?B?aklnaG9hbkcyM0MrNWVOMk1zMzU2cTJLaWE1MHRxWGtvT2VvbWJkcnlFL0VO?=
 =?utf-8?B?TkhJdkhZQUJKWm1xWmg5NkR0d3VMRTVYNW1TMzNySnh6SlpKWXVYWGViemlJ?=
 =?utf-8?B?TFRtaEZ5QzBpWmJweG1rTWsvMXdnVUpBTVg1Z1V5dmJtVHpYLzl5VjREK3ps?=
 =?utf-8?B?d2Y2VnVmOGJNR0lzT1hFajU4YTl4ZFhoMDVDRzRrblc1a29uanZhdXJqU3Zm?=
 =?utf-8?B?MVN6MFVJRVZTM25SeVozaFhwVWhaRklIbk0rc3pHOVhPSlM5ekNjcThBQ09a?=
 =?utf-8?B?VXdVWDUwa2krL0RYOEVINlBnNHdmdHJhd0lZK21WUVNJY3FtcDR4STZpWHJT?=
 =?utf-8?B?bUVJVVkrcHEwTXgrMU9VNGgyMnhsZEoyQldwa3ViMEUvdGdDTUFYR0lDVFZS?=
 =?utf-8?B?blN4Q1k2L2xFS2F5NkhtcVNtQWFDSmdlOEdCb3VGeUs2eDREaFVQNlhZaG40?=
 =?utf-8?B?YUFJRnVBQmNROFRWZEp5b0FROXJIVnludHlMOGJXRkhQVHUzb0FsdFNQQzgy?=
 =?utf-8?B?RjJhR2k3ck1pUWkwMVRyVzNwZGNYT0xxZnVBeTdLZkthdHBDOG0vQitxdFVn?=
 =?utf-8?B?Y005UzVvcVNaNGxzZnJvaE1HMHlBODlTM09tSG4zdXA0K0tGd1E4RVRhT3lE?=
 =?utf-8?B?VS9tVmt3UW1RenlMTDdKUzEweG4xL0dJMDl1cnBvY2FCYXpnaXM4d09sb3Y2?=
 =?utf-8?B?S0tjS0hOdCtuZjlKcVY0dk94S3RvOGJrbUxMcEZvek56TWdZdFU2cXE3b3dV?=
 =?utf-8?B?T01YNmdDOUVLRThWbktFSHhsTlY5SDBDc1lKY3Ewc05nSWlrMjd4SzdLcXpI?=
 =?utf-8?B?dG1BUDlWd2pxL0IwMmxoS1hVQnNRbnFReXN5d2g0bGEwbXpmbC90U0xEOHVE?=
 =?utf-8?B?am5ZTnA5ZUdqV2lkNEsvSWhycHdER2x4OEdtbE1qN3FIc0MwWk9NbkIrVnVW?=
 =?utf-8?B?NS9lNzY2Y29aeUR1S0RwV2lXeVhRaElqUDJNbm1Ec3J4RGxIMGh3di9JWHpU?=
 =?utf-8?B?d0ovTENwTEFFMHVRMkNVTm8rOFhabmlYRXdiMlhQT3hNTTY5YWlxS0dKeEdL?=
 =?utf-8?B?UzMrKzY1U0RPRVl1QlVicWlJYlBCeXkxM09vR0ZoS1R6VCtaUWN6TXZFSkov?=
 =?utf-8?B?YUpBWjR1MHFnTG1ESDI5YUh3TlZUY21lb0VSUHhNWDk2UFVqYzJ1YlIvKzJ3?=
 =?utf-8?Q?EKBCvjEziZWq4xtqH5YC6uTBT?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 5dbd0f7c-534c-4a6c-3d1f-08dc7f54bd07
X-MS-Exchange-CrossTenant-AuthSource: BL1PR12MB5732.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 May 2024 20:28:30.6714
 (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: eacWYjqnCnypuUGzdnBfRBjde/sil+sBiu31leDPZtlUq76AkJCsn+cX4xTy54Guogmy53koV9BbAH3gs9Kj0A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY5PR12MB6407

On 5/27/24 07:33, Borislav Petkov wrote:
> On Wed, Apr 24, 2024 at 10:58:03AM -0500, Tom Lendacky wrote:
>> -static int snp_set_vmsa(void *va, bool vmsa)
>> +static int base_snp_set_vmsa(void *va, bool vmsa)
> 
> s/base_/__/

Ok.

> 
> The svsm_-prefixed ones are already a good enough distinction...
> 
>>   {
>>   	u64 attrs;
>>   
>> @@ -1013,6 +1013,40 @@ static int snp_set_vmsa(void *va, bool vmsa)
>>   	return rmpadjust((unsigned long)va, RMP_PG_SIZE_4K, attrs);
>>   }
>>   
>> +static int svsm_snp_set_vmsa(void *va, void *caa, int apic_id, bool vmsa)
> 								  ^^^^^^^^^^^
> 
> bool create_vmsa or so, to denote what this arg means.

Ok. I'll change it on the original function, too.

> 
>> +{
>> +	struct svsm_call call = {};
>> +	unsigned long flags;
>> +	int ret;
>> +
>> +	local_irq_save(flags);
>> +
>> +	call.caa = this_cpu_read(svsm_caa);
>> +	call.rcx = __pa(va);
>> +
>> +	if (vmsa) {
>> +		/* Protocol 0, Call ID 2 */
>> +		call.rax = SVSM_CORE_CALL(SVSM_CORE_CREATE_VCPU);
>> +		call.rdx = __pa(caa);
>> +		call.r8  = apic_id;
>> +	} else {
>> +		/* Protocol 0, Call ID 3 */
>> +		call.rax = SVSM_CORE_CALL(SVSM_CORE_DELETE_VCPU);
>> +	}
>> +
>> +	ret = svsm_protocol(&call);
>> +
>> +	local_irq_restore(flags);
>> +
>> +	return ret;
>> +}
>> +
>> +static int snp_set_vmsa(void *va, void *caa, int apic_id, bool vmsa)
>> +{
>> +	return vmpl ? svsm_snp_set_vmsa(va, caa, apic_id, vmsa)
>> +		    : base_snp_set_vmsa(va, vmsa);
> 
> Why do you even need helpers if you're not going to use them somewhere
> else? Just put the whole logic inside snp_set_vmsa().

I just think it's easier to follow, with specific functions for the 
situation and less indentation. But if you want, I can put it all in one 
function.

> 
>> +}
>> +
>>   #define __ATTR_BASE		(SVM_SELECTOR_P_MASK | SVM_SELECTOR_S_MASK)
>>   #define INIT_CS_ATTRIBS		(__ATTR_BASE | SVM_SELECTOR_READ_MASK | SVM_SELECTOR_CODE_MASK)
>>   #define INIT_DS_ATTRIBS		(__ATTR_BASE | SVM_SELECTOR_WRITE_MASK)
>> @@ -1044,11 +1078,11 @@ static void *snp_alloc_vmsa_page(int cpu)
>>   	return page_address(p + 1);
>>   }
>>   
>> -static void snp_cleanup_vmsa(struct sev_es_save_area *vmsa)
>> +static void snp_cleanup_vmsa(struct sev_es_save_area *vmsa, int apic_id)
>>   {
>>   	int err;
>>   
>> -	err = snp_set_vmsa(vmsa, false);
>> +	err = snp_set_vmsa(vmsa, NULL, apic_id, false);
>>   	if (err)
>>   		pr_err("clear VMSA page failed (%u), leaking page\n", err);
>>   	else
>> @@ -1059,6 +1093,7 @@ static int wakeup_cpu_via_vmgexit(u32 apic_id, unsigned long start_ip)
>>   {
>>   	struct sev_es_save_area *cur_vmsa, *vmsa;
>>   	struct ghcb_state state;
>> +	struct svsm_ca *caa;
>>   	unsigned long flags;
>>   	struct ghcb *ghcb;
>>   	u8 sipi_vector;
>> @@ -1105,6 +1140,12 @@ static int wakeup_cpu_via_vmgexit(u32 apic_id, unsigned long start_ip)
>>   	if (!vmsa)
>>   		return -ENOMEM;
>>   
>> +	/*
>> +	 * If an SVSM is present, then the SVSM CAA per-CPU variable will
>> +	 * have a value, otherwise it will be NULL.
>> +	 */
> 
> 	/* If an SVSM is present, the SVSM per-CPU CAA will be !NULL. */
> 
> Shorter.

Yep.

Thanks,
Tom

>