On 3/15/21 1:08 PM, Peter Maydell wrote:
> On Mon, 15 Mar 2021 at 11:34, Paolo Bonzini <pbonz...@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?
