Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47658 )
Change subject: Add xlate_window ......................................................................
Add xlate_window
Change-Id: Id5b21ffca2c8d6a9dfc37a878429aed4a8301651 Signed-off-by: Furquan Shaikh furquan@google.com --- M src/commonlib/include/commonlib/region.h M src/commonlib/region.c M src/soc/intel/apollolake/mmap_boot.c 3 files changed, 78 insertions(+), 56 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/47658/1
diff --git a/src/commonlib/include/commonlib/region.h b/src/commonlib/include/commonlib/region.h index b9a984f..f8e0de4 100644 --- a/src/commonlib/include/commonlib/region.h +++ b/src/commonlib/include/commonlib/region.h @@ -215,9 +215,14 @@ * space. The sub region is the window within the 1st address space and * the request is modified prior to accessing the second address space * provided by access_dev. */ -struct xlate_region_device { +struct xlate_window { const struct region_device *access_dev; struct region sub_region; +}; + +struct xlate_region_device { + size_t window_count; + const struct xlate_window *window_arr; struct region_device rdev; };
@@ -225,36 +230,28 @@
extern const struct region_device_ops xlate_rdev_rw_ops;
-#define XLATE_REGION_DEV_INIT(access_dev_, sub_offset_, sub_size_, \ - parent_sz_, ops_) \ +#define XLATE_REGION_DEV_INIT(window_count_, window_arr_, parent_sz_, ops_) \ { \ - .access_dev = access_dev_, \ - .sub_region = { \ - .offset = (sub_offset_), \ - .size = (sub_size_), \ - }, \ + .window_count = window_count_, \ + .window_arr = window_arr_, \ .rdev = REGION_DEV_INIT((ops_), 0, (parent_sz_)), \ }
-#define XLATE_REGION_DEV_RO_INIT(access_dev_, sub_offset_, sub_size_, \ - parent_sz_) \ - XLATE_REGION_DEV_INIT(access_dev_, sub_offset_, \ - sub_size_, parent_sz_, &xlate_rdev_ro_ops), \ +#define XLATE_REGION_DEV_RO_INIT(window_count_, window_arr_, parent_sz_) \ + XLATE_REGION_DEV_INIT(window_count_, window_arr_, \ + parent_sz_, &xlate_rdev_ro_ops), \
-#define XLATE_REGION_DEV_RW_INIT(access_dev_, sub_offset_, sub_size_, \ - parent_sz_) \ - XLATE_REGION_DEV_INIT(access_dev_, sub_offset_, \ - sub_size_, parent_sz_, &xlate_rdev_rw_ops), \ +#define XLATE_REGION_DEV_RW_INIT(window_count_, window_arr_, parent_sz_) \ + XLATE_REGION_DEV_INIT(window_count_, window_arr_, \ + parent_sz_, &xlate_rdev_rw_ops), \
/* Helper to dynamically initialize xlate region device. */ void xlate_region_device_ro_init(struct xlate_region_device *xdev, - const struct region_device *access_dev, - size_t sub_offset, size_t sub_size, + size_t window_count, const struct xlate_window *window_arr, size_t parent_size);
void xlate_region_device_rw_init(struct xlate_region_device *xdev, - const struct region_device *access_dev, - size_t sub_offset, size_t sub_size, + size_t window_count, const struct xlate_window *window_arr, size_t parent_size);
/* This type can be used for incoherent access where the read and write diff --git a/src/commonlib/region.c b/src/commonlib/region.c index 467e8ff..ece8848 100644 --- a/src/commonlib/region.c +++ b/src/commonlib/region.c @@ -188,33 +188,29 @@
static void xlate_region_device_init(struct xlate_region_device *xdev, const struct region_device_ops *ops, - const struct region_device *access_dev, - size_t sub_offset, size_t sub_size, + size_t window_count, const struct xlate_window *window_arr, size_t parent_size) { memset(xdev, 0, sizeof(*xdev)); - xdev->access_dev = access_dev; - xdev->sub_region.offset = sub_offset; - xdev->sub_region.size = sub_size; + xdev->window_count = window_count; + xdev->window_arr = window_arr; region_device_init(&xdev->rdev, ops, 0, parent_size); }
void xlate_region_device_ro_init(struct xlate_region_device *xdev, - const struct region_device *access_dev, - size_t sub_offset, size_t sub_size, + size_t window_count, const struct xlate_window *window_arr, size_t parent_size) { - xlate_region_device_init(xdev, &xlate_rdev_ro_ops, access_dev, - sub_offset, sub_size, parent_size); + xlate_region_device_init(xdev, &xlate_rdev_ro_ops, window_count, window_arr, + parent_size); }
void xlate_region_device_rw_init(struct xlate_region_device *xdev, - const struct region_device *access_dev, - size_t sub_offset, size_t sub_size, + size_t window_count, const struct xlate_window *window_arr, size_t parent_size) { - xlate_region_device_init(xdev, &xlate_rdev_rw_ops, access_dev, - sub_offset, sub_size, parent_size); + xlate_region_device_init(xdev, &xlate_rdev_rw_ops, window_count, window_arr, + parent_size); }
static void *mdev_mmap(const struct region_device *rd, size_t offset, @@ -321,6 +317,21 @@ return 0; }
+static const struct xlate_window *xlate_find_window(const struct xlate_region_device *xldev, + const struct region *req) +{ + size_t i; + const struct xlate_window *xlwindow; + + for (i = 0; i < xldev->window_count; i++) { + xlwindow = &xldev->window_arr[i]; + if (region_is_subregion(&xlwindow->sub_region, req)) + return xlwindow; + } + + return NULL; +} + static void *xlate_mmap(const struct region_device *rd, size_t offset, size_t size) { @@ -329,24 +340,22 @@ .offset = offset, .size = size, }; + const struct xlate_window *xlwindow;
xldev = container_of(rd, __typeof__(*xldev), rdev);
- if (!region_is_subregion(&xldev->sub_region, &req)) + xlwindow = xlate_find_window(xldev, &req); + if (!xlwindow) return NULL;
- offset -= region_offset(&xldev->sub_region); + offset -= region_offset(&xlwindow->sub_region);
- return rdev_mmap(xldev->access_dev, offset, size); + return rdev_mmap(xlwindow->access_dev, offset, size); }
-static int xlate_munmap(const struct region_device *rd, void *mapping) +static int xlate_munmap(const struct region_device *rd __unused, void *mapping __unused) { - const struct xlate_region_device *xldev; - - xldev = container_of(rd, __typeof__(*xldev), rdev); - - return rdev_munmap(xldev->access_dev, mapping); + return 0; }
static ssize_t xlate_readat(const struct region_device *rd, void *b, @@ -356,16 +365,18 @@ .offset = offset, .size = size, }; + const struct xlate_window *xlwindow; const struct xlate_region_device *xldev;
xldev = container_of(rd, __typeof__(*xldev), rdev);
- if (!region_is_subregion(&xldev->sub_region, &req)) + xlwindow = xlate_find_window(xldev, &req); + if (!xlwindow) return -1;
- offset -= region_offset(&xldev->sub_region); + offset -= region_offset(&xlwindow->sub_region);
- return rdev_readat(xldev->access_dev, b, offset, size); + return rdev_readat(xlwindow->access_dev, b, offset, size); }
static ssize_t xlate_writeat(const struct region_device *rd, const void *b, @@ -375,16 +386,18 @@ .offset = offset, .size = size, }; + const struct xlate_window *xlwindow; const struct xlate_region_device *xldev;
xldev = container_of(rd, __typeof__(*xldev), rdev);
- if (!region_is_subregion(&xldev->sub_region, &req)) + xlwindow = xlate_find_window(xldev, &req); + if (!xlwindow) return -1;
- offset -= region_offset(&xldev->sub_region); + offset -= region_offset(&xlwindow->sub_region);
- return rdev_writeat(xldev->access_dev, b, offset, size); + return rdev_writeat(xlwindow->access_dev, b, offset, size); }
static ssize_t xlate_eraseat(const struct region_device *rd, @@ -394,16 +407,18 @@ .offset = offset, .size = size, }; + const struct xlate_window *xlwindow; const struct xlate_region_device *xldev;
xldev = container_of(rd, __typeof__(*xldev), rdev);
- if (!region_is_subregion(&xldev->sub_region, &req)) + xlwindow = xlate_find_window(xldev, &req); + if (!xlwindow) return -1;
- offset -= region_offset(&xldev->sub_region); + offset -= region_offset(&xlwindow->sub_region);
- return rdev_eraseat(xldev->access_dev, offset, size); + return rdev_eraseat(xlwindow->access_dev, offset, size); }
const struct region_device_ops xlate_rdev_ro_ops = { diff --git a/src/soc/intel/apollolake/mmap_boot.c b/src/soc/intel/apollolake/mmap_boot.c index 30c629e..775354d 100644 --- a/src/soc/intel/apollolake/mmap_boot.c +++ b/src/soc/intel/apollolake/mmap_boot.c @@ -43,6 +43,7 @@
static struct mem_region_device shadow_dev; static struct xlate_region_device real_dev; +static struct xlate_window real_dev_window;
static void bios_mmap_init(void) { @@ -68,17 +69,26 @@ mem_region_device_ro_init(&shadow_dev, (void *)base, bios_mapped_size);
- xlate_region_device_ro_init(&real_dev, &shadow_dev.rdev, - start, bios_mapped_size, - CONFIG_ROM_SIZE); + real_dev_window.access_dev = &shadow_dev.rdev; + real_dev_window.sub_region.offset = start; + real_dev_window.sub_region.size = bios_mapped_size; + + xlate_region_device_ro_init(&real_dev, 1, &real_dev_window, CONFIG_ROM_SIZE);
bios_size = size;
/* Check that the CBFS lies within the memory mapped area. It's too easy to forget the SRAM mapping when crafting an FMAP file. */ struct region cbfs_region; - if (!fmap_locate_area("COREBOOT", &cbfs_region) && - !region_is_subregion(&real_dev.sub_region, &cbfs_region)) + void *map; + + if (fmap_locate_area("COREBOOT", &cbfs_region)) + return; + + map = rdev_mmap(&real_dev.rdev, region_offset(&cbfs_region), region_sz(&cbfs_region)); + if (map) + rdev_munmap(&real_dev.rdev, map); + else printk(BIOS_CRIT, "ERROR: CBFS @ %zx size %zx exceeds mem-mapped area @ %zx size %zx\n", region_offset(&cbfs_region), region_sz(&cbfs_region),
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47658 )
Change subject: commonlib/region: Allow multiple windows for xlate_region_dev ......................................................................
Patch Set 2:
(2 comments)
This change is ready for review.
https://review.coreboot.org/c/coreboot/+/47658/1/src/commonlib/include/commo... File src/commonlib/include/commonlib/region.h:
https://review.coreboot.org/c/coreboot/+/47658/1/src/commonlib/include/commo... PS1, Line 217: * provided by access_dev. */
Agreed. As you noted, there are quite a few challenges/restrictions that this imposes. […]
Done
https://review.coreboot.org/c/coreboot/+/47658/1/src/commonlib/include/commo... PS1, Line 235: window_count_
Can't this just use […]
Done
Srinidhi N Kaushik has uploaded a new patch set (#6) to the change originally created by Furquan Shaikh. ( https://review.coreboot.org/c/coreboot/+/47658 )
Change subject: commonlib/region: Allow multiple windows for xlate_region_dev ......................................................................
commonlib/region: Allow multiple windows for xlate_region_dev
This change updates the translated region device (xlate_region_dev) to support multiple translation windows from the 1st address space to 2nd address space. The address spaces described by the translation windows can be non-contiguous in both spaces. This is required so that newer x86 platforms can describe memory mapping of SPI flash into multiple decode windows in order to support greater than 16MiB of memory mapped space.
Since the windows can be non-contiguous, it introduces new restrictions on the region device ops - any operation performed on the translated region device is limited to only 1 window at a time. This restriction is primarily because of the mmap operation. The caller expects that the memory mapped space is contiguous, however, that is not true anymore. Thus, even though the other operations (readat, writeat, eraseat) can be updated to translate into multiple operations one for each access device, all operations across multiple windows are prohibited for the sake of consistency.
It is the responsibility of the platform to ensure that any section that is operated on using the translated region device does not span multiple windows in the fmap description.
BUG=b:171534504
Change-Id: Id5b21ffca2c8d6a9dfc37a878429aed4a8301651 Signed-off-by: Furquan Shaikh furquan@google.com --- M src/commonlib/include/commonlib/region.h M src/commonlib/region.c M src/soc/intel/apollolake/mmap_boot.c 3 files changed, 104 insertions(+), 61 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/47658/6
Hello build bot (Jenkins), Duncan Laurie, Tim Wawrzynczak, Julius Werner, Srinidhi N Kaushik, Andrey Petrov, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47658
to look at the new patch set (#10).
Change subject: commonlib/region: Allow multiple windows for xlate_region_dev ......................................................................
commonlib/region: Allow multiple windows for xlate_region_dev
This change updates the translated region device (xlate_region_dev) to support multiple translation windows from the 1st address space to 2nd address space. The address spaces described by the translation windows can be non-contiguous in both spaces. This is required so that newer x86 platforms can describe memory mapping of SPI flash into multiple decode windows in order to support greater than 16MiB of memory mapped space.
Since the windows can be non-contiguous, it introduces new restrictions on the region device ops - any operation performed on the translated region device is limited to only 1 window at a time. This restriction is primarily because of the mmap operation. The caller expects that the memory mapped space is contiguous, however, that is not true anymore. Thus, even though the other operations (readat, writeat, eraseat) can be updated to translate into multiple operations one for each access device, all operations across multiple windows are prohibited for the sake of consistency.
It is the responsibility of the platform to ensure that any section that is operated on using the translated region device does not span multiple windows in the fmap description.
One additional difference in behavior is xlate_region_device does not perform any action in munmap call. This is because it does not keep track of the access device that was used to service the mmap request. Currently, xlate_region_device is used only by memory mapped boot media on the backend. So, not doing unmap is fine. If this needs to be changed in the future, xlate_region_device will have to accept a pre-allocated space from the caller to keep track of all mapping requests.
BUG=b:171534504
Change-Id: Id5b21ffca2c8d6a9dfc37a878429aed4a8301651 Signed-off-by: Furquan Shaikh furquan@google.com --- M src/commonlib/include/commonlib/region.h M src/commonlib/region.c M src/soc/intel/apollolake/mmap_boot.c 3 files changed, 111 insertions(+), 61 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/47658/10
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47658 )
Change subject: commonlib/region: Allow multiple windows for xlate_region_dev ......................................................................
Patch Set 12: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/47658/12/src/soc/intel/apollolake/m... File src/soc/intel/apollolake/mmap_boot.c:
https://review.coreboot.org/c/coreboot/+/47658/12/src/soc/intel/apollolake/m... PS12, Line 85: map = rdev_mmap(&real_dev.rdev, region_offset(&cbfs_region), region_sz(&cbfs_region)); : if (map) : rdev_munmap(&real_dev.rdev, map); if I understand this part right, wouldn't ``` if (!region_overlap(&real_dev.rdev, &cbfs_region)) printk(error); ``` work to avoid the unnecessary mmap op ? I guess mmap for x86 boot media is a noop for now so it's not a big deal.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47658 )
Change subject: commonlib/region: Allow multiple windows for xlate_region_dev ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47658/12/src/soc/intel/apollolake/m... File src/soc/intel/apollolake/mmap_boot.c:
https://review.coreboot.org/c/coreboot/+/47658/12/src/soc/intel/apollolake/m... PS12, Line 85: map = rdev_mmap(&real_dev.rdev, region_offset(&cbfs_region), region_sz(&cbfs_region)); : if (map) : rdev_munmap(&real_dev.rdev, map);
if I understand this part right, wouldn't […]
region_overlap won't work because it returns true for partial overlap too. But, I can still use
``` if (!fmap_locate_area("COREBOOT", &cbfs_region) && !region_is_subregion(&real_dev_window.sub_region, &cbfs_region)) ```
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47658 )
Change subject: commonlib/region: Allow multiple windows for xlate_region_dev ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47658/12/src/soc/intel/apollolake/m... File src/soc/intel/apollolake/mmap_boot.c:
https://review.coreboot.org/c/coreboot/+/47658/12/src/soc/intel/apollolake/m... PS12, Line 85: map = rdev_mmap(&real_dev.rdev, region_offset(&cbfs_region), region_sz(&cbfs_region)); : if (map) : rdev_munmap(&real_dev.rdev, map);
if I understand this part right, wouldn't […]
region_overlap() would be true if only part of it were overlapping so maybe not enough? whereas mmap would end up calling region_is_subregion() for checking.
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47658 )
Change subject: commonlib/region: Allow multiple windows for xlate_region_dev ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47658/12/src/soc/intel/apollolake/m... File src/soc/intel/apollolake/mmap_boot.c:
https://review.coreboot.org/c/coreboot/+/47658/12/src/soc/intel/apollolake/m... PS12, Line 85: map = rdev_mmap(&real_dev.rdev, region_offset(&cbfs_region), region_sz(&cbfs_region)); : if (map) : rdev_munmap(&real_dev.rdev, map);
region_overlap() would be true if only part of it were overlapping so maybe not enough? whereas mmap […]
oops should have refreshed, furquan has the same idea.
The mmap call seems ok since it is doing all the checks, but maybe a comment that that is what it is being used for..
Hello build bot (Jenkins), Duncan Laurie, Tim Wawrzynczak, Julius Werner, Srinidhi N Kaushik, Andrey Petrov, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47658
to look at the new patch set (#13).
Change subject: commonlib/region: Allow multiple windows for xlate_region_dev ......................................................................
commonlib/region: Allow multiple windows for xlate_region_dev
This change updates the translated region device (xlate_region_dev) to support multiple translation windows from the 1st address space to 2nd address space. The address spaces described by the translation windows can be non-contiguous in both spaces. This is required so that newer x86 platforms can describe memory mapping of SPI flash into multiple decode windows in order to support greater than 16MiB of memory mapped space.
Since the windows can be non-contiguous, it introduces new restrictions on the region device ops - any operation performed on the translated region device is limited to only 1 window at a time. This restriction is primarily because of the mmap operation. The caller expects that the memory mapped space is contiguous, however, that is not true anymore. Thus, even though the other operations (readat, writeat, eraseat) can be updated to translate into multiple operations one for each access device, all operations across multiple windows are prohibited for the sake of consistency.
It is the responsibility of the platform to ensure that any section that is operated on using the translated region device does not span multiple windows in the fmap description.
One additional difference in behavior is xlate_region_device does not perform any action in munmap call. This is because it does not keep track of the access device that was used to service the mmap request. Currently, xlate_region_device is used only by memory mapped boot media on the backend. So, not doing unmap is fine. If this needs to be changed in the future, xlate_region_device will have to accept a pre-allocated space from the caller to keep track of all mapping requests.
BUG=b:171534504
Change-Id: Id5b21ffca2c8d6a9dfc37a878429aed4a8301651 Signed-off-by: Furquan Shaikh furquan@google.com --- M src/commonlib/include/commonlib/region.h M src/commonlib/region.c M src/soc/intel/apollolake/mmap_boot.c 3 files changed, 105 insertions(+), 60 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/47658/13
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47658 )
Change subject: commonlib/region: Allow multiple windows for xlate_region_dev ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47658/12/src/soc/intel/apollolake/m... File src/soc/intel/apollolake/mmap_boot.c:
https://review.coreboot.org/c/coreboot/+/47658/12/src/soc/intel/apollolake/m... PS12, Line 85: map = rdev_mmap(&real_dev.rdev, region_offset(&cbfs_region), region_sz(&cbfs_region)); : if (map) : rdev_munmap(&real_dev.rdev, map);
oops should have refreshed, furquan has the same idea. […]
Actually, I changed it back to `region_is_subregion` in patchset #13 to keep it similar to how it was handled before.
Hello build bot (Jenkins), Duncan Laurie, Tim Wawrzynczak, Julius Werner, Srinidhi N Kaushik, Andrey Petrov, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47658
to look at the new patch set (#14).
Change subject: commonlib/region: Allow multiple windows for xlate_region_dev ......................................................................
commonlib/region: Allow multiple windows for xlate_region_dev
This change updates the translated region device (xlate_region_dev) to support multiple translation windows from the 1st address space to 2nd address space. The address spaces described by the translation windows can be non-contiguous in both spaces. This is required so that newer x86 platforms can describe memory mapping of SPI flash into multiple decode windows in order to support greater than 16MiB of memory mapped space.
Since the windows can be non-contiguous, it introduces new restrictions on the region device ops - any operation performed on the translated region device is limited to only 1 window at a time. This restriction is primarily because of the mmap operation. The caller expects that the memory mapped space is contiguous, however, that is not true anymore. Thus, even though the other operations (readat, writeat, eraseat) can be updated to translate into multiple operations one for each access device, all operations across multiple windows are prohibited for the sake of consistency.
It is the responsibility of the platform to ensure that any section that is operated on using the translated region device does not span multiple windows in the fmap description.
One additional difference in behavior is xlate_region_device does not perform any action in munmap call. This is because it does not keep track of the access device that was used to service the mmap request. Currently, xlate_region_device is used only by memory mapped boot media on the backend. So, not doing unmap is fine. If this needs to be changed in the future, xlate_region_device will have to accept a pre-allocated space from the caller to keep track of all mapping requests.
BUG=b:171534504
Change-Id: Id5b21ffca2c8d6a9dfc37a878429aed4a8301651 Signed-off-by: Furquan Shaikh furquan@google.com --- M src/commonlib/include/commonlib/region.h M src/commonlib/region.c M src/soc/intel/apollolake/mmap_boot.c 3 files changed, 103 insertions(+), 60 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/47658/14
Hello build bot (Jenkins), Duncan Laurie, Tim Wawrzynczak, Julius Werner, Srinidhi N Kaushik, Andrey Petrov, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47658
to look at the new patch set (#15).
Change subject: commonlib/region: Allow multiple windows for xlate_region_dev ......................................................................
commonlib/region: Allow multiple windows for xlate_region_dev
This change updates the translated region device (xlate_region_dev) to support multiple translation windows from the 1st address space to 2nd address space. The address spaces described by the translation windows can be non-contiguous in both spaces. This is required so that newer x86 platforms can describe memory mapping of SPI flash into multiple decode windows in order to support greater than 16MiB of memory mapped space.
Since the windows can be non-contiguous, it introduces new restrictions on the region device ops - any operation performed on the translated region device is limited to only 1 window at a time. This restriction is primarily because of the mmap operation. The caller expects that the memory mapped space is contiguous, however, that is not true anymore. Thus, even though the other operations (readat, writeat, eraseat) can be updated to translate into multiple operations one for each access device, all operations across multiple windows are prohibited for the sake of consistency.
It is the responsibility of the platform to ensure that any section that is operated on using the translated region device does not span multiple windows in the fmap description.
One additional difference in behavior is xlate_region_device does not perform any action in munmap call. This is because it does not keep track of the access device that was used to service the mmap request. Currently, xlate_region_device is used only by memory mapped boot media on the backend. So, not doing unmap is fine. If this needs to be changed in the future, xlate_region_device will have to accept a pre-allocated space from the caller to keep track of all mapping requests.
BUG=b:171534504
Change-Id: Id5b21ffca2c8d6a9dfc37a878429aed4a8301651 Signed-off-by: Furquan Shaikh furquan@google.com --- M src/commonlib/include/commonlib/region.h M src/commonlib/region.c M src/soc/intel/apollolake/mmap_boot.c 3 files changed, 103 insertions(+), 60 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/47658/15
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47658 )
Change subject: commonlib/region: Allow multiple windows for xlate_region_dev ......................................................................
Patch Set 15: Code-Review+1
ah yes region_is_subregion is what i was looking for
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47658 )
Change subject: commonlib/region: Allow multiple windows for xlate_region_dev ......................................................................
Patch Set 16: Code-Review+2
Furquan Shaikh has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47658 )
Change subject: commonlib/region: Allow multiple windows for xlate_region_dev ......................................................................
commonlib/region: Allow multiple windows for xlate_region_dev
This change updates the translated region device (xlate_region_dev) to support multiple translation windows from the 1st address space to 2nd address space. The address spaces described by the translation windows can be non-contiguous in both spaces. This is required so that newer x86 platforms can describe memory mapping of SPI flash into multiple decode windows in order to support greater than 16MiB of memory mapped space.
Since the windows can be non-contiguous, it introduces new restrictions on the region device ops - any operation performed on the translated region device is limited to only 1 window at a time. This restriction is primarily because of the mmap operation. The caller expects that the memory mapped space is contiguous, however, that is not true anymore. Thus, even though the other operations (readat, writeat, eraseat) can be updated to translate into multiple operations one for each access device, all operations across multiple windows are prohibited for the sake of consistency.
It is the responsibility of the platform to ensure that any section that is operated on using the translated region device does not span multiple windows in the fmap description.
One additional difference in behavior is xlate_region_device does not perform any action in munmap call. This is because it does not keep track of the access device that was used to service the mmap request. Currently, xlate_region_device is used only by memory mapped boot media on the backend. So, not doing unmap is fine. If this needs to be changed in the future, xlate_region_device will have to accept a pre-allocated space from the caller to keep track of all mapping requests.
BUG=b:171534504
Change-Id: Id5b21ffca2c8d6a9dfc37a878429aed4a8301651 Signed-off-by: Furquan Shaikh furquan@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/47658 Reviewed-by: Duncan Laurie dlaurie@chromium.org Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/commonlib/include/commonlib/region.h M src/commonlib/region.c M src/soc/intel/apollolake/mmap_boot.c 3 files changed, 103 insertions(+), 60 deletions(-)
Approvals: build bot (Jenkins): Verified Duncan Laurie: Looks good to me, approved Tim Wawrzynczak: Looks good to me, but someone else must approve
diff --git a/src/commonlib/include/commonlib/region.h b/src/commonlib/include/commonlib/region.h index b9a984f..4d095b7 100644 --- a/src/commonlib/include/commonlib/region.h +++ b/src/commonlib/include/commonlib/region.h @@ -6,6 +6,7 @@ #include <sys/types.h> #include <stddef.h> #include <stdbool.h> +#include <commonlib/bsd/helpers.h> #include <commonlib/mem_pool.h>
/* @@ -210,14 +211,33 @@ void *mmap_helper_rdev_mmap(const struct region_device *, size_t, size_t); int mmap_helper_rdev_munmap(const struct region_device *, void *);
-/* A translated region device provides the ability to publish a region device - * in one address space and use an access mechanism within another address - * space. The sub region is the window within the 1st address space and - * the request is modified prior to accessing the second address space - * provided by access_dev. */ -struct xlate_region_device { +/* + * A translated region device provides the ability to publish a region device in one address + * space and use an access mechanism within another address space. The sub region is the window + * within the 1st address space and the request is modified prior to accessing the second + * address space provided by access_dev. + * + * Each xlate_region_device can support multiple translation windows described using + * xlate_window structure. The windows need not be contiguous in either address space. However, + * this poses restrictions on the operations being performed i.e. callers cannot perform + * operations across multiple windows of a translated region device. It is possible to support + * readat/writeat/eraseat by translating them into multiple calls one to access device in each + * window. However, mmap support is tricky because the caller expects that the memory mapped + * region is contiguous in both address spaces. Thus, to keep the semantics consistent for all + * region ops, xlate_region_device does not support any operations across the window + * boundary. + * + * Note: The platform is expected to ensure that the fmap description does not place any + * section (that will be operated using the translated region device) across multiple windows. + */ +struct xlate_window { const struct region_device *access_dev; struct region sub_region; +}; + +struct xlate_region_device { + size_t window_count; + const struct xlate_window *window_arr; struct region_device rdev; };
@@ -225,38 +245,31 @@
extern const struct region_device_ops xlate_rdev_rw_ops;
-#define XLATE_REGION_DEV_INIT(access_dev_, sub_offset_, sub_size_, \ - parent_sz_, ops_) \ +#define XLATE_REGION_DEV_INIT(window_arr_, parent_sz_, ops_) \ { \ - .access_dev = access_dev_, \ - .sub_region = { \ - .offset = (sub_offset_), \ - .size = (sub_size_), \ - }, \ + .window_count = ARRAY_SIZE(window_arr_), \ + .window_arr = window_arr_, \ .rdev = REGION_DEV_INIT((ops_), 0, (parent_sz_)), \ }
-#define XLATE_REGION_DEV_RO_INIT(access_dev_, sub_offset_, sub_size_, \ - parent_sz_) \ - XLATE_REGION_DEV_INIT(access_dev_, sub_offset_, \ - sub_size_, parent_sz_, &xlate_rdev_ro_ops), \ +#define XLATE_REGION_DEV_RO_INIT(window_arr_, parent_sz_) \ + XLATE_REGION_DEV_INIT(window_arr_, parent_sz_, &xlate_rdev_ro_ops)
-#define XLATE_REGION_DEV_RW_INIT(access_dev_, sub_offset_, sub_size_, \ - parent_sz_) \ - XLATE_REGION_DEV_INIT(access_dev_, sub_offset_, \ - sub_size_, parent_sz_, &xlate_rdev_rw_ops), \ +#define XLATE_REGION_DEV_RW_INIT(window_count_, window_arr_, parent_sz_) \ + XLATE_REGION_DEV_INIT(window_arr_, parent_sz_, &xlate_rdev_rw_ops)
/* Helper to dynamically initialize xlate region device. */ void xlate_region_device_ro_init(struct xlate_region_device *xdev, - const struct region_device *access_dev, - size_t sub_offset, size_t sub_size, + size_t window_count, const struct xlate_window *window_arr, size_t parent_size);
void xlate_region_device_rw_init(struct xlate_region_device *xdev, - const struct region_device *access_dev, - size_t sub_offset, size_t sub_size, + size_t window_count, const struct xlate_window *window_arr, size_t parent_size);
+void xlate_window_init(struct xlate_window *window, const struct region_device *access_dev, + size_t sub_region_offset, size_t sub_region_size); + /* This type can be used for incoherent access where the read and write * operations are backed by separate drivers. An example is x86 systems * with memory mapped media for reading but use a spi flash driver for diff --git a/src/commonlib/region.c b/src/commonlib/region.c index 467e8ff..a10702a 100644 --- a/src/commonlib/region.c +++ b/src/commonlib/region.c @@ -188,33 +188,37 @@
static void xlate_region_device_init(struct xlate_region_device *xdev, const struct region_device_ops *ops, - const struct region_device *access_dev, - size_t sub_offset, size_t sub_size, + size_t window_count, const struct xlate_window *window_arr, size_t parent_size) { memset(xdev, 0, sizeof(*xdev)); - xdev->access_dev = access_dev; - xdev->sub_region.offset = sub_offset; - xdev->sub_region.size = sub_size; + xdev->window_count = window_count; + xdev->window_arr = window_arr; region_device_init(&xdev->rdev, ops, 0, parent_size); }
void xlate_region_device_ro_init(struct xlate_region_device *xdev, - const struct region_device *access_dev, - size_t sub_offset, size_t sub_size, + size_t window_count, const struct xlate_window *window_arr, size_t parent_size) { - xlate_region_device_init(xdev, &xlate_rdev_ro_ops, access_dev, - sub_offset, sub_size, parent_size); + xlate_region_device_init(xdev, &xlate_rdev_ro_ops, window_count, window_arr, + parent_size); }
void xlate_region_device_rw_init(struct xlate_region_device *xdev, - const struct region_device *access_dev, - size_t sub_offset, size_t sub_size, + size_t window_count, const struct xlate_window *window_arr, size_t parent_size) { - xlate_region_device_init(xdev, &xlate_rdev_rw_ops, access_dev, - sub_offset, sub_size, parent_size); + xlate_region_device_init(xdev, &xlate_rdev_rw_ops, window_count, window_arr, + parent_size); +} + +void xlate_window_init(struct xlate_window *window, const struct region_device *access_dev, + size_t sub_region_offset, size_t sub_region_size) +{ + window->access_dev = access_dev; + window->sub_region.offset = sub_region_offset; + window->sub_region.size = sub_region_size; }
static void *mdev_mmap(const struct region_device *rd, size_t offset, @@ -321,6 +325,21 @@ return 0; }
+static const struct xlate_window *xlate_find_window(const struct xlate_region_device *xldev, + const struct region *req) +{ + size_t i; + const struct xlate_window *xlwindow; + + for (i = 0; i < xldev->window_count; i++) { + xlwindow = &xldev->window_arr[i]; + if (region_is_subregion(&xlwindow->sub_region, req)) + return xlwindow; + } + + return NULL; +} + static void *xlate_mmap(const struct region_device *rd, size_t offset, size_t size) { @@ -329,24 +348,29 @@ .offset = offset, .size = size, }; + const struct xlate_window *xlwindow;
xldev = container_of(rd, __typeof__(*xldev), rdev);
- if (!region_is_subregion(&xldev->sub_region, &req)) + xlwindow = xlate_find_window(xldev, &req); + if (!xlwindow) return NULL;
- offset -= region_offset(&xldev->sub_region); + offset -= region_offset(&xlwindow->sub_region);
- return rdev_mmap(xldev->access_dev, offset, size); + return rdev_mmap(xlwindow->access_dev, offset, size); }
-static int xlate_munmap(const struct region_device *rd, void *mapping) +static int xlate_munmap(const struct region_device *rd __unused, void *mapping __unused) { - const struct xlate_region_device *xldev; - - xldev = container_of(rd, __typeof__(*xldev), rdev); - - return rdev_munmap(xldev->access_dev, mapping); + /* + * xlate_region_device does not keep track of the access device that was used to service + * a mmap request. So, munmap does not do anything. If munmap functionality is required, + * then xlate_region_device will have to be updated to accept some pre-allocated space + * from caller to keep track of the mapping requests. Since xlate_region_device is only + * used for memory mapped boot media on the backend right now, skipping munmap is fine. + */ + return 0; }
static ssize_t xlate_readat(const struct region_device *rd, void *b, @@ -356,16 +380,18 @@ .offset = offset, .size = size, }; + const struct xlate_window *xlwindow; const struct xlate_region_device *xldev;
xldev = container_of(rd, __typeof__(*xldev), rdev);
- if (!region_is_subregion(&xldev->sub_region, &req)) + xlwindow = xlate_find_window(xldev, &req); + if (!xlwindow) return -1;
- offset -= region_offset(&xldev->sub_region); + offset -= region_offset(&xlwindow->sub_region);
- return rdev_readat(xldev->access_dev, b, offset, size); + return rdev_readat(xlwindow->access_dev, b, offset, size); }
static ssize_t xlate_writeat(const struct region_device *rd, const void *b, @@ -375,16 +401,18 @@ .offset = offset, .size = size, }; + const struct xlate_window *xlwindow; const struct xlate_region_device *xldev;
xldev = container_of(rd, __typeof__(*xldev), rdev);
- if (!region_is_subregion(&xldev->sub_region, &req)) + xlwindow = xlate_find_window(xldev, &req); + if (!xlwindow) return -1;
- offset -= region_offset(&xldev->sub_region); + offset -= region_offset(&xlwindow->sub_region);
- return rdev_writeat(xldev->access_dev, b, offset, size); + return rdev_writeat(xlwindow->access_dev, b, offset, size); }
static ssize_t xlate_eraseat(const struct region_device *rd, @@ -394,16 +422,18 @@ .offset = offset, .size = size, }; + const struct xlate_window *xlwindow; const struct xlate_region_device *xldev;
xldev = container_of(rd, __typeof__(*xldev), rdev);
- if (!region_is_subregion(&xldev->sub_region, &req)) + xlwindow = xlate_find_window(xldev, &req); + if (!xlwindow) return -1;
- offset -= region_offset(&xldev->sub_region); + offset -= region_offset(&xlwindow->sub_region);
- return rdev_eraseat(xldev->access_dev, offset, size); + return rdev_eraseat(xlwindow->access_dev, offset, size); }
const struct region_device_ops xlate_rdev_ro_ops = { diff --git a/src/soc/intel/apollolake/mmap_boot.c b/src/soc/intel/apollolake/mmap_boot.c index 30c629e..79c2a07 100644 --- a/src/soc/intel/apollolake/mmap_boot.c +++ b/src/soc/intel/apollolake/mmap_boot.c @@ -43,6 +43,7 @@
static struct mem_region_device shadow_dev; static struct xlate_region_device real_dev; +static struct xlate_window real_dev_window;
static void bios_mmap_init(void) { @@ -68,9 +69,8 @@ mem_region_device_ro_init(&shadow_dev, (void *)base, bios_mapped_size);
- xlate_region_device_ro_init(&real_dev, &shadow_dev.rdev, - start, bios_mapped_size, - CONFIG_ROM_SIZE); + xlate_window_init(&real_dev_window, &shadow_dev.rdev, start, bios_mapped_size); + xlate_region_device_ro_init(&real_dev, 1, &real_dev_window, CONFIG_ROM_SIZE);
bios_size = size;
@@ -78,7 +78,7 @@ easy to forget the SRAM mapping when crafting an FMAP file. */ struct region cbfs_region; if (!fmap_locate_area("COREBOOT", &cbfs_region) && - !region_is_subregion(&real_dev.sub_region, &cbfs_region)) + !region_is_subregion(&real_dev_window.sub_region, &cbfs_region)) printk(BIOS_CRIT, "ERROR: CBFS @ %zx size %zx exceeds mem-mapped area @ %zx size %zx\n", region_offset(&cbfs_region), region_sz(&cbfs_region),
-
Duncan Laurie (Code Review) -
Furquan Shaikh (Code Review)
-
Srinidhi N Kaushik (Code Review)
-
Tim Wawrzynczak (Code Review)