Received-SPF: pass (google.com: domain of linux-kernel+bounces-147921-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <32181e52-da8e-404c-9832-cb29e9e31064@amd.com>
Date: Wed, 17 Apr 2024 09:48:09 +0530
User-Agent: Mozilla Thunderbird
Reply-To: nikunj@amd.com
Subject: Re: [PATCH v8 04/16] virt: sev-guest: Add vmpck_id to snp_guest_dev
 struct
To: Borislav Petkov <bp@alien8.de>
Cc: linux-kernel@vger.kernel.org, thomas.lendacky@amd.com, x86@kernel.org,
 kvm@vger.kernel.org, mingo@redhat.com, tglx@linutronix.de,
 dave.hansen@linux.intel.com, pgonda@google.com, seanjc@google.com,
 pbonzini@redhat.com
References: <20240215113128.275608-1-nikunj@amd.com>
 <20240215113128.275608-5-nikunj@amd.com>
 <20240409102348.GBZhUXND0CDk7tGv8a@fat_crate.local>
 <74f17321-42ed-417c-ad24-8bc4e7207117@amd.com>
 <20240416090658.GBZh4_sj7ursRtzijI@fat_crate.local>
Content-Language: en-US
From: "Nikunj A. Dadhania" <nikunj@amd.com>
In-Reply-To: <20240416090658.GBZh4_sj7ursRtzijI@fat_crate.local>
Content-Type: text/plain; charset=UTF-8
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?WVVvZk9ram9WSndnbkdDTXMxZThlOXJsdGt1clpSNzFjaUhsK1B1QU8vRHV6?=
 =?utf-8?B?RE5jdmtsK05UK1QzbjJLaU1QRHptaEFkY3dYSllLZUxvUkRGOFNvUEJldThI?=
 =?utf-8?B?K3ZkNGhKQ3M4dmxxalNBMkFaUy9LSHJyYUlmbWpnU1I3eVRrTHJ5Y2gxQ2o1?=
 =?utf-8?B?TzJsZGwyeENvK09PM1N5Zy95eTU1NmRFR0RsdVZmNmVEWVQvdUJSWUsrSTJE?=
 =?utf-8?B?MnRZb0tGcTVhUU5XOUZKai9iblBad3pyVlN5YlE0SU9QM2xmeVQvMmV1RmJw?=
 =?utf-8?B?QlJWRlRYdmp3OE5zRE9NTUxuZXI3UDVpR2ZyMi8rNDdCV2JvRnh5Y3RmUjY1?=
 =?utf-8?B?T1NvTW91YU9vZC91MGxjUnBqREMvenN0WUFreXM3NUI2ek52RElmby9nQlVG?=
 =?utf-8?B?L0t4MUpZNjloOWtYOUQzVmw3clYvZXJqTTRyaTZlWGNLVlNIU2JGK1F4NTFY?=
 =?utf-8?B?bXlQN2xnVERlZG4yMXIxWG1MVm1IUEJlMXBwRm1uS3grRjdPbXVXNFhuZGxi?=
 =?utf-8?B?WjBtd0dsMk1raWxDSzhQWlh5YVVnZFA2djlpWG5OVkxPWTZSaUtJUHB4WXZV?=
 =?utf-8?B?UUs3aC82TUNoanBiU0YzK1ZQTFgvK3FWQ0lqZVdZRTFiNGN3cW1LNlJpUEdw?=
 =?utf-8?B?Vk9zM0JOVkwyb2wzY2oyRVh4SjBmUEhrUTBTQ0dWUnJIaWF2eGo3TDBrU3JE?=
 =?utf-8?B?NDNOdGVTbk5TMzBQbWQrYTc4ZkN0T09YcGtVWU13Q1ZraGtPd1VEWmgwaytt?=
 =?utf-8?B?TzNXRGVYK0svcHlzcFJCTU54WWhQYWk4WTZ6RGZBMitxUGJ2TW9jYnRyR2Vj?=
 =?utf-8?B?T1BSU09BNXlzYnFpTGNKc3JGZ0RCc2NSZkw4RGE2VjVhUTdVSXp3SGRTTHEv?=
 =?utf-8?B?QnV2TTd3UmhnbnlzblpzUjZYVEk2c2padTdwSDZDV1NiVUhKZE84MlVlUUlP?=
 =?utf-8?B?VlFZUWJhdE5hb3RmbUVuUU95Q3FSUmFTT0Z0UzZWcndYTURxTk9XVFg4MURE?=
 =?utf-8?B?S1ZPVTZuRzNLRUVvVCtZRjFaSXd1SWpIa0thSkoreVg5alFrMXdsR3FQNDZW?=
 =?utf-8?B?bUw4K2ZkL2YvWUY0Z2RySmVjSTM4NWJpa2p0a3dXWG1WYUxtak5ENkpRODJP?=
 =?utf-8?B?L0tXdUw0bW1sU2lxOFQxaWdLODR3b255OHd2REZObklqRmRzWEdHSlM0OFBN?=
 =?utf-8?B?bWs1b3ZPMlhBdXdoalJtYjZKN3d6ZjE3M0xsTnhDWmwyQ2V1UzJxand2VENJ?=
 =?utf-8?B?MXRabmNSQXVlTXc3M0tEbE9VNTNiL0JGUXdKMHpqMjBiTGRpVk1CY2xUTytz?=
 =?utf-8?B?bHZvMXI0VGJrMGFCQXFqc0xRd29QUTBhdXdab2JxNkFQek54dDVsLzhxVjJ1?=
 =?utf-8?B?ais0MlJZNm1VZEtoMEtJTy9KN2djVXMzM3BzQ3ljU1JzQm5vSjNjRjhzYVBh?=
 =?utf-8?B?Sm5iMWlkUEJYU3NiZUF6bWhPYkZFUEFaTHphRUlFSVF3eTNMaWlIV1lYUVZp?=
 =?utf-8?B?c0t0L3NVM0lZa3BmK1lFaG1obVVOUFlmdXplazZSbWVkaWFUNWo5SFpwQVpZ?=
 =?utf-8?B?STR4clp3YjM1NXVTY0dGVlFLZE5VSll5anRkTVNiVTRCek9tYi94NWx1Yitp?=
 =?utf-8?B?N3RDdGN5UXREOE95MjZweVU4SnZQdU1jK1VpNzNwT3JJN0YrTUh0RkJmdkc4?=
 =?utf-8?B?TnZKSjhyRzd6QTg3WVNoRFV5L1UrUDZtK1VRb2RqbzNuMVdrdXRxeGttYjhC?=
 =?utf-8?B?SktsQ2dkNThnZ2pHbzNPTTNhQXhqT2pSY2EyaG1yZTloeGxVQ2luZHExc2g0?=
 =?utf-8?B?aGIydld6S1dReGRuTFdiU3J6ckgySGVBdmNaUXRROGN0YUFURGVzU3QvT1JZ?=
 =?utf-8?B?d25zQkY2bnpPT29TRW9YSFVORDZwaWFpaDNwR3FLS0hyZVhXSEFxNFVpN1pN?=
 =?utf-8?B?VDNLNGRrL29vNWFGejdyZ0s4Tlp3bEhEaFJVZlIxcklPejd6TVB2SDV4djlm?=
 =?utf-8?B?UmJoeEtsWjhnbGdlY25jRUIrTTR6UWp0N1ZaZ0p3VGNoajRwRmJNaTlteVp5?=
 =?utf-8?B?MDVqM3E4SHJDNjEyRHFjem90ZktMQjJyWmp6NnA5UDU3b1FkVm1UQ1piTkNY?=
 =?utf-8?Q?+Q6MMz68hgHgqz+Jh9ssKNcf8?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 25f5fdc3-7d3e-47c3-1d97-08dc5e956a19
X-MS-Exchange-CrossTenant-AuthSource: DS7PR12MB6309.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Apr 2024 04:18:20.4119
 (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: C7DkrcT4l08qEVFjUTx9h2b7WS3Gy+7fSsU0Dj9x9qvtbCPsnHES5agPP3ILWdjuDwDvm6O+Q5gSNafYwc3nGg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR12MB6492

On 4/16/2024 2:36 PM, Borislav Petkov wrote:
> On Tue, Apr 16, 2024 at 11:27:24AM +0530, Nikunj A. Dadhania wrote:
>>> Why does that snp_get_os_area_msg_seqno() returns a pointer when you
>>> deref it here again?
>>>
>>> A function which returns a sequence number should return that number
>>> - not a pointer to it.
>>>
>>> Which then makes that u32 *os_area_msg_seqno redundant and you can use
>>> the function directly.
>>>
>>> IOW:
>>>
>>> static inline u32 snp_get_os_area_msg_seqno(struct snp_guest_dev *snp_dev)
>>> {
>>>         return snp_dev->layout->os_area.msg_seqno_0 + snp_dev->vmpck_id;
>>
>> This patch removes setting of layour page in snp_dev structure.
> 
> So?

* Instead of using snp_dev->layout, we will need to access it using platform_data->layout structure.
* Below will give incorrect value of sequence number, it will get VMPCK_0's sequence number and will add vmpck_id to that. Will work by fluke for VMPCK=0, but will fail for all other keys.

  return snp_dev->layout->os_area.msg_seqno_0 + snp_dev->vmpck_id;


struct secrets_os_area {
..
        u32 msg_seqno_0;
        u32 msg_seqno_1;
        u32 msg_seqno_2;
        u32 msg_seqno_3;
..
}

* I am using vmpck_id to index to correct msg_seqno_*


Changing this to

struct secrets_os_area {
..
        u32 msg_seqno[VMPCK_MAX_NUM];
..
}


> 
>> static inline u32 snp_get_os_area_msg_seqno(struct snp_guest_dev *snp_dev)
>> {
>>         if (!platform_data)
>>                 return NULL;
>>
>>         return *(&platform_data->layout->os_area.msg_seqno_0 + vmpck_id);
>> }
> 
> What?!

I can change the secrets_os_area like below to simplify things:

struct secrets_os_area {
..
        u32 msg_seqno[VMPCK_MAX_NUM];
..
}


static inline u32 snp_get_os_area_msg_seqno(struct snp_guest_dev *snp_dev)
{
         if (!platform_data)
                 return NULL;

         return platform_data->layout->os_area.msg_seqno[snp_dev->vmpck_id];
}


> 
> This snp_get_os_area_msg_seqno() is a new function added by this patch.
> 
>> I had a getter for getting the os_area_msg_seqno pointer, probably not
>> a good function name.
> 
> Probably you need to go back to the drawing board and think about how
> this thing should look like.
> 
>>> Do you see the imbalance in the APIs?
>>
>> The msg_seqno should only be incremented by 2 (always), that was the reason to avoid a setter.
> 
> And what's wrong with the setter doing the incrementation so that
> callers can't even get it wrong?

Are you suggesting that setter should always increment by 2?

static inline u32 snp_set_os_area_msg_seqno(struct snp_guest_dev *snp_dev)
{
..
	os_area.msg_seqno[snp_dev->vmpck_id] += 2;
..

}

> 
> It sounds to me like you should redesign this sequence number handling
> in a *separate* patch.

Sure, let me rethink and will post it as separate patch.

Regards
Nikunj