| Message ID | 20240422200625.2768-5-shentey@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | X86: Alias isa-bios area and clean up | expand |
Hi Bernhard, On 22/4/24 22:06, Bernhard Beschow wrote: > Now that the -bios and -pflash code paths work the same it is possible to have a > common implementation. > > While at it convert the magic number 0x100000 (== 1MiB) to increase readability. > > Signed-off-by: Bernhard Beschow <shentey@gmail.com> > --- > include/hw/i386/x86.h | 2 ++ > hw/i386/pc_sysfw.c | 28 ++++------------------------ > hw/i386/x86.c | 29 ++++++++++++++++++----------- > 3 files changed, 24 insertions(+), 35 deletions(-) > diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c > index 6e89671c26..e529182b48 100644 > --- a/hw/i386/pc_sysfw.c > +++ b/hw/i386/pc_sysfw.c > @@ -28,7 +28,6 @@ > #include "sysemu/block-backend.h" > #include "qemu/error-report.h" > #include "qemu/option.h" > -#include "qemu/units.h" > #include "hw/sysbus.h" > #include "hw/i386/x86.h" > #include "hw/i386/pc.h" > @@ -40,27 +39,6 @@ > > #define FLASH_SECTOR_SIZE 4096 > > -static void pc_isa_bios_init(MemoryRegion *rom_memory, > - MemoryRegion *flash_mem) > -{ > - int isa_bios_size; > - MemoryRegion *isa_bios; > - uint64_t flash_size; > - > - 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_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); > - memory_region_set_readonly(isa_bios, true); > -} > diff --git a/hw/i386/x86.c b/hw/i386/x86.c > index 32cd22a4a8..7366b0cee4 100644 > --- a/hw/i386/x86.c > +++ b/hw/i386/x86.c > @@ -1136,13 +1136,28 @@ void x86_load_linux(X86MachineState *x86ms, > nb_option_roms++; > } > > +void x86_isa_bios_init(MemoryRegion *rom_memory, MemoryRegion *bios, > + bool isapc_ram_fw) > +{ > + int bios_size = memory_region_size(bios); > + int isa_bios_size = MIN(bios_size, 128 * KiB); > + MemoryRegion *isa_bios; > + > + isa_bios = g_malloc(sizeof(*isa_bios)); Pre-existing, but we shouldn't leak MR like that. Probably better to pass pre-allocated as argument, smth like: /** * x86_isa_bios_init: ... at fixed addr ... * @isa_bios: MR to initialize * @isa_mr: ISA bus * @bios: BIOS MR to map on ISA bus * @read_only: Map the BIOS as read-only */ void x86_isa_bios_init(MemoryRegion *isa_bios, const MemoryRegion *isa_mr, const MemoryRegion *bios, bool read_only); > + memory_region_init_alias(isa_bios, NULL, "isa-bios", bios, > + bios_size - isa_bios_size, isa_bios_size); > + memory_region_add_subregion_overlap(rom_memory, 1 * MiB - isa_bios_size, > + isa_bios, 1); > + memory_region_set_readonly(isa_bios, !isapc_ram_fw); > +}
Am 25. April 2024 07:19:27 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>: >Hi Bernhard, > >On 22/4/24 22:06, Bernhard Beschow wrote: >> Now that the -bios and -pflash code paths work the same it is possible to have a >> common implementation. >> >> While at it convert the magic number 0x100000 (== 1MiB) to increase readability. >> >> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >> --- >> include/hw/i386/x86.h | 2 ++ >> hw/i386/pc_sysfw.c | 28 ++++------------------------ >> hw/i386/x86.c | 29 ++++++++++++++++++----------- >> 3 files changed, 24 insertions(+), 35 deletions(-) > > >> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c >> index 6e89671c26..e529182b48 100644 >> --- a/hw/i386/pc_sysfw.c >> +++ b/hw/i386/pc_sysfw.c >> @@ -28,7 +28,6 @@ >> #include "sysemu/block-backend.h" >> #include "qemu/error-report.h" >> #include "qemu/option.h" >> -#include "qemu/units.h" >> #include "hw/sysbus.h" >> #include "hw/i386/x86.h" >> #include "hw/i386/pc.h" >> @@ -40,27 +39,6 @@ >> #define FLASH_SECTOR_SIZE 4096 >> -static void pc_isa_bios_init(MemoryRegion *rom_memory, >> - MemoryRegion *flash_mem) >> -{ >> - int isa_bios_size; >> - MemoryRegion *isa_bios; >> - uint64_t flash_size; >> - >> - 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_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); >> - memory_region_set_readonly(isa_bios, true); >> -} > > >> diff --git a/hw/i386/x86.c b/hw/i386/x86.c >> index 32cd22a4a8..7366b0cee4 100644 >> --- a/hw/i386/x86.c >> +++ b/hw/i386/x86.c >> @@ -1136,13 +1136,28 @@ void x86_load_linux(X86MachineState *x86ms, >> nb_option_roms++; >> } >> +void x86_isa_bios_init(MemoryRegion *rom_memory, MemoryRegion *bios, >> + bool isapc_ram_fw) >> +{ >> + int bios_size = memory_region_size(bios); >> + int isa_bios_size = MIN(bios_size, 128 * KiB); >> + MemoryRegion *isa_bios; >> + >> + isa_bios = g_malloc(sizeof(*isa_bios)); > >Pre-existing, but we shouldn't leak MR like that. > >Probably better to pass pre-allocated as argument, >smth like: > > /** > * x86_isa_bios_init: ... at fixed addr ... > * @isa_bios: MR to initialize > * @isa_mr: ISA bus > * @bios: BIOS MR to map on ISA bus > * @read_only: Map the BIOS as read-only > */ > void x86_isa_bios_init(MemoryRegion *isa_bios, > const MemoryRegion *isa_mr, > const MemoryRegion *bios, > bool read_only); > Same would apply for the "pc.bios" region. I'll fix both then. Best regards, Bernhard >> + memory_region_init_alias(isa_bios, NULL, "isa-bios", bios, >> + bios_size - isa_bios_size, isa_bios_size); >> + memory_region_add_subregion_overlap(rom_memory, 1 * MiB - isa_bios_size, >> + isa_bios, 1); >> + memory_region_set_readonly(isa_bios, !isapc_ram_fw); >> +} >
diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h index 4dc30dcb4d..8e6ba4a726 100644 --- a/include/hw/i386/x86.h +++ b/include/hw/i386/x86.h @@ -116,6 +116,8 @@ void x86_cpu_unplug_request_cb(HotplugHandler *hotplug_dev, void x86_cpu_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp); +void x86_isa_bios_init(MemoryRegion *rom_memory, MemoryRegion *bios, + bool isapc_ram_fw); void x86_bios_rom_init(MachineState *ms, const char *default_firmware, MemoryRegion *rom_memory, bool isapc_ram_fw); diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c index 6e89671c26..e529182b48 100644 --- a/hw/i386/pc_sysfw.c +++ b/hw/i386/pc_sysfw.c @@ -28,7 +28,6 @@ #include "sysemu/block-backend.h" #include "qemu/error-report.h" #include "qemu/option.h" -#include "qemu/units.h" #include "hw/sysbus.h" #include "hw/i386/x86.h" #include "hw/i386/pc.h" @@ -40,27 +39,6 @@ #define FLASH_SECTOR_SIZE 4096 -static void pc_isa_bios_init(MemoryRegion *rom_memory, - MemoryRegion *flash_mem) -{ - int isa_bios_size; - MemoryRegion *isa_bios; - uint64_t flash_size; - - 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_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); - memory_region_set_readonly(isa_bios, true); -} - static PFlashCFI01 *pc_pflash_create(PCMachineState *pcms, const char *name, const char *alias_prop_name) @@ -121,7 +99,7 @@ void pc_system_flash_cleanup_unused(PCMachineState *pcms) * pcms->max_fw_size. * * If pcms->flash[0] has a block backend, its memory is passed to - * pc_isa_bios_init(). Merging several flash devices for isa-bios is + * x86_isa_bios_init(). Merging several flash devices for isa-bios is * not supported. */ static void pc_system_flash_map(PCMachineState *pcms, @@ -176,7 +154,9 @@ static void pc_system_flash_map(PCMachineState *pcms, if (i == 0) { flash_mem = pflash_cfi01_get_memory(system_flash); - pc_isa_bios_init(rom_memory, flash_mem); + + /* Map the last 128KB of the BIOS in ISA space */ + x86_isa_bios_init(rom_memory, flash_mem, false); /* Encrypt the pflash boot ROM */ if (sev_enabled()) { diff --git a/hw/i386/x86.c b/hw/i386/x86.c index 32cd22a4a8..7366b0cee4 100644 --- a/hw/i386/x86.c +++ b/hw/i386/x86.c @@ -1136,13 +1136,28 @@ void x86_load_linux(X86MachineState *x86ms, nb_option_roms++; } +void x86_isa_bios_init(MemoryRegion *rom_memory, MemoryRegion *bios, + bool isapc_ram_fw) +{ + int bios_size = memory_region_size(bios); + int isa_bios_size = MIN(bios_size, 128 * KiB); + MemoryRegion *isa_bios; + + isa_bios = g_malloc(sizeof(*isa_bios)); + memory_region_init_alias(isa_bios, NULL, "isa-bios", bios, + bios_size - isa_bios_size, isa_bios_size); + memory_region_add_subregion_overlap(rom_memory, 1 * MiB - isa_bios_size, + isa_bios, 1); + memory_region_set_readonly(isa_bios, !isapc_ram_fw); +} + void x86_bios_rom_init(MachineState *ms, const char *default_firmware, MemoryRegion *rom_memory, bool isapc_ram_fw) { const char *bios_name; char *filename; - MemoryRegion *bios, *isa_bios; - int bios_size, isa_bios_size; + MemoryRegion *bios; + int bios_size; ssize_t ret; /* BIOS load */ @@ -1180,15 +1195,7 @@ void x86_bios_rom_init(MachineState *ms, const char *default_firmware, g_free(filename); /* map the last 128KB of the BIOS in ISA space */ - isa_bios_size = MIN(bios_size, 128 * KiB); - isa_bios = g_malloc(sizeof(*isa_bios)); - memory_region_init_alias(isa_bios, NULL, "isa-bios", bios, - bios_size - isa_bios_size, isa_bios_size); - memory_region_add_subregion_overlap(rom_memory, - 0x100000 - isa_bios_size, - isa_bios, - 1); - memory_region_set_readonly(isa_bios, !isapc_ram_fw); + x86_isa_bios_init(rom_memory, bios, isapc_ram_fw); /* map all the bios at the top of memory */ memory_region_add_subregion(rom_memory,
Now that the -bios and -pflash code paths work the same it is possible to have a common implementation. While at it convert the magic number 0x100000 (== 1MiB) to increase readability. Signed-off-by: Bernhard Beschow <shentey@gmail.com> --- include/hw/i386/x86.h | 2 ++ hw/i386/pc_sysfw.c | 28 ++++------------------------ hw/i386/x86.c | 29 ++++++++++++++++++----------- 3 files changed, 24 insertions(+), 35 deletions(-)