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

[Nikunj A. Dadhania]

On 2/28/2024 5:20 PM, Borislav Petkov wrote:
> On Thu, Feb 15, 2024 at 05:01:15PM +0530, Nikunj A Dadhania wrote:
>> +enum aead_algo {
> 
> Looks unused.

This is being moved from drivers/virt/coco/sev-guest/sev-guest.h and is 
used in drivers/virt/coco/sev-guest/sev-guest.c::enc_payload()

 	hdr->algo = SNP_AEAD_AES_256_GCM;

> Add new struct definitions, etc, together with their first user - not
> preemptively.
> 
> Please audit all your patches.

Sure.

> 
>> +	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.

Responded on that thread.

> 
>> +} __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!

Thank you.

> 
> ...
> 
>> @@ -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.

Will update.

>>  	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.
> 

Regards
Nikunj