Am 25. April 2024 07:19:27 UTC schrieb "Philippe Mathieu-Daudé"
<phi...@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 <shen...@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);
>> +}
>