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@xxxxxxx>
One minor comment below, otherwise, for the general logic of using an array:
Reviewed-by: Tom Lendacky <thomas.lendacky@xxxxxxx>
> ---
> 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 {
