[Qemu-devel] [PATCH 0/4] pam: make pam configurable

[Qemu-devel] [PATCH 0/4] pam: make pam configurable

[Anthony Xu]

Qemu creates 13 enabled memory regions and 39 disabled memory regions
for pam. in normal setup, more than half of all memory regions in
system root region are memory regions for pam. Which slow down all
memory region related operations. 

This patch makes pam configurable, by default it is enabled.
it keeps the old behavior by default.

you can disable pam by appending pam=off to -machine. If pam is
disabled, all memory regions for pam are gone, and some memory
region operations are gone since these memory regions are gone,
and memory region operations is fast because there are much less
memory regions. This patch works on both seabios and qboot,
it reduces Qemu heap size from ~12MB to ~9MB

if pam is disabled, pc.rom is useless, so it is disabled as well.

when pam is disabled, pc.bios and isa.bios are writeable memory
region, and isa.bios is put under system memory region, otherwise
isa.bios is acctually disabled because it is under pci memory region
which has lower priority than pc.ram region.

Anthony Xu (4):
  pam: refactor PAM related code
  pam: Make PAM configurable
  pam: disable pc.rom when pam is disabled
  pam: setup pc.bios

 hw/i386/pc.c         | 33 ++++++++++++++++++++++------
 hw/i386/pc_sysfw.c   | 30 +++++++++++++++++--------
 hw/pci-host/piix.c   | 62 ++++++++++++++++++++++++++++++++++++++--------------
 hw/pci-host/q35.c    | 43 ++++++++++++++++++++++--------------
 include/hw/i386/pc.h |  6 ++++-
 5 files changed, 123 insertions(+), 51 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/4] pam:refactor PAM related code

[Anthony Xu]

split PAM SMRAM functions in piix.c
create mch_init_pam in q35.c

Signed-off-by: Anthony Xu <anthony.xu@intel.com>
---
 hw/pci-host/piix.c | 58 ++++++++++++++++++++++++++++++++++++++----------------
 hw/pci-host/q35.c  | 23 +++++++++++++---------
 2 files changed, 55 insertions(+), 26 deletions(-)

diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index f9218aa..ff4e8b5 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -138,16 +138,11 @@ static int pci_slot_get_pirq(PCIDevice *pci_dev, int pci_intx)
     return (pci_intx + slot_addend) & 3;
 }
 
-static void i440fx_update_memory_mappings(PCII440FXState *d)
+static void i440fx_update_smram(PCII440FXState *d)
 {
-    int i;
     PCIDevice *pd = PCI_DEVICE(d);
 
     memory_region_transaction_begin();
-    for (i = 0; i < 13; i++) {
-        pam_update(&d->pam_regions[i], i,
-                   pd->config[I440FX_PAM + ((i + 1) / 2)]);
-    }
     memory_region_set_enabled(&d->smram_region,
                               !(pd->config[I440FX_SMRAM] & SMRAM_D_OPEN));
     memory_region_set_enabled(&d->smram,
@@ -155,6 +150,39 @@ static void i440fx_update_memory_mappings(PCII440FXState *d)
     memory_region_transaction_commit();
 }
 
+static void i440fx_update_pam(PCII440FXState *d)
+{
+    int i;
+    PCIDevice *pd = PCI_DEVICE(d);
+    memory_region_transaction_begin();
+    pam_update(&d->pam_regions[0], 0,
+               pd->config[I440FX_PAM]);
+    for (i = 1; i < 13; i++) {
+        pam_update(&d->pam_regions[i], i,
+                   pd->config[I440FX_PAM + ((i + 1) / 2)]);
+    }
+    memory_region_transaction_commit();
+}
+
+static void i440fx_update_memory_mappings(PCII440FXState *d)
+{
+    i440fx_update_pam(d);
+    i440fx_update_smram(d);
+}
+
+
+static void i440fx_init_pam(PCII440FXState *d)
+{
+    int i;
+    init_pam(DEVICE(d), d->ram_memory, d->system_memory,
+             d->pci_address_space, &d->pam_regions[0],
+             PAM_BIOS_BASE,  PAM_BIOS_SIZE);
+    for (i = 0; i < 12; ++i) {
+        init_pam(DEVICE(d), d->ram_memory, d->system_memory,
+                 d->pci_address_space, &d->pam_regions[i + 1],
+                 PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
+    }
+}
 
 static void i440fx_write_config(PCIDevice *dev,
                                 uint32_t address, uint32_t val, int len)
@@ -163,9 +191,12 @@ static void i440fx_write_config(PCIDevice *dev,
 
     /* XXX: implement SMRAM.D_LOCK */
     pci_default_write_config(dev, address, val, len);
-    if (ranges_overlap(address, len, I440FX_PAM, I440FX_PAM_SIZE) ||
-        range_covers_byte(address, len, I440FX_SMRAM)) {
-        i440fx_update_memory_mappings(d);
+    if (ranges_overlap(address, len, I440FX_PAM, I440FX_PAM_SIZE)) {
+        i440fx_update_pam(d);
+    }
+
+    if (range_covers_byte(address, len, I440FX_SMRAM)) {
+        i440fx_update_smram(d);
     }
 }
 
@@ -335,7 +366,6 @@ PCIBus *i440fx_init(const char *host_type, const char *pci_type,
     PCIHostState *s;
     PIIX3State *piix3;
     PCII440FXState *f;
-    unsigned i;
     I440FXState *i440fx;
 
     dev = qdev_create(NULL, host_type);
@@ -378,13 +408,7 @@ PCIBus *i440fx_init(const char *host_type, const char *pci_type,
     object_property_add_const_link(qdev_get_machine(), "smram",
                                    OBJECT(&f->smram), &error_abort);
 
-    init_pam(dev, f->ram_memory, f->system_memory, f->pci_address_space,
-             &f->pam_regions[0], PAM_BIOS_BASE, PAM_BIOS_SIZE);
-    for (i = 0; i < 12; ++i) {
-        init_pam(dev, f->ram_memory, f->system_memory, f->pci_address_space,
-                 &f->pam_regions[i+1], PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE,
-                 PAM_EXPAN_SIZE);
-    }
+    i440fx_init_pam(f);
 
     /* Xen supports additional interrupt routes from the PCI devices to
      * the IOAPIC: the four pins of each PCI device on the bus are also
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 344f77b..8866357 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -460,9 +460,21 @@ static void mch_reset(DeviceState *qdev)
     mch_update(mch);
 }
 
-static void mch_realize(PCIDevice *d, Error **errp)
+static void mch_init_pam(MCHPCIState *mch)
 {
     int i;
+    init_pam(DEVICE(mch), mch->ram_memory, mch->system_memory,
+             mch->pci_address_space, &mch->pam_regions[0],
+             PAM_BIOS_BASE, PAM_BIOS_SIZE);
+    for (i = 0; i < 12; ++i) {
+        init_pam(DEVICE(mch), mch->ram_memory, mch->system_memory,
+                 mch->pci_address_space, &mch->pam_regions[i + 1],
+                 PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
+    }
+}
+
+static void mch_realize(PCIDevice *d, Error **errp)
+{
     MCHPCIState *mch = MCH_PCI_DEVICE(d);
 
     /* setup pci memory mapping */
@@ -510,14 +522,7 @@ static void mch_realize(PCIDevice *d, Error **errp)
     object_property_add_const_link(qdev_get_machine(), "smram",
                                    OBJECT(&mch->smram), &error_abort);
 
-    init_pam(DEVICE(mch), mch->ram_memory, mch->system_memory,
-             mch->pci_address_space, &mch->pam_regions[0],
-             PAM_BIOS_BASE, PAM_BIOS_SIZE);
-    for (i = 0; i < 12; ++i) {
-        init_pam(DEVICE(mch), mch->ram_memory, mch->system_memory,
-                 mch->pci_address_space, &mch->pam_regions[i+1],
-                 PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
-    }
+    mch_init_pam(mch);
 }
 
 uint64_t mch_mcfg_base(void)
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/4] pam: Make PAM configurable

[Anthony Xu]

by default PAM is enabled. when PAM is disabled,
*_init_pam and *_update_pam are dummy functions

Signed-off-by: Anthony Xu <anthony.xu@intel.com>
---
 hw/i386/pc.c         | 18 ++++++++++++++++++
 hw/pci-host/piix.c   | 36 ++++++++++++++++++++----------------
 hw/pci-host/q35.c    | 34 +++++++++++++++++++---------------
 include/hw/i386/pc.h |  2 ++
 4 files changed, 59 insertions(+), 31 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index d24388e..9d154c2 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2210,6 +2210,20 @@ static void pc_machine_set_pit(Object *obj, bool value, Error **errp)
     pcms->pit = value;
 }
 
+static bool pc_machine_get_pam(Object *obj, Error **errp)
+{
+    PCMachineState *pcms = PC_MACHINE(obj);
+
+    return pcms->pam;
+}
+
+static void pc_machine_set_pam(Object *obj, bool value, Error **errp)
+{
+    PCMachineState *pcms = PC_MACHINE(obj);
+
+    pcms->pam = value;
+}
+
 static void pc_machine_initfn(Object *obj)
 {
     PCMachineState *pcms = PC_MACHINE(obj);
@@ -2224,6 +2238,7 @@ static void pc_machine_initfn(Object *obj)
     pcms->smbus = true;
     pcms->sata = true;
     pcms->pit = true;
+    pcms->pam = true;
 }
 
 static void pc_machine_reset(void)
@@ -2372,6 +2387,9 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
 
     object_class_property_add_bool(oc, PC_MACHINE_PIT,
         pc_machine_get_pit, pc_machine_set_pit, &error_abort);
+
+    object_class_property_add_bool(oc, PC_MACHINE_PAM,
+        pc_machine_get_pam, pc_machine_set_pam, &error_abort);
 }
 
 static const TypeInfo pc_machine_info = {
diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index ff4e8b5..7497516 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -152,16 +152,18 @@ static void i440fx_update_smram(PCII440FXState *d)
 
 static void i440fx_update_pam(PCII440FXState *d)
 {
-    int i;
-    PCIDevice *pd = PCI_DEVICE(d);
-    memory_region_transaction_begin();
-    pam_update(&d->pam_regions[0], 0,
-               pd->config[I440FX_PAM]);
-    for (i = 1; i < 13; i++) {
-        pam_update(&d->pam_regions[i], i,
-                   pd->config[I440FX_PAM + ((i + 1) / 2)]);
+    if (PC_MACHINE(current_machine)->pam) {
+        int i;
+        PCIDevice *pd = PCI_DEVICE(d);
+        memory_region_transaction_begin();
+        pam_update(&d->pam_regions[0], 0,
+                   pd->config[I440FX_PAM]);
+        for (i = 1; i < 13; i++) {
+            pam_update(&d->pam_regions[i], i,
+                       pd->config[I440FX_PAM + ((i + 1) / 2)]);
+        }
+        memory_region_transaction_commit();
     }
-    memory_region_transaction_commit();
 }
 
 static void i440fx_update_memory_mappings(PCII440FXState *d)
@@ -173,14 +175,16 @@ static void i440fx_update_memory_mappings(PCII440FXState *d)
 
 static void i440fx_init_pam(PCII440FXState *d)
 {
-    int i;
-    init_pam(DEVICE(d), d->ram_memory, d->system_memory,
-             d->pci_address_space, &d->pam_regions[0],
-             PAM_BIOS_BASE,  PAM_BIOS_SIZE);
-    for (i = 0; i < 12; ++i) {
+    if (PC_MACHINE(current_machine)->pam) {
+        int i;
         init_pam(DEVICE(d), d->ram_memory, d->system_memory,
-                 d->pci_address_space, &d->pam_regions[i + 1],
-                 PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
+                 d->pci_address_space, &d->pam_regions[0],
+                 PAM_BIOS_BASE,  PAM_BIOS_SIZE);
+        for (i = 0; i < 12; ++i) {
+            init_pam(DEVICE(d), d->ram_memory, d->system_memory,
+                     d->pci_address_space, &d->pam_regions[i + 1],
+                     PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
+        }
     }
 }
 
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 8866357..e63b057 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -310,15 +310,17 @@ static void mch_update_pciexbar(MCHPCIState *mch)
 /* PAM */
 static void mch_update_pam(MCHPCIState *mch)
 {
-    PCIDevice *pd = PCI_DEVICE(mch);
-    int i;
-
-    memory_region_transaction_begin();
-    for (i = 0; i < 13; i++) {
-        pam_update(&mch->pam_regions[i], i,
-                   pd->config[MCH_HOST_BRIDGE_PAM0 + ((i + 1) / 2)]);
+    if (PC_MACHINE(current_machine)->pam) {
+        PCIDevice *pd = PCI_DEVICE(mch);
+        int i;
+
+        memory_region_transaction_begin();
+        for (i = 0; i < 13; i++) {
+            pam_update(&mch->pam_regions[i], i,
+                       pd->config[MCH_HOST_BRIDGE_PAM0 + ((i + 1) / 2)]);
+        }
+        memory_region_transaction_commit();
     }
-    memory_region_transaction_commit();
 }
 
 /* SMRAM */
@@ -462,14 +464,16 @@ static void mch_reset(DeviceState *qdev)
 
 static void mch_init_pam(MCHPCIState *mch)
 {
-    int i;
-    init_pam(DEVICE(mch), mch->ram_memory, mch->system_memory,
-             mch->pci_address_space, &mch->pam_regions[0],
-             PAM_BIOS_BASE, PAM_BIOS_SIZE);
-    for (i = 0; i < 12; ++i) {
+    if (PC_MACHINE(current_machine)->pam) {
+        int i;
         init_pam(DEVICE(mch), mch->ram_memory, mch->system_memory,
-                 mch->pci_address_space, &mch->pam_regions[i + 1],
-                 PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
+                 mch->pci_address_space, &mch->pam_regions[0],
+                 PAM_BIOS_BASE, PAM_BIOS_SIZE);
+        for (i = 0; i < 12; ++i) {
+            init_pam(DEVICE(mch), mch->ram_memory, mch->system_memory,
+                     mch->pci_address_space, &mch->pam_regions[i + 1],
+                     PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
+        }
     }
 }
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index f278b3a..05f4df5 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -66,6 +66,7 @@ struct PCMachineState {
     bool smbus;
     bool sata;
     bool pit;
+    bool pam;
 
     /* RAM information (sizes, addresses, configuration): */
     ram_addr_t below_4g_mem_size, above_4g_mem_size;
@@ -93,6 +94,7 @@ struct PCMachineState {
 #define PC_MACHINE_SMBUS            "smbus"
 #define PC_MACHINE_SATA             "sata"
 #define PC_MACHINE_PIT              "pit"
+#define PC_MACHINE_PAM              "pam"
 
 /**
  * PCMachineClass:
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/4] pam: disable pc.rom when pam is disabled

[Anthony Xu]

pc.rom depends on pam. When pam is disabled, pc.rom is useless

Signed-off-by: Anthony Xu <anthony.xu@intel.com>
---
 hw/i386/pc.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 9d154c2..455f7fe 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1358,7 +1358,7 @@ void pc_memory_init(PCMachineState *pcms,
                     MemoryRegion **ram_memory)
 {
     int linux_boot, i;
-    MemoryRegion *ram, *option_rom_mr;
+    MemoryRegion *ram;
     MemoryRegion *ram_below_4g, *ram_above_4g;
     FWCfgState *fw_cfg;
     MachineState *machine = MACHINE(pcms);
@@ -1445,15 +1445,16 @@ void pc_memory_init(PCMachineState *pcms,
     /* Initialize PC system firmware */
     pc_system_firmware_init(rom_memory, !pcmc->pci_enabled);
 
-    option_rom_mr = g_malloc(sizeof(*option_rom_mr));
-    memory_region_init_ram(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE,
+    if (pcms->pam) {
+        MemoryRegion *option_rom_mr = g_malloc(sizeof(*option_rom_mr));
+        memory_region_init_ram(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE,
                            &error_fatal);
-    vmstate_register_ram_global(option_rom_mr);
-    memory_region_add_subregion_overlap(rom_memory,
+        vmstate_register_ram_global(option_rom_mr);
+        memory_region_add_subregion_overlap(rom_memory,
                                         PC_ROM_MIN_VGA,
                                         option_rom_mr,
                                         1);
-
+    }
     fw_cfg = bochs_bios_init(&address_space_memory, pcms);
 
     rom_set_fw(fw_cfg);
-- 
1.8.3.1

[Qemu-devel] [PATCH 4/4] pam: setup pc.bios

[Anthony Xu]

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 <anthony.xu@intel.com>
---
 hw/i386/pc.c         |  2 +-
 hw/i386/pc_sysfw.c   | 30 +++++++++++++++++++++---------
 include/hw/i386/pc.h |  4 +++-
 3 files changed, 25 insertions(+), 11 deletions(-)

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 */
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 4/4] pam: setup pc.bios

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH 1/4] pam:refactor PAM related code

[Paolo Bonzini]

On 08/04/2017 08:45, Anthony Xu wrote:
> split PAM SMRAM functions in piix.c
> create mch_init_pam in q35.c

Could you further move

    MemoryRegion *ram_memory;
    MemoryRegion *system_memory;
    MemoryRegion *pci_address_space;
    PAMMemoryRegion pam_regions[13];

from the northbridge devices up to PCMachineState, and move the common
code for PIIX and Q35 to hw/pci-host/pam.c?

It looks like you can define a better API than what pam.c currently
provides:

void pc_init_pam(PCMachineState *pc_machine, DeviceState *d);
void pc_update_pam(PCMachineState *pc_machine, uint8_t *pam_config);

or even remove "DeviceState *d" because the owner of the memory regions
can be the PCMachineState.

Thanks,

Paolo

> Signed-off-by: Anthony Xu <anthony.xu@intel.com>
> ---
>  hw/pci-host/piix.c | 58 ++++++++++++++++++++++++++++++++++++++----------------
>  hw/pci-host/q35.c  | 23 +++++++++++++---------
>  2 files changed, 55 insertions(+), 26 deletions(-)
> 
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index f9218aa..ff4e8b5 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -138,16 +138,11 @@ static int pci_slot_get_pirq(PCIDevice *pci_dev, int pci_intx)
>      return (pci_intx + slot_addend) & 3;
>  }
>  
> -static void i440fx_update_memory_mappings(PCII440FXState *d)
> +static void i440fx_update_smram(PCII440FXState *d)
>  {
> -    int i;
>      PCIDevice *pd = PCI_DEVICE(d);
>  
>      memory_region_transaction_begin();
> -    for (i = 0; i < 13; i++) {
> -        pam_update(&d->pam_regions[i], i,
> -                   pd->config[I440FX_PAM + ((i + 1) / 2)]);
> -    }
>      memory_region_set_enabled(&d->smram_region,
>                                !(pd->config[I440FX_SMRAM] & SMRAM_D_OPEN));
>      memory_region_set_enabled(&d->smram,
> @@ -155,6 +150,39 @@ static void i440fx_update_memory_mappings(PCII440FXState *d)
>      memory_region_transaction_commit();
>  }
>  
> +static void i440fx_update_pam(PCII440FXState *d)
> +{
> +    int i;
> +    PCIDevice *pd = PCI_DEVICE(d);
> +    memory_region_transaction_begin();
> +    pam_update(&d->pam_regions[0], 0,
> +               pd->config[I440FX_PAM]);
> +    for (i = 1; i < 13; i++) {
> +        pam_update(&d->pam_regions[i], i,
> +                   pd->config[I440FX_PAM + ((i + 1) / 2)]);
> +    }
> +    memory_region_transaction_commit();
> +}
> +
> +static void i440fx_update_memory_mappings(PCII440FXState *d)
> +{
> +    i440fx_update_pam(d);
> +    i440fx_update_smram(d);
> +}
> +
> +
> +static void i440fx_init_pam(PCII440FXState *d)
> +{
> +    int i;
> +    init_pam(DEVICE(d), d->ram_memory, d->system_memory,
> +             d->pci_address_space, &d->pam_regions[0],
> +             PAM_BIOS_BASE,  PAM_BIOS_SIZE);
> +    for (i = 0; i < 12; ++i) {
> +        init_pam(DEVICE(d), d->ram_memory, d->system_memory,
> +                 d->pci_address_space, &d->pam_regions[i + 1],
> +                 PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
> +    }
> +}
>  
>  static void i440fx_write_config(PCIDevice *dev,
>                                  uint32_t address, uint32_t val, int len)
> @@ -163,9 +191,12 @@ static void i440fx_write_config(PCIDevice *dev,
>  
>      /* XXX: implement SMRAM.D_LOCK */
>      pci_default_write_config(dev, address, val, len);
> -    if (ranges_overlap(address, len, I440FX_PAM, I440FX_PAM_SIZE) ||
> -        range_covers_byte(address, len, I440FX_SMRAM)) {
> -        i440fx_update_memory_mappings(d);
> +    if (ranges_overlap(address, len, I440FX_PAM, I440FX_PAM_SIZE)) {
> +        i440fx_update_pam(d);
> +    }
> +
> +    if (range_covers_byte(address, len, I440FX_SMRAM)) {
> +        i440fx_update_smram(d);
>      }
>  }
>  
> @@ -335,7 +366,6 @@ PCIBus *i440fx_init(const char *host_type, const char *pci_type,
>      PCIHostState *s;
>      PIIX3State *piix3;
>      PCII440FXState *f;
> -    unsigned i;
>      I440FXState *i440fx;
>  
>      dev = qdev_create(NULL, host_type);
> @@ -378,13 +408,7 @@ PCIBus *i440fx_init(const char *host_type, const char *pci_type,
>      object_property_add_const_link(qdev_get_machine(), "smram",
>                                     OBJECT(&f->smram), &error_abort);
>  
> -    init_pam(dev, f->ram_memory, f->system_memory, f->pci_address_space,
> -             &f->pam_regions[0], PAM_BIOS_BASE, PAM_BIOS_SIZE);
> -    for (i = 0; i < 12; ++i) {
> -        init_pam(dev, f->ram_memory, f->system_memory, f->pci_address_space,
> -                 &f->pam_regions[i+1], PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE,
> -                 PAM_EXPAN_SIZE);
> -    }
> +    i440fx_init_pam(f);
>  
>      /* Xen supports additional interrupt routes from the PCI devices to
>       * the IOAPIC: the four pins of each PCI device on the bus are also
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index 344f77b..8866357 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -460,9 +460,21 @@ static void mch_reset(DeviceState *qdev)
>      mch_update(mch);
>  }
>  
> -static void mch_realize(PCIDevice *d, Error **errp)
> +static void mch_init_pam(MCHPCIState *mch)
>  {
>      int i;
> +    init_pam(DEVICE(mch), mch->ram_memory, mch->system_memory,
> +             mch->pci_address_space, &mch->pam_regions[0],
> +             PAM_BIOS_BASE, PAM_BIOS_SIZE);
> +    for (i = 0; i < 12; ++i) {
> +        init_pam(DEVICE(mch), mch->ram_memory, mch->system_memory,
> +                 mch->pci_address_space, &mch->pam_regions[i + 1],
> +                 PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
> +    }
> +}
> +
> +static void mch_realize(PCIDevice *d, Error **errp)
> +{
>      MCHPCIState *mch = MCH_PCI_DEVICE(d);
>  
>      /* setup pci memory mapping */
> @@ -510,14 +522,7 @@ static void mch_realize(PCIDevice *d, Error **errp)
>      object_property_add_const_link(qdev_get_machine(), "smram",
>                                     OBJECT(&mch->smram), &error_abort);
>  
> -    init_pam(DEVICE(mch), mch->ram_memory, mch->system_memory,
> -             mch->pci_address_space, &mch->pam_regions[0],
> -             PAM_BIOS_BASE, PAM_BIOS_SIZE);
> -    for (i = 0; i < 12; ++i) {
> -        init_pam(DEVICE(mch), mch->ram_memory, mch->system_memory,
> -                 mch->pci_address_space, &mch->pam_regions[i+1],
> -                 PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
> -    }
> +    mch_init_pam(mch);
>  }
>  
>  uint64_t mch_mcfg_base(void)
> 

Re: [Qemu-devel] [PATCH 1/4] pam:refactor PAM related code

[Xu, Anthony]

> On 08/04/2017 08:45, Anthony Xu wrote:
> > split PAM SMRAM functions in piix.c
> > create mch_init_pam in q35.c
> 
> Could you further move
> 
>     MemoryRegion *ram_memory;
>     MemoryRegion *system_memory;
>     MemoryRegion *pci_address_space;
>     PAMMemoryRegion pam_regions[13];
> 
> from the northbridge devices up to PCMachineState, and move the common
> code for PIIX and Q35 to hw/pci-host/pam.c?
> 
> It looks like you can define a better API than what pam.c currently
> provides:
> 
> void pc_init_pam(PCMachineState *pc_machine, DeviceState *d);
> void pc_update_pam(PCMachineState *pc_machine, uint8_t *pam_config);
> 
> or even remove "DeviceState *d" because the owner of the memory regions
> can be the PCMachineState.
> 

Good idea!
Will do.

Anthony

Re: [Qemu-devel] [PATCH 4/4] pam: setup pc.bios

[Xu, Anthony]

> 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

Re: [Qemu-devel] [PATCH 4/4] pam: setup pc.bios

[Paolo Bonzini]

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