Received-SPF: pass (google.com: domain of linux-kernel+bounces-197328-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <f3bed7b1-0044-0962-3094-2d7ae2ac3737@amd.com>
Date: Fri, 31 May 2024 14:03:21 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [PATCH v4 14/15] x86/sev: Extend the config-fs attestation
 support for an SVSM
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>
 <9a4c4a16d00834c1b7ff458e25c185ac1c9bcf79.1713974291.git.thomas.lendacky@amd.com>
 <20240531131619.GKZlnNo20bj98_ILiM@fat_crate.local>
From: Tom Lendacky <thomas.lendacky@amd.com>
In-Reply-To: <20240531131619.GKZlnNo20bj98_ILiM@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?bE1xMDFQZDFFN0JvaGh5dmRKK0dPTlYyT0RhQnVuTkJ1dmRhVGIyQklsVnlJ?=
 =?utf-8?B?Z0NqWldVLzVlYmZ3M2hTZGVtZmcvSGVsTkU1TXozTDNnVkU3QkJyclhqUHJ6?=
 =?utf-8?B?UnRlUHNVWldiYUh5TDNtaG1tU0VRQ2xIVlhySkNFWDlhdGU0YkwzM1FRUEh2?=
 =?utf-8?B?R0tVK1FuNmVvOEw0WHVOWFQyTlBRbmppWlRsMXVyNGNaUU9xdGo1ZVl4K1Zh?=
 =?utf-8?B?bEFUS3hPYTBiUGMwWFNDV0E0bUZlZE1jc1dqZGlESjRuZFFvU3lURm5nQUtU?=
 =?utf-8?B?aFZjazhYTlQ3TjFoMmtSSU1MYU9jY0lDYjRUQ3c4OGlXWWhGRE5BQ0w3dzR0?=
 =?utf-8?B?WEdzNWdjaWZkY3lKYzJ0a2JyUVJENlRSbm5BME8wbFdveW8vbEE2elc0c2Nh?=
 =?utf-8?B?ckR5cFAwM3ZhVVJudkpQRDJGNWNsWDFqTVdZb29MMUExR21NeFhwN09YUkdn?=
 =?utf-8?B?YUV3eldLanIrQXo1YmVRVUJNSjRlY3UyZzJobnA1bHRaOWM0SUxvenNCeUxW?=
 =?utf-8?B?WkhweEV6NXRGbnFxdDNYdmxwakFTWGozWGRxMUFLV3h2Sk9GTUNNZVMzc3NT?=
 =?utf-8?B?ZkdwcTUvb1lEeU5QeVZlNndwTzNUcGNTOVVZcjdoRmQxUUw3MW5hVkNnWTU2?=
 =?utf-8?B?dFc0RnlsVlVJOHBWVFFscVBIZFJlWm92bUtBY05FdWMwZlc5bVU1YkNsd1Rw?=
 =?utf-8?B?VGhxelpCSTZmS0ZlNlFOZndSUXNDeUlscFptcHJzVklPdnNGTXhTQlI3cExS?=
 =?utf-8?B?ckN3WGF6aWo1cTdUSzU3MVc2aGV0cGlNL2ZuaEFMNDBZSC93YVRZR2F3Z3JU?=
 =?utf-8?B?eXBiNG1YMnJvK0QrbW9FNmFESUs4a3BWdVE0MmxsaTg5K08wMkFjb01WUlN1?=
 =?utf-8?B?T0VKMzIrL1lieC9jYWNEcmluaXNnOW0vVUJ3eTU5V29KTTJSZjZXY2RXQ3JR?=
 =?utf-8?B?V09NVEhPaU5yR3lKWm5XT0Z3NDBqNHFIUi9URjNWV2lBc0tLa1BGby9YSHQ5?=
 =?utf-8?B?cEZpQTU4NFQ2ejdNa0VjWmxPUW1GVDczc3MxRVR2VXUrcXFKYnJxc3BJTm9m?=
 =?utf-8?B?bmFFQVRpZjdHc1ZMVG5NeXhwUHd1Q3Z5YUUvWWJ3Nm15WkJwNURxeEpVRi93?=
 =?utf-8?B?NFlmMmtnMUQxSjFTVWoxWDdTN0ZrQzBWWWcvZUFjdG0wZGtvdUx6elAybnlq?=
 =?utf-8?B?aUhaOUZGZTMrci9tVytPZU1yU29ZdnQ5eWlKZTcza0NJdmJiYmxvakxqSXBK?=
 =?utf-8?B?TXRTYXR5MDlPOVAyeG9ESVptQ0xuTmptQ2NjYXM4YWhqaGMxZEdXZHViZ0pU?=
 =?utf-8?B?Z0hqMGVieVFOcE1DVjg4elJTdjllOWZ1V29QNkhRRDl3d1J2V2hRZW13MmJ0?=
 =?utf-8?B?Y2hjSVVhQitDUXFZdFpxajA3VDU4ZWpzV0xiNUp3Mi9mM3EreCtCcFBDTXQw?=
 =?utf-8?B?WWhNeXI4czBNOFkyTWdUYjlBMm5HZTYrSGZPcWR2OE1Ham9LZGplVWRKcmU4?=
 =?utf-8?B?UXFmWitXMXIxRXlnN2tlQ0VQSWZVQ2FYN1VFK1I0bSs5Y3owWmhuM0tRR1ZO?=
 =?utf-8?B?bzdsWlMvMlkxdXViZzRNcXFaVENHYlg1Y0dnWnpZRTVQUVkxM0tWSVQzVjI2?=
 =?utf-8?B?Z1ZERUxwZ29Oak02K0RKaGJmeTloUkh3RCtSV2NSYTI1alpHSDg2T3ptRzZq?=
 =?utf-8?B?UzIzYUp4dzV0VUZyNFhaTC9UZ2ZBRGMwdGRnL0R6TEs5Z3Y1eGFoUklhdGlW?=
 =?utf-8?B?NWEvUklyLzhnbTJOZXNtSjAvbE1XWENJVDgvT1dsYUdmdnRjU2VQeTRrdWZI?=
 =?utf-8?B?NHdRU0NnaHhnb09ETFA2R2FxekZTYlYxNXY1ZEFkMVZtVk5UZHJpdHpyWU5I?=
 =?utf-8?B?SnJGa1M3a3NHdVZmbldYTXJDVXlveWNFMUJUcmxxYmJrTFZwZUN5dWw5Mi9D?=
 =?utf-8?B?cXJGN0RQL0RkeWZ4a3dvSHdvMmRoQU10YzVRNjUreHRuSjQzNEtITTNHc3pi?=
 =?utf-8?B?U1RHVlZjcHJYUkpGWUNBUlRjNENFT0t4QktVR1UxOUtyRGtoRGhDcFhTR1FQ?=
 =?utf-8?B?bjBRczBmL3NpcVFlYkdVaFpXQW9QTkIrSVJ1dEdJOVZOV3pyNXFUdThrcHBT?=
 =?utf-8?Q?U0qCLE6oQJqstzvsb3QBtRzKI?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 29ea3840-22ea-4e35-3759-08dc81a45862
X-MS-Exchange-CrossTenant-AuthSource: BL1PR12MB5732.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 May 2024 19:03:23.8547
 (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: a34TtWAiMvj6f74XRlgsp5SJJYGg8qt0Bb7ACM/LilQODHMbUxt4gWKW8KHvuQY5h1ESR2DzN9s9FcwyfgGBPA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY8PR12MB7123

On 5/31/24 08:16, Borislav Petkov wrote:
> On Wed, Apr 24, 2024 at 10:58:10AM -0500, Tom Lendacky wrote:
>> +/*
>> + * The SVSM Attestation related structures
>> + */
>> +struct svsm_location_entry {
> 
> "svsm_loc" should be enough.

Ok

> 
>> +	u64 pa;
>> +	u32 len;
>> +	u8 rsvd[4];
>> +};
>> +
>> +struct svsm_attestation_call {
> 
> Can we shorten all "attestion" to "atst" or "attest"?

Will shorten to attest.

> 
>> +	struct svsm_location_entry report_buffer;
>> +	struct svsm_location_entry nonce;
>> +	struct svsm_location_entry manifest_buffer;
>> +	struct svsm_location_entry certificates_buffer;
> 
> report_buf;
> nonce;
> manifest_buf;
> certs_buf;
> 
> Please shorten all those.

Ok.

> 
>> +	/* For attesting a single service */
>> +	u8 service_guid[16];
>> +	u32 service_manifest_version;
> 
> manifest_ver;

Yep.

> 
>> +	u8 rsvd[4];
>> +};
>> +
>>   /*
>>    * SVSM protocol structure
> 
> ...
> 
>> +static void update_attestation_input(struct svsm_call *call, struct svsm_attestation_call *input)
>> +{
>> +	/* If (new) lengths have been returned, propograte them up */
> 
> "propagate"

Fixed.

> 
>> +	if (call->rcx_out != call->rcx)
>> +		input->manifest_buffer.len = call->rcx_out;
>> +
>> +	if (call->rdx_out != call->rdx)
>> +		input->certificates_buffer.len = call->rdx_out;
>> +
>> +	if (call->r8_out != call->r8)
>> +		input->report_buffer.len = call->r8_out;
>> +}
>> +
>> +int snp_issue_svsm_attestation_request(u64 call_id, struct svsm_attestation_call *input)
> 
> This looks like BIOS code. The only thing that's missing is the
> CamelCase. :-)
> 
> int snp_issue_svsm_attest_req(u64 call_id, struct svsm_attest_call *input_call)
> 
> Now that's more like it!

Ok.

> 
>> +{
>> +	struct svsm_attestation_call *attest_call;
> 
> 	struct svsm_attest_call *atst_call;

Ok, I'll use 'ac'

> 
>> +	struct svsm_call call = {};
>> +	unsigned long flags;
>> +	u64 attest_call_pa;
>> +	int ret;
>> +
>> +	if (!vmpl)
>> +		return -EINVAL;
>> +
> 
> ...
> 
>> +static int sev_svsm_report_new(struct tsm_report *report, void *data)
>> +{
>> +	unsigned int report_len, manifest_len, certificates_len;
>> +	void *report_blob, *manifest_blob, *certificates_blob;
> 
> Prose. Shorter pls.

Ok.

Thanks,
Tom

> 
>> +	struct svsm_attestation_call attest_call = {};
>> +	struct tsm_desc *desc = &report->desc;
>> +	unsigned int retry_count;
>> +	unsigned int size;
>> +	bool try_again;
>> +	void *buffer;
>> +	u64 call_id;
>> +	int ret;
> 
> ...
>