| Message ID | 20240621123903.2411843-7-nikunj@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Secure TSC support for SNP guests | expand |
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...? > 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? > { > - 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; ? Also that id needs to be unsigned as it is an array index.
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
On Fri, Jun 28, 2024 at 09:55:28AM +0530, Nikunj A. Dadhania wrote: > Yes, changed in following patch 07/24. Do you want me to pull those > changes to this patch ? Nah, not needed.
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) { - 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; - return key; + *seqno = &secrets->os_area.msg_seqno[id]; + return secrets->vmpck[id]; } struct snp_msg_report_resp_hdr {