Received-SPF: pass (google.com: domain of linux-kernel+bounces-173789-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <d3b59450-fdbd-2813-bbc0-58f9dc2ba817@amd.com>
Date: Wed, 8 May 2024 15:09:02 -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>
 <07266b47e749267ef9a9ccbc9e8e9df78ed54857.1713974291.git.thomas.lendacky@amd.com>
 <20240508080509.GWZjsyNS0xBXUzKPUG@fat_crate.local>
 <22dc9ceb-4b16-be68-ed8e-c6a222619032@amd.com>
 <20240508195842.GIZjvZcpRz0bdkiPWA@fat_crate.local>
From: Tom Lendacky <thomas.lendacky@amd.com>
Subject: Re: [PATCH v4 05/15] x86/sev: Use kernel provided SVSM Calling Areas
In-Reply-To: <20240508195842.GIZjvZcpRz0bdkiPWA@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?aVppYldrU3lmVnQ1UEluR2lEUy80NDJZbVNSeGtaZm5wdGZjODd4ZksrQlBt?=
 =?utf-8?B?Wm9iQW1hQmNoWXhuOWxoNzlCZ2JXU1JJNlRxSmV3WENVTEZNYWhhUWFRd21V?=
 =?utf-8?B?bXBYWFVmUEdoL0VvSXA3cDZ6dXdNRDBWRjFsYzh5Y1BMOXZFQ3pKeEFma3p5?=
 =?utf-8?B?TkV5S0kyUHA0T0NvMmREMnVjTnZsZEp4cWxtN1MrSW0zQm1tSW1aN1ZIZmk2?=
 =?utf-8?B?RVVzVGNaOXhDdEhON01wNjNMY1BwS2swUXdjOU1ud2xISlFNUGMvM1puWDZ6?=
 =?utf-8?B?czdhQ29McHpFTlFUWTRlc2JINUFGRXJ0WjlnV2ViQ3Z5ejQyUGREWlhEMUJl?=
 =?utf-8?B?QVNhTG05SG8rZFBJZ0VPcEJhOXBBK0NIaUl4OE5BclRMNG4rTTM1Z21OaXVN?=
 =?utf-8?B?SERlTHZsZ0ZuQjUrK3JLWWsrR09JYWtsSXpHSHJhNVhYOVJmR29qa0QwNGRj?=
 =?utf-8?B?eHFsQ0czeXI3RG5uZjFJaTJyT2xjODNwWDF0NXBWbW5CMHFrODZ1QndkMWQx?=
 =?utf-8?B?QmJLcmlGSUlOS0dwR3ZHaVRwendGYjduSXN5ZGZlK0I5Q0R1bEs2b29obGhE?=
 =?utf-8?B?aUx2UDhoRU9oSXVIWlN6ak9MZ3cyb0NPSHRtUUdHV2dWK0NoU1RGaUFiMllv?=
 =?utf-8?B?OXFJSEliQytBZVpPQmw1SkdUSmYzU1NIbUYzNXBHbVYydDdNVmZoRGlPdG1U?=
 =?utf-8?B?OFl0ZU1JMDkvejVZdFR4TzFCRHE1ZUNUM2N0cEQ0emZUNm5qQVg3TWNoUWhV?=
 =?utf-8?B?TUgzVHFEdlRtUnVZNlhNOStTeUtHM2gyM1R5elA4eGFQYlRnUTJ1MW83RC9K?=
 =?utf-8?B?VEEyb0ZsRlpKQ3BQeWhZZTFRdFBNUnNwWTdDWFJSbllRQXFxK0hjN0JsSmFq?=
 =?utf-8?B?MGc4OUtkVUZWTW9kekRFeDRjaWhZVXQraHNER1U0L280TXF2QVpQMkNrc2dx?=
 =?utf-8?B?RVhUZC9wSnJDcThlUHhxRitaTno3dGlER1MrT2pXYlhOVkhQNk9QTll2cHRj?=
 =?utf-8?B?RTlBME1qZVBhOEtqTTRBMHRFM08zSVNxWVg2aTFmQnVaZWVCdXlLYmtXOFky?=
 =?utf-8?B?TGp2QjNFdnFxcXdQalE0UnlIdFdnK0dVTDAwdjhxMlZucGViWkNjbUpWSnM4?=
 =?utf-8?B?Ynlsd1Z0RjJSL29XTW9xbHpFbjJ1eVYrMU1ncERCNHZNcDY1cEdvUHRVSGhV?=
 =?utf-8?B?alFmejJZeSszVnFEM2twckttQjdaZm5YY0RXei9MMi8zQkNSTzVqbU5LTnZ0?=
 =?utf-8?B?QUg1NFp1U2VoK0toc1h4SWxQZVkzY3UrK3JVWUJ2SStJL3IyNktiTkpDTDg5?=
 =?utf-8?B?b25oTVMxRVNENGYvSE1DM1VHWjZReFBqM05nSWxpYkgxVHZ0TTY4TkQ0eGpi?=
 =?utf-8?B?SEF5QWVzS29jbmpra0laUWhqZmc1cG5Vd2JUOEJKQlJVWlByYnQ3MjhXMHV1?=
 =?utf-8?B?Zm1NZjZvaitoY05JTHhEQmlKTExTVlRuSWU5UG9zbnFBSWxIaEthNUhuQ2pj?=
 =?utf-8?B?SVI3UUorSWZIYkYzb1BZTVo1bWYvZVI4ejMwTmwwRGxjWXNOR1UwVlJiQWdh?=
 =?utf-8?B?Y2k0VitoNlpUMGJ2bUJTWmhEdUU2ZmM2OGhSUFQwOHY5b1c3bHl0Z3pUKzRQ?=
 =?utf-8?B?dDdOLzJrak5aVmRiWk5IMXFHQTcybTh4MDhNR1ZiNkE4TU5uSFBDN2dEMDZu?=
 =?utf-8?B?djl6VWp5VE5ZNGFCY21FekJSQlExcEJnM3hjWkwyY0hVRHpDOGIzblVVaVM3?=
 =?utf-8?B?MVNYY2R2T1N2a2FndTlnSm9oaGpkQVF1VVVXbkVKd0ZoUzg2V0NEek1ZUysz?=
 =?utf-8?B?TXZaaVo5b253bG96c1M1U1VUR1JTckE3TGh2djFrektUMkxqcnM0YjBsREJC?=
 =?utf-8?B?ZDVILy92Z3Bnak5EU3VlWkc0cjRJZm1vbklGMXhwZnNwYTFxQnBQcDFPUFpl?=
 =?utf-8?B?VGloeWtpMEhWR1psTVlrNW05Z3hsNHlLSHpTd3k5VjZEdmQ0VE5oTXdBcW5M?=
 =?utf-8?B?Y0hiaFJtUW5aT3AyU3RqMHNPZFNWd2E2cnM0TExLUUNDUm85RjRPYmpHQnhw?=
 =?utf-8?B?bmdMVy9SWkYzZkpwNGZ2a3kzbE5HeGtUbjF3WEFOU3JFOStnK0tBTnBQWlFZ?=
 =?utf-8?Q?MdCcOdPibU2hyQ7Y3CzEhRex5?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b3205069-08ef-4e68-f2cf-08dc6f9ab60f
X-MS-Exchange-CrossTenant-AuthSource: BL1PR12MB5732.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 May 2024 20:09:05.2022
 (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: 8cPhO3wV7UjBn1q6DLatU+AsgJTCmpCkjPgl8S80UuMj2oBfF9yOW6QujUasiQGcyIwqD+9QeBRsr/qugRuF1Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN0PR12MB6004

On 5/8/24 14:58, Borislav Petkov wrote:
> On Wed, May 08, 2024 at 02:13:17PM -0500, Tom Lendacky wrote:
>> ok, maybe __perform_svsm_msr_protocol or such.
> 
> We'll bikeshed it in the coming weeks.

:)

> 
>> There's quite a bit of interaction so I'll make sure to prefix everything.
> 
> Ack.
> 
>> The paravirt versions of local_irq_save and local_irq_restore can't be used
>> as early as this routine is called.
> 
> tglx says you should do native_local_irq_save()/.._restore() helpers
> just like the arch_local_irq_save()/..._restore() ones but use only
> native_ functions without the paravirt gunk.
> 
> In a prepatch pls.

Will do.

> 
>>>> +	struct svsm_call call = {};
>>>> +	int ret;
>>>> +	u64 pa;
>>>> +
>>>> +	/*
>>>> +	 * Record the SVSM Calling Area address (CAA) if the guest is not
>>>> +	 * running at VMPL0. The CA will be used to communicate with the
>>>> +	 * SVSM to perform the SVSM services.
>>>> +	 */
>>>> +	setup_svsm_ca(cc_info);
>>>> +
>>>> +	/* Nothing to do if not running under an SVSM. */
>>>> +	if (!vmpl)
>>>> +		return;
>>>
>>> You set up stuff above and now you bail out?
>>
>> setup_svsm_ca() is what sets the vmpl variable. So nothing will have been
>> setup if the VMPL is zero, in which case we don't continue on.
> 
> You still assign
> 
>          /*
>           * The CA is identity mapped when this routine is called, both by the
>           * decompressor code and the early kernel code.
>           */
>          boot_svsm_caa = (struct svsm_ca *)caa;
>          boot_svsm_caa_pa = caa;
> 
> regardless of vmpl.

If we're not running at VMPL0 (based on the RMPADJUST check) and if the 
SVSM doesn't advertise a non-zero VMPL value, we will self-terminate. So 
those values are only set if we are not running at VMPL0 and the SVSM 
has provided a non-zero value to us.

I'm going to turn the function into a bool function so that the call 
becomes:

	if (!svsm_setup_caa(cc_info))
		return;

> 
> I think you should assign those only when vmpl != 0.

I do. I think you're missing the RMPADJUST check that causes the 
function to return early if we're running at VMPL0.

> 
> Otherwise the code is confusing.
> 
>>> Judging by setup_svsm_ca() you don't really need that vmpl var but you
>>> can check
>>>
>>> 	if (!boot_svsm_caa)
>>> 		return;
>>>
>>> to determine whether a SVSM was detected.
>>
>> Yes, but the vmpl var will be used for attestation requests, sysfs, etc.
> 
> I guess that comes later in the patchset...
> 
>>> Huh, setup_svsm_ca() already assigned those...
>>
>> setup_svsm_ca() assigned the ones from the secrets page. The kernel now
>> switches to using its own CA.
> 
> Comment pls.

There's a block comment above it all, but maybe it isn't clear enough. 
I'll rework it.

Thanks,
Tom

> 
> Thx.
>