| Message ID | 20210810134050.396747-2-david.edmondson@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/pflash_cfi01: Allow an administrator to reduce the memory consumption of flash devices | expand |
Hi David, On 8/10/21 3:40 PM, 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. > > Signed-off-by: David Edmondson <david.edmondson@oracle.com> > --- > hw/block/pflash_cfi01.c | 105 ++++++++++++++++++++++++++++++++-------- > hw/block/trace-events | 3 ++ > 2 files changed, 87 insertions(+), 21 deletions(-) > > diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c > index 81f9f971d8..f3289b6a2f 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; > @@ -434,7 +436,6 @@ static inline void pflash_data_write(PFlashCFI01 *pfl, hwaddr offset, > } > break; > } > - > } > > static void pflash_write(PFlashCFI01 *pfl, hwaddr offset, > @@ -656,8 +657,44 @@ 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; This seems to break the "width" and "old-multiple-chip-handling" parameters ("emulating a number of flash devices wired up in parallel"). Also this breaks booting with SEV enabled on X86... See commits 9617cddb726 ("pc: add parser for OVMF reset block") and b2f73a0784b ("sev/i386: Allow AP booting under SEV-ES"). > + 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 { > + warn_report_once("%s: " > + "attempt to write outside of the backing block device " > + "(offset " TARGET_FMT_plx ") ignored", > + pfl->name, addr); This doesn't seem acceptable on mainstream, see: https://www.mail-archive.com/qemu-devel@nongnu.org/msg607292.html > + 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)); > @@ -670,8 +707,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)); > @@ -800,7 +838,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; > > if (pfl->sector_len == 0) { > @@ -816,35 +854,60 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp) > return; > } > > - total_len = pfl->sector_len * pfl->nb_blocs; > - > - 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); > + outer_len = pfl->sector_len * pfl->nb_blocs; > > 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, "%s: " > + "block backend provides %" PRIu64 " bytes, " > + "device limited to %" PRIu64 " bytes", > + pfl->name, inner_len, outer_len); > + return; > + } > } else { > pfl->ro = false; > + inner_len = outer_len; > } > > + 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); Here you create an I/O region but name it "container" ... > + > + 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); ... then add it inside the previous region, so &pfl->mem is used as container (containing &pfl->mem_outer named "container"...). This is confusing. Anyhow we shouldn't add subregions to I/O regions but use real containers instead, creating the container with memory_region_init(), then adding subregions inside. > + > + 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); Have you audited no code uses: mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(pflash), 0); Because we'd need to change 0 -> 1. See also the problem with pflash_cfi01_get_memory(): https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg01988.html https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg02026.html > if (pfl->blk) { > - if (!blk_check_size_and_read_all(pfl->blk, pfl->storage, total_len, > - errp)) { > + int ret = blk_pread(pfl->blk, 0, pfl->storage, inner_len); > + > + if (ret < 0) { > + error_setg_errno(errp, -ret, > + "cannot read %" PRIu64 " " > + "bytes from block backend", inner_len); > vmstate_unregister_ram(&pfl->mem, DEVICE(pfl)); > return; > }
On Thursday, 2021-09-09 at 11:27:56 +02, Philippe Mathieu-Daudé wrote: > Hi David, Philippe, thank you for the careful and considered review. >> +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 { >> + warn_report_once("%s: " >> + "attempt to write outside of the backing block device " >> + "(offset " TARGET_FMT_plx ") ignored", >> + pfl->name, addr); > > This doesn't seem acceptable on mainstream, see: > https://www.mail-archive.com/qemu-devel@nongnu.org/msg607292.html If this position still holds (it seems reasonable to assume so), then the approach in this patch isn't going to be useful for writeable pflash devices. With appropriate adjustment for your other comments, it may well be fine for read-only devices. Given that, I will go back to trying to find an approach that works for writeable devices, perhaps by always intercepting reads/writes and driving the block layer directly (while I previously had that working, the performance was poor, which I can re-examine).
diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c index 81f9f971d8..f3289b6a2f 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; @@ -434,7 +436,6 @@ static inline void pflash_data_write(PFlashCFI01 *pfl, hwaddr offset, } break; } - } static void pflash_write(PFlashCFI01 *pfl, hwaddr offset, @@ -656,8 +657,44 @@ 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 { + 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)); @@ -670,8 +707,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)); @@ -800,7 +838,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; if (pfl->sector_len == 0) { @@ -816,35 +854,60 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp) return; } - total_len = pfl->sector_len * pfl->nb_blocs; - - 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); + outer_len = pfl->sector_len * pfl->nb_blocs; 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, "%s: " + "block backend provides %" PRIu64 " bytes, " + "device limited to %" PRIu64 " bytes", + pfl->name, inner_len, outer_len); + return; + } } else { pfl->ro = false; + inner_len = outer_len; } + 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) { - if (!blk_check_size_and_read_all(pfl->blk, pfl->storage, total_len, - errp)) { + int ret = blk_pread(pfl->blk, 0, pfl->storage, inner_len); + + if (ret < 0) { + error_setg_errno(errp, -ret, + "cannot read %" PRIu64 " " + "bytes from block backend", inner_len); vmstate_unregister_ram(&pfl->mem, DEVICE(pfl)); return; } diff --git a/hw/block/trace-events b/hw/block/trace-events index d86b53520c..3d1e07261e 100644 --- a/hw/block/trace-events +++ b/hw/block/trace-events @@ -21,10 +21,13 @@ pflash_io_read(const char *name, uint64_t offset, unsigned int size, uint32_t va 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_mode_read_array(const char *name) "%s: read array mode" +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"
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 | 105 ++++++++++++++++++++++++++++++++-------- hw/block/trace-events | 3 ++ 2 files changed, 87 insertions(+), 21 deletions(-)