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

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(-)
> 

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

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?

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