[Qemu-devel] [PATCH] isapc: Shadow ISA BIOS by default

[Qemu-devel] [PATCH] isapc: Shadow ISA BIOS by default

[Jan Kiszka]

Our one and only BIOS depends on a writable shadowed BIOS in the ISA
range. As we have no interface to control the write property, make that
region writable by default.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

This unbreaks isapc for TCG, and keep it working for KVM once it starts
supporting read-only memslots.

 hw/pc_sysfw.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/hw/pc_sysfw.c b/hw/pc_sysfw.c
index b45f0ac..027d98a 100644
--- a/hw/pc_sysfw.c
+++ b/hw/pc_sysfw.c
@@ -136,6 +136,7 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory)
 {
     char *filename;
     MemoryRegion *bios, *isa_bios;
+    void *isa_bios_ptr;
     int bios_size, isa_bios_size;
     int ret;
 
@@ -167,19 +168,23 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory)
         g_free(filename);
     }
 
-    /* map the last 128KB of the BIOS in ISA space */
+    /* Shadow the last 128KB of the BIOS in ISA space as RAM -
+     * Seabios depends on this */
     isa_bios_size = bios_size;
     if (isa_bios_size > (128 * 1024)) {
         isa_bios_size = 128 * 1024;
     }
     isa_bios = g_malloc(sizeof(*isa_bios));
-    memory_region_init_alias(isa_bios, "isa-bios", bios,
-                             bios_size - isa_bios_size, isa_bios_size);
+    memory_region_init_ram(isa_bios, "isa-bios", isa_bios_size);
+    vmstate_register_ram_global(isa_bios);
     memory_region_add_subregion_overlap(rom_memory,
                                         0x100000 - isa_bios_size,
                                         isa_bios,
                                         1);
-    memory_region_set_readonly(isa_bios, true);
+
+    /* copy ISA rom image from top of the ROM */
+    isa_bios_ptr = memory_region_get_ram_ptr(isa_bios);
+    rom_copy(isa_bios_ptr, (uint32_t)(-isa_bios_size), isa_bios_size);
 
     /* map all the bios at the top of memory */
     memory_region_add_subregion(rom_memory,
-- 
1.7.3.4

Re: [Qemu-devel] [PATCH] isapc: Shadow ISA BIOS by default

[Michael Tokarev]

On 11.09.2012 19:53, Jan Kiszka wrote:
> Our one and only BIOS depends on a writable shadowed BIOS in the ISA
> range. As we have no interface to control the write property, make that
> region writable by default.

We've a long-standing bug --
 http://bugs.debian.org/605525
 https://bugs.launchpad.net/qemu/+bug/1024248

(qemu-system-x86_64 -M isapc shows blank guest screen)

I assume this patch fixes it for qemu/tcg case, but not for
qemu/kvm case right?

I just tested this patch on qemu 1.1 (it applies cleanly), and
it appears to work as expected, ie, the issue mentioned above
is fixed by it.  But I also noticed that kvm mode is not
affected -- at least the sympthoms mentioned above does not
apply.

> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
> This unbreaks isapc for TCG, and keep it working for KVM once it starts
> supporting read-only memslots.

Can you clarify please, -- referring to the above? :)

Thanks,

/mjt

Re: [Qemu-devel] [PATCH] isapc: Shadow ISA BIOS by default

[Avi Kivity]

On 09/12/2012 08:57 AM, Michael Tokarev wrote:
>> 
>> This unbreaks isapc for TCG, and keep it working for KVM once it starts
>> supporting read-only memslots.
> 
> Can you clarify please, -- referring to the above? :)

kvm only works due to a bug.  Once the kvm bug is fixed, isapc will
break, unless the patch above is applied.


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] isapc: Shadow ISA BIOS by default

[Jan Kiszka]

[forgot to CC stable: this one apparently qualifies for older QEMU
releases as well but would require some adaptions for < 1.1]

On 2012-09-11 17:53, Jan Kiszka wrote:
> Our one and only BIOS depends on a writable shadowed BIOS in the ISA
> range. As we have no interface to control the write property, make that
> region writable by default.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
> This unbreaks isapc for TCG, and keep it working for KVM once it starts
> supporting read-only memslots.
> 
>  hw/pc_sysfw.c |   13 +++++++++----
>  1 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/pc_sysfw.c b/hw/pc_sysfw.c
> index b45f0ac..027d98a 100644
> --- a/hw/pc_sysfw.c
> +++ b/hw/pc_sysfw.c
> @@ -136,6 +136,7 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory)
>  {
>      char *filename;
>      MemoryRegion *bios, *isa_bios;
> +    void *isa_bios_ptr;
>      int bios_size, isa_bios_size;
>      int ret;
>  
> @@ -167,19 +168,23 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory)
>          g_free(filename);
>      }
>  
> -    /* map the last 128KB of the BIOS in ISA space */
> +    /* Shadow the last 128KB of the BIOS in ISA space as RAM -
> +     * Seabios depends on this */
>      isa_bios_size = bios_size;
>      if (isa_bios_size > (128 * 1024)) {
>          isa_bios_size = 128 * 1024;
>      }
>      isa_bios = g_malloc(sizeof(*isa_bios));
> -    memory_region_init_alias(isa_bios, "isa-bios", bios,
> -                             bios_size - isa_bios_size, isa_bios_size);
> +    memory_region_init_ram(isa_bios, "isa-bios", isa_bios_size);
> +    vmstate_register_ram_global(isa_bios);
>      memory_region_add_subregion_overlap(rom_memory,
>                                          0x100000 - isa_bios_size,
>                                          isa_bios,
>                                          1);
> -    memory_region_set_readonly(isa_bios, true);
> +
> +    /* copy ISA rom image from top of the ROM */
> +    isa_bios_ptr = memory_region_get_ram_ptr(isa_bios);
> +    rom_copy(isa_bios_ptr, (uint32_t)(-isa_bios_size), isa_bios_size);
>  
>      /* map all the bios at the top of memory */
>      memory_region_add_subregion(rom_memory,
> 

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] isapc: Shadow ISA BIOS by default

[Jan Kiszka]

On 2012-09-11 17:53, Jan Kiszka wrote:
> Our one and only BIOS depends on a writable shadowed BIOS in the ISA
> range. As we have no interface to control the write property, make that
> region writable by default.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
> This unbreaks isapc for TCG, and keep it working for KVM once it starts
> supporting read-only memslots.
> 
>  hw/pc_sysfw.c |   13 +++++++++----
>  1 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/pc_sysfw.c b/hw/pc_sysfw.c
> index b45f0ac..027d98a 100644
> --- a/hw/pc_sysfw.c
> +++ b/hw/pc_sysfw.c
> @@ -136,6 +136,7 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory)
>  {
>      char *filename;
>      MemoryRegion *bios, *isa_bios;
> +    void *isa_bios_ptr;
>      int bios_size, isa_bios_size;
>      int ret;
>  
> @@ -167,19 +168,23 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory)
>          g_free(filename);
>      }
>  
> -    /* map the last 128KB of the BIOS in ISA space */
> +    /* Shadow the last 128KB of the BIOS in ISA space as RAM -
> +     * Seabios depends on this */
>      isa_bios_size = bios_size;
>      if (isa_bios_size > (128 * 1024)) {
>          isa_bios_size = 128 * 1024;
>      }
>      isa_bios = g_malloc(sizeof(*isa_bios));
> -    memory_region_init_alias(isa_bios, "isa-bios", bios,
> -                             bios_size - isa_bios_size, isa_bios_size);
> +    memory_region_init_ram(isa_bios, "isa-bios", isa_bios_size);
> +    vmstate_register_ram_global(isa_bios);
>      memory_region_add_subregion_overlap(rom_memory,
>                                          0x100000 - isa_bios_size,
>                                          isa_bios,
>                                          1);
> -    memory_region_set_readonly(isa_bios, true);
> +
> +    /* copy ISA rom image from top of the ROM */
> +    isa_bios_ptr = memory_region_get_ram_ptr(isa_bios);
> +    rom_copy(isa_bios_ptr, (uint32_t)(-isa_bios_size), isa_bios_size);
>  
>      /* map all the bios at the top of memory */
>      memory_region_add_subregion(rom_memory,
> 

Ping. Or already queued?

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] isapc: Shadow ISA BIOS by default

[Anthony Liguori]

Jan Kiszka <jan.kiszka@siemens.com> writes:

> On 2012-09-11 17:53, Jan Kiszka wrote:
>> Our one and only BIOS depends on a writable shadowed BIOS in the ISA
>> range. As we have no interface to control the write property, make that
>> region writable by default.
>> 
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>> 
>> This unbreaks isapc for TCG, and keep it working for KVM once it starts
>> supporting read-only memslots.
>> 
>>  hw/pc_sysfw.c |   13 +++++++++----
>>  1 files changed, 9 insertions(+), 4 deletions(-)
>> 
>> diff --git a/hw/pc_sysfw.c b/hw/pc_sysfw.c
>> index b45f0ac..027d98a 100644
>> --- a/hw/pc_sysfw.c
>> +++ b/hw/pc_sysfw.c
>> @@ -136,6 +136,7 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory)
>>  {
>>      char *filename;
>>      MemoryRegion *bios, *isa_bios;
>> +    void *isa_bios_ptr;
>>      int bios_size, isa_bios_size;
>>      int ret;
>>  
>> @@ -167,19 +168,23 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory)
>>          g_free(filename);
>>      }
>>  
>> -    /* map the last 128KB of the BIOS in ISA space */
>> +    /* Shadow the last 128KB of the BIOS in ISA space as RAM -
>> +     * Seabios depends on this */
>>      isa_bios_size = bios_size;
>>      if (isa_bios_size > (128 * 1024)) {
>>          isa_bios_size = 128 * 1024;
>>      }
>>      isa_bios = g_malloc(sizeof(*isa_bios));
>> -    memory_region_init_alias(isa_bios, "isa-bios", bios,
>> -                             bios_size - isa_bios_size, isa_bios_size);
>> +    memory_region_init_ram(isa_bios, "isa-bios", isa_bios_size);
>> +    vmstate_register_ram_global(isa_bios);
>>      memory_region_add_subregion_overlap(rom_memory,
>>                                          0x100000 - isa_bios_size,
>>                                          isa_bios,
>>                                          1);
>> -    memory_region_set_readonly(isa_bios, true);
>> +
>> +    /* copy ISA rom image from top of the ROM */
>> +    isa_bios_ptr = memory_region_get_ram_ptr(isa_bios);
>> +    rom_copy(isa_bios_ptr, (uint32_t)(-isa_bios_size), isa_bios_size);
>>  
>>      /* map all the bios at the top of memory */
>>      memory_region_add_subregion(rom_memory,
>> 
>
> Ping. Or already queued?

I've got it queued now.  Thanks.

Regards,

Anthony Liguori

>
> Jan
>
> -- 
> Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
> Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] isapc: Shadow ISA BIOS by default

[Jan Kiszka]

On 2012-10-08 20:52, Anthony Liguori wrote:
> Jan Kiszka <jan.kiszka@siemens.com> writes:
> 
>> On 2012-09-11 17:53, Jan Kiszka wrote:
>>> Our one and only BIOS depends on a writable shadowed BIOS in the ISA
>>> range. As we have no interface to control the write property, make that
>>> region writable by default.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>> ---
>>>
>>> This unbreaks isapc for TCG, and keep it working for KVM once it starts
>>> supporting read-only memslots.
>>>
>>>  hw/pc_sysfw.c |   13 +++++++++----
>>>  1 files changed, 9 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/pc_sysfw.c b/hw/pc_sysfw.c
>>> index b45f0ac..027d98a 100644
>>> --- a/hw/pc_sysfw.c
>>> +++ b/hw/pc_sysfw.c
>>> @@ -136,6 +136,7 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory)
>>>  {
>>>      char *filename;
>>>      MemoryRegion *bios, *isa_bios;
>>> +    void *isa_bios_ptr;
>>>      int bios_size, isa_bios_size;
>>>      int ret;
>>>  
>>> @@ -167,19 +168,23 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory)
>>>          g_free(filename);
>>>      }
>>>  
>>> -    /* map the last 128KB of the BIOS in ISA space */
>>> +    /* Shadow the last 128KB of the BIOS in ISA space as RAM -
>>> +     * Seabios depends on this */
>>>      isa_bios_size = bios_size;
>>>      if (isa_bios_size > (128 * 1024)) {
>>>          isa_bios_size = 128 * 1024;
>>>      }
>>>      isa_bios = g_malloc(sizeof(*isa_bios));
>>> -    memory_region_init_alias(isa_bios, "isa-bios", bios,
>>> -                             bios_size - isa_bios_size, isa_bios_size);
>>> +    memory_region_init_ram(isa_bios, "isa-bios", isa_bios_size);
>>> +    vmstate_register_ram_global(isa_bios);
>>>      memory_region_add_subregion_overlap(rom_memory,
>>>                                          0x100000 - isa_bios_size,
>>>                                          isa_bios,
>>>                                          1);
>>> -    memory_region_set_readonly(isa_bios, true);
>>> +
>>> +    /* copy ISA rom image from top of the ROM */
>>> +    isa_bios_ptr = memory_region_get_ram_ptr(isa_bios);
>>> +    rom_copy(isa_bios_ptr, (uint32_t)(-isa_bios_size), isa_bios_size);
>>>  
>>>      /* map all the bios at the top of memory */
>>>      memory_region_add_subregion(rom_memory,
>>>
>>
>> Ping. Or already queued?
> 
> I've got it queued now.  Thanks.

I'm withdrawing it: This breaks Windows 95 booting. For unknown reasons,
this nice OS decided to overwrite the F-segment during boot. That is
fine as long as it is properly protected. But it breaks under current
KVM and with the patch above for the isapc. So we need a firmware
interface to enable/disable write protection for this segment in isapc
mode, specifically as that machine targets these old OSes.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] isapc: Shadow ISA BIOS by default

[Anthony Liguori]

Jan Kiszka <jan.kiszka@siemens.com> writes:

> On 2012-10-08 20:52, Anthony Liguori wrote:
>> Jan Kiszka <jan.kiszka@siemens.com> writes:
>> 
>>> On 2012-09-11 17:53, Jan Kiszka wrote:
>>>> Our one and only BIOS depends on a writable shadowed BIOS in the ISA
>>>> range. As we have no interface to control the write property, make that
>>>> region writable by default.
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>> ---
>>>>
>>>> This unbreaks isapc for TCG, and keep it working for KVM once it starts
>>>> supporting read-only memslots.
>>>>
>>>>  hw/pc_sysfw.c |   13 +++++++++----
>>>>  1 files changed, 9 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/hw/pc_sysfw.c b/hw/pc_sysfw.c
>>>> index b45f0ac..027d98a 100644
>>>> --- a/hw/pc_sysfw.c
>>>> +++ b/hw/pc_sysfw.c
>>>> @@ -136,6 +136,7 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory)
>>>>  {
>>>>      char *filename;
>>>>      MemoryRegion *bios, *isa_bios;
>>>> +    void *isa_bios_ptr;
>>>>      int bios_size, isa_bios_size;
>>>>      int ret;
>>>>  
>>>> @@ -167,19 +168,23 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory)
>>>>          g_free(filename);
>>>>      }
>>>>  
>>>> -    /* map the last 128KB of the BIOS in ISA space */
>>>> +    /* Shadow the last 128KB of the BIOS in ISA space as RAM -
>>>> +     * Seabios depends on this */
>>>>      isa_bios_size = bios_size;
>>>>      if (isa_bios_size > (128 * 1024)) {
>>>>          isa_bios_size = 128 * 1024;
>>>>      }
>>>>      isa_bios = g_malloc(sizeof(*isa_bios));
>>>> -    memory_region_init_alias(isa_bios, "isa-bios", bios,
>>>> -                             bios_size - isa_bios_size, isa_bios_size);
>>>> +    memory_region_init_ram(isa_bios, "isa-bios", isa_bios_size);
>>>> +    vmstate_register_ram_global(isa_bios);
>>>>      memory_region_add_subregion_overlap(rom_memory,
>>>>                                          0x100000 - isa_bios_size,
>>>>                                          isa_bios,
>>>>                                          1);
>>>> -    memory_region_set_readonly(isa_bios, true);
>>>> +
>>>> +    /* copy ISA rom image from top of the ROM */
>>>> +    isa_bios_ptr = memory_region_get_ram_ptr(isa_bios);
>>>> +    rom_copy(isa_bios_ptr, (uint32_t)(-isa_bios_size), isa_bios_size);
>>>>  
>>>>      /* map all the bios at the top of memory */
>>>>      memory_region_add_subregion(rom_memory,
>>>>
>>>
>>> Ping. Or already queued?
>> 
>> I've got it queued now.  Thanks.
>
> I'm withdrawing it: This breaks Windows 95 booting. For unknown reasons,
> this nice OS decided to overwrite the F-segment during boot. That is
> fine as long as it is properly protected. But it breaks under current
> KVM and with the patch above for the isapc. So we need a firmware
> interface to enable/disable write protection for this segment in isapc
> mode, specifically as that machine targets these old OSes.

Ah, if it wasn't for a build break caused by one of the pull requests, I
would have pushed last night.  Thanks for the heads up, I'll remove it
from my queue.

Is fw_cfg the right interface?  I presume this is i440fx specific?  How
does q35 handle this?  Presumably there's a second window for the BIOS
mapping.  There's got to be some way to do shadowing of it I would
think.

Regards,

Anthony Liguori

>
> Jan
>
> -- 
> Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
> Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] isapc: Shadow ISA BIOS by default

[Jan Kiszka]

On 2012-10-12 15:41, Anthony Liguori wrote:
> Jan Kiszka <jan.kiszka@siemens.com> writes:
> 
>> On 2012-10-08 20:52, Anthony Liguori wrote:
>>> Jan Kiszka <jan.kiszka@siemens.com> writes:
>>>
>>>> On 2012-09-11 17:53, Jan Kiszka wrote:
>>>>> Our one and only BIOS depends on a writable shadowed BIOS in the ISA
>>>>> range. As we have no interface to control the write property, make that
>>>>> region writable by default.
>>>>>
>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>> ---
>>>>>
>>>>> This unbreaks isapc for TCG, and keep it working for KVM once it starts
>>>>> supporting read-only memslots.
>>>>>
>>>>>  hw/pc_sysfw.c |   13 +++++++++----
>>>>>  1 files changed, 9 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/hw/pc_sysfw.c b/hw/pc_sysfw.c
>>>>> index b45f0ac..027d98a 100644
>>>>> --- a/hw/pc_sysfw.c
>>>>> +++ b/hw/pc_sysfw.c
>>>>> @@ -136,6 +136,7 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory)
>>>>>  {
>>>>>      char *filename;
>>>>>      MemoryRegion *bios, *isa_bios;
>>>>> +    void *isa_bios_ptr;
>>>>>      int bios_size, isa_bios_size;
>>>>>      int ret;
>>>>>  
>>>>> @@ -167,19 +168,23 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory)
>>>>>          g_free(filename);
>>>>>      }
>>>>>  
>>>>> -    /* map the last 128KB of the BIOS in ISA space */
>>>>> +    /* Shadow the last 128KB of the BIOS in ISA space as RAM -
>>>>> +     * Seabios depends on this */
>>>>>      isa_bios_size = bios_size;
>>>>>      if (isa_bios_size > (128 * 1024)) {
>>>>>          isa_bios_size = 128 * 1024;
>>>>>      }
>>>>>      isa_bios = g_malloc(sizeof(*isa_bios));
>>>>> -    memory_region_init_alias(isa_bios, "isa-bios", bios,
>>>>> -                             bios_size - isa_bios_size, isa_bios_size);
>>>>> +    memory_region_init_ram(isa_bios, "isa-bios", isa_bios_size);
>>>>> +    vmstate_register_ram_global(isa_bios);
>>>>>      memory_region_add_subregion_overlap(rom_memory,
>>>>>                                          0x100000 - isa_bios_size,
>>>>>                                          isa_bios,
>>>>>                                          1);
>>>>> -    memory_region_set_readonly(isa_bios, true);
>>>>> +
>>>>> +    /* copy ISA rom image from top of the ROM */
>>>>> +    isa_bios_ptr = memory_region_get_ram_ptr(isa_bios);
>>>>> +    rom_copy(isa_bios_ptr, (uint32_t)(-isa_bios_size), isa_bios_size);
>>>>>  
>>>>>      /* map all the bios at the top of memory */
>>>>>      memory_region_add_subregion(rom_memory,
>>>>>
>>>>
>>>> Ping. Or already queued?
>>>
>>> I've got it queued now.  Thanks.
>>
>> I'm withdrawing it: This breaks Windows 95 booting. For unknown reasons,
>> this nice OS decided to overwrite the F-segment during boot. That is
>> fine as long as it is properly protected. But it breaks under current
>> KVM and with the patch above for the isapc. So we need a firmware
>> interface to enable/disable write protection for this segment in isapc
>> mode, specifically as that machine targets these old OSes.
> 
> Ah, if it wasn't for a build break caused by one of the pull requests, I
> would have pushed last night.  Thanks for the heads up, I'll remove it
> from my queue.
> 
> Is fw_cfg the right interface?  I presume this is i440fx specific?  How
> does q35 handle this?

No, there is no i440fx or q35 in that case. There are discrete chips
and wiring on an undefined ISA motherboard. As Seabios depends on a
writable E&F-segments (maybe only on E, still need to find out) for a
certain period, we need to invent a pv channel (probably via fw_cfg) to
provide the necessary control knob.

>  Presumably there's a second window for the BIOS
> mapping.  There's got to be some way to do shadowing of it I would
> think.

Not sure what you mean here. This is only about shadowing the top 128K
of the BIOS into the E/F-segment and providing a write-enable knob for it.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] isapc: Shadow ISA BIOS by default

[Anthony Liguori]

Jan Kiszka <jan.kiszka@siemens.com> writes:

> On 2012-10-12 15:41, Anthony Liguori wrote:
>> Jan Kiszka <jan.kiszka@siemens.com> writes:
>> 
>>> On 2012-10-08 20:52, Anthony Liguori wrote:
>>>> Jan Kiszka <jan.kiszka@siemens.com> writes:
>>>>
>>>>> On 2012-09-11 17:53, Jan Kiszka wrote:
>>>>>> Our one and only BIOS depends on a writable shadowed BIOS in the ISA
>>>>>> range. As we have no interface to control the write property, make that
>>>>>> region writable by default.
>>>>>>
>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>> ---
>>>>>>
>>>>>> This unbreaks isapc for TCG, and keep it working for KVM once it starts
>>>>>> supporting read-only memslots.
>>>>>>
>>>>>>  hw/pc_sysfw.c |   13 +++++++++----
>>>>>>  1 files changed, 9 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/pc_sysfw.c b/hw/pc_sysfw.c
>>>>>> index b45f0ac..027d98a 100644
>>>>>> --- a/hw/pc_sysfw.c
>>>>>> +++ b/hw/pc_sysfw.c
>>>>>> @@ -136,6 +136,7 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory)
>>>>>>  {
>>>>>>      char *filename;
>>>>>>      MemoryRegion *bios, *isa_bios;
>>>>>> +    void *isa_bios_ptr;
>>>>>>      int bios_size, isa_bios_size;
>>>>>>      int ret;
>>>>>>  
>>>>>> @@ -167,19 +168,23 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory)
>>>>>>          g_free(filename);
>>>>>>      }
>>>>>>  
>>>>>> -    /* map the last 128KB of the BIOS in ISA space */
>>>>>> +    /* Shadow the last 128KB of the BIOS in ISA space as RAM -
>>>>>> +     * Seabios depends on this */
>>>>>>      isa_bios_size = bios_size;
>>>>>>      if (isa_bios_size > (128 * 1024)) {
>>>>>>          isa_bios_size = 128 * 1024;
>>>>>>      }
>>>>>>      isa_bios = g_malloc(sizeof(*isa_bios));
>>>>>> -    memory_region_init_alias(isa_bios, "isa-bios", bios,
>>>>>> -                             bios_size - isa_bios_size, isa_bios_size);
>>>>>> +    memory_region_init_ram(isa_bios, "isa-bios", isa_bios_size);
>>>>>> +    vmstate_register_ram_global(isa_bios);
>>>>>>      memory_region_add_subregion_overlap(rom_memory,
>>>>>>                                          0x100000 - isa_bios_size,
>>>>>>                                          isa_bios,
>>>>>>                                          1);
>>>>>> -    memory_region_set_readonly(isa_bios, true);
>>>>>> +
>>>>>> +    /* copy ISA rom image from top of the ROM */
>>>>>> +    isa_bios_ptr = memory_region_get_ram_ptr(isa_bios);
>>>>>> +    rom_copy(isa_bios_ptr, (uint32_t)(-isa_bios_size), isa_bios_size);
>>>>>>  
>>>>>>      /* map all the bios at the top of memory */
>>>>>>      memory_region_add_subregion(rom_memory,
>>>>>>
>>>>>
>>>>> Ping. Or already queued?
>>>>
>>>> I've got it queued now.  Thanks.
>>>
>>> I'm withdrawing it: This breaks Windows 95 booting. For unknown reasons,
>>> this nice OS decided to overwrite the F-segment during boot. That is
>>> fine as long as it is properly protected. But it breaks under current
>>> KVM and with the patch above for the isapc. So we need a firmware
>>> interface to enable/disable write protection for this segment in isapc
>>> mode, specifically as that machine targets these old OSes.
>> 
>> Ah, if it wasn't for a build break caused by one of the pull requests, I
>> would have pushed last night.  Thanks for the heads up, I'll remove it
>> from my queue.
>> 
>> Is fw_cfg the right interface?  I presume this is i440fx specific?  How
>> does q35 handle this?
>
> No, there is no i440fx or q35 in that case. There are discrete chips
> and wiring on an undefined ISA motherboard. As Seabios depends on a
> writable E&F-segments (maybe only on E, still need to find out) for a
> certain period, we need to invent a pv channel (probably via fw_cfg) to
> provide the necessary control knob.

I see, I thought this was primarily for shadowing.  But it's a
SeaBIOS-ism.  fw_cfg is the right answer.

Regards,

Anthony Liguori

>
>>  Presumably there's a second window for the BIOS
>> mapping.  There's got to be some way to do shadowing of it I would
>> think.
>
> Not sure what you mean here. This is only about shadowing the top 128K
> of the BIOS into the E/F-segment and providing a write-enable knob for it.
>
> Jan
>
> -- 
> Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
> Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] isapc: Shadow ISA BIOS by default

[Kevin O'Connor]

On Fri, Oct 12, 2012 at 11:29:47AM +0200, Jan Kiszka wrote:
> On 2012-10-08 20:52, Anthony Liguori wrote:
> > Jan Kiszka <jan.kiszka@siemens.com> writes:
> > 
> >> On 2012-09-11 17:53, Jan Kiszka wrote:
> >>> Our one and only BIOS depends on a writable shadowed BIOS in the ISA
> >>> range. As we have no interface to control the write property, make that
> >>> region writable by default.
> >>>
> >>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>> ---
> >>>
> >>> This unbreaks isapc for TCG, and keep it working for KVM once it starts
> >>> supporting read-only memslots.
[...]
> >> Ping. Or already queued?
> > 
> > I've got it queued now.  Thanks.
> 
> I'm withdrawing it: This breaks Windows 95 booting. For unknown reasons,
> this nice OS decided to overwrite the F-segment during boot. That is
> fine as long as it is properly protected. But it breaks under current
> KVM and with the patch above for the isapc. So we need a firmware
> interface to enable/disable write protection for this segment in isapc
> mode, specifically as that machine targets these old OSes.

Why withdraw the patch?  With the patch and when not using isapc,
seabios will write protect the f-segment and nothing will change.
Without the patch and with isapc the machine wont boot at all, so will
never get far enough along to have Win95 crash.

That is, the patch has no harm as it only impacts isapc and with isapc
today the machine doesn't boot at all.

-Kevin