Re: [PATCH 4/4] hw/i386: Consolidate isa-bios creation

[Bernhard Beschow]

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);
>> +}
>