| Message ID | 20200224205533.23798-19-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw: Sanitize various MemoryRegion calls | expand |
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é <philmd@redhat.com> > --- > 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, >
On 25/02/20 11:05, Philippe Mathieu-Daudé wrote: >> >> 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? Old->new is broken because the "isa-bios" memory region is not found. qemu-system-x86_64: Unknown ramblock "isa-bios", cannot accept migration qemu-system-x86_64: error while loading state for instance 0x0 of device 'ram' qemu-system-x86_64: load of migration failed: Invalid argument Paolo
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,
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 Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- hw/i386/pc_sysfw.c | 24 ++++++------------------ 1 file changed, 6 insertions(+), 18 deletions(-)