[4/4] pam: setup pc.bios

Commit Message

Xu, Anthony April 8, 2017, 12:45 a.m. UTC

when pam is disabled, set pc.bios and isa.bios region as writeable,
and add isa.bios to system memory region.

Signed-off-by: Anthony Xu <[e-mail]>
---
 hw/i386/pc.c         |  2 +-
 hw/i386/pc_sysfw.c   | 30 +++++++++++++++++++++---------
 include/hw/i386/pc.h |  4 +++-
 3 files changed, 25 insertions(+), 11 deletions(-)

Comments

Paolo Bonzini April 8, 2017, 9:49 a.m. UTC | #1

On 08/04/2017 08:45, Anthony Xu wrote:
> -    if (!isapc_ram_fw) {
> -        memory_region_set_readonly(bios, true);
> +    if (PC_MACHINE(current_machine)->pam) {
> +        /* if PAM is disabled, set it as readwrite */
> +        if (!isapc_ram_fw) {
> +            memory_region_set_readonly(bios, true);
> +        }
>      }

I think this is wrong, the high copy should remain read-only or pflash
stops working when you remove PAM.

The comment only explains the "what" but not the "why" and the "why" is
not in the commit message.  See also here:

> +    if (PC_MACHINE(current_machine)->pam) {
> +        memory_region_add_subregion_overlap(rom_memory,
> +                                        0x100000 - isa_bios_size,
> +                                        isa_bios,
> +                                        1);
> +        if (!isapc_ram_fw) {
> +            memory_region_set_readonly(isa_bios, true);
> +        }
> +    } else {
> +        /* if PAM is disabed, add isa-bios to system memory region */
> +        memory_region_add_subregion_overlap(system_memory,
>                                          0x100000 - isa_bios_size,
>                                          isa_bios,
>                                          1);

Thanks,

Paolo

Xu, Anthony April 11, 2017, 1:42 a.m. UTC | #2

> I think this is wrong, the high copy should remain read-only or pflash

> stops working when you remove PAM.


I tried to set pc.bios as read-only and isa.bios as read&write,
it doesn't work. render_memory_region doesn't honor readonly
field of alias MemoryRegion. 

Two FlatRanges created for pc.bios and isa.bios point to the same
MemoryRegion pc.bios. Both get readonly from pc.bios.

Is this a bug or by design?

Pc.bios and isa.bios are backed by the same memory block,
so it may cause CPU TLB alias, any issue here?


> 

> The comment only explains the "what" but not the "why" and the "why" is

> not in the commit message.  See also here:

> 

Will do.


Anthony

Paolo Bonzini April 11, 2017, 9:18 a.m. UTC | #3

On 11/04/2017 09:42, Xu, Anthony wrote:
>> I think this is wrong, the high copy should remain read-only or pflash
>> stops working when you remove PAM.
> 
> I tried to set pc.bios as read-only and isa.bios as read&write,
> it doesn't work. render_memory_region doesn't honor readonly
> field of alias MemoryRegion. 
> 
> Two FlatRanges created for pc.bios and isa.bios point to the same
> MemoryRegion pc.bios. Both get readonly from pc.bios.
> Is this a bug or by design?

Read-write can use an alias to become read-only, but read-only cannot
use an alias to become read-write.

Let's sort this out later.

> Pc.bios and isa.bios are backed by the same memory block,
> so it may cause CPU TLB alias, any issue here?

No, it's okay.

Paolo

Patch

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 455f7fe..3e23f58 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1443,7 +1443,7 @@  void pc_memory_init(PCMachineState *pcms,
     }
 
     /* Initialize PC system firmware */
-    pc_system_firmware_init(rom_memory, !pcmc->pci_enabled);
+    pc_system_firmware_init(system_memory, rom_memory, !pcmc->pci_enabled);
 
     if (pcms->pam) {
         MemoryRegion *option_rom_mr = g_malloc(sizeof(*option_rom_mr));
diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index f915ad0..e5ac615 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -173,7 +173,8 @@  static void pc_system_flash_init(MemoryRegion *rom_memory)
     }
 }
 
-static void old_pc_system_rom_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
+static void old_pc_system_rom_init(MemoryRegion *system_memory,
+                                   MemoryRegion *rom_memory, bool isapc_ram_fw)
 {
     char *filename;
     MemoryRegion *bios, *isa_bios;
@@ -197,8 +198,11 @@  static void old_pc_system_rom_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
     bios = g_malloc(sizeof(*bios));
     memory_region_init_ram(bios, NULL, "pc.bios", bios_size, &error_fatal);
     vmstate_register_ram_global(bios);
-    if (!isapc_ram_fw) {
-        memory_region_set_readonly(bios, true);
+    if (PC_MACHINE(current_machine)->pam) {
+        /* if PAM is disabled, set it as readwrite */
+        if (!isapc_ram_fw) {
+            memory_region_set_readonly(bios, true);
+        }
     }
     ret = rom_add_file_fixed(bios_name, (uint32_t)(-bios_size), -1);
     if (ret != 0) {
@@ -216,21 +220,29 @@  static void old_pc_system_rom_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
     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,
+    if (PC_MACHINE(current_machine)->pam) {
+        memory_region_add_subregion_overlap(rom_memory,
+                                        0x100000 - isa_bios_size,
+                                        isa_bios,
+                                        1);
+        if (!isapc_ram_fw) {
+            memory_region_set_readonly(isa_bios, true);
+        }
+    } else {
+        /* if PAM is disabed, add isa-bios to system memory region */
+        memory_region_add_subregion_overlap(system_memory,
                                         0x100000 - isa_bios_size,
                                         isa_bios,
                                         1);
-    if (!isapc_ram_fw) {
-        memory_region_set_readonly(isa_bios, true);
     }
-
     /* map all the bios at the top of memory */
     memory_region_add_subregion(rom_memory,
                                 (uint32_t)(-bios_size),
                                 bios);
 }
 
-void pc_system_firmware_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
+void pc_system_firmware_init(MemoryRegion *system_memory,
+                             MemoryRegion *rom_memory, bool isapc_ram_fw)
 {
     DriveInfo *pflash_drv;
 
@@ -238,7 +250,7 @@  void pc_system_firmware_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
 
     if (isapc_ram_fw || pflash_drv == NULL) {
         /* When a pflash drive is not found, use rom-mode */
-        old_pc_system_rom_init(rom_memory, isapc_ram_fw);
+        old_pc_system_rom_init(system_memory, rom_memory, isapc_ram_fw);
         return;
     }
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 05f4df5..eb5b853 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -354,7 +354,9 @@  static inline bool isa_ne2000_init(ISABus *bus, int base, int irq, NICInfo *nd)
 }
 
 /* pc_sysfw.c */
-void pc_system_firmware_init(MemoryRegion *rom_memory,
+
+void pc_system_firmware_init(MemoryRegion *system_memory,
+                             MemoryRegion *rom_memory,
                              bool isapc_ram_fw);
 
 /* pvpanic.c */