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

["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);

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