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

[Borislav Petkov]

On Thu, Feb 15, 2024 at 05:01:15PM +0530, Nikunj A Dadhania wrote:
> +enum aead_algo {

Looks unused.

Add new struct definitions, etc, together with their first user - not
preemptively.

Please audit all your patches.

> +	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];

What Tom said.

> +} __packed;
> +
> +struct snp_guest_req {
> +	void *req_buf;
> +	size_t req_sz;
> +
> +	void *resp_buf;
> +	size_t resp_sz;
> +
> +	void *data;
> +	size_t data_npages;
> +
> +	u64 exit_code;
> +	unsigned int vmpck_id;
> +	u8 msg_version;
> +	u8 msg_type;
> +};
> +
>  #ifdef CONFIG_AMD_MEM_ENCRYPT
>  extern void __sev_es_ist_enter(struct pt_regs *regs);
>  extern void __sev_es_ist_exit(void);
> @@ -223,7 +288,8 @@ void snp_set_memory_private(unsigned long vaddr, unsigned long npages);
>  void snp_set_wakeup_secondary_cpu(void);
>  bool snp_init(struct boot_params *bp);
>  void __init __noreturn snp_abort(void);
> -int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, struct snp_guest_request_ioctl *rio);
> +int snp_issue_guest_request(struct snp_guest_req *req, struct snp_req_data *input,
> +			    struct snp_guest_request_ioctl *rio);

Much nicer!

..

> @@ -868,7 +890,6 @@ static int __init sev_guest_probe(struct platform_device *pdev)
>  	/* initial the input address for guest request */

That comment reads wrong - might fix it while here.

>  	snp_dev->input.req_gpa = __pa(snp_dev->request);
>  	snp_dev->input.resp_gpa = __pa(snp_dev->response);
> -	snp_dev->input.data_gpa = __pa(snp_dev->certs_data);

>  
>  	ret = tsm_register(&sev_tsm_ops, snp_dev, &tsm_report_extra_type);
>  	if (ret)

Rest looks ok. I'd chose shorter local vars if I were you but yours are
ok too.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette