| Message ID | 20240531043038.3370793-7-nikunj@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Secure TSC support for SNP guests | expand |
On 5/30/24 23:30, Nikunj A Dadhania wrote: > Preparatory patch to remove direct usage of VMPCK and message sequence > number in the 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> One minor comment below, otherwise, for the general logic of using an array: 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 dbf17e66d52a..d06b08f7043c 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]; > @@ -145,10 +144,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 rsvd3[3840]; > } __packed; > diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c > index 5c0cbdad9fa2..a3c0b22d2e14 100644 > --- a/drivers/virt/coco/sev-guest/sev-guest.c > +++ b/drivers/virt/coco/sev-guest/sev-guest.c > @@ -668,30 +668,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 + 1) > VMPCK_MAX_NUM) > + return NULL; This looks a bit confusing to me, because of the way it has to be written with the "+ 1". I wonder if something like the following would read better: switch (id) { case 0 ... 3: *seqno = &secrets->os_area.msg_seqno[id]; return secrets->vmpck[id]; default: return NULL; } Just my opinion, if others are fine with it, then that's fine. Thanks, Tom > > - return key; > + *seqno = &secrets->os_area.msg_seqno[id]; > + return secrets->vmpck[id]; > } > > struct snp_msg_report_resp_hdr {
On 6/19/2024 2:57 AM, Tom Lendacky wrote: > On 5/30/24 23:30, Nikunj A Dadhania wrote: >> Preparatory patch to remove direct usage of VMPCK and message sequence >> number in the 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> > > One minor comment below, otherwise, for the general logic of using an array: > > 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 dbf17e66d52a..d06b08f7043c 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]; >> @@ -145,10 +144,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 rsvd3[3840]; >> } __packed; >> diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c >> index 5c0cbdad9fa2..a3c0b22d2e14 100644 >> --- a/drivers/virt/coco/sev-guest/sev-guest.c >> +++ b/drivers/virt/coco/sev-guest/sev-guest.c >> @@ -668,30 +668,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 + 1) > VMPCK_MAX_NUM) >> + return NULL; > > This looks a bit confusing to me, because of the way it has to be > written with the "+ 1". I wonder if something like the following would > read better: > > switch (id) { > case 0 ... 3: > *seqno = &secrets->os_area.msg_seqno[id]; > return secrets->vmpck[id]; > default: > return NULL; > } > > Just my opinion, if others are fine with it, then that's fine. I have separated patch 6 and 7 for better code review and modular changes. The next patch simplifes this further to: static inline u8 *get_vmpck(struct snp_guest_dev *snp_dev) { return snp_dev->secrets->vmpck[snp_dev->vmpck_id]; } static bool assign_vmpck(struct snp_guest_dev *dev, unsigned int vmpck_id) { if ((vmpck_id + 1) > VMPCK_MAX_NUM) return false; dev->vmpck_id = vmpck_id; return true; } Regards Nikunj
On 6/19/24 01:06, Nikunj A. Dadhania wrote: > On 6/19/2024 2:57 AM, Tom Lendacky wrote: >> On 5/30/24 23:30, Nikunj A Dadhania wrote: > > I have separated patch 6 and 7 for better code review and modular changes. > > The next patch simplifes this further to: > > static inline u8 *get_vmpck(struct snp_guest_dev *snp_dev) > { > return snp_dev->secrets->vmpck[snp_dev->vmpck_id]; > } > > static bool assign_vmpck(struct snp_guest_dev *dev, unsigned int vmpck_id) > { > if ((vmpck_id + 1) > VMPCK_MAX_NUM) Ok, this still has the "+ 1" thing (and it should be >=, right?). How about: if (!(vmpck_id < VMPCK_MAX_NUM)) return false; Just makes it easier to read for me, but if no one else has an issue, don't worry about it. Thanks, Tom > return false; > > dev->vmpck_id = vmpck_id; > > return true; > } > > > Regards > Nikunj
On 6/19/2024 8:42 PM, Tom Lendacky wrote: > On 6/19/24 01:06, Nikunj A. Dadhania wrote: >> On 6/19/2024 2:57 AM, Tom Lendacky wrote: >>> On 5/30/24 23:30, Nikunj A Dadhania wrote: > >> >> I have separated patch 6 and 7 for better code review and modular changes. >> >> The next patch simplifes this further to: >> >> static inline u8 *get_vmpck(struct snp_guest_dev *snp_dev) >> { >> return snp_dev->secrets->vmpck[snp_dev->vmpck_id]; >> } >> >> static bool assign_vmpck(struct snp_guest_dev *dev, unsigned int vmpck_id) >> { >> if ((vmpck_id + 1) > VMPCK_MAX_NUM) > > Ok, this still has the "+ 1" thing (and it should be >=, right?). How about: For vmpck_id=3 which is valid, ((3 + 1) >= 4) will exit, which is not correct. > > if (!(vmpck_id < VMPCK_MAX_NUM)) > return false; > Sure, this is better. > Just makes it easier to read for me, but if no one else has an issue, > don't worry about it. Thanks, Nikunj
diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h index dbf17e66d52a..d06b08f7043c 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]; @@ -145,10 +144,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 rsvd3[3840]; } __packed; diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c index 5c0cbdad9fa2..a3c0b22d2e14 100644 --- a/drivers/virt/coco/sev-guest/sev-guest.c +++ b/drivers/virt/coco/sev-guest/sev-guest.c @@ -668,30 +668,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 + 1) > VMPCK_MAX_NUM) + return NULL; - return key; + *seqno = &secrets->os_area.msg_seqno[id]; + return secrets->vmpck[id]; } struct snp_msg_report_resp_hdr {
Preparatory patch to remove direct usage of VMPCK and message sequence number in the 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> --- arch/x86/include/asm/sev.h | 12 ++++------- drivers/virt/coco/sev-guest/sev-guest.c | 27 ++++--------------------- 2 files changed, 8 insertions(+), 31 deletions(-)