Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30432 )
Change subject: drivers/smmstore: Fix some issues ......................................................................
Patch Set 13:
(6 comments)
https://review.coreboot.org/c/coreboot/+/30432/13/src/drivers/smmstore/store... File src/drivers/smmstore/store.c:
https://review.coreboot.org/c/coreboot/+/30432/13/src/drivers/smmstore/store... PS13, Line 60: rdev.region *region_device_region(&rdev);
https://review.coreboot.org/c/coreboot/+/30432/13/src/drivers/smmstore/store... PS13, Line 87: store_rdev This change seems like quite the short cut. store_rdev only is used once below. However, the comment I made on patchset 12 on line 106 equally applies still as you have violated the conditions needed to use the API.
https://review.coreboot.org/c/coreboot/+/30432/13/src/drivers/smmstore/store... PS13, Line 118: int smmstore_read_region(void *buf, ssize_t *bufsize) There are 3 APIs to SMM store from the interface:
1. smmstore_read_region() 2. smmstore_append_data() 3. smmstore_clear_region()
All 3 need to determine the location of the region itself. They are all one-shot and expect no runtime state to persist this compliation unit outside of those calls.
#1 only needs read access #2 needs read and write access #3 only needs write access
If you want one function to rule them all then you will need to construct the incoherent_rdev correctly. The code below should work the way you want:
static int lookup_store(struct region_device *rstore) { static struct region_device read_rdev, write_rdev; static struct incoherent_rdev store_irdev; struct region region; struct region_device *rdev;
if (lookup_store_region(®ion)) return -1;
if (boot_device_ro_subregion(®ion, &read_rdev) < 0) return -1; if (boot_device_rw_subregion(®ion, &write_rdev) < 0) return -1; rdev = incoherent_rdev_init(&store_irdev, ®ion, &read_rdev, &write_rdev); if (rdev == NULL) return -1;
return rdev_chain(rstore, rdev, 0, region_device_sz(rdev)); }
https://review.coreboot.org/c/coreboot/+/30432/13/src/drivers/smmstore/store... PS13, Line 211: data_sz I don't understand why one is passing in data_sz scan_end() can obtain it directly since 'store' is being passed in. And since end is onl used once, by this function, you can just adjust store within scan_end() to have store only point to the remaining region. Then below you would rdev_chain(..., 0, size) for a bounds check.
https://review.coreboot.org/c/coreboot/+/30432/13/src/drivers/smmstore/store... PS13, Line 219: &store) < 0) { You already have a writable rdev by way of using incoherent_rdev in lookup_store().
https://review.coreboot.org/c/coreboot/+/30432/13/src/drivers/smmstore/store... PS13, Line 231: end This should be 0. The rdev_chain() tracks the window of the region it is operating on. As such the offsets are relative to that: (0, size]. Therefore end needs to be reset to 0 to start the writes.