Re: [PATCH v15 03/13] x86/sev: Add Secure TSC support for SNP guests

[Nikunj A. Dadhania]

On 12/5/2024 5:25 PM, Borislav Petkov wrote:
> On Tue, Dec 03, 2024 at 02:30:35PM +0530, Nikunj A Dadhania wrote:
>> Add support for Secure TSC in SNP-enabled guests. Secure TSC allows guests
>> to securely use RDTSC/RDTSCP instructions, ensuring that the parameters
>> used cannot be altered by the hypervisor once the guest is launched.
>>
>> Secure TSC-enabled guests need to query TSC information from the AMD
>> Security Processor. This communication channel is encrypted between the AMD
>> Security Processor and the guest, with the hypervisor acting merely as a
>> conduit to deliver the guest messages to the AMD Security Processor. Each
>> message is protected with AEAD (AES-256 GCM).
>>
>> Signed-off-by: Nikunj A Dadhania <nikunj@xxxxxxx>
>> Tested-by: Peter Gonda <pgonda@xxxxxxxxxx>
> 
> 
>> Reviewed-by: Tom Lendacky <thomas.lendacky@xxxxxxx>
> 
> This patch changed somewhat from last time. 

Yes, most of the change was dynamic allocation in snp_get_tsc_info().

> When did Peter test it again and
> Tom review it again?

It makes sense to drop both.

> 
>> diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
>> index a61898c7f114..39683101b526 100644
>> --- a/arch/x86/coco/sev/core.c
>> +++ b/arch/x86/coco/sev/core.c
>> @@ -96,6 +96,14 @@ static u64 sev_hv_features __ro_after_init;
>>  /* Secrets page physical address from the CC blob */
>>  static u64 secrets_pa __ro_after_init;
>>  
>> +/*
>> + * For Secure TSC guests, the BP fetches TSC_INFO using SNP guest messaging and
> 
> s/BP/BSP/
> 
>> + * initializes snp_tsc_scale and snp_tsc_offset. These values are replicated
>> + * across the APs VMSA fields (TSC_SCALE and TSC_OFFSET).
>> + */
>> +static u64 snp_tsc_scale __ro_after_init;
>> +static u64 snp_tsc_offset __ro_after_init;
>> +
>>  /* #VC handler runtime per-CPU data */
>>  struct sev_es_runtime_data {
>>  	struct ghcb ghcb_page;
> 
> ...
> 
>> +	memcpy(tsc_resp, buf, sizeof(*tsc_resp));
>> +	pr_debug("%s: response status 0x%x scale 0x%llx offset 0x%llx factor 0x%x\n",
>> +		 __func__, tsc_resp->status, tsc_resp->tsc_scale, tsc_resp->tsc_offset,
>> +		 tsc_resp->tsc_factor);
>> +
>> +	if (tsc_resp->status == 0) {
> 
> Like the last time:
> 
> 	if (!tsc_resp->status)

Ack.
 
Regards
Nikunj