[PATCH v3 0/9] PC cleanups

[PATCH v3 0/9] PC cleanups

[Bernhard Beschow]

This series contains some cleanups I came across when working on the PC
machines. It consists of reducing the usage of global variables and eliminating
some redundancies.

One notable change is that the SMRAM memory region gets moved from the i440fx
and q35 host bridges into the x86 machine. This will simplify cleaning up these
host bridges which will be done in a separate series.

Testing done:
* `make check`
' `make check-avocado`
* `qemu-system-x86_64 -M q35 -m 2G -cdrom \
   manjaro-kde-21.3.2-220704-linux515.iso`
* `qemu-system-x86_64 -M pc -m 2G -cdrom manjaro-kde-21.3.2-220704-linux515.iso`

v3:
* Add three patches regarding init_pam() and SMRAM.
* Drop 'hw/i386/pc_q35: Resolve redundant q35_host variable' since Phil posted
  a similar patch in a more comprehensive series:
  https://lore.kernel.org/qemu-devel/20230203180914.49112-13-philmd@linaro.org/
* Drop 'hw/isa/lpc_ich9: Reuse memory and io address space of PCI bus' since
  it inadvertantly changed the memory hierarchy.
* Drop ICH9 cleanups again in favor of a separate series.

v2:
* Factor out 'hw/i386/pc_q35: Reuse machine parameter' from 'hw/i386/pc_q35:
  Resolve redundant q35_host variable' (Zoltan)
* Lower type of phb to Object in 'hw/i386/pc_q35: Resolve redundant q35_host
  variable' (Zoltan)
* Add ICH9 cleanups

Bernhard Beschow (9):
  hw/pci-host/i440fx: Inline sysbus_add_io()
  hw/pci-host/q35: Inline sysbus_add_io()
  hw/i386/pc_q35: Reuse machine parameter
  hw/i386/pc_{q35,piix}: Reuse MachineClass::desc as SMB product name
  hw/i386/pc_{q35,piix}: Minimize usage of get_system_memory()
  hw/i386/pc: Initialize ram_memory variable directly
  hw/pci-host/pam: Make init_pam() usage more readable
  hw/i386/x86: Make TYPE_X86_MACHINE the owner of smram
  target/i386/tcg/sysemu/tcg-cpu: Avoid own opinion about smram size

 include/hw/i386/pc.h             |  1 -
 include/hw/i386/x86.h            |  2 ++
 include/hw/pci-host/i440fx.h     |  7 ++++---
 include/hw/pci-host/pam.h        |  5 +++--
 include/hw/pci-host/q35.h        |  4 +++-
 hw/i386/pc.c                     |  2 --
 hw/i386/pc_piix.c                | 10 +++++-----
 hw/i386/pc_q35.c                 | 16 +++++++++-------
 hw/i386/x86.c                    |  4 ++++
 hw/pci-host/i440fx.c             | 28 +++++++++++++---------------
 hw/pci-host/pam.c                | 12 ++++++------
 hw/pci-host/q35.c                | 31 ++++++++++++++++---------------
 target/i386/tcg/sysemu/tcg-cpu.c |  3 +--
 13 files changed, 66 insertions(+), 59 deletions(-)

-- 
2.39.1

[PATCH v3 1/9] hw/pci-host/i440fx: Inline sysbus_add_io()

[Bernhard Beschow]

sysbus_add_io() just wraps memory_region_add_subregion() while also
obscuring where the memory is attached. So use
memory_region_add_subregion() directly and attach it to the existing
memory region s->bus->address_space_io which is set as an alias to
get_system_io() by the pc machine.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 hw/pci-host/i440fx.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
index 262f82c303..9c6882d3fc 100644
--- a/hw/pci-host/i440fx.c
+++ b/hw/pci-host/i440fx.c
@@ -27,6 +27,7 @@
 #include "qemu/range.h"
 #include "hw/i386/pc.h"
 #include "hw/pci/pci.h"
+#include "hw/pci/pci_bus.h"
 #include "hw/pci/pci_host.h"
 #include "hw/pci-host/i440fx.h"
 #include "hw/qdev-properties.h"
@@ -217,10 +218,10 @@ static void i440fx_pcihost_realize(DeviceState *dev, Error **errp)
     PCIHostState *s = PCI_HOST_BRIDGE(dev);
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
 
-    sysbus_add_io(sbd, 0xcf8, &s->conf_mem);
+    memory_region_add_subregion(s->bus->address_space_io, 0xcf8, &s->conf_mem);
     sysbus_init_ioports(sbd, 0xcf8, 4);
 
-    sysbus_add_io(sbd, 0xcfc, &s->data_mem);
+    memory_region_add_subregion(s->bus->address_space_io, 0xcfc, &s->data_mem);
     sysbus_init_ioports(sbd, 0xcfc, 4);
 
     /* register i440fx 0xcf8 port as coalesced pio */
-- 
2.39.1

[PATCH v3 2/9] hw/pci-host/q35: Inline sysbus_add_io()

[Bernhard Beschow]

sysbus_add_io() just wraps memory_region_add_subregion() while also
obscuring where the memory is attached. So use
memory_region_add_subregion() directly and attach it to the existing
memory region s->mch.address_space_io which is set as an alias to
get_system_io() by the q35 machine.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 hw/pci-host/q35.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 26390863d6..fa05844319 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -50,10 +50,12 @@ static void q35_host_realize(DeviceState *dev, Error **errp)
     Q35PCIHost *s = Q35_HOST_DEVICE(dev);
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
 
-    sysbus_add_io(sbd, MCH_HOST_BRIDGE_CONFIG_ADDR, &pci->conf_mem);
+    memory_region_add_subregion(s->mch.address_space_io,
+                                MCH_HOST_BRIDGE_CONFIG_ADDR, &pci->conf_mem);
     sysbus_init_ioports(sbd, MCH_HOST_BRIDGE_CONFIG_ADDR, 4);
 
-    sysbus_add_io(sbd, MCH_HOST_BRIDGE_CONFIG_DATA, &pci->data_mem);
+    memory_region_add_subregion(s->mch.address_space_io,
+                                MCH_HOST_BRIDGE_CONFIG_DATA, &pci->data_mem);
     sysbus_init_ioports(sbd, MCH_HOST_BRIDGE_CONFIG_DATA, 4);
 
     /* register q35 0xcf8 port as coalesced pio */
-- 
2.39.1

[PATCH v3 3/9] hw/i386/pc_q35: Reuse machine parameter

[Bernhard Beschow]

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 hw/i386/pc_q35.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 66cd718b70..dee2b38474 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -218,7 +218,7 @@ static void pc_q35_init(MachineState *machine)
     pc_memory_init(pcms, get_system_memory(), rom_memory, &ram_memory,
                    pci_hole64_size);
 
-    object_property_add_child(qdev_get_machine(), "q35", OBJECT(q35_host));
+    object_property_add_child(OBJECT(machine), "q35", OBJECT(q35_host));
     object_property_set_link(OBJECT(q35_host), MCH_HOST_PROP_RAM_MEM,
                              OBJECT(ram_memory), NULL);
     object_property_set_link(OBJECT(q35_host), MCH_HOST_PROP_PCI_MEM,
-- 
2.39.1

[PATCH v3 4/9] hw/i386/pc_{q35, piix}: Reuse MachineClass::desc as SMB product name

[Bernhard Beschow]

No need to repeat the descriptions.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 hw/i386/pc_piix.c | 2 +-
 hw/i386/pc_q35.c  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index df64dd8dcc..ee9d9a4175 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -197,7 +197,7 @@ static void pc_init1(MachineState *machine,
     if (pcmc->smbios_defaults) {
         MachineClass *mc = MACHINE_GET_CLASS(machine);
         /* These values are guest ABI, do not change */
-        smbios_set_defaults("QEMU", "Standard PC (i440FX + PIIX, 1996)",
+        smbios_set_defaults("QEMU", mc->desc,
                             mc->name, pcmc->smbios_legacy_mode,
                             pcmc->smbios_uuid_encoded,
                             pcms->smbios_entry_point_type);
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index dee2b38474..71b7a30bb9 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -199,7 +199,7 @@ static void pc_q35_init(MachineState *machine)
 
     if (pcmc->smbios_defaults) {
         /* These values are guest ABI, do not change */
-        smbios_set_defaults("QEMU", "Standard PC (Q35 + ICH9, 2009)",
+        smbios_set_defaults("QEMU", mc->desc,
                             mc->name, pcmc->smbios_legacy_mode,
                             pcmc->smbios_uuid_encoded,
                             pcms->smbios_entry_point_type);
-- 
2.39.1

[PATCH v3 5/9] hw/i386/pc_{q35, piix}: Minimize usage of get_system_memory()

[Bernhard Beschow]

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 hw/i386/pc_piix.c | 2 +-
 hw/i386/pc_q35.c  | 7 ++++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index ee9d9a4175..5bde4533cc 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -241,7 +241,7 @@ static void pc_init1(MachineState *machine,
         isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(piix3), "isa.0"));
     } else {
         pci_bus = NULL;
-        isa_bus = isa_bus_new(NULL, get_system_memory(), system_io,
+        isa_bus = isa_bus_new(NULL, system_memory, system_io,
                               &error_abort);
         i8257_dma_init(isa_bus, 0);
         pcms->hpet_enabled = false;
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 71b7a30bb9..8253b49296 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -125,6 +125,7 @@ static void pc_q35_init(MachineState *machine)
     DeviceState *lpc_dev;
     BusState *idebus[MAX_SATA_PORTS];
     ISADevice *rtc_state;
+    MemoryRegion *system_memory = get_system_memory();
     MemoryRegion *system_io = get_system_io();
     MemoryRegion *pci_memory;
     MemoryRegion *rom_memory;
@@ -192,7 +193,7 @@ static void pc_q35_init(MachineState *machine)
         rom_memory = pci_memory;
     } else {
         pci_memory = NULL;
-        rom_memory = get_system_memory();
+        rom_memory = system_memory;
     }
 
     pc_guest_info_init(pcms);
@@ -215,7 +216,7 @@ static void pc_q35_init(MachineState *machine)
     }
 
     /* allocate ram and load rom/bios */
-    pc_memory_init(pcms, get_system_memory(), rom_memory, &ram_memory,
+    pc_memory_init(pcms, system_memory, rom_memory, &ram_memory,
                    pci_hole64_size);
 
     object_property_add_child(OBJECT(machine), "q35", OBJECT(q35_host));
@@ -224,7 +225,7 @@ static void pc_q35_init(MachineState *machine)
     object_property_set_link(OBJECT(q35_host), MCH_HOST_PROP_PCI_MEM,
                              OBJECT(pci_memory), NULL);
     object_property_set_link(OBJECT(q35_host), MCH_HOST_PROP_SYSTEM_MEM,
-                             OBJECT(get_system_memory()), NULL);
+                             OBJECT(system_memory), NULL);
     object_property_set_link(OBJECT(q35_host), MCH_HOST_PROP_IO_MEM,
                              OBJECT(system_io), NULL);
     object_property_set_int(OBJECT(q35_host), PCI_HOST_BELOW_4G_MEM_SIZE,
-- 
2.39.1

[PATCH v3 6/9] hw/i386/pc: Initialize ram_memory variable directly

[Bernhard Beschow]

Going through pc_memory_init() seems quite complicated for a simple
assignment.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/i386/pc.h | 1 -
 hw/i386/pc.c         | 2 --
 hw/i386/pc_piix.c    | 4 ++--
 hw/i386/pc_q35.c     | 5 ++---
 4 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 66e3d059ef..b60b95921b 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -162,7 +162,6 @@ void xen_load_linux(PCMachineState *pcms);
 void pc_memory_init(PCMachineState *pcms,
                     MemoryRegion *system_memory,
                     MemoryRegion *rom_memory,
-                    MemoryRegion **ram_memory,
                     uint64_t pci_hole64_size);
 uint64_t pc_pci_hole64_start(void);
 DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus);
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 6e592bd969..8898cc9961 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -936,7 +936,6 @@ static hwaddr pc_max_used_gpa(PCMachineState *pcms, uint64_t pci_hole64_size)
 void pc_memory_init(PCMachineState *pcms,
                     MemoryRegion *system_memory,
                     MemoryRegion *rom_memory,
-                    MemoryRegion **ram_memory,
                     uint64_t pci_hole64_size)
 {
     int linux_boot, i;
@@ -994,7 +993,6 @@ void pc_memory_init(PCMachineState *pcms,
      * Split single memory region and use aliases to address portions of it,
      * done for backwards compatibility with older qemus.
      */
-    *ram_memory = machine->ram;
     ram_below_4g = g_malloc(sizeof(*ram_below_4g));
     memory_region_init_alias(ram_below_4g, NULL, "ram-below-4g", machine->ram,
                              0, x86ms->below_4g_mem_size);
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 5bde4533cc..00ba725656 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -143,6 +143,7 @@ static void pc_init1(MachineState *machine,
     if (xen_enabled()) {
         xen_hvm_init_pc(pcms, &ram_memory);
     } else {
+        ram_memory = machine->ram;
         if (!pcms->max_ram_below_4g) {
             pcms->max_ram_below_4g = 0xe0000000; /* default: 3.5G */
         }
@@ -205,8 +206,7 @@ static void pc_init1(MachineState *machine,
 
     /* allocate ram and load rom/bios */
     if (!xen_enabled()) {
-        pc_memory_init(pcms, system_memory,
-                       rom_memory, &ram_memory, hole64_size);
+        pc_memory_init(pcms, system_memory, rom_memory, hole64_size);
     } else {
         pc_system_flash_cleanup_unused(pcms);
         if (machine->kernel_filename != NULL) {
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 8253b49296..88f0981f50 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -129,7 +129,7 @@ static void pc_q35_init(MachineState *machine)
     MemoryRegion *system_io = get_system_io();
     MemoryRegion *pci_memory;
     MemoryRegion *rom_memory;
-    MemoryRegion *ram_memory;
+    MemoryRegion *ram_memory = machine->ram;
     GSIState *gsi_state;
     ISABus *isa_bus;
     int i;
@@ -216,8 +216,7 @@ static void pc_q35_init(MachineState *machine)
     }
 
     /* allocate ram and load rom/bios */
-    pc_memory_init(pcms, system_memory, rom_memory, &ram_memory,
-                   pci_hole64_size);
+    pc_memory_init(pcms, system_memory, rom_memory, pci_hole64_size);
 
     object_property_add_child(OBJECT(machine), "q35", OBJECT(q35_host));
     object_property_set_link(OBJECT(q35_host), MCH_HOST_PROP_RAM_MEM,
-- 
2.39.1

[PATCH v3 7/9] hw/pci-host/pam: Make init_pam() usage more readable

[Bernhard Beschow]

Unlike pam_update() which takes the subject -- PAMMemoryRegion -- as
first argument, init_pam() takes it as fifth (!) argument. This makes it
quite hard to figure out what an init_pam() invocation actually
initializes. By moving the subject to the front this should become
clearer.

While at it, lower the DeviceState parameter to Object, also
communicating more clearly that this parameter is just the owner rather
than some (heavy?) dependency.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/pci-host/pam.h |  5 +++--
 hw/pci-host/i440fx.c      | 10 +++++-----
 hw/pci-host/pam.c         | 12 ++++++------
 hw/pci-host/q35.c         |  8 ++++----
 4 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/include/hw/pci-host/pam.h b/include/hw/pci-host/pam.h
index c1fd06ba2a..005916f826 100644
--- a/include/hw/pci-host/pam.h
+++ b/include/hw/pci-host/pam.h
@@ -87,8 +87,9 @@ typedef struct PAMMemoryRegion {
     unsigned current;
 } PAMMemoryRegion;
 
-void init_pam(DeviceState *dev, MemoryRegion *ram, MemoryRegion *system,
-              MemoryRegion *pci, PAMMemoryRegion *mem, uint32_t start, uint32_t size);
+void init_pam(PAMMemoryRegion *mem, Object *owner, MemoryRegion *ram,
+              MemoryRegion *system, MemoryRegion *pci,
+              uint32_t start, uint32_t size);
 void pam_update(PAMMemoryRegion *mem, int idx, uint8_t val);
 
 #endif /* QEMU_PAM_H */
diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
index 9c6882d3fc..61e7b97ff4 100644
--- a/hw/pci-host/i440fx.c
+++ b/hw/pci-host/i440fx.c
@@ -292,12 +292,12 @@ PCIBus *i440fx_init(const char *pci_type,
     object_property_add_const_link(qdev_get_machine(), "smram",
                                    OBJECT(&f->smram));
 
-    init_pam(dev, f->ram_memory, f->system_memory, f->pci_address_space,
-             &f->pam_regions[0], PAM_BIOS_BASE, PAM_BIOS_SIZE);
+    init_pam(&f->pam_regions[0], OBJECT(d), f->ram_memory, f->system_memory,
+             f->pci_address_space, PAM_BIOS_BASE, PAM_BIOS_SIZE);
     for (i = 0; i < ARRAY_SIZE(f->pam_regions) - 1; ++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);
+        init_pam(&f->pam_regions[i + 1], OBJECT(d), f->ram_memory,
+                 f->system_memory, f->pci_address_space,
+                 PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
     }
 
     ram_size = ram_size / 8 / 1024 / 1024;
diff --git a/hw/pci-host/pam.c b/hw/pci-host/pam.c
index 454dd120db..68e9884d27 100644
--- a/hw/pci-host/pam.c
+++ b/hw/pci-host/pam.c
@@ -30,24 +30,24 @@
 #include "qemu/osdep.h"
 #include "hw/pci-host/pam.h"
 
-void init_pam(DeviceState *dev, MemoryRegion *ram_memory,
+void init_pam(PAMMemoryRegion *mem, Object *owner, MemoryRegion *ram_memory,
               MemoryRegion *system_memory, MemoryRegion *pci_address_space,
-              PAMMemoryRegion *mem, uint32_t start, uint32_t size)
+              uint32_t start, uint32_t size)
 {
     int i;
 
     /* RAM */
-    memory_region_init_alias(&mem->alias[3], OBJECT(dev), "pam-ram", ram_memory,
+    memory_region_init_alias(&mem->alias[3], owner, "pam-ram", ram_memory,
                              start, size);
     /* ROM (XXX: not quite correct) */
-    memory_region_init_alias(&mem->alias[1], OBJECT(dev), "pam-rom", ram_memory,
+    memory_region_init_alias(&mem->alias[1], owner, "pam-rom", ram_memory,
                              start, size);
     memory_region_set_readonly(&mem->alias[1], true);
 
     /* XXX: should distinguish read/write cases */
-    memory_region_init_alias(&mem->alias[0], OBJECT(dev), "pam-pci", pci_address_space,
+    memory_region_init_alias(&mem->alias[0], owner, "pam-pci", pci_address_space,
                              start, size);
-    memory_region_init_alias(&mem->alias[2], OBJECT(dev), "pam-pci", ram_memory,
+    memory_region_init_alias(&mem->alias[2], owner, "pam-pci", ram_memory,
                              start, size);
 
     memory_region_transaction_begin();
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index fa05844319..fd18920e7f 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -645,12 +645,12 @@ static void mch_realize(PCIDevice *d, Error **errp)
     object_property_add_const_link(qdev_get_machine(), "smram",
                                    OBJECT(&mch->smram));
 
-    init_pam(DEVICE(mch), mch->ram_memory, mch->system_memory,
-             mch->pci_address_space, &mch->pam_regions[0],
+    init_pam(&mch->pam_regions[0], OBJECT(mch), mch->ram_memory,
+             mch->system_memory, mch->pci_address_space,
              PAM_BIOS_BASE, PAM_BIOS_SIZE);
     for (i = 0; i < ARRAY_SIZE(mch->pam_regions) - 1; ++i) {
-        init_pam(DEVICE(mch), mch->ram_memory, mch->system_memory,
-                 mch->pci_address_space, &mch->pam_regions[i+1],
+        init_pam(&mch->pam_regions[i + 1], OBJECT(mch), mch->ram_memory,
+                 mch->system_memory, mch->pci_address_space,
                  PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
     }
 }
-- 
2.39.1

[PATCH v3 8/9] hw/i386/x86: Make TYPE_X86_MACHINE the owner of smram

[Bernhard Beschow]

Treat the smram MemoryRegion analoguous to other memory regions such as
ram, pci, io, ... , making the used memory regions more explicit when
instantiating q35 or i440fx.

Note that the q35 device uses these memory regions only during the
realize phase which suggests that it shouldn't be the owner of smram.
i440fx activates/deactivates the whole smram memory region depending on
the SMRAM_G_SMRAME bit which seems wrong since it should only handle the
128kb region. If this got changed, i440fx would also only use smram
during its realize phase.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/i386/x86.h        |  2 ++
 include/hw/pci-host/i440fx.h |  7 ++++---
 include/hw/pci-host/q35.h    |  4 +++-
 hw/i386/pc_piix.c            |  2 +-
 hw/i386/pc_q35.c             |  2 ++
 hw/i386/x86.c                |  4 ++++
 hw/pci-host/i440fx.c         | 13 +++++--------
 hw/pci-host/q35.c            | 17 ++++++++---------
 8 files changed, 29 insertions(+), 22 deletions(-)

diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
index 62fa5774f8..ba6912b721 100644
--- a/include/hw/i386/x86.h
+++ b/include/hw/i386/x86.h
@@ -59,6 +59,8 @@ struct X86MachineState {
     /* Start address of the initial RAM above 4G */
     uint64_t above_4g_mem_start;
 
+    MemoryRegion smram;
+
     /* CPU and apic information: */
     bool apic_xrupt_override;
     unsigned pci_irq_mask;
diff --git a/include/hw/pci-host/i440fx.h b/include/hw/pci-host/i440fx.h
index bf57216c78..e9efdb3c5f 100644
--- a/include/hw/pci-host/i440fx.h
+++ b/include/hw/pci-host/i440fx.h
@@ -28,9 +28,10 @@ struct PCII440FXState {
     MemoryRegion *system_memory;
     MemoryRegion *pci_address_space;
     MemoryRegion *ram_memory;
+    MemoryRegion *smram;
     PAMMemoryRegion pam_regions[PAM_REGIONS_COUNT];
     MemoryRegion smram_region;
-    MemoryRegion smram, low_smram;
+    MemoryRegion low_smram;
 };
 
 #define TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE "igd-passthrough-i440FX"
@@ -43,7 +44,7 @@ PCIBus *i440fx_init(const char *pci_type,
                     ram_addr_t below_4g_mem_size,
                     ram_addr_t above_4g_mem_size,
                     MemoryRegion *pci_memory,
-                    MemoryRegion *ram_memory);
-
+                    MemoryRegion *ram_memory,
+                    MemoryRegion *smram);
 
 #endif
diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
index e89329c51e..fcbe57b42d 100644
--- a/include/hw/pci-host/q35.h
+++ b/include/hw/pci-host/q35.h
@@ -44,9 +44,10 @@ struct MCHPCIState {
     MemoryRegion *pci_address_space;
     MemoryRegion *system_memory;
     MemoryRegion *address_space_io;
+    MemoryRegion *smram;
     PAMMemoryRegion pam_regions[PAM_REGIONS_COUNT];
     MemoryRegion smram_region, open_high_smram;
-    MemoryRegion smram, low_smram, high_smram;
+    MemoryRegion low_smram, high_smram;
     MemoryRegion tseg_blackhole, tseg_window;
     MemoryRegion smbase_blackhole, smbase_window;
     bool has_smram_at_smbase;
@@ -75,6 +76,7 @@ struct Q35PCIHost {
  */
 
 #define MCH_HOST_PROP_RAM_MEM "ram-mem"
+#define MCH_HOST_PROP_SMRAM_MEM "smram-mem"
 #define MCH_HOST_PROP_PCI_MEM "pci-mem"
 #define MCH_HOST_PROP_SYSTEM_MEM "system-mem"
 #define MCH_HOST_PROP_IO_MEM "io-mem"
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 00ba725656..41aaaa5465 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -228,7 +228,7 @@ static void pc_init1(MachineState *machine,
                               system_memory, system_io, machine->ram_size,
                               x86ms->below_4g_mem_size,
                               x86ms->above_4g_mem_size,
-                              pci_memory, ram_memory);
+                              pci_memory, ram_memory, &x86ms->smram);
         pci_bus_map_irqs(pci_bus,
                          xen_enabled() ? xen_pci_slot_get_pirq
                                        : pc_pci_slot_get_pirq);
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 88f0981f50..480226de2c 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -221,6 +221,8 @@ static void pc_q35_init(MachineState *machine)
     object_property_add_child(OBJECT(machine), "q35", OBJECT(q35_host));
     object_property_set_link(OBJECT(q35_host), MCH_HOST_PROP_RAM_MEM,
                              OBJECT(ram_memory), NULL);
+    object_property_set_link(OBJECT(q35_host), MCH_HOST_PROP_SMRAM_MEM,
+                             OBJECT(&x86ms->smram), NULL);
     object_property_set_link(OBJECT(q35_host), MCH_HOST_PROP_PCI_MEM,
                              OBJECT(pci_memory), NULL);
     object_property_set_link(OBJECT(q35_host), MCH_HOST_PROP_SYSTEM_MEM,
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index eaff4227bd..d7e219b1eb 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -1436,6 +1436,10 @@ static void x86_machine_initfn(Object *obj)
     x86ms->oem_table_id = g_strndup(ACPI_BUILD_APPNAME8, 8);
     x86ms->bus_lock_ratelimit = 0;
     x86ms->above_4g_mem_start = 4 * GiB;
+
+    memory_region_init(&x86ms->smram, obj, "smram", 4 * GiB);
+    memory_region_set_enabled(&x86ms->smram, true);
+    object_property_add_const_link(obj, "smram", OBJECT(&x86ms->smram));
 }
 
 static void x86_machine_class_init(ObjectClass *oc, void *data)
diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
index 61e7b97ff4..8f4a4f59a6 100644
--- a/hw/pci-host/i440fx.c
+++ b/hw/pci-host/i440fx.c
@@ -23,7 +23,6 @@
  */
 
 #include "qemu/osdep.h"
-#include "qemu/units.h"
 #include "qemu/range.h"
 #include "hw/i386/pc.h"
 #include "hw/pci/pci.h"
@@ -77,7 +76,7 @@ static void i440fx_update_memory_mappings(PCII440FXState *d)
     }
     memory_region_set_enabled(&d->smram_region,
                               !(pd->config[I440FX_SMRAM] & SMRAM_D_OPEN));
-    memory_region_set_enabled(&d->smram,
+    memory_region_set_enabled(d->smram,
                               pd->config[I440FX_SMRAM] & SMRAM_G_SMRAME);
     memory_region_transaction_commit();
 }
@@ -246,7 +245,8 @@ PCIBus *i440fx_init(const char *pci_type,
                     ram_addr_t below_4g_mem_size,
                     ram_addr_t above_4g_mem_size,
                     MemoryRegion *pci_address_space,
-                    MemoryRegion *ram_memory)
+                    MemoryRegion *ram_memory,
+                    MemoryRegion *smram)
 {
     PCIBus *b;
     PCIDevice *d;
@@ -267,6 +267,7 @@ PCIBus *i440fx_init(const char *pci_type,
     f->system_memory = address_space_mem;
     f->pci_address_space = pci_address_space;
     f->ram_memory = ram_memory;
+    f->smram = smram;
 
     i440fx = I440FX_PCI_HOST_BRIDGE(dev);
     range_set_bounds(&i440fx->pci_hole, below_4g_mem_size,
@@ -283,14 +284,10 @@ PCIBus *i440fx_init(const char *pci_type,
     memory_region_set_enabled(&f->smram_region, true);
 
     /* smram, as seen by SMM CPUs */
-    memory_region_init(&f->smram, OBJECT(d), "smram", 4 * GiB);
-    memory_region_set_enabled(&f->smram, true);
     memory_region_init_alias(&f->low_smram, OBJECT(d), "smram-low",
                              f->ram_memory, 0xa0000, 0x20000);
     memory_region_set_enabled(&f->low_smram, true);
-    memory_region_add_subregion(&f->smram, 0xa0000, &f->low_smram);
-    object_property_add_const_link(qdev_get_machine(), "smram",
-                                   OBJECT(&f->smram));
+    memory_region_add_subregion(f->smram, 0xa0000, &f->low_smram);
 
     init_pam(&f->pam_regions[0], OBJECT(d), f->ram_memory, f->system_memory,
              f->pci_address_space, PAM_BIOS_BASE, PAM_BIOS_SIZE);
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index fd18920e7f..83f2a98c71 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -246,6 +246,10 @@ static void q35_host_initfn(Object *obj)
                              (Object **) &s->mch.ram_memory,
                              qdev_prop_allow_set_link_before_realize, 0);
 
+    object_property_add_link(obj, MCH_HOST_PROP_SMRAM_MEM, TYPE_MEMORY_REGION,
+                             (Object **) &s->mch.smram,
+                             qdev_prop_allow_set_link_before_realize, 0);
+
     object_property_add_link(obj, MCH_HOST_PROP_PCI_MEM, TYPE_MEMORY_REGION,
                              (Object **) &s->mch.pci_address_space,
                              qdev_prop_allow_set_link_before_realize, 0);
@@ -594,19 +598,17 @@ static void mch_realize(PCIDevice *d, Error **errp)
     memory_region_set_enabled(&mch->open_high_smram, false);
 
     /* smram, as seen by SMM CPUs */
-    memory_region_init(&mch->smram, OBJECT(mch), "smram", 4 * GiB);
-    memory_region_set_enabled(&mch->smram, true);
     memory_region_init_alias(&mch->low_smram, OBJECT(mch), "smram-low",
                              mch->ram_memory, MCH_HOST_BRIDGE_SMRAM_C_BASE,
                              MCH_HOST_BRIDGE_SMRAM_C_SIZE);
     memory_region_set_enabled(&mch->low_smram, true);
-    memory_region_add_subregion(&mch->smram, MCH_HOST_BRIDGE_SMRAM_C_BASE,
+    memory_region_add_subregion(mch->smram, MCH_HOST_BRIDGE_SMRAM_C_BASE,
                                 &mch->low_smram);
     memory_region_init_alias(&mch->high_smram, OBJECT(mch), "smram-high",
                              mch->ram_memory, MCH_HOST_BRIDGE_SMRAM_C_BASE,
                              MCH_HOST_BRIDGE_SMRAM_C_SIZE);
     memory_region_set_enabled(&mch->high_smram, true);
-    memory_region_add_subregion(&mch->smram, 0xfeda0000, &mch->high_smram);
+    memory_region_add_subregion(mch->smram, 0xfeda0000, &mch->high_smram);
 
     memory_region_init_io(&mch->tseg_blackhole, OBJECT(mch),
                           &blackhole_ops, NULL,
@@ -619,7 +621,7 @@ static void mch_realize(PCIDevice *d, Error **errp)
     memory_region_init_alias(&mch->tseg_window, OBJECT(mch), "tseg-window",
                              mch->ram_memory, mch->below_4g_mem_size, 0);
     memory_region_set_enabled(&mch->tseg_window, false);
-    memory_region_add_subregion(&mch->smram, mch->below_4g_mem_size,
+    memory_region_add_subregion(mch->smram, mch->below_4g_mem_size,
                                 &mch->tseg_window);
 
     /*
@@ -639,12 +641,9 @@ static void mch_realize(PCIDevice *d, Error **errp)
                              MCH_HOST_BRIDGE_SMBASE_ADDR,
                              MCH_HOST_BRIDGE_SMBASE_SIZE);
     memory_region_set_enabled(&mch->smbase_window, false);
-    memory_region_add_subregion(&mch->smram, MCH_HOST_BRIDGE_SMBASE_ADDR,
+    memory_region_add_subregion(mch->smram, MCH_HOST_BRIDGE_SMBASE_ADDR,
                                 &mch->smbase_window);
 
-    object_property_add_const_link(qdev_get_machine(), "smram",
-                                   OBJECT(&mch->smram));
-
     init_pam(&mch->pam_regions[0], OBJECT(mch), mch->ram_memory,
              mch->system_memory, mch->pci_address_space,
              PAM_BIOS_BASE, PAM_BIOS_SIZE);
-- 
2.39.1

[PATCH v3 9/9] target/i386/tcg/sysemu/tcg-cpu: Avoid own opinion about smram size

[Bernhard Beschow]

When setting up the CPU's smram memory region alias, the code currently
assumes that the smram size is 4 GiB. While this is true, it repeats a
decision made elsewhere which seems redundant and prone to
inconsistencies. Avoid this by reusing whatever size the smram region
was set to.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 target/i386/tcg/sysemu/tcg-cpu.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/target/i386/tcg/sysemu/tcg-cpu.c b/target/i386/tcg/sysemu/tcg-cpu.c
index c223c0fe9b..8f5ce6093c 100644
--- a/target/i386/tcg/sysemu/tcg-cpu.c
+++ b/target/i386/tcg/sysemu/tcg-cpu.c
@@ -22,7 +22,6 @@
 #include "tcg/helper-tcg.h"
 
 #include "sysemu/sysemu.h"
-#include "qemu/units.h"
 #include "exec/address-spaces.h"
 
 #include "tcg/tcg-cpu.h"
@@ -36,7 +35,7 @@ static void tcg_cpu_machine_done(Notifier *n, void *unused)
     if (smram) {
         cpu->smram = g_new(MemoryRegion, 1);
         memory_region_init_alias(cpu->smram, OBJECT(cpu), "smram",
-                                 smram, 0, 4 * GiB);
+                                 smram, 0, memory_region_size(smram));
         memory_region_set_enabled(cpu->smram, true);
         memory_region_add_subregion_overlap(cpu->cpu_as_root, 0,
                                             cpu->smram, 1);
-- 
2.39.1

Re: [PATCH v3 6/9] hw/i386/pc: Initialize ram_memory variable directly

[BALATON Zoltan]

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

On Sat, 4 Feb 2023, Bernhard Beschow wrote:
> Going through pc_memory_init() seems quite complicated for a simple
> assignment.
>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> include/hw/i386/pc.h | 1 -
> hw/i386/pc.c         | 2 --
> hw/i386/pc_piix.c    | 4 ++--
> hw/i386/pc_q35.c     | 5 ++---
> 4 files changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 66e3d059ef..b60b95921b 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -162,7 +162,6 @@ void xen_load_linux(PCMachineState *pcms);
> void pc_memory_init(PCMachineState *pcms,
>                     MemoryRegion *system_memory,
>                     MemoryRegion *rom_memory,
> -                    MemoryRegion **ram_memory,
>                     uint64_t pci_hole64_size);
> uint64_t pc_pci_hole64_start(void);
> DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus);
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 6e592bd969..8898cc9961 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -936,7 +936,6 @@ static hwaddr pc_max_used_gpa(PCMachineState *pcms, uint64_t pci_hole64_size)
> void pc_memory_init(PCMachineState *pcms,
>                     MemoryRegion *system_memory,
>                     MemoryRegion *rom_memory,
> -                    MemoryRegion **ram_memory,
>                     uint64_t pci_hole64_size)
> {
>     int linux_boot, i;
> @@ -994,7 +993,6 @@ void pc_memory_init(PCMachineState *pcms,
>      * Split single memory region and use aliases to address portions of it,
>      * done for backwards compatibility with older qemus.
>      */
> -    *ram_memory = machine->ram;
>     ram_below_4g = g_malloc(sizeof(*ram_below_4g));
>     memory_region_init_alias(ram_below_4g, NULL, "ram-below-4g", machine->ram,
>                              0, x86ms->below_4g_mem_size);
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 5bde4533cc..00ba725656 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -143,6 +143,7 @@ static void pc_init1(MachineState *machine,
>     if (xen_enabled()) {
>         xen_hvm_init_pc(pcms, &ram_memory);
>     } else {
> +        ram_memory = machine->ram;

Maybe you could just replace the few places it's used with machine->ram 
directly and get rid of the local variable. There seems to be no advantage 
storing it in a local just to use it once (in q35 below) or twice in 
pc-piix. The local name is not even that much shorter so I don't see a 
reason to have it in the first place,

Regards,
BALATON Zoltan

>         if (!pcms->max_ram_below_4g) {
>             pcms->max_ram_below_4g = 0xe0000000; /* default: 3.5G */
>         }
> @@ -205,8 +206,7 @@ static void pc_init1(MachineState *machine,
>
>     /* allocate ram and load rom/bios */
>     if (!xen_enabled()) {
> -        pc_memory_init(pcms, system_memory,
> -                       rom_memory, &ram_memory, hole64_size);
> +        pc_memory_init(pcms, system_memory, rom_memory, hole64_size);
>     } else {
>         pc_system_flash_cleanup_unused(pcms);
>         if (machine->kernel_filename != NULL) {
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 8253b49296..88f0981f50 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -129,7 +129,7 @@ static void pc_q35_init(MachineState *machine)
>     MemoryRegion *system_io = get_system_io();
>     MemoryRegion *pci_memory;
>     MemoryRegion *rom_memory;
> -    MemoryRegion *ram_memory;
> +    MemoryRegion *ram_memory = machine->ram;
>     GSIState *gsi_state;
>     ISABus *isa_bus;
>     int i;
> @@ -216,8 +216,7 @@ static void pc_q35_init(MachineState *machine)
>     }
>
>     /* allocate ram and load rom/bios */
> -    pc_memory_init(pcms, system_memory, rom_memory, &ram_memory,
> -                   pci_hole64_size);
> +    pc_memory_init(pcms, system_memory, rom_memory, pci_hole64_size);
>
>     object_property_add_child(OBJECT(machine), "q35", OBJECT(q35_host));
>     object_property_set_link(OBJECT(q35_host), MCH_HOST_PROP_RAM_MEM,
>

Re: [PATCH v3 1/9] hw/pci-host/i440fx: Inline sysbus_add_io()

[Philippe Mathieu-Daudé]

On 4/2/23 16:10, Bernhard Beschow wrote:
> sysbus_add_io() just wraps memory_region_add_subregion() while also
> obscuring where the memory is attached. So use
> memory_region_add_subregion() directly and attach it to the existing
> memory region s->bus->address_space_io which is set as an alias to
> get_system_io() by the pc machine.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> ---
>   hw/pci-host/i440fx.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
> index 262f82c303..9c6882d3fc 100644
> --- a/hw/pci-host/i440fx.c
> +++ b/hw/pci-host/i440fx.c
> @@ -27,6 +27,7 @@
>   #include "qemu/range.h"
>   #include "hw/i386/pc.h"
>   #include "hw/pci/pci.h"
> +#include "hw/pci/pci_bus.h"
>   #include "hw/pci/pci_host.h"
>   #include "hw/pci-host/i440fx.h"
>   #include "hw/qdev-properties.h"
> @@ -217,10 +218,10 @@ static void i440fx_pcihost_realize(DeviceState *dev, Error **errp)
>       PCIHostState *s = PCI_HOST_BRIDGE(dev);
>       SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>   
> -    sysbus_add_io(sbd, 0xcf8, &s->conf_mem);
> +    memory_region_add_subregion(s->bus->address_space_io, 0xcf8, &s->conf_mem);

address_space_io is internal to PCIBus. Better would be adding a helper
like pci_bus_io_region() IMO. Anyhow, removing sysbus_add_io() uses is
good, so:

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

Re: [PATCH v3 2/9] hw/pci-host/q35: Inline sysbus_add_io()

[Philippe Mathieu-Daudé]

On 4/2/23 16:10, Bernhard Beschow wrote:
> sysbus_add_io() just wraps memory_region_add_subregion() while also
> obscuring where the memory is attached. So use
> memory_region_add_subregion() directly and attach it to the existing
> memory region s->mch.address_space_io which is set as an alias to
> get_system_io() by the q35 machine.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> ---
>   hw/pci-host/q35.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index 26390863d6..fa05844319 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -50,10 +50,12 @@ static void q35_host_realize(DeviceState *dev, Error **errp)
>       Q35PCIHost *s = Q35_HOST_DEVICE(dev);
>       SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>   
> -    sysbus_add_io(sbd, MCH_HOST_BRIDGE_CONFIG_ADDR, &pci->conf_mem);
> +    memory_region_add_subregion(s->mch.address_space_io,
> +                                MCH_HOST_BRIDGE_CONFIG_ADDR, &pci->conf_mem);
>       sysbus_init_ioports(sbd, MCH_HOST_BRIDGE_CONFIG_ADDR, 4);

This makes me wonder why MCH_PCI_DEVICE doesn't use the bus I/O space
via pci_address_space_io(). IOW, why the MR like is in MCH_PCI_DEVICE
and not Q35_HOST_DEVICE?

Re: [PATCH v3 3/9] hw/i386/pc_q35: Reuse machine parameter

[Philippe Mathieu-Daudé]

On 4/2/23 16:10, Bernhard Beschow wrote:
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> ---
>   hw/i386/pc_q35.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

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

Re: [PATCH v3 5/9] hw/i386/pc_{q35, piix}: Minimize usage of get_system_memory()

[Philippe Mathieu-Daudé]

On 4/2/23 16:10, Bernhard Beschow wrote:
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> ---
>   hw/i386/pc_piix.c | 2 +-
>   hw/i386/pc_q35.c  | 7 ++++---
>   2 files changed, 5 insertions(+), 4 deletions(-)

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

Re: [PATCH v3 7/9] hw/pci-host/pam: Make init_pam() usage more readable

[Philippe Mathieu-Daudé]

On 4/2/23 16:10, Bernhard Beschow wrote:
> Unlike pam_update() which takes the subject -- PAMMemoryRegion -- as
> first argument, init_pam() takes it as fifth (!) argument. This makes it
> quite hard to figure out what an init_pam() invocation actually
> initializes. By moving the subject to the front this should become
> clearer.
> 
> While at it, lower the DeviceState parameter to Object, also
> communicating more clearly that this parameter is just the owner rather
> than some (heavy?) dependency.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   include/hw/pci-host/pam.h |  5 +++--
>   hw/pci-host/i440fx.c      | 10 +++++-----
>   hw/pci-host/pam.c         | 12 ++++++------
>   hw/pci-host/q35.c         |  8 ++++----
>   4 files changed, 18 insertions(+), 17 deletions(-)

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

Re: [PATCH v3 8/9] hw/i386/x86: Make TYPE_X86_MACHINE the owner of smram

[Philippe Mathieu-Daudé]

On 4/2/23 16:10, Bernhard Beschow wrote:
> Treat the smram MemoryRegion analoguous to other memory regions such as
> ram, pci, io, ... , making the used memory regions more explicit when
> instantiating q35 or i440fx.
> 
> Note that the q35 device uses these memory regions only during the
> realize phase which suggests that it shouldn't be the owner of smram.

Few years ago I tried something similar and it wasn't accepted because
the MR owner name is used in the migration stream, so this was breaking
migrating from older machines.

Adding David/Juan for double-checking that.

> i440fx activates/deactivates the whole smram memory region depending on
> the SMRAM_G_SMRAME bit which seems wrong since it should only handle the
> 128kb region. If this got changed, i440fx would also only use smram
> during its realize phase.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   include/hw/i386/x86.h        |  2 ++
>   include/hw/pci-host/i440fx.h |  7 ++++---
>   include/hw/pci-host/q35.h    |  4 +++-
>   hw/i386/pc_piix.c            |  2 +-
>   hw/i386/pc_q35.c             |  2 ++
>   hw/i386/x86.c                |  4 ++++
>   hw/pci-host/i440fx.c         | 13 +++++--------
>   hw/pci-host/q35.c            | 17 ++++++++---------
>   8 files changed, 29 insertions(+), 22 deletions(-)
> 
> diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
> index 62fa5774f8..ba6912b721 100644
> --- a/include/hw/i386/x86.h
> +++ b/include/hw/i386/x86.h
> @@ -59,6 +59,8 @@ struct X86MachineState {
>       /* Start address of the initial RAM above 4G */
>       uint64_t above_4g_mem_start;
>   
> +    MemoryRegion smram;
> +
>       /* CPU and apic information: */
>       bool apic_xrupt_override;
>       unsigned pci_irq_mask;
> diff --git a/include/hw/pci-host/i440fx.h b/include/hw/pci-host/i440fx.h
> index bf57216c78..e9efdb3c5f 100644
> --- a/include/hw/pci-host/i440fx.h
> +++ b/include/hw/pci-host/i440fx.h
> @@ -28,9 +28,10 @@ struct PCII440FXState {
>       MemoryRegion *system_memory;
>       MemoryRegion *pci_address_space;
>       MemoryRegion *ram_memory;
> +    MemoryRegion *smram;
>       PAMMemoryRegion pam_regions[PAM_REGIONS_COUNT];
>       MemoryRegion smram_region;
> -    MemoryRegion smram, low_smram;
> +    MemoryRegion low_smram;
>   };
>   
>   #define TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE "igd-passthrough-i440FX"
> @@ -43,7 +44,7 @@ PCIBus *i440fx_init(const char *pci_type,
>                       ram_addr_t below_4g_mem_size,
>                       ram_addr_t above_4g_mem_size,
>                       MemoryRegion *pci_memory,
> -                    MemoryRegion *ram_memory);
> -
> +                    MemoryRegion *ram_memory,
> +                    MemoryRegion *smram);
>   
>   #endif
> diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
> index e89329c51e..fcbe57b42d 100644
> --- a/include/hw/pci-host/q35.h
> +++ b/include/hw/pci-host/q35.h
> @@ -44,9 +44,10 @@ struct MCHPCIState {
>       MemoryRegion *pci_address_space;
>       MemoryRegion *system_memory;
>       MemoryRegion *address_space_io;
> +    MemoryRegion *smram;
>       PAMMemoryRegion pam_regions[PAM_REGIONS_COUNT];
>       MemoryRegion smram_region, open_high_smram;
> -    MemoryRegion smram, low_smram, high_smram;
> +    MemoryRegion low_smram, high_smram;
>       MemoryRegion tseg_blackhole, tseg_window;
>       MemoryRegion smbase_blackhole, smbase_window;
>       bool has_smram_at_smbase;
> @@ -75,6 +76,7 @@ struct Q35PCIHost {
>    */
>   
>   #define MCH_HOST_PROP_RAM_MEM "ram-mem"
> +#define MCH_HOST_PROP_SMRAM_MEM "smram-mem"
>   #define MCH_HOST_PROP_PCI_MEM "pci-mem"
>   #define MCH_HOST_PROP_SYSTEM_MEM "system-mem"
>   #define MCH_HOST_PROP_IO_MEM "io-mem"
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 00ba725656..41aaaa5465 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -228,7 +228,7 @@ static void pc_init1(MachineState *machine,
>                                 system_memory, system_io, machine->ram_size,
>                                 x86ms->below_4g_mem_size,
>                                 x86ms->above_4g_mem_size,
> -                              pci_memory, ram_memory);
> +                              pci_memory, ram_memory, &x86ms->smram);
>           pci_bus_map_irqs(pci_bus,
>                            xen_enabled() ? xen_pci_slot_get_pirq
>                                          : pc_pci_slot_get_pirq);
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 88f0981f50..480226de2c 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -221,6 +221,8 @@ static void pc_q35_init(MachineState *machine)
>       object_property_add_child(OBJECT(machine), "q35", OBJECT(q35_host));
>       object_property_set_link(OBJECT(q35_host), MCH_HOST_PROP_RAM_MEM,
>                                OBJECT(ram_memory), NULL);
> +    object_property_set_link(OBJECT(q35_host), MCH_HOST_PROP_SMRAM_MEM,
> +                             OBJECT(&x86ms->smram), NULL);
>       object_property_set_link(OBJECT(q35_host), MCH_HOST_PROP_PCI_MEM,
>                                OBJECT(pci_memory), NULL);
>       object_property_set_link(OBJECT(q35_host), MCH_HOST_PROP_SYSTEM_MEM,
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index eaff4227bd..d7e219b1eb 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -1436,6 +1436,10 @@ static void x86_machine_initfn(Object *obj)
>       x86ms->oem_table_id = g_strndup(ACPI_BUILD_APPNAME8, 8);
>       x86ms->bus_lock_ratelimit = 0;
>       x86ms->above_4g_mem_start = 4 * GiB;
> +
> +    memory_region_init(&x86ms->smram, obj, "smram", 4 * GiB);
> +    memory_region_set_enabled(&x86ms->smram, true);
> +    object_property_add_const_link(obj, "smram", OBJECT(&x86ms->smram));
>   }
>   
>   static void x86_machine_class_init(ObjectClass *oc, void *data)
> diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
> index 61e7b97ff4..8f4a4f59a6 100644
> --- a/hw/pci-host/i440fx.c
> +++ b/hw/pci-host/i440fx.c
> @@ -23,7 +23,6 @@
>    */
>   
>   #include "qemu/osdep.h"
> -#include "qemu/units.h"
>   #include "qemu/range.h"
>   #include "hw/i386/pc.h"
>   #include "hw/pci/pci.h"
> @@ -77,7 +76,7 @@ static void i440fx_update_memory_mappings(PCII440FXState *d)
>       }
>       memory_region_set_enabled(&d->smram_region,
>                                 !(pd->config[I440FX_SMRAM] & SMRAM_D_OPEN));
> -    memory_region_set_enabled(&d->smram,
> +    memory_region_set_enabled(d->smram,
>                                 pd->config[I440FX_SMRAM] & SMRAM_G_SMRAME);
>       memory_region_transaction_commit();
>   }
> @@ -246,7 +245,8 @@ PCIBus *i440fx_init(const char *pci_type,
>                       ram_addr_t below_4g_mem_size,
>                       ram_addr_t above_4g_mem_size,
>                       MemoryRegion *pci_address_space,
> -                    MemoryRegion *ram_memory)
> +                    MemoryRegion *ram_memory,
> +                    MemoryRegion *smram)
>   {
>       PCIBus *b;
>       PCIDevice *d;
> @@ -267,6 +267,7 @@ PCIBus *i440fx_init(const char *pci_type,
>       f->system_memory = address_space_mem;
>       f->pci_address_space = pci_address_space;
>       f->ram_memory = ram_memory;
> +    f->smram = smram;
>   
>       i440fx = I440FX_PCI_HOST_BRIDGE(dev);
>       range_set_bounds(&i440fx->pci_hole, below_4g_mem_size,
> @@ -283,14 +284,10 @@ PCIBus *i440fx_init(const char *pci_type,
>       memory_region_set_enabled(&f->smram_region, true);
>   
>       /* smram, as seen by SMM CPUs */
> -    memory_region_init(&f->smram, OBJECT(d), "smram", 4 * GiB);
> -    memory_region_set_enabled(&f->smram, true);
>       memory_region_init_alias(&f->low_smram, OBJECT(d), "smram-low",
>                                f->ram_memory, 0xa0000, 0x20000);
>       memory_region_set_enabled(&f->low_smram, true);
> -    memory_region_add_subregion(&f->smram, 0xa0000, &f->low_smram);
> -    object_property_add_const_link(qdev_get_machine(), "smram",
> -                                   OBJECT(&f->smram));
> +    memory_region_add_subregion(f->smram, 0xa0000, &f->low_smram);
>   
>       init_pam(&f->pam_regions[0], OBJECT(d), f->ram_memory, f->system_memory,
>                f->pci_address_space, PAM_BIOS_BASE, PAM_BIOS_SIZE);
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index fd18920e7f..83f2a98c71 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -246,6 +246,10 @@ static void q35_host_initfn(Object *obj)
>                                (Object **) &s->mch.ram_memory,
>                                qdev_prop_allow_set_link_before_realize, 0);
>   
> +    object_property_add_link(obj, MCH_HOST_PROP_SMRAM_MEM, TYPE_MEMORY_REGION,
> +                             (Object **) &s->mch.smram,
> +                             qdev_prop_allow_set_link_before_realize, 0);
> +
>       object_property_add_link(obj, MCH_HOST_PROP_PCI_MEM, TYPE_MEMORY_REGION,
>                                (Object **) &s->mch.pci_address_space,
>                                qdev_prop_allow_set_link_before_realize, 0);
> @@ -594,19 +598,17 @@ static void mch_realize(PCIDevice *d, Error **errp)
>       memory_region_set_enabled(&mch->open_high_smram, false);
>   
>       /* smram, as seen by SMM CPUs */
> -    memory_region_init(&mch->smram, OBJECT(mch), "smram", 4 * GiB);
> -    memory_region_set_enabled(&mch->smram, true);
>       memory_region_init_alias(&mch->low_smram, OBJECT(mch), "smram-low",
>                                mch->ram_memory, MCH_HOST_BRIDGE_SMRAM_C_BASE,
>                                MCH_HOST_BRIDGE_SMRAM_C_SIZE);
>       memory_region_set_enabled(&mch->low_smram, true);
> -    memory_region_add_subregion(&mch->smram, MCH_HOST_BRIDGE_SMRAM_C_BASE,
> +    memory_region_add_subregion(mch->smram, MCH_HOST_BRIDGE_SMRAM_C_BASE,
>                                   &mch->low_smram);
>       memory_region_init_alias(&mch->high_smram, OBJECT(mch), "smram-high",
>                                mch->ram_memory, MCH_HOST_BRIDGE_SMRAM_C_BASE,
>                                MCH_HOST_BRIDGE_SMRAM_C_SIZE);
>       memory_region_set_enabled(&mch->high_smram, true);
> -    memory_region_add_subregion(&mch->smram, 0xfeda0000, &mch->high_smram);
> +    memory_region_add_subregion(mch->smram, 0xfeda0000, &mch->high_smram);
>   
>       memory_region_init_io(&mch->tseg_blackhole, OBJECT(mch),
>                             &blackhole_ops, NULL,
> @@ -619,7 +621,7 @@ static void mch_realize(PCIDevice *d, Error **errp)
>       memory_region_init_alias(&mch->tseg_window, OBJECT(mch), "tseg-window",
>                                mch->ram_memory, mch->below_4g_mem_size, 0);
>       memory_region_set_enabled(&mch->tseg_window, false);
> -    memory_region_add_subregion(&mch->smram, mch->below_4g_mem_size,
> +    memory_region_add_subregion(mch->smram, mch->below_4g_mem_size,
>                                   &mch->tseg_window);
>   
>       /*
> @@ -639,12 +641,9 @@ static void mch_realize(PCIDevice *d, Error **errp)
>                                MCH_HOST_BRIDGE_SMBASE_ADDR,
>                                MCH_HOST_BRIDGE_SMBASE_SIZE);
>       memory_region_set_enabled(&mch->smbase_window, false);
> -    memory_region_add_subregion(&mch->smram, MCH_HOST_BRIDGE_SMBASE_ADDR,
> +    memory_region_add_subregion(mch->smram, MCH_HOST_BRIDGE_SMBASE_ADDR,
>                                   &mch->smbase_window);
>   
> -    object_property_add_const_link(qdev_get_machine(), "smram",
> -                                   OBJECT(&mch->smram));
> -
>       init_pam(&mch->pam_regions[0], OBJECT(mch), mch->ram_memory,
>                mch->system_memory, mch->pci_address_space,
>                PAM_BIOS_BASE, PAM_BIOS_SIZE);

Re: [PATCH v3 9/9] target/i386/tcg/sysemu/tcg-cpu: Avoid own opinion about smram size

[Philippe Mathieu-Daudé]

On 4/2/23 16:10, Bernhard Beschow wrote:
> When setting up the CPU's smram memory region alias, the code currently
> assumes that the smram size is 4 GiB. While this is true, it repeats a
> decision made elsewhere which seems redundant and prone to
> inconsistencies. Avoid this by reusing whatever size the smram region
> was set to.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   target/i386/tcg/sysemu/tcg-cpu.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/target/i386/tcg/sysemu/tcg-cpu.c b/target/i386/tcg/sysemu/tcg-cpu.c
> index c223c0fe9b..8f5ce6093c 100644
> --- a/target/i386/tcg/sysemu/tcg-cpu.c
> +++ b/target/i386/tcg/sysemu/tcg-cpu.c
> @@ -22,7 +22,6 @@
>   #include "tcg/helper-tcg.h"
>   
>   #include "sysemu/sysemu.h"
> -#include "qemu/units.h"
>   #include "exec/address-spaces.h"
>   
>   #include "tcg/tcg-cpu.h"
> @@ -36,7 +35,7 @@ static void tcg_cpu_machine_done(Notifier *n, void *unused)
>       if (smram) {
>           cpu->smram = g_new(MemoryRegion, 1);
>           memory_region_init_alias(cpu->smram, OBJECT(cpu), "smram",
> -                                 smram, 0, 4 * GiB);
> +                                 smram, 0, memory_region_size(smram));

Or define SMRAM_REGION_SIZE and use it?

(subject header can be shortened to "target/i386:").

Re: [PATCH v3 6/9] hw/i386/pc: Initialize ram_memory variable directly

[Bernhard Beschow]

Am 4. Februar 2023 15:26:13 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Sat, 4 Feb 2023, Bernhard Beschow wrote:
>> Going through pc_memory_init() seems quite complicated for a simple
>> assignment.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> include/hw/i386/pc.h | 1 -
>> hw/i386/pc.c         | 2 --
>> hw/i386/pc_piix.c    | 4 ++--
>> hw/i386/pc_q35.c     | 5 ++---
>> 4 files changed, 4 insertions(+), 8 deletions(-)
>> 
>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> index 66e3d059ef..b60b95921b 100644
>> --- a/include/hw/i386/pc.h
>> +++ b/include/hw/i386/pc.h
>> @@ -162,7 +162,6 @@ void xen_load_linux(PCMachineState *pcms);
>> void pc_memory_init(PCMachineState *pcms,
>>                     MemoryRegion *system_memory,
>>                     MemoryRegion *rom_memory,
>> -                    MemoryRegion **ram_memory,
>>                     uint64_t pci_hole64_size);
>> uint64_t pc_pci_hole64_start(void);
>> DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus);
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 6e592bd969..8898cc9961 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -936,7 +936,6 @@ static hwaddr pc_max_used_gpa(PCMachineState *pcms, uint64_t pci_hole64_size)
>> void pc_memory_init(PCMachineState *pcms,
>>                     MemoryRegion *system_memory,
>>                     MemoryRegion *rom_memory,
>> -                    MemoryRegion **ram_memory,
>>                     uint64_t pci_hole64_size)
>> {
>>     int linux_boot, i;
>> @@ -994,7 +993,6 @@ void pc_memory_init(PCMachineState *pcms,
>>      * Split single memory region and use aliases to address portions of it,
>>      * done for backwards compatibility with older qemus.
>>      */
>> -    *ram_memory = machine->ram;
>>     ram_below_4g = g_malloc(sizeof(*ram_below_4g));
>>     memory_region_init_alias(ram_below_4g, NULL, "ram-below-4g", machine->ram,
>>                              0, x86ms->below_4g_mem_size);
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index 5bde4533cc..00ba725656 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -143,6 +143,7 @@ static void pc_init1(MachineState *machine,
>>     if (xen_enabled()) {
>>         xen_hvm_init_pc(pcms, &ram_memory);
>>     } else {
>> +        ram_memory = machine->ram;
>
>Maybe you could just replace the few places it's used with machine->ram directly and get rid of the local variable. There seems to be no advantage storing it in a local just to use it once (in q35 below) or twice in pc-piix. The local name is not even that much shorter so I don't see a reason to have it in the first place,

Possible with q35 but not with piix which needs to get the RAM from Xen if running in this mode. I'll adjust q35 then.

Best regards,
Bernhard
>
>Regards,
>BALATON Zoltan
>
>>         if (!pcms->max_ram_below_4g) {
>>             pcms->max_ram_below_4g = 0xe0000000; /* default: 3.5G */
>>         }
>> @@ -205,8 +206,7 @@ static void pc_init1(MachineState *machine,
>> 
>>     /* allocate ram and load rom/bios */
>>     if (!xen_enabled()) {
>> -        pc_memory_init(pcms, system_memory,
>> -                       rom_memory, &ram_memory, hole64_size);
>> +        pc_memory_init(pcms, system_memory, rom_memory, hole64_size);
>>     } else {
>>         pc_system_flash_cleanup_unused(pcms);
>>         if (machine->kernel_filename != NULL) {
>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>> index 8253b49296..88f0981f50 100644
>> --- a/hw/i386/pc_q35.c
>> +++ b/hw/i386/pc_q35.c
>> @@ -129,7 +129,7 @@ static void pc_q35_init(MachineState *machine)
>>     MemoryRegion *system_io = get_system_io();
>>     MemoryRegion *pci_memory;
>>     MemoryRegion *rom_memory;
>> -    MemoryRegion *ram_memory;
>> +    MemoryRegion *ram_memory = machine->ram;
>>     GSIState *gsi_state;
>>     ISABus *isa_bus;
>>     int i;
>> @@ -216,8 +216,7 @@ static void pc_q35_init(MachineState *machine)
>>     }
>> 
>>     /* allocate ram and load rom/bios */
>> -    pc_memory_init(pcms, system_memory, rom_memory, &ram_memory,
>> -                   pci_hole64_size);
>> +    pc_memory_init(pcms, system_memory, rom_memory, pci_hole64_size);
>> 
>>     object_property_add_child(OBJECT(machine), "q35", OBJECT(q35_host));
>>     object_property_set_link(OBJECT(q35_host), MCH_HOST_PROP_RAM_MEM,
>>

Re: [PATCH v3 2/9] hw/pci-host/q35: Inline sysbus_add_io()

[Bernhard Beschow]

Am 5. Februar 2023 11:12:26 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>On 4/2/23 16:10, Bernhard Beschow wrote:
>> sysbus_add_io() just wraps memory_region_add_subregion() while also
>> obscuring where the memory is attached. So use
>> memory_region_add_subregion() directly and attach it to the existing
>> memory region s->mch.address_space_io which is set as an alias to
>> get_system_io() by the q35 machine.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   hw/pci-host/q35.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>> 
>> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
>> index 26390863d6..fa05844319 100644
>> --- a/hw/pci-host/q35.c
>> +++ b/hw/pci-host/q35.c
>> @@ -50,10 +50,12 @@ static void q35_host_realize(DeviceState *dev, Error **errp)
>>       Q35PCIHost *s = Q35_HOST_DEVICE(dev);
>>       SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>>   -    sysbus_add_io(sbd, MCH_HOST_BRIDGE_CONFIG_ADDR, &pci->conf_mem);
>> +    memory_region_add_subregion(s->mch.address_space_io,
>> +                                MCH_HOST_BRIDGE_CONFIG_ADDR, &pci->conf_mem);
>>       sysbus_init_ioports(sbd, MCH_HOST_BRIDGE_CONFIG_ADDR, 4);
>
>This makes me wonder why MCH_PCI_DEVICE doesn't use the bus I/O space
>via pci_address_space_io(). IOW, why the MR like is in MCH_PCI_DEVICE
>and not Q35_HOST_DEVICE?

I think I have follow-up patches in the pipeline moving the MemoryRegion pointers to the host device. Interestingly, these pointers are only used during the realize phase and just needlessly occupy memory during the rest of the device's lifetime...

Re: [PATCH v3 8/9] hw/i386/x86: Make TYPE_X86_MACHINE the owner of smram

[Juan Quintela]

Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> On 4/2/23 16:10, Bernhard Beschow wrote:
>> Treat the smram MemoryRegion analoguous to other memory regions such as
>> ram, pci, io, ... , making the used memory regions more explicit when
>> instantiating q35 or i440fx.
>> Note that the q35 device uses these memory regions only during the
>> realize phase which suggests that it shouldn't be the owner of smram.
>
> Few years ago I tried something similar and it wasn't accepted because
> the MR owner name is used in the migration stream, so this was breaking
> migrating from older machines.

I don't remember the details O:-)

Migration code, really depends on RAMBlocks names, not memory region
names.  But as far as I remember, that don't matter too much because the
memory region names ends tangled quite a bit with the RAMBlock name, right?

> Adding David/Juan for double-checking that.

    trace_vmstate_save(se->idstr, se->vmsd ? se->vmsd->name : "(old)");

You can try to enable this trace and see that every section has the same
name with and without your change (i.e. that memory region name is not
seen by the migration stream).

But that is the only help that I can came with.

The code that you are changing (smram) is something that I don't know
about to give you more help.

Looking at the patch, it looks that the name was before and now the
"sram", so perhaps it could help.  But I don't know.

In the i440fx you say that you only use it until realize, so you should
be safe.

For q35, it is not clear to me.

If the trace don't show new names, I will just try:
- migrate a i440fx machine from binary without your patch to one with
  your patch
- the same for q35.

And depending on the result, we can go from there.

Later, Juan.

Re: [PATCH v3 8/9] hw/i386/x86: Make TYPE_X86_MACHINE the owner of smram

[Bernhard Beschow]

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

On Mon, Feb 6, 2023 at 11:06 AM Juan Quintela <quintela@redhat.com> wrote:

> Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> > On 4/2/23 16:10, Bernhard Beschow wrote:
> >> Treat the smram MemoryRegion analoguous to other memory regions such as
> >> ram, pci, io, ... , making the used memory regions more explicit when
> >> instantiating q35 or i440fx.
> >> Note that the q35 device uses these memory regions only during the
> >> realize phase which suggests that it shouldn't be the owner of smram.
> >
> > Few years ago I tried something similar and it wasn't accepted because
> > the MR owner name is used in the migration stream, so this was breaking
> > migrating from older machines.
>
> I don't remember the details O:-)
>
> Migration code, really depends on RAMBlocks names, not memory region
> names.  But as far as I remember, that don't matter too much because the
> memory region names ends tangled quite a bit with the RAMBlock name, right?
>
> > Adding David/Juan for double-checking that.
>
>     trace_vmstate_save(se->idstr, se->vmsd ? se->vmsd->name : "(old)");
>
> You can try to enable this trace and see that every section has the same
> name with and without your change (i.e. that memory region name is not
> seen by the migration stream).
>
> But that is the only help that I can came with.
>
> The code that you are changing (smram) is something that I don't know
> about to give you more help.
>
> Looking at the patch, it looks that the name was before and now the
> "sram", so perhaps it could help.  But I don't know.
>
> In the i440fx you say that you only use it until realize, so you should
> be safe.
>
> For q35, it is not clear to me.
>
> If the trace don't show new names, I will just try:
> - migrate a i440fx machine from binary without your patch to one with
>   your patch
> - the same for q35.
>
> And depending on the result, we can go from there.
>

Thanks for the pointers, Juan!

I took some inspiration and created four migration files,
{pc,q35}-{before,after}.mig by running `qemu-system-x86_64 -M {pc,q35} -S`
with qemu built from master and from my branch. Then I basically ran
 `./scripts/analyze-migration.py -d desc -f *.mig > *.json` on the four
files and compared the diffs. Both diffs were empty. AFAIU this proves that
there is no binary change, right?

Best regards,
Bernhard
>
>
> Later, Juan.
>
>

[-- Attachment #2: Type: text/html, Size: 3179 bytes --]

Re: [PATCH v3 8/9] hw/i386/x86: Make TYPE_X86_MACHINE the owner of smram

[Juan Quintela]

Bernhard Beschow <shentey@gmail.com> wrote:
v> On Mon, Feb 6, 2023 at 11:06 AM Juan Quintela <quintela@redhat.com> wrote:
>
>> Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>> > On 4/2/23 16:10, Bernhard Beschow wrote:
>> >> Treat the smram MemoryRegion analoguous to other memory regions such as
>> >> ram, pci, io, ... , making the used memory regions more explicit when
>> >> instantiating q35 or i440fx.
>> >> Note that the q35 device uses these memory regions only during the
>> >> realize phase which suggests that it shouldn't be the owner of smram.
>> >
>> > Few years ago I tried something similar and it wasn't accepted because
>> > the MR owner name is used in the migration stream, so this was breaking
>> > migrating from older machines.
>>
>> I don't remember the details O:-)
>>
>> Migration code, really depends on RAMBlocks names, not memory region
>> names.  But as far as I remember, that don't matter too much because the
>> memory region names ends tangled quite a bit with the RAMBlock name, right?
>>
>> > Adding David/Juan for double-checking that.
>>
>>     trace_vmstate_save(se->idstr, se->vmsd ? se->vmsd->name : "(old)");
>>
>> You can try to enable this trace and see that every section has the same
>> name with and without your change (i.e. that memory region name is not
>> seen by the migration stream).
>>
>> But that is the only help that I can came with.
>>
>> The code that you are changing (smram) is something that I don't know
>> about to give you more help.
>>
>> Looking at the patch, it looks that the name was before and now the
>> "sram", so perhaps it could help.  But I don't know.
>>
>> In the i440fx you say that you only use it until realize, so you should
>> be safe.
>>
>> For q35, it is not clear to me.
>>
>> If the trace don't show new names, I will just try:
>> - migrate a i440fx machine from binary without your patch to one with
>>   your patch
>> - the same for q35.
>>
>> And depending on the result, we can go from there.
>>
>
> Thanks for the pointers, Juan!
>
> I took some inspiration and created four migration files,
> {pc,q35}-{before,after}.mig by running `qemu-system-x86_64 -M {pc,q35} -S`
> with qemu built from master and from my branch. Then I basically ran
>  `./scripts/analyze-migration.py -d desc -f *.mig > *.json` on the four
> files and compared the diffs. Both diffs were empty. AFAIU this proves that
> there is no binary change, right?

We have two options here:
- you are right (my opinion)
- you got a bug in analyze-migration.py script and you have a new job.

But I think you can send this patch.

Later, Juan.

> Best regards,
> Bernhard
>>
>>
>> Later, Juan.
>>
>>

Re: [PATCH v3 8/9] hw/i386/x86: Make TYPE_X86_MACHINE the owner of smram

[Bernhard Beschow]

Am 7. Februar 2023 18:34:40 UTC schrieb Juan Quintela <quintela@redhat.com>:
>Bernhard Beschow <shentey@gmail.com> wrote:
>v> On Mon, Feb 6, 2023 at 11:06 AM Juan Quintela <quintela@redhat.com> wrote:
>>
>>> Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>> > On 4/2/23 16:10, Bernhard Beschow wrote:
>>> >> Treat the smram MemoryRegion analoguous to other memory regions such as
>>> >> ram, pci, io, ... , making the used memory regions more explicit when
>>> >> instantiating q35 or i440fx.
>>> >> Note that the q35 device uses these memory regions only during the
>>> >> realize phase which suggests that it shouldn't be the owner of smram.
>>> >
>>> > Few years ago I tried something similar and it wasn't accepted because
>>> > the MR owner name is used in the migration stream, so this was breaking
>>> > migrating from older machines.
>>>
>>> I don't remember the details O:-)
>>>
>>> Migration code, really depends on RAMBlocks names, not memory region
>>> names.  But as far as I remember, that don't matter too much because the
>>> memory region names ends tangled quite a bit with the RAMBlock name, right?
>>>
>>> > Adding David/Juan for double-checking that.
>>>
>>>     trace_vmstate_save(se->idstr, se->vmsd ? se->vmsd->name : "(old)");
>>>
>>> You can try to enable this trace and see that every section has the same
>>> name with and without your change (i.e. that memory region name is not
>>> seen by the migration stream).
>>>
>>> But that is the only help that I can came with.
>>>
>>> The code that you are changing (smram) is something that I don't know
>>> about to give you more help.
>>>
>>> Looking at the patch, it looks that the name was before and now the
>>> "sram", so perhaps it could help.  But I don't know.
>>>
>>> In the i440fx you say that you only use it until realize, so you should
>>> be safe.
>>>
>>> For q35, it is not clear to me.
>>>
>>> If the trace don't show new names, I will just try:
>>> - migrate a i440fx machine from binary without your patch to one with
>>>   your patch
>>> - the same for q35.
>>>
>>> And depending on the result, we can go from there.
>>>
>>
>> Thanks for the pointers, Juan!
>>
>> I took some inspiration and created four migration files,
>> {pc,q35}-{before,after}.mig by running `qemu-system-x86_64 -M {pc,q35} -S`
>> with qemu built from master and from my branch. Then I basically ran
>>  `./scripts/analyze-migration.py -d desc -f *.mig > *.json` on the four
>> files and compared the diffs. Both diffs were empty. AFAIU this proves that
>> there is no binary change, right?
>
>We have two options here:
>- you are right (my opinion)
>- you got a bug in analyze-migration.py script and you have a new job.

I take option one ;)

>But I think you can send this patch.

The patches still need reviews.

Best regards,
Bernhard
>
>Later, Juan.
>
>> Best regards,
>> Bernhard
>>>
>>>
>>> Later, Juan.
>>>
>>>
>

Re: [PATCH v3 9/9] target/i386/tcg/sysemu/tcg-cpu: Avoid own opinion about smram size

[Bernhard Beschow]

Am 5. Februar 2023 11:26:10 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>On 4/2/23 16:10, Bernhard Beschow wrote:
>> When setting up the CPU's smram memory region alias, the code currently
>> assumes that the smram size is 4 GiB. While this is true, it repeats a
>> decision made elsewhere which seems redundant and prone to
>> inconsistencies. Avoid this by reusing whatever size the smram region
>> was set to.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   target/i386/tcg/sysemu/tcg-cpu.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>> 
>> diff --git a/target/i386/tcg/sysemu/tcg-cpu.c b/target/i386/tcg/sysemu/tcg-cpu.c
>> index c223c0fe9b..8f5ce6093c 100644
>> --- a/target/i386/tcg/sysemu/tcg-cpu.c
>> +++ b/target/i386/tcg/sysemu/tcg-cpu.c
>> @@ -22,7 +22,6 @@
>>   #include "tcg/helper-tcg.h"
>>     #include "sysemu/sysemu.h"
>> -#include "qemu/units.h"
>>   #include "exec/address-spaces.h"
>>     #include "tcg/tcg-cpu.h"
>> @@ -36,7 +35,7 @@ static void tcg_cpu_machine_done(Notifier *n, void *unused)
>>       if (smram) {
>>           cpu->smram = g_new(MemoryRegion, 1);
>>           memory_region_init_alias(cpu->smram, OBJECT(cpu), "smram",
>> -                                 smram, 0, 4 * GiB);
>> +                                 smram, 0, memory_region_size(smram));
>
>Or define SMRAM_REGION_SIZE and use it?

This could still lead to inconsistencies if the size was changed in one place only, no?

>
>(subject header can be shortened to "target/i386:").

Okay.