Re: [PATCH RESEND v2 18/32] hw/i386/pc_sysfw: Simplify using memory_region_init_alias()

[Philippe Mathieu-Daudé]

On 2/24/20 9:55 PM, Philippe Mathieu-Daudé wrote:
> The scripts/coccinelle/memory-region-housekeeping.cocci reported:
> * TODO 
> [[view:hw/i386/pc_sysfw.c::face=ovl-face1::linb=67::colb=4::cole=26][potential 
> use of memory_region_init_rom*() in  hw/i386/pc_sysfw.c::67]]
>
> pc_isa_bios_init() does a manual copy of a part of the BIOS,
> from a read-only region. We can simplify by directly aliasing
> the same part.
>
> Before:
>
>    (qemu) info mtree
>    memory-region: system
>      0000000000000000-ffffffffffffffff (prio 0, i/o): system
>        0000000000000000-0000000007ffffff (prio 0, ram): alias ram-below-4g 
> @pc.ram 0000000000000000-0000000007ffffff
>        0000000000000000-ffffffffffffffff (prio -1, i/o): pci
>          00000000000a0000-00000000000bffff (prio 1, i/o): vga-lowmem
>          00000000000c0000-00000000000dffff (prio 1, rom): pc.rom
>          00000000000e0000-00000000000fffff (prio 1, rom): isa-bios
>        ...
>        00000000fff00000-00000000ffffffff (prio 0, romd): system.flash0
>
> After:
>
>    (qemu) info mtree
>    memory-region: system
>      0000000000000000-ffffffffffffffff (prio 0, i/o): system
>        0000000000000000-0000000007ffffff (prio 0, ram): alias ram-below-4g 
> @pc.ram 0000000000000000-0000000007ffffff
>        0000000000000000-ffffffffffffffff (prio -1, i/o): pci
>          00000000000a0000-00000000000bffff (prio 1, i/o): vga-lowmem
>          00000000000c0000-00000000000dffff (prio 1, rom): pc.rom
>          00000000000e0000-00000000000fffff (prio 1, romd): alias isa-bios 
> @system.flash0 00000000000e0000-00000000000fffff
>        ...
>        00000000fff00000-00000000ffffffff (prio 0, romd): system.flash0

IIUC migrating old -> new is OK, the previous ROM copy is discarded.

What about new -> old, does it require specific handling? Do we care?

> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> ---
>   hw/i386/pc_sysfw.c | 24 ++++++------------------
>   1 file changed, 6 insertions(+), 18 deletions(-)
>
> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
> index f5f3f466b0..e864c09ea8 100644
> --- a/hw/i386/pc_sysfw.c
> +++ b/hw/i386/pc_sysfw.c
> @@ -54,31 +54,19 @@ static void pc_isa_bios_init(MemoryRegion *rom_memory,
>                                MemoryRegion *flash_mem,
>                                int ram_size)
>   {
> -    int isa_bios_size;
> -    MemoryRegion *isa_bios;
> -    uint64_t flash_size;
> -    void *flash_ptr, *isa_bios_ptr;
> -
> -    flash_size = memory_region_size(flash_mem);
> +    uint64_t isa_bios_size;
> +    MemoryRegion *isa_bios = g_new(MemoryRegion, 1);
> +    uint64_t flash_size = memory_region_size(flash_mem);
>   
>       /* map the last 128KB of the BIOS in ISA space */
>       isa_bios_size = MIN(flash_size, 128 * KiB);
> -    isa_bios = g_malloc(sizeof(*isa_bios));
> -    memory_region_init_ram(isa_bios, NULL, "isa-bios", isa_bios_size,
> -                           &error_fatal);
> +    memory_region_init_alias(isa_bios, NULL, "isa-bios", flash_mem,
> +                             flash_size - isa_bios_size,
> +                             isa_bios_size);
>       memory_region_add_subregion_overlap(rom_memory,
>                                           0x100000 - isa_bios_size,
>                                           isa_bios,
>                                           1);
> -
> -    /* copy ISA rom image from top of flash memory */
> -    flash_ptr = memory_region_get_ram_ptr(flash_mem);
> -    isa_bios_ptr = memory_region_get_ram_ptr(isa_bios);
> -    memcpy(isa_bios_ptr,
> -           ((uint8_t*)flash_ptr) + (flash_size - isa_bios_size),
> -           isa_bios_size);
> -
> -    memory_region_set_readonly(isa_bios, true);
>   }
>   
>   static PFlashCFI01 *pc_pflash_create(PCMachineState *pcms,