[PATCH 0/4] X86: Alias isa-bios area and clean up

[PATCH 0/4] X86: Alias isa-bios area and clean up

[Bernhard Beschow]

This series changes the "isa-bios" MemoryRegion to be an alias rather than a
copy in the pflash case. This fixes issuing pflash commands in the isa-bios
region which matches real hardware and which some real-world legacy bioses I'm
running rely on. Furthermore, aliasing in the isa-bios area is already the
current behavior in the bios (a.k.a. ROM) case, so this series consolidates
behavior.

The consolidateion results in duplicate code which is resolved in the second
half (patches 3 and 4) in this series.

Question: AFAIU, patch 2 changes the behavior for SEV-enabled guests since the
isa-bios area is now encrypted. Does this need compat machinery or is it a
bugfix?

Testing done:
* `make check` with qemu-system-x86_64 (QEMU 8.2.2) installed. All tests
  including migration tests pass.
* `make check-avocado`

Best regards,
Bernhard

Bernhard Beschow (4):
  hw/i386/pc_sysfw: Remove unused parameter from pc_isa_bios_init()
  hw/i386/pc_sysfw: Alias rather than copy isa-bios region
  hw/i386/x86: Eliminate two if statements in x86_bios_rom_init()
  hw/i386: Consolidate isa-bios creation

 include/hw/i386/x86.h |  2 ++
 hw/i386/pc_sysfw.c    | 38 ++++----------------------------------
 hw/i386/x86.c         | 35 +++++++++++++++++++----------------
 3 files changed, 25 insertions(+), 50 deletions(-)

-- 
2.44.0

[PATCH 1/4] hw/i386/pc_sysfw: Remove unused parameter from pc_isa_bios_init()

[Bernhard Beschow]

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/i386/pc_sysfw.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index 3efabbbab2..87b5bf59d6 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -41,8 +41,7 @@
 #define FLASH_SECTOR_SIZE 4096
 
 static void pc_isa_bios_init(MemoryRegion *rom_memory,
-                             MemoryRegion *flash_mem,
-                             int ram_size)
+                             MemoryRegion *flash_mem)
 {
     int isa_bios_size;
     MemoryRegion *isa_bios;
@@ -186,7 +185,7 @@ 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, size);
+            pc_isa_bios_init(rom_memory, flash_mem);
 
             /* Encrypt the pflash boot ROM */
             if (sev_enabled()) {
-- 
2.44.0

[PATCH 2/4] hw/i386/pc_sysfw: Alias rather than copy isa-bios region

[Bernhard Beschow]

In the -bios case the "isa-bios" memory region is an alias to the BIOS mapped
to the top of the 4G memory boundary. Do the same in the -pflash case to have
common behavior. This also makes pflash commands work in the "isa-bios" region
which some real-world legacy bioses rely on.

Note that in the sev_enabled() case, the "isa-bios" memory region in the -pflash
case will now also point to encrypted memory, just like it already does in the
-bios case.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/i386/pc_sysfw.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index 87b5bf59d6..6e89671c26 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -46,27 +46,18 @@ static void pc_isa_bios_init(MemoryRegion *rom_memory,
     int isa_bios_size;
     MemoryRegion *isa_bios;
     uint64_t flash_size;
-    void *flash_ptr, *isa_bios_ptr;
 
     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);
 }
 
-- 
2.44.0

[PATCH 3/4] hw/i386/x86: Eliminate two if statements in x86_bios_rom_init()

[Bernhard Beschow]

Given that memory_region_set_readonly() is a no-op when the readonlyness is
already as requested it is possible to simplify the pattern

  if (condition) {
    foo(true);
  }

to

  foo(condition);

which is shorter and allows to see the invariant of the code more easily.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/i386/x86.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index ffbda48917..32cd22a4a8 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -1171,9 +1171,7 @@ void x86_bios_rom_init(MachineState *ms, const char *default_firmware,
         load_image_size(filename, ptr, bios_size);
         x86_firmware_configure(ptr, bios_size);
     } else {
-        if (!isapc_ram_fw) {
-            memory_region_set_readonly(bios, true);
-        }
+        memory_region_set_readonly(bios, !isapc_ram_fw);
         ret = rom_add_file_fixed(bios_name, (uint32_t)(-bios_size), -1);
         if (ret != 0) {
             goto bios_error;
@@ -1190,9 +1188,7 @@ void x86_bios_rom_init(MachineState *ms, const char *default_firmware,
                                         0x100000 - isa_bios_size,
                                         isa_bios,
                                         1);
-    if (!isapc_ram_fw) {
-        memory_region_set_readonly(isa_bios, true);
-    }
+    memory_region_set_readonly(isa_bios, !isapc_ram_fw);
 
     /* map all the bios at the top of memory */
     memory_region_add_subregion(rom_memory,
-- 
2.44.0

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

[Bernhard Beschow]

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/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,
-- 
2.44.0

Re: [PATCH 0/4] X86: Alias isa-bios area and clean up

[Bernhard Beschow]

+Michael 

Am 22. April 2024 20:06:21 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>This series changes the "isa-bios" MemoryRegion to be an alias rather than a
>
>copy in the pflash case. This fixes issuing pflash commands in the isa-bios
>
>region which matches real hardware and which some real-world legacy bioses I'm
>
>running rely on. Furthermore, aliasing in the isa-bios area is already the
>
>current behavior in the bios (a.k.a. ROM) case, so this series consolidates
>
>behavior.
>
>
>
>The consolidateion results in duplicate code which is resolved in the second
>
>half (patches 3 and 4) in this series.
>
>
>
>Question: AFAIU, patch 2 changes the behavior for SEV-enabled guests since the
>
>isa-bios area is now encrypted. Does this need compat machinery or is it a
>
>bugfix?
>
>
>
>Testing done:
>
>* `make check` with qemu-system-x86_64 (QEMU 8.2.2) installed. All tests
>
>  including migration tests pass.
>
>* `make check-avocado`
>
>
>
>Best regards,
>
>Bernhard
>
>
>
>Bernhard Beschow (4):
>
>  hw/i386/pc_sysfw: Remove unused parameter from pc_isa_bios_init()
>
>  hw/i386/pc_sysfw: Alias rather than copy isa-bios region
>
>  hw/i386/x86: Eliminate two if statements in x86_bios_rom_init()
>
>  hw/i386: Consolidate isa-bios creation
>
>
>
> include/hw/i386/x86.h |  2 ++
>
> hw/i386/pc_sysfw.c    | 38 ++++----------------------------------
>
> hw/i386/x86.c         | 35 +++++++++++++++++++----------------
>
> 3 files changed, 25 insertions(+), 50 deletions(-)
>
>
>
>-- >
>2.44.0
>
>
>

Re: [PATCH 1/4] hw/i386/pc_sysfw: Remove unused parameter from pc_isa_bios_init()

[Philippe Mathieu-Daudé]

On 22/4/24 22:06, Bernhard Beschow wrote:
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/i386/pc_sysfw.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 3/4] hw/i386/x86: Eliminate two if statements in x86_bios_rom_init()

[Philippe Mathieu-Daudé]

On 22/4/24 22:06, Bernhard Beschow wrote:
> Given that memory_region_set_readonly() is a no-op when the readonlyness is
> already as requested it is possible to simplify the pattern
> 
>    if (condition) {
>      foo(true);
>    }
> 
> to
> 
>    foo(condition);
> 
> which is shorter and allows to see the invariant of the code more easily.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/i386/x86.c | 8 ++------
>   1 file changed, 2 insertions(+), 6 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

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

[Philippe Mathieu-Daudé]

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

Re: [PATCH 0/4] X86: Alias isa-bios area and clean up

[Philippe Mathieu-Daudé]

On 22/4/24 22:06, Bernhard Beschow wrote:
> This series changes the "isa-bios" MemoryRegion to be an alias rather than a
> copy in the pflash case. This fixes issuing pflash commands in the isa-bios
> region which matches real hardware and which some real-world legacy bioses I'm
> running rely on. Furthermore, aliasing in the isa-bios area is already the
> current behavior in the bios (a.k.a. ROM) case, so this series consolidates
> behavior.


> Bernhard Beschow (4):
>    hw/i386/pc_sysfw: Remove unused parameter from pc_isa_bios_init()

To reduce respin churn, I'm queuing the first patch via hw-misc.

Re: [PATCH 0/4] X86: Alias isa-bios area and clean up

[Michael S. Tsirkin]

On Mon, Apr 22, 2024 at 10:06:21PM +0200, Bernhard Beschow wrote:
> This series changes the "isa-bios" MemoryRegion to be an alias rather than a
> copy in the pflash case. This fixes issuing pflash commands in the isa-bios
> region which matches real hardware and which some real-world legacy bioses I'm
> running rely on. Furthermore, aliasing in the isa-bios area is already the
> current behavior in the bios (a.k.a. ROM) case, so this series consolidates
> behavior.
> 
> The consolidateion results in duplicate code which is resolved in the second
> half (patches 3 and 4) in this series.
> 
> Question: AFAIU, patch 2 changes the behavior for SEV-enabled guests since the
> isa-bios area is now encrypted. Does this need compat machinery or is it a
> bugfix?

When in doubt, do a compat thing.

> Testing done:
> * `make check` with qemu-system-x86_64 (QEMU 8.2.2) installed. All tests
>   including migration tests pass.
> * `make check-avocado`
> 
> Best regards,
> Bernhard
> 
> Bernhard Beschow (4):
>   hw/i386/pc_sysfw: Remove unused parameter from pc_isa_bios_init()
>   hw/i386/pc_sysfw: Alias rather than copy isa-bios region
>   hw/i386/x86: Eliminate two if statements in x86_bios_rom_init()
>   hw/i386: Consolidate isa-bios creation
> 
>  include/hw/i386/x86.h |  2 ++
>  hw/i386/pc_sysfw.c    | 38 ++++----------------------------------
>  hw/i386/x86.c         | 35 +++++++++++++++++++----------------
>  3 files changed, 25 insertions(+), 50 deletions(-)
> 
> -- 
> 2.44.0
> 

Re: [PATCH 0/4] X86: Alias isa-bios area and clean up

[Bernhard Beschow]

Am 25. April 2024 10:16:10 UTC schrieb "Michael S. Tsirkin" <mst@redhat.com>:
>On Mon, Apr 22, 2024 at 10:06:21PM +0200, Bernhard Beschow wrote:
>> This series changes the "isa-bios" MemoryRegion to be an alias rather than a
>> copy in the pflash case. This fixes issuing pflash commands in the isa-bios
>> region which matches real hardware and which some real-world legacy bioses I'm
>> running rely on. Furthermore, aliasing in the isa-bios area is already the
>> current behavior in the bios (a.k.a. ROM) case, so this series consolidates
>> behavior.
>> 
>> The consolidateion results in duplicate code which is resolved in the second
>> half (patches 3 and 4) in this series.
>> 
>> Question: AFAIU, patch 2 changes the behavior for SEV-enabled guests since the
>> isa-bios area is now encrypted. Does this need compat machinery or is it a
>> bugfix?
>
>When in doubt, do a compat thing.

Will do.

Thanks,
Bernhard

>
>> Testing done:
>> * `make check` with qemu-system-x86_64 (QEMU 8.2.2) installed. All tests
>>   including migration tests pass.
>> * `make check-avocado`
>> 
>> Best regards,
>> Bernhard
>> 
>> Bernhard Beschow (4):
>>   hw/i386/pc_sysfw: Remove unused parameter from pc_isa_bios_init()
>>   hw/i386/pc_sysfw: Alias rather than copy isa-bios region
>>   hw/i386/x86: Eliminate two if statements in x86_bios_rom_init()
>>   hw/i386: Consolidate isa-bios creation
>> 
>>  include/hw/i386/x86.h |  2 ++
>>  hw/i386/pc_sysfw.c    | 38 ++++----------------------------------
>>  hw/i386/x86.c         | 35 +++++++++++++++++++----------------
>>  3 files changed, 25 insertions(+), 50 deletions(-)
>> 
>> -- 
>> 2.44.0
>> 
>

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

Re: [PATCH 0/4] X86: Alias isa-bios area and clean up

[Bernhard Beschow]

Am 25. April 2024 08:07:43 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>On 22/4/24 22:06, Bernhard Beschow wrote:
>> This series changes the "isa-bios" MemoryRegion to be an alias rather than a
>> copy in the pflash case. This fixes issuing pflash commands in the isa-bios
>> region which matches real hardware and which some real-world legacy bioses I'm
>> running rely on. Furthermore, aliasing in the isa-bios area is already the
>> current behavior in the bios (a.k.a. ROM) case, so this series consolidates
>> behavior.
>
>
>> Bernhard Beschow (4):
>>    hw/i386/pc_sysfw: Remove unused parameter from pc_isa_bios_init()
>
>To reduce respin churn, I'm queuing the first patch via hw-misc.

Nice!