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
