Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36208 )
Change subject: security/vboot: Add vboot callbacks to support EC software sync ......................................................................
Patch Set 15:
(6 comments)
https://review.coreboot.org/c/coreboot/+/36208/13/src/security/vboot/ec_sync... File src/security/vboot/ec_sync.c:
https://review.coreboot.org/c/coreboot/+/36208/13/src/security/vboot/ec_sync... PS13, Line 268: burst = pdata_max_size - sizeof(*params);
Actually, this does have to take into account sizeof(struct ec_host_request). See https://review. […]
Sorry, yes, I wasn't thinking right. It *does* matter for the EC max packet size, but not for the stack-allocated buffer. So need to pull those two things apart a bit again.
https://review.coreboot.org/c/coreboot/+/36208/13/src/security/vboot/ec_sync... PS13, Line 279: bufsize
Oops, good catch. Yeah bufsize should be burst.
burst + sizeof(*params), to be precise
https://review.coreboot.org/c/coreboot/+/36208/15/src/security/vboot/ec_sync... File src/security/vboot/ec_sync.c:
https://review.coreboot.org/c/coreboot/+/36208/15/src/security/vboot/ec_sync... PS15, Line 26: nit: why?
https://review.coreboot.org/c/coreboot/+/36208/15/src/security/vboot/ec_sync... PS15, Line 267: burst = (pdata_max_size - sizeof(*params)) - sizeof(struct ec_host_request); Well, you're throwing away a bit of stack space here now. Instead, I would just change the line above to
pdata_max_size = MIN(1024, resp_proto.max_request_packet_size - sizeof(struct ec_host_request));
https://review.coreboot.org/c/coreboot/+/36208/15/src/security/vboot/ec_sync... PS15, Line 295: file_buf += todo; This doesn't do anything anymore now (in fact it would probably break the munmap() below on a platform where that's not a no-op).
https://review.coreboot.org/c/coreboot/+/36208/15/src/security/vboot/ec_sync... PS15, Line 301: todo += sizeof(struct ec_params_flash_write); nit: This just threw me for a loop a bit, I think it would be easier to follow if you just write 'sizeof(*params) + todo' explicitly below.