Re: [PATCH v8 03/16] virt: sev-guest: Add SNP guest request structure

[Tom Lendacky <thomas.lendacky@amd.com>]

On 2/15/24 05:31, Nikunj A Dadhania wrote:
> Add a snp_guest_req structure to simplify the function arguments. The
> structure will be used to call the SNP Guest message request API
> instead of passing a long list of parameters.
> 
> Update snp_issue_guest_request() prototype to include the new guest request
> structure and move the prototype to sev.h.
> 
> Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
> ---
>   arch/x86/include/asm/sev.h              |  75 ++++++++-
>   arch/x86/kernel/sev.c                   |  15 +-
>   drivers/virt/coco/sev-guest/sev-guest.c | 195 +++++++++++++-----------
>   drivers/virt/coco/sev-guest/sev-guest.h |  66 --------
>   4 files changed, 187 insertions(+), 164 deletions(-)
>   delete mode 100644 drivers/virt/coco/sev-guest/sev-guest.h
> 
> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
> index bed95e1f4d52..0c0b11af9f89 100644
> --- a/arch/x86/include/asm/sev.h
> +++ b/arch/x86/include/asm/sev.h
> @@ -111,8 +111,6 @@ struct rmp_state {
>   struct snp_req_data {
>   	unsigned long req_gpa;
>   	unsigned long resp_gpa;
> -	unsigned long data_gpa;
> -	unsigned int data_npages;
>   };
>   
>   struct sev_guest_platform_data {
> @@ -154,6 +152,73 @@ struct snp_secrets_page_layout {
>   	u8 rsvd3[3840];
>   } __packed;
>   
> +#define MAX_AUTHTAG_LEN		32
> +#define AUTHTAG_LEN		16
> +#define AAD_LEN			48
> +#define MSG_HDR_VER		1
> +
> +/* See SNP spec SNP_GUEST_REQUEST section for the structure */
> +enum msg_type {
> +	SNP_MSG_TYPE_INVALID = 0,
> +	SNP_MSG_CPUID_REQ,
> +	SNP_MSG_CPUID_RSP,
> +	SNP_MSG_KEY_REQ,
> +	SNP_MSG_KEY_RSP,
> +	SNP_MSG_REPORT_REQ,
> +	SNP_MSG_REPORT_RSP,
> +	SNP_MSG_EXPORT_REQ,
> +	SNP_MSG_EXPORT_RSP,
> +	SNP_MSG_IMPORT_REQ,
> +	SNP_MSG_IMPORT_RSP,
> +	SNP_MSG_ABSORB_REQ,
> +	SNP_MSG_ABSORB_RSP,
> +	SNP_MSG_VMRK_REQ,
> +	SNP_MSG_VMRK_RSP,
> +
> +	SNP_MSG_TYPE_MAX
> +};
> +
> +enum aead_algo {
> +	SNP_AEAD_INVALID,
> +	SNP_AEAD_AES_256_GCM,
> +};
> +
> +struct snp_guest_msg_hdr {
> +	u8 authtag[MAX_AUTHTAG_LEN];
> +	u64 msg_seqno;
> +	u8 rsvd1[8];
> +	u8 algo;
> +	u8 hdr_version;
> +	u16 hdr_sz;
> +	u8 msg_type;
> +	u8 msg_version;
> +	u16 msg_sz;
> +	u32 rsvd2;
> +	u8 msg_vmpck;
> +	u8 rsvd3[35];
> +} __packed;
> +
> +struct snp_guest_msg {
> +	struct snp_guest_msg_hdr hdr;
> +	u8 payload[4000];

If the idea is to ensure that payload never goes beyond a page boundary 
(assuming page allocation/backing), it would be better to have:

	u8 payload[PAGE_SIZE - sizeof(struct snp_guest_msg_hdr)];

instead of hard-coding 4000 (I realize this is existing code). Although, 
since you probably want to ensure that you don't exceed the page 
allocation by testing against the size or page offset, you can just make 
this a variable length array:

	u8 payload[];

and ensure that you don't overrun.

Thanks,
Tom

> +} __packed;
> +