[PATCH 0/4] hw/block/pflash_cfi01: Remove pflash_cfi01_get_memory()

[PATCH 0/4] hw/block/pflash_cfi01: Remove pflash_cfi01_get_memory()

[Philippe Mathieu-Daudé]

TYPE_PFLASH_CFI01 is a TYPE_SYS_BUS_DEVICE which registers its romd
MemoryRegion with sysbus_init_mmio(), so we can use the generic
sysbus_mmio_get_region() to get the region, no need for a specific
pflash_cfi01_get_memory() helper.

First replace the few pflash_cfi01_get_memory() uses by
sysbus_mmio_get_region(), then remove the now unused helper.

Philippe Mathieu-Daudé (4):
  hw/i386/pc: Get pflash MemoryRegion with sysbus_mmio_get_region()
  hw/mips/malta: Get pflash MemoryRegion with sysbus_mmio_get_region()
  hw/xtensa/xtfpga: Get pflash MemoryRegion with
    sysbus_mmio_get_region()
  hw/block/pflash_cfi01: Remove pflash_cfi01_get_memory()

 include/hw/block/flash.h | 1 -
 hw/block/pflash_cfi01.c  | 5 -----
 hw/i386/pc_sysfw.c       | 2 +-
 hw/mips/malta.c          | 2 +-
 hw/xtensa/xtfpga.c       | 3 ++-
 5 files changed, 4 insertions(+), 9 deletions(-)

-- 
2.26.2

[PATCH 1/4] hw/i386/pc: Get pflash MemoryRegion with sysbus_mmio_get_region()

[Philippe Mathieu-Daudé]

TYPE_PFLASH_CFI01 is a TYPE_SYS_BUS_DEVICE which registers its romd
MemoryRegion with sysbus_init_mmio(), so we can use the generic
sysbus_mmio_get_region() to get the region, no need for a specific
pflash_cfi01_get_memory() helper.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/i386/pc_sysfw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index 9fe72b370e8..caad9cbd5eb 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -297,7 +297,7 @@ static void pc_system_flash_map(PCMachineState *pcms,
                         0x100000000ULL - total_size);
 
         if (i == 0) {
-            flash_mem = pflash_cfi01_get_memory(system_flash);
+            flash_mem = sysbus_mmio_get_region(SYS_BUS_DEVICE(system_flash), 0);
             pc_isa_bios_init(rom_memory, flash_mem, size);
 
             /* Encrypt the pflash boot ROM */
-- 
2.26.2

Re: [PATCH 1/4] hw/i386/pc: Get pflash MemoryRegion with sysbus_mmio_get_region()

[Philippe Mathieu-Daudé]

Michael, Paolo, if you Ack this patch I can queue the series
via pflash-next.

On 3/7/21 11:26 PM, Philippe Mathieu-Daudé wrote:
> TYPE_PFLASH_CFI01 is a TYPE_SYS_BUS_DEVICE which registers its romd
> MemoryRegion with sysbus_init_mmio(), so we can use the generic
> sysbus_mmio_get_region() to get the region, no need for a specific
> pflash_cfi01_get_memory() helper.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/i386/pc_sysfw.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
> index 9fe72b370e8..caad9cbd5eb 100644
> --- a/hw/i386/pc_sysfw.c
> +++ b/hw/i386/pc_sysfw.c
> @@ -297,7 +297,7 @@ static void pc_system_flash_map(PCMachineState *pcms,
>                          0x100000000ULL - total_size);
>  
>          if (i == 0) {
> -            flash_mem = pflash_cfi01_get_memory(system_flash);
> +            flash_mem = sysbus_mmio_get_region(SYS_BUS_DEVICE(system_flash), 0);
>              pc_isa_bios_init(rom_memory, flash_mem, size);
>  
>              /* Encrypt the pflash boot ROM */
> 

[PATCH 2/4] hw/mips/malta: Get pflash MemoryRegion with sysbus_mmio_get_region()

[Philippe Mathieu-Daudé]

TYPE_PFLASH_CFI01 is a TYPE_SYS_BUS_DEVICE which registers its romd
MemoryRegion with sysbus_init_mmio(), so we can use the generic
sysbus_mmio_get_region() to get the region, no need for a specific
pflash_cfi01_get_memory() helper.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/mips/malta.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/mips/malta.c b/hw/mips/malta.c
index 9afc0b427bf..43b985bccf9 100644
--- a/hw/mips/malta.c
+++ b/hw/mips/malta.c
@@ -1295,7 +1295,7 @@ void mips_malta_init(MachineState *machine)
                                dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
                                65536,
                                4, 0x0000, 0x0000, 0x0000, 0x0000, be);
-    bios = pflash_cfi01_get_memory(fl);
+    bios = sysbus_mmio_get_region(SYS_BUS_DEVICE(fl), 0);
     fl_idx++;
     if (kernel_filename) {
         ram_low_size = MIN(ram_size, 256 * MiB);
-- 
2.26.2

[PATCH 3/4] hw/xtensa/xtfpga: Get pflash MemoryRegion with sysbus_mmio_get_region()

[Philippe Mathieu-Daudé]

TYPE_PFLASH_CFI01 is a TYPE_SYS_BUS_DEVICE which registers its romd
MemoryRegion with sysbus_init_mmio(), so we can use the generic
sysbus_mmio_get_region() to get the region, no need for a specific
pflash_cfi01_get_memory() helper.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/xtensa/xtfpga.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/xtensa/xtfpga.c b/hw/xtensa/xtfpga.c
index 7be53f1895b..1d15a9aae9f 100644
--- a/hw/xtensa/xtfpga.c
+++ b/hw/xtensa/xtfpga.c
@@ -460,10 +460,11 @@ static void xtfpga_init(const XtfpgaBoardDesc *board, MachineState *machine)
         }
     } else {
         if (flash) {
-            MemoryRegion *flash_mr = pflash_cfi01_get_memory(flash);
+            MemoryRegion *flash_mr;
             MemoryRegion *flash_io = g_malloc(sizeof(*flash_io));
             uint32_t size = env->config->sysrom.location[0].size;
 
+            flash_mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(flash), 0);
             if (board->flash->size - board->flash->boot_base < size) {
                 size = board->flash->size - board->flash->boot_base;
             }
-- 
2.26.2

Re: [PATCH 3/4] hw/xtensa/xtfpga: Get pflash MemoryRegion with sysbus_mmio_get_region()

[Max Filippov]

On Sun, Mar 7, 2021 at 2:26 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> TYPE_PFLASH_CFI01 is a TYPE_SYS_BUS_DEVICE which registers its romd
> MemoryRegion with sysbus_init_mmio(), so we can use the generic
> sysbus_mmio_get_region() to get the region, no need for a specific
> pflash_cfi01_get_memory() helper.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/xtensa/xtfpga.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Acked-by: Max Filippov <jcmvbkbc@gmail.com>

-- 
Thanks.
-- Max

[PATCH 4/4] hw/block/pflash_cfi01: Remove pflash_cfi01_get_memory()

[Philippe Mathieu-Daudé]

Since we replaced all uses of pflash_cfi01_get_memory() with
sysbus_mmio_get_region(), we can now remove this unused helper.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/block/flash.h | 1 -
 hw/block/pflash_cfi01.c  | 5 -----
 2 files changed, 6 deletions(-)

diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h
index 7dde0adcee7..6c87b584c1d 100644
--- a/include/hw/block/flash.h
+++ b/include/hw/block/flash.h
@@ -22,7 +22,6 @@ PFlashCFI01 *pflash_cfi01_register(hwaddr base,
                                    uint16_t id2, uint16_t id3,
                                    int be);
 BlockBackend *pflash_cfi01_get_blk(PFlashCFI01 *fl);
-MemoryRegion *pflash_cfi01_get_memory(PFlashCFI01 *fl);
 void pflash_cfi01_legacy_drive(PFlashCFI01 *dev, DriveInfo *dinfo);
 
 /* pflash_cfi02.c */
diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 22287a1522e..d6d3d94ac68 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -984,11 +984,6 @@ BlockBackend *pflash_cfi01_get_blk(PFlashCFI01 *fl)
     return fl->blk;
 }
 
-MemoryRegion *pflash_cfi01_get_memory(PFlashCFI01 *fl)
-{
-    return &fl->mem;
-}
-
 /*
  * Handle -drive if=pflash for machines that use properties.
  * If @dinfo is null, do nothing.
-- 
2.26.2

Re: [PATCH 0/4] hw/block/pflash_cfi01: Remove pflash_cfi01_get_memory()

[Paolo Bonzini]

On 07/03/21 23:26, Philippe Mathieu-Daudé wrote:
> TYPE_PFLASH_CFI01 is a TYPE_SYS_BUS_DEVICE which registers its romd
> MemoryRegion with sysbus_init_mmio(), so we can use the generic
> sysbus_mmio_get_region() to get the region, no need for a specific
> pflash_cfi01_get_memory() helper.
> 
> First replace the few pflash_cfi01_get_memory() uses by
> sysbus_mmio_get_region(), then remove the now unused helper.

Why is this an improvement?  You're replacing nice and readable code 
with an implementation-dependent function whose second argument is a 
magic number.  The right patch would _add_ more of these helpers, not 
remove them.

Paolo

> Philippe Mathieu-Daudé (4):
>    hw/i386/pc: Get pflash MemoryRegion with sysbus_mmio_get_region()
>    hw/mips/malta: Get pflash MemoryRegion with sysbus_mmio_get_region()
>    hw/xtensa/xtfpga: Get pflash MemoryRegion with
>      sysbus_mmio_get_region()
>    hw/block/pflash_cfi01: Remove pflash_cfi01_get_memory()
> 
>   include/hw/block/flash.h | 1 -
>   hw/block/pflash_cfi01.c  | 5 -----
>   hw/i386/pc_sysfw.c       | 2 +-
>   hw/mips/malta.c          | 2 +-
>   hw/xtensa/xtfpga.c       | 3 ++-
>   5 files changed, 4 insertions(+), 9 deletions(-)
> 

Re: [PATCH 0/4] hw/block/pflash_cfi01: Remove pflash_cfi01_get_memory()

[Peter Maydell]

On Mon, 15 Mar 2021 at 11:34, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 07/03/21 23:26, Philippe Mathieu-Daudé wrote:
> > TYPE_PFLASH_CFI01 is a TYPE_SYS_BUS_DEVICE which registers its romd
> > MemoryRegion with sysbus_init_mmio(), so we can use the generic
> > sysbus_mmio_get_region() to get the region, no need for a specific
> > pflash_cfi01_get_memory() helper.
> >
> > First replace the few pflash_cfi01_get_memory() uses by
> > sysbus_mmio_get_region(), then remove the now unused helper.
>
> Why is this an improvement?  You're replacing nice and readable code
> with an implementation-dependent function whose second argument is a
> magic number.  The right patch would _add_ more of these helpers, not
> remove them.

I agree that sysbus_mmio_get_region()'s use of arbitrary
integers is unfortunate (we should look at improving that
to use usefully named regions I guess), but I don't see
why pflash_cfi01 should expose its MemoryRegion to users
in a different way to every other sysbus device.

thanks
-- PMM

Re: [PATCH 0/4] hw/block/pflash_cfi01: Remove pflash_cfi01_get_memory()

[Philippe Mathieu-Daudé]

On 3/15/21 1:08 PM, Peter Maydell wrote:
> On Mon, 15 Mar 2021 at 11:34, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> On 07/03/21 23:26, Philippe Mathieu-Daudé wrote:
>>> TYPE_PFLASH_CFI01 is a TYPE_SYS_BUS_DEVICE which registers its romd
>>> MemoryRegion with sysbus_init_mmio(), so we can use the generic
>>> sysbus_mmio_get_region() to get the region, no need for a specific
>>> pflash_cfi01_get_memory() helper.
>>>
>>> First replace the few pflash_cfi01_get_memory() uses by
>>> sysbus_mmio_get_region(), then remove the now unused helper.
>>
>> Why is this an improvement?  You're replacing nice and readable code
>> with an implementation-dependent function whose second argument is a
>> magic number.  The right patch would _add_ more of these helpers, not
>> remove them.
> 
> I agree that sysbus_mmio_get_region()'s use of arbitrary
> integers is unfortunate (we should look at improving that
> to use usefully named regions I guess), but I don't see
> why pflash_cfi01 should expose its MemoryRegion to users
> in a different way to every other sysbus device.

It is used that way (x86/pc):

        if (i == 0) {
            flash_mem = pflash_cfi01_get_memory(system_flash);
            pc_isa_bios_init(rom_memory, flash_mem, size);

            /* Encrypt the pflash boot ROM */
            if (sev_enabled()) {
                flash_ptr = memory_region_get_ram_ptr(flash_mem);
                flash_size = memory_region_size(flash_mem);
                /*
                 * OVMF places a GUIDed structures in the flash, so
                 * search for them
                 */
                pc_system_parse_ovmf_flash(flash_ptr, flash_size);

                ret = sev_es_save_reset_vector(flash_ptr, flash_size);

The problems I see:

- pflash_cfi01_get_memory() doesn't really document what it returns,
  simply an internal MemoryRegion* in pflash device. Neither we
  document this is a ROMD device providing a RAM buffer initialized
  by qemu_ram_alloc().

- to update the flash content, we get the internal buffer via
  memory_region_get_ram_ptr(). If the pflash implementation is
  changed (.i.e. reworked to expose a MR container) we break
  everything.

- memory_region_get_ram_ptr() doesn't do any check on the MR type,
  it simply calls qemu_map_ram_ptr(mr->ram_block, offset).

I agree with Peter pflash_cfi01_get_memory() has nothing special.

Now what if we want a safer function to access pflash internal
buffer, I'd prefer we use an explicit function such:

  /**
   * pflash_cfi01_get_ram_ptr_size: Return information on eventual RAMBlock
   *                                associated with the device
   *
   * @pfl: the flash device being queried.
   * @ptr: optional pointer to hold the ram address associated with the
RAMBlock
   * @size: optional pointer to hold length of the RAMBlock
   * Return %true on success, %false on failure.
   */
  bool pflash_cfi01_get_ram_ptr_size(PFlashCFI01 *pfl,
                                     void **ptr, uint64_t *size);

Thoughts?

Re: [PATCH 0/4] hw/block/pflash_cfi01: Remove pflash_cfi01_get_memory()

[Peter Maydell]

On Tue, 7 Sept 2021 at 15:45, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> The problems I see:
>
> - pflash_cfi01_get_memory() doesn't really document what it returns,
>   simply an internal MemoryRegion* in pflash device. Neither we
>   document this is a ROMD device providing a RAM buffer initialized
>   by qemu_ram_alloc().
>
> - to update the flash content, we get the internal buffer via
>   memory_region_get_ram_ptr(). If the pflash implementation is
>   changed (.i.e. reworked to expose a MR container) we break
>   everything.
>
> - memory_region_get_ram_ptr() doesn't do any check on the MR type,
>   it simply calls qemu_map_ram_ptr(mr->ram_block, offset).

Using memory_region_get_ram_ptr() is tricky to get right, too --
if you're writing directly to the underlying ram while the system is
running you need to use memory_region_flush_rom_device() to make
sure it's marked dirty. I think the current users of this on the
pflash devices get away with it because they do it during initial
machine init.

-- PMM