[Qemu-devel] ISA BIOS mapping for system flash emulation

[Qemu-devel] ISA BIOS mapping for system flash emulation

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 480 bytes --]

Hi Jordan,

I was starring at pc_isa_bios_init and wondering why you are creating a
copy of the system flash for the low ISA range instead of using an alias
here as well, just like old_pc_system_rom_init does. That means the ISA
BIOS range can run out of sync when the system flash is updated during
runtime and requires a restart of QEMU then. Switching to an alias would
also allow some code consolidation. Can you explain the idea behind the
current version?

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [Qemu-devel] ISA BIOS mapping for system flash emulation

[Jordan Justen]

On Tue, Oct 30, 2012 at 11:14 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
> Hi Jordan,
>
> I was starring at pc_isa_bios_init and wondering why you are creating a
> copy of the system flash for the low ISA range instead of using an alias
> here as well, just like old_pc_system_rom_init does. That means the ISA
> BIOS range can run out of sync when the system flash is updated during
> runtime and requires a restart of QEMU then. Switching to an alias would
> also allow some code consolidation. Can you explain the idea behind the
> current version?

I'm pretty sure I tried this and found that it did not work on the flash device.

I wrote an email to the list on Oct 17, 2011 about this, but I didn't
get a response. (Re: [Qemu-devel] [PATCH 2/4] pc: Support system flash
memory with pflash)

-Jordan

Re: [Qemu-devel] ISA BIOS mapping for system flash emulation

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 980 bytes --]

On 2012-11-01 03:55, Jordan Justen wrote:
> On Tue, Oct 30, 2012 at 11:14 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>> Hi Jordan,
>>
>> I was starring at pc_isa_bios_init and wondering why you are creating a
>> copy of the system flash for the low ISA range instead of using an alias
>> here as well, just like old_pc_system_rom_init does. That means the ISA
>> BIOS range can run out of sync when the system flash is updated during
>> runtime and requires a restart of QEMU then. Switching to an alias would
>> also allow some code consolidation. Can you explain the idea behind the
>> current version?
> 
> I'm pretty sure I tried this and found that it did not work on the flash device.
> 
> I wrote an email to the list on Oct 17, 2011 about this, but I didn't
> get a response. (Re: [Qemu-devel] [PATCH 2/4] pc: Support system flash
> memory with pflash)

Can you be more specific how/when it failed? A trivial test cannot
confirm this so far.

Jan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [Qemu-devel] ISA BIOS mapping for system flash emulation

[Jordan Justen]

[-- Attachment #1: Type: text/plain, Size: 1279 bytes --]

On Wed, Oct 31, 2012 at 11:57 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
> On 2012-11-01 03:55, Jordan Justen wrote:
>> On Tue, Oct 30, 2012 at 11:14 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>>> Hi Jordan,
>>>
>>> I was starring at pc_isa_bios_init and wondering why you are creating a
>>> copy of the system flash for the low ISA range instead of using an alias
>>> here as well, just like old_pc_system_rom_init does. That means the ISA
>>> BIOS range can run out of sync when the system flash is updated during
>>> runtime and requires a restart of QEMU then. Switching to an alias would
>>> also allow some code consolidation. Can you explain the idea behind the
>>> current version?
>>
>> I'm pretty sure I tried this and found that it did not work on the flash device.
>>
>> I wrote an email to the list on Oct 17, 2011 about this, but I didn't
>> get a response. (Re: [Qemu-devel] [PATCH 2/4] pc: Support system flash
>> memory with pflash)
>
> Can you be more specific how/when it failed? A trivial test cannot
> confirm this so far.

I reproduced this on 1b89fafe (where x86-flash was introduced). When I
setup the alias, I saw random data in the F000 segment.

When I tried the same thing on the current master (patch attached), it
appeared to work correctly.

-Jordan

[-- Attachment #2: enable-f000-alias.diff --]
[-- Type: application/octet-stream, Size: 1325 bytes --]

diff --git a/hw/pc_sysfw.c b/hw/pc_sysfw.c
index 9d7c5f4..c540e61 100644
--- a/hw/pc_sysfw.c
+++ b/hw/pc_sysfw.c
@@ -47,7 +47,6 @@ 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);
 
@@ -57,20 +56,12 @@ static void pc_isa_bios_init(MemoryRegion *rom_memory,
         isa_bios_size = 128 * 1024;
     }
     isa_bios = g_malloc(sizeof(*isa_bios));
-    memory_region_init_ram(isa_bios, "isa-bios", isa_bios_size);
-    vmstate_register_ram_global(isa_bios);
+    memory_region_init_alias(isa_bios, "isa-bios", flash_mem,
+                             ram_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);
 }
 

Re: [Qemu-devel] ISA BIOS mapping for system flash emulation

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1574 bytes --]

On 2012-11-01 17:13, Jordan Justen wrote:
> On Wed, Oct 31, 2012 at 11:57 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>> On 2012-11-01 03:55, Jordan Justen wrote:
>>> On Tue, Oct 30, 2012 at 11:14 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>>>> Hi Jordan,
>>>>
>>>> I was starring at pc_isa_bios_init and wondering why you are creating a
>>>> copy of the system flash for the low ISA range instead of using an alias
>>>> here as well, just like old_pc_system_rom_init does. That means the ISA
>>>> BIOS range can run out of sync when the system flash is updated during
>>>> runtime and requires a restart of QEMU then. Switching to an alias would
>>>> also allow some code consolidation. Can you explain the idea behind the
>>>> current version?
>>>
>>> I'm pretty sure I tried this and found that it did not work on the flash device.
>>>
>>> I wrote an email to the list on Oct 17, 2011 about this, but I didn't
>>> get a response. (Re: [Qemu-devel] [PATCH 2/4] pc: Support system flash
>>> memory with pflash)
>>
>> Can you be more specific how/when it failed? A trivial test cannot
>> confirm this so far.
> 
> I reproduced this on 1b89fafe (where x86-flash was introduced). When I
> setup the alias, I saw random data in the F000 segment.
> 
> When I tried the same thing on the current master (patch attached), it
> appeared to work correctly.

I vaguely remember issues with aliasing in the early memory region days,
maybe that was causing it. As there is no conceptual reason to avoid an
alias, let's file a refactoring patch.

Jan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]