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?