diff mbox series

[RFC,v2,3/3] hw/pflash_cfi01: Allow devices to have a smaller backing device

Message ID 20210222090747.2310104-4-david.edmondson@oracle.com (mailing list archive)
State New, archived
Headers show
Series hw/flash_cfi01: Reduce memory consumption when flash image is smaller than region | expand

Commit Message

David Edmondson Feb. 22, 2021, 9:07 a.m. UTC 
Allow the backing device to be smaller than the extent of the flash
device by mapping it as a subregion of the flash device region.

Return zeroes for all reads of the flash device beyond the extent of
the backing device.

For writes beyond the extent of the underlying device, fail on
read-only devices and discard them for writable devices.

Signed-off-by: David Edmondson <david.edmondson@oracle.com>
---
 hw/block/pflash_cfi01.c | 108 ++++++++++++++++++++++++++++++----------
 hw/block/trace-events   |   3 ++
 2 files changed, 86 insertions(+), 25 deletions(-)

Comments

Philippe Mathieu-Daudé Feb. 22, 2021, 2:06 p.m. UTC | #1 
Hi David,

On 2/22/21 10:07 AM, David Edmondson wrote:
> Allow the backing device to be smaller than the extent of the flash
> device by mapping it as a subregion of the flash device region.
> 
> Return zeroes for all reads of the flash device beyond the extent of
> the backing device.
> 
> For writes beyond the extent of the underlying device, fail on
> read-only devices and discard them for writable devices.

This looks much simpler now.

> Signed-off-by: David Edmondson <david.edmondson@oracle.com>
> ---
>  hw/block/pflash_cfi01.c | 108 ++++++++++++++++++++++++++++++----------
>  hw/block/trace-events   |   3 ++
>  2 files changed, 86 insertions(+), 25 deletions(-)

>      if (pfl->blk) {
>          uint64_t perm;
> +
>          pfl->ro = !blk_supports_write_perm(pfl->blk);
>          perm = BLK_PERM_CONSISTENT_READ | (pfl->ro ? 0 : BLK_PERM_WRITE);
>          ret = blk_set_perm(pfl->blk, perm, BLK_PERM_ALL, errp);
>          if (ret < 0) {
>              return;
>          }
> +
> +        inner_len = blk_getlength(pfl->blk);
> +
> +        if (inner_len > outer_len) {
> +            error_setg(errp,
> +                       "block backend provides %" HWADDR_PRIu " bytes, "
> +                       "device limited to %" PRIu64 " bytes",
> +                       inner_len, outer_len);
> +            return;
> +        }

Do you mind extracting this change in a previous patch?

Thanks,

Phil.
David Edmondson Feb. 22, 2021, 2:31 p.m. UTC | #2 
On Monday, 2021-02-22 at 15:06:35 +01, Philippe Mathieu-Daudé wrote:

> Hi David,
>
> On 2/22/21 10:07 AM, David Edmondson wrote:
>> Allow the backing device to be smaller than the extent of the flash
>> device by mapping it as a subregion of the flash device region.
>> 
>> Return zeroes for all reads of the flash device beyond the extent of
>> the backing device.
>> 
>> For writes beyond the extent of the underlying device, fail on
>> read-only devices and discard them for writable devices.
>
> This looks much simpler now.

Thanks for looking at it.

>> Signed-off-by: David Edmondson <david.edmondson@oracle.com>
>> ---
>>  hw/block/pflash_cfi01.c | 108 ++++++++++++++++++++++++++++++----------
>>  hw/block/trace-events   |   3 ++
>>  2 files changed, 86 insertions(+), 25 deletions(-)
>
>>      if (pfl->blk) {
>>          uint64_t perm;
>> +
>>          pfl->ro = !blk_supports_write_perm(pfl->blk);
>>          perm = BLK_PERM_CONSISTENT_READ | (pfl->ro ? 0 : BLK_PERM_WRITE);
>>          ret = blk_set_perm(pfl->blk, perm, BLK_PERM_ALL, errp);
>>          if (ret < 0) {
>>              return;
>>          }
>> +
>> +        inner_len = blk_getlength(pfl->blk);
>> +
>> +        if (inner_len > outer_len) {
>> +            error_setg(errp,
>> +                       "block backend provides %" HWADDR_PRIu " bytes, "
>> +                       "device limited to %" PRIu64 " bytes",
>> +                       inner_len, outer_len);
>> +            return;
>> +        }
>
> Do you mind extracting this change in a previous patch?

Before this change that test was performed by
blk_check_size_and_read_all(), which insisted that the block device was
exactly the same size as the region (because it was told to read enough
bytes to fill the region).

Now that we pass the size of the backing device to that function, rather
than the size of the region, it's necessary to add the extra check
before the call.

Hence, I don't think that it's useful to add this as an earlier patch.

It would perhaps make sense to switch away from
blk_check_size_and_read_all(), preferring to just call blk_pread() now,
which would be cleaner?

dme.
diff mbox series

Patch

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 6b21b4af52..94970816a6 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -83,6 +83,8 @@  struct PFlashCFI01 {
     uint64_t counter;
     unsigned int writeblock_size;
     MemoryRegion mem;
+    MemoryRegion mem_outer;
+    char outer_name[64];
     char *name;
     void *storage;
     VMChangeStateEntry *vmstate;
@@ -425,7 +427,6 @@  static inline void pflash_data_write(PFlashCFI01 *pfl, hwaddr offset,
         }
         break;
     }
-
 }
 
 static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
@@ -646,8 +647,45 @@  static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
 }
 
 
-static MemTxResult pflash_mem_read_with_attrs(void *opaque, hwaddr addr, uint64_t *value,
-                                              unsigned len, MemTxAttrs attrs)
+static MemTxResult pflash_outer_read_with_attrs(void *opaque, hwaddr addr,
+                                                    uint64_t *value,
+                                                    unsigned len,
+                                                    MemTxAttrs attrs)
+{
+    PFlashCFI01 *pfl = opaque;
+
+    trace_pflash_outer_read(pfl->name, addr, len);
+    *value = 0;
+    return MEMTX_OK;
+}
+
+static MemTxResult pflash_outer_write_with_attrs(void *opaque, hwaddr addr,
+                                                     uint64_t value,
+                                                     unsigned len,
+                                                     MemTxAttrs attrs)
+{
+    PFlashCFI01 *pfl = opaque;
+
+    trace_pflash_outer_write(pfl->name, addr, len);
+    if (pfl->ro) {
+        return MEMTX_ERROR;
+    } else {
+        /* Discard writes. */
+        warn_report_once("%s: attempt to write outside of the backing block device "
+                         "(offset " TARGET_FMT_plx ") ignored", pfl->name, addr);
+        return MEMTX_OK;
+    }
+}
+
+static const MemoryRegionOps pflash_cfi01_outer_ops = {
+    .read_with_attrs = pflash_outer_read_with_attrs,
+    .write_with_attrs = pflash_outer_write_with_attrs,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static MemTxResult pflash_mem_read_with_attrs(void *opaque, hwaddr addr,
+                                              uint64_t *value, unsigned len,
+                                              MemTxAttrs attrs)
 {
     PFlashCFI01 *pfl = opaque;
     bool be = !!(pfl->features & (1 << PFLASH_BE));
@@ -660,8 +698,9 @@  static MemTxResult pflash_mem_read_with_attrs(void *opaque, hwaddr addr, uint64_
     return MEMTX_OK;
 }
 
-static MemTxResult pflash_mem_write_with_attrs(void *opaque, hwaddr addr, uint64_t value,
-                                               unsigned len, MemTxAttrs attrs)
+static MemTxResult pflash_mem_write_with_attrs(void *opaque, hwaddr addr,
+                                               uint64_t value, unsigned len,
+                                               MemTxAttrs attrs)
 {
     PFlashCFI01 *pfl = opaque;
     bool be = !!(pfl->features & (1 << PFLASH_BE));
@@ -684,7 +723,7 @@  static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
 {
     ERRP_GUARD();
     PFlashCFI01 *pfl = PFLASH_CFI01(dev);
-    uint64_t total_len;
+    uint64_t outer_len, inner_len;
     int ret;
     uint64_t blocks_per_device, sector_len_per_device, device_len;
     int num_devices;
@@ -702,7 +741,7 @@  static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    total_len = pfl->sector_len * pfl->nb_blocs;
+    outer_len = pfl->sector_len * pfl->nb_blocs;
 
     /* These are only used to expose the parameters of each device
      * in the cfi_table[].
@@ -717,36 +756,55 @@  static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
     }
     device_len = sector_len_per_device * blocks_per_device;
 
-    memory_region_init_rom_device(
-        &pfl->mem, OBJECT(dev),
-        &pflash_cfi01_ops,
-        pfl,
-        pfl->name, total_len, errp);
-    if (*errp) {
-        return;
-    }
-
-    pfl->storage = memory_region_get_ram_ptr(&pfl->mem);
-    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &pfl->mem);
-
     if (pfl->blk) {
         uint64_t perm;
+
         pfl->ro = !blk_supports_write_perm(pfl->blk);
         perm = BLK_PERM_CONSISTENT_READ | (pfl->ro ? 0 : BLK_PERM_WRITE);
         ret = blk_set_perm(pfl->blk, perm, BLK_PERM_ALL, errp);
         if (ret < 0) {
             return;
         }
+
+        inner_len = blk_getlength(pfl->blk);
+
+        if (inner_len > outer_len) {
+            error_setg(errp,
+                       "block backend provides %" HWADDR_PRIu " bytes, "
+                       "device limited to %" PRIu64 " bytes",
+                       inner_len, outer_len);
+            return;
+        }
     } else {
         pfl->ro = false;
+        inner_len = outer_len;
     }
 
-    if (pfl->blk) {
-        if (!blk_check_size_and_read_all(pfl->blk, pfl->storage, total_len,
-                                         errp)) {
-            vmstate_unregister_ram(&pfl->mem, DEVICE(pfl));
-            return;
-        }
+    trace_pflash_realize(pfl->name, pfl->ro, inner_len, outer_len);
+
+    snprintf(pfl->outer_name, sizeof(pfl->outer_name),
+             "%s container", pfl->name);
+    memory_region_init_io(&pfl->mem_outer, OBJECT(dev),
+                          &pflash_cfi01_outer_ops,
+                          pfl, pfl->outer_name, outer_len);
+
+    memory_region_init_rom_device(&pfl->mem, OBJECT(dev),
+                                  &pflash_cfi01_ops,
+                                  pfl, pfl->name, inner_len, errp);
+    if (*errp) {
+        return;
+    }
+
+    memory_region_add_subregion(&pfl->mem_outer, 0, &pfl->mem);
+
+    pfl->storage = memory_region_get_ram_ptr(&pfl->mem);
+    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &pfl->mem_outer);
+    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &pfl->mem);
+
+    if (pfl->blk &&
+        !blk_check_size_and_read_all(pfl->blk, pfl->storage, inner_len, errp)) {
+        vmstate_unregister_ram(&pfl->mem, DEVICE(pfl));
+        return;
     }
 
     /* Default to devices being used at their maximum device width. This was
diff --git a/hw/block/trace-events b/hw/block/trace-events
index a715a2e173..85b501e23e 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -17,10 +17,13 @@  pflash_erase_timeout(const char *name, int count) "%s: erase timeout fired; eras
 pflash_io_read(const char *name, uint64_t offset, unsigned int size, uint32_t value, uint8_t cmd, uint8_t wcycle) "%s: offset:0x%04" PRIx64 " size:%u value:0x%04x cmd:0x%02x wcycle:%u"
 pflash_io_write(const char *name, uint64_t offset, unsigned int size, uint32_t value, uint8_t wcycle) "%s: offset:0x%04"PRIx64" size:%u value:0x%04x wcycle:%u"
 pflash_manufacturer_id(const char *name, uint16_t id) "%s: read manufacturer ID: 0x%04x"
+pflash_outer_read(const char *name, uint64_t addr, unsigned int len) "%s: addr:0x%" PRIx64 " len:%d"
+pflash_outer_write(const char *name, uint64_t addr, unsigned int len) "%s: addr:0x%" PRIx64 " len:%d"
 pflash_postload_cb(const char *name)  "%s: updating bdrv"
 pflash_read_done(const char *name, uint64_t offset, uint64_t ret) "%s: ID:0x%" PRIx64 " ret:0x%" PRIx64
 pflash_read_status(const char *name, uint32_t ret) "%s: status:0x%x"
 pflash_read_unknown_state(const char *name, uint8_t cmd) "%s: unknown command state:0x%x"
+pflash_realize(const char *name, bool ro, uint64_t blk_len, uint64_t total_len) "%s: ro:%d blk_len:0x%" PRIx64 " total_len:0x%" PRIx64
 pflash_reset(const char *name) "%s: reset"
 pflash_sector_erase_start(const char *name, int width1, uint64_t start, int width2, uint64_t end) "%s: start sector erase at: 0x%0*" PRIx64 "-0x%0*" PRIx64
 pflash_timer_expired(const char *name, uint8_t cmd) "%s: command 0x%02x done"