On 6/25/2024 10:34 PM, Borislav Petkov wrote:
> On Fri, Jun 21, 2024 at 06:08:45PM +0530, Nikunj A Dadhania wrote:
>> Preparatory patch to remove direct usage of VMPCK and message sequence
>
> "Prepare the code for removing... "
>
> From Documentation/process/submitting-patches.rst:
>
> "Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
> instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
> to do frotz", as if you are giving orders to the codebase to change
> its behaviour."
>
>> number in the SEV guest driver.
>
> remove, because...?
SNP guest driver currently is accessing os_area and VMPCK of secrets page.
Prepare the code for removing direct usage of these and later provide clean
accessor API to SEV guest driver.
>
>> Use arrays for the VM platform communication key and message sequence number
>> to simplify the function and usage.
>>
>> Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
>> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
>> ---
>> arch/x86/include/asm/sev.h | 12 ++++-------
>> drivers/virt/coco/sev-guest/sev-guest.c | 27 ++++---------------------
>> 2 files changed, 8 insertions(+), 31 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
>> index 2ac899adcbf6..473760208764 100644
>> --- a/arch/x86/include/asm/sev.h
>> +++ b/arch/x86/include/asm/sev.h
>> @@ -118,6 +118,8 @@ struct sev_guest_platform_data {
>> u64 secrets_gpa;
>> };
>>
>> +#define VMPCK_MAX_NUM 4
>> +
>> /*
>> * The secrets page contains 96-bytes of reserved field that can be used by
>> * the guest OS. The guest OS uses the area to save the message sequence
>> @@ -126,10 +128,7 @@ struct sev_guest_platform_data {
>> * See the GHCB spec section Secret page layout for the format for this area.
>> */
>> struct secrets_os_area {
>> - u32 msg_seqno_0;
>> - u32 msg_seqno_1;
>> - u32 msg_seqno_2;
>> - u32 msg_seqno_3;
>> + u32 msg_seqno[VMPCK_MAX_NUM];
>> u64 ap_jump_table_pa;
>> u8 rsvd[40];
>> u8 guest_usage[32];
>> @@ -214,10 +213,7 @@ struct snp_secrets_page {
>> u32 fms;
>> u32 rsvd2;
>> u8 gosvw[16];
>> - u8 vmpck0[VMPCK_KEY_LEN];
>> - u8 vmpck1[VMPCK_KEY_LEN];
>> - u8 vmpck2[VMPCK_KEY_LEN];
>> - u8 vmpck3[VMPCK_KEY_LEN];
>> + u8 vmpck[VMPCK_MAX_NUM][VMPCK_KEY_LEN];
>> struct secrets_os_area os_area;
>>
>> u8 vmsa_tweak_bitmap[64];
>> diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
>> index 61e190ecfa3a..a5602c84769f 100644
>> --- a/drivers/virt/coco/sev-guest/sev-guest.c
>> +++ b/drivers/virt/coco/sev-guest/sev-guest.c
>> @@ -678,30 +678,11 @@ static const struct file_operations snp_guest_fops = {
>>
>> static u8 *get_vmpck(int id, struct snp_secrets_page *secrets, u32 **seqno)
>
> Why is this a separate function when it is used only once?
This will later be moved and an API provided, will be used from SEV guest driver
and Secure TSC code. I had that as a single patch, you had suggested to split the
a separate patch.
>
>> {
>> - u8 *key = NULL;
>> -
>> - switch (id) {
>> - case 0:
>> - *seqno = &secrets->os_area.msg_seqno_0;
>> - key = secrets->vmpck0;
>> - break;
>> - case 1:
>> - *seqno = &secrets->os_area.msg_seqno_1;
>> - key = secrets->vmpck1;
>> - break;
>> - case 2:
>> - *seqno = &secrets->os_area.msg_seqno_2;
>> - key = secrets->vmpck2;
>> - break;
>> - case 3:
>> - *seqno = &secrets->os_area.msg_seqno_3;
>> - key = secrets->vmpck3;
>> - break;
>> - default:
>> - break;
>> - }
>> + if (!(id < VMPCK_MAX_NUM))
>> + return NULL;
>
> Or
>
> if (id >= VMPCK_MAX_NUM)
> return NULL;
>
> ?
Sure
>
> Also that id needs to be unsigned as it is an array index.
>
Yes, changed in following patch 07/24. Do you want me to pull those changes to this patch ?
Regards,
Nikunj