* [PATCH v3 1/9] hw/pci-host/i440fx: Inline sysbus_add_io()
2023-02-04 15:10 [PATCH v3 0/9] PC cleanups Bernhard Beschow
@ 2023-02-04 15:10 ` Bernhard Beschow
2023-02-05 11:05 ` Philippe Mathieu-Daudé
2023-02-04 15:10 ` [PATCH v3 2/9] hw/pci-host/q35: " Bernhard Beschow
` (7 subsequent siblings)
8 siblings, 1 reply; 25+ messages in thread
From: Bernhard Beschow @ 2023-02-04 15:10 UTC (permalink / raw)
To: qemu-devel
Cc: Thomas Huth, Richard Henderson, Eduardo Habkost, qemu-trivial,
BALATON Zoltan, Laurent Vivier, Sunil Muthuswamy,
Marcel Apfelbaum, Paolo Bonzini, Michael S. Tsirkin,
Igor Mammedov, Ani Sinha, 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
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH v3 1/9] hw/pci-host/i440fx: Inline sysbus_add_io()
2023-02-04 15:10 ` [PATCH v3 1/9] hw/pci-host/i440fx: Inline sysbus_add_io() Bernhard Beschow
@ 2023-02-05 11:05 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 25+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-02-05 11:05 UTC (permalink / raw)
To: Bernhard Beschow, qemu-devel
Cc: Thomas Huth, Richard Henderson, Eduardo Habkost, qemu-trivial,
BALATON Zoltan, Laurent Vivier, Sunil Muthuswamy,
Marcel Apfelbaum, Paolo Bonzini, Michael S. Tsirkin,
Igor Mammedov, Ani Sinha
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>
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v3 2/9] hw/pci-host/q35: Inline sysbus_add_io()
2023-02-04 15:10 [PATCH v3 0/9] PC cleanups Bernhard Beschow
2023-02-04 15:10 ` [PATCH v3 1/9] hw/pci-host/i440fx: Inline sysbus_add_io() Bernhard Beschow
@ 2023-02-04 15:10 ` Bernhard Beschow
2023-02-05 11:12 ` Philippe Mathieu-Daudé
2023-02-04 15:10 ` [PATCH v3 3/9] hw/i386/pc_q35: Reuse machine parameter Bernhard Beschow
` (6 subsequent siblings)
8 siblings, 1 reply; 25+ messages in thread
From: Bernhard Beschow @ 2023-02-04 15:10 UTC (permalink / raw)
To: qemu-devel
Cc: Thomas Huth, Richard Henderson, Eduardo Habkost, qemu-trivial,
BALATON Zoltan, Laurent Vivier, Sunil Muthuswamy,
Marcel Apfelbaum, Paolo Bonzini, Michael S. Tsirkin,
Igor Mammedov, Ani Sinha, 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
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH v3 2/9] hw/pci-host/q35: Inline sysbus_add_io()
2023-02-04 15:10 ` [PATCH v3 2/9] hw/pci-host/q35: " Bernhard Beschow
@ 2023-02-05 11:12 ` Philippe Mathieu-Daudé
2023-02-06 0:15 ` Bernhard Beschow
0 siblings, 1 reply; 25+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-02-05 11:12 UTC (permalink / raw)
To: Bernhard Beschow, qemu-devel
Cc: Thomas Huth, Richard Henderson, Eduardo Habkost, qemu-trivial,
BALATON Zoltan, Laurent Vivier, Sunil Muthuswamy,
Marcel Apfelbaum, Paolo Bonzini, Michael S. Tsirkin,
Igor Mammedov, Ani Sinha
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?
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v3 2/9] hw/pci-host/q35: Inline sysbus_add_io()
2023-02-05 11:12 ` Philippe Mathieu-Daudé
@ 2023-02-06 0:15 ` Bernhard Beschow
0 siblings, 0 replies; 25+ messages in thread
From: Bernhard Beschow @ 2023-02-06 0:15 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Thomas Huth, Richard Henderson, Eduardo Habkost, qemu-trivial,
BALATON Zoltan, Laurent Vivier, Sunil Muthuswamy,
Marcel Apfelbaum, Paolo Bonzini, Michael S. Tsirkin,
Igor Mammedov, Ani Sinha
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...
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v3 3/9] hw/i386/pc_q35: Reuse machine parameter
2023-02-04 15:10 [PATCH v3 0/9] PC cleanups Bernhard Beschow
2023-02-04 15:10 ` [PATCH v3 1/9] hw/pci-host/i440fx: Inline sysbus_add_io() Bernhard Beschow
2023-02-04 15:10 ` [PATCH v3 2/9] hw/pci-host/q35: " Bernhard Beschow
@ 2023-02-04 15:10 ` Bernhard Beschow
2023-02-05 11:13 ` Philippe Mathieu-Daudé
2023-02-04 15:10 ` [PATCH v3 4/9] hw/i386/pc_{q35, piix}: Reuse MachineClass::desc as SMB product name Bernhard Beschow
` (5 subsequent siblings)
8 siblings, 1 reply; 25+ messages in thread
From: Bernhard Beschow @ 2023-02-04 15:10 UTC (permalink / raw)
To: qemu-devel
Cc: Thomas Huth, Richard Henderson, Eduardo Habkost, qemu-trivial,
BALATON Zoltan, Laurent Vivier, Sunil Muthuswamy,
Marcel Apfelbaum, Paolo Bonzini, Michael S. Tsirkin,
Igor Mammedov, Ani Sinha, 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
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH v3 3/9] hw/i386/pc_q35: Reuse machine parameter
2023-02-04 15:10 ` [PATCH v3 3/9] hw/i386/pc_q35: Reuse machine parameter Bernhard Beschow
@ 2023-02-05 11:13 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 25+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-02-05 11:13 UTC (permalink / raw)
To: Bernhard Beschow, qemu-devel
Cc: Thomas Huth, Richard Henderson, Eduardo Habkost, qemu-trivial,
BALATON Zoltan, Laurent Vivier, Sunil Muthuswamy,
Marcel Apfelbaum, Paolo Bonzini, Michael S. Tsirkin,
Igor Mammedov, Ani Sinha
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>
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v3 4/9] hw/i386/pc_{q35, piix}: Reuse MachineClass::desc as SMB product name
2023-02-04 15:10 [PATCH v3 0/9] PC cleanups Bernhard Beschow
` (2 preceding siblings ...)
2023-02-04 15:10 ` [PATCH v3 3/9] hw/i386/pc_q35: Reuse machine parameter Bernhard Beschow
@ 2023-02-04 15:10 ` Bernhard Beschow
2023-02-04 15:10 ` [PATCH v3 5/9] hw/i386/pc_{q35, piix}: Minimize usage of get_system_memory() Bernhard Beschow
` (4 subsequent siblings)
8 siblings, 0 replies; 25+ messages in thread
From: Bernhard Beschow @ 2023-02-04 15:10 UTC (permalink / raw)
To: qemu-devel
Cc: Thomas Huth, Richard Henderson, Eduardo Habkost, qemu-trivial,
BALATON Zoltan, Laurent Vivier, Sunil Muthuswamy,
Marcel Apfelbaum, Paolo Bonzini, Michael S. Tsirkin,
Igor Mammedov, Ani Sinha, Bernhard Beschow,
Philippe Mathieu-Daudé
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
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH v3 5/9] hw/i386/pc_{q35, piix}: Minimize usage of get_system_memory()
2023-02-04 15:10 [PATCH v3 0/9] PC cleanups Bernhard Beschow
` (3 preceding siblings ...)
2023-02-04 15:10 ` [PATCH v3 4/9] hw/i386/pc_{q35, piix}: Reuse MachineClass::desc as SMB product name Bernhard Beschow
@ 2023-02-04 15:10 ` Bernhard Beschow
2023-02-05 11:13 ` Philippe Mathieu-Daudé
2023-02-04 15:10 ` [PATCH v3 6/9] hw/i386/pc: Initialize ram_memory variable directly Bernhard Beschow
` (3 subsequent siblings)
8 siblings, 1 reply; 25+ messages in thread
From: Bernhard Beschow @ 2023-02-04 15:10 UTC (permalink / raw)
To: qemu-devel
Cc: Thomas Huth, Richard Henderson, Eduardo Habkost, qemu-trivial,
BALATON Zoltan, Laurent Vivier, Sunil Muthuswamy,
Marcel Apfelbaum, Paolo Bonzini, Michael S. Tsirkin,
Igor Mammedov, Ani Sinha, 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
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH v3 5/9] hw/i386/pc_{q35, piix}: Minimize usage of get_system_memory()
2023-02-04 15:10 ` [PATCH v3 5/9] hw/i386/pc_{q35, piix}: Minimize usage of get_system_memory() Bernhard Beschow
@ 2023-02-05 11:13 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 25+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-02-05 11:13 UTC (permalink / raw)
To: Bernhard Beschow, qemu-devel
Cc: Thomas Huth, Richard Henderson, Eduardo Habkost, qemu-trivial,
BALATON Zoltan, Laurent Vivier, Sunil Muthuswamy,
Marcel Apfelbaum, Paolo Bonzini, Michael S. Tsirkin,
Igor Mammedov, Ani Sinha
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>
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v3 6/9] hw/i386/pc: Initialize ram_memory variable directly
2023-02-04 15:10 [PATCH v3 0/9] PC cleanups Bernhard Beschow
` (4 preceding siblings ...)
2023-02-04 15:10 ` [PATCH v3 5/9] hw/i386/pc_{q35, piix}: Minimize usage of get_system_memory() Bernhard Beschow
@ 2023-02-04 15:10 ` Bernhard Beschow
2023-02-04 15:26 ` BALATON Zoltan
2023-02-04 15:10 ` [PATCH v3 7/9] hw/pci-host/pam: Make init_pam() usage more readable Bernhard Beschow
` (2 subsequent siblings)
8 siblings, 1 reply; 25+ messages in thread
From: Bernhard Beschow @ 2023-02-04 15:10 UTC (permalink / raw)
To: qemu-devel
Cc: Thomas Huth, Richard Henderson, Eduardo Habkost, qemu-trivial,
BALATON Zoltan, Laurent Vivier, Sunil Muthuswamy,
Marcel Apfelbaum, Paolo Bonzini, Michael S. Tsirkin,
Igor Mammedov, Ani Sinha, Bernhard Beschow,
Philippe Mathieu-Daudé
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
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH v3 6/9] hw/i386/pc: Initialize ram_memory variable directly
2023-02-04 15:10 ` [PATCH v3 6/9] hw/i386/pc: Initialize ram_memory variable directly Bernhard Beschow
@ 2023-02-04 15:26 ` BALATON Zoltan
2023-02-06 0:07 ` Bernhard Beschow
0 siblings, 1 reply; 25+ messages in thread
From: BALATON Zoltan @ 2023-02-04 15:26 UTC (permalink / raw)
To: Bernhard Beschow
Cc: qemu-devel, Thomas Huth, Richard Henderson, Eduardo Habkost,
qemu-trivial, Laurent Vivier, Sunil Muthuswamy, Marcel Apfelbaum,
Paolo Bonzini, Michael S. Tsirkin, Igor Mammedov, Ani Sinha,
Philippe Mathieu-Daudé
[-- 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,
>
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v3 6/9] hw/i386/pc: Initialize ram_memory variable directly
2023-02-04 15:26 ` BALATON Zoltan
@ 2023-02-06 0:07 ` Bernhard Beschow
0 siblings, 0 replies; 25+ messages in thread
From: Bernhard Beschow @ 2023-02-06 0:07 UTC (permalink / raw)
To: BALATON Zoltan
Cc: qemu-devel, Thomas Huth, Richard Henderson, Eduardo Habkost,
qemu-trivial, Laurent Vivier, Sunil Muthuswamy, Marcel Apfelbaum,
Paolo Bonzini, Michael S. Tsirkin, Igor Mammedov, Ani Sinha,
Philippe Mathieu-Daudé
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,
>>
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v3 7/9] hw/pci-host/pam: Make init_pam() usage more readable
2023-02-04 15:10 [PATCH v3 0/9] PC cleanups Bernhard Beschow
` (5 preceding siblings ...)
2023-02-04 15:10 ` [PATCH v3 6/9] hw/i386/pc: Initialize ram_memory variable directly Bernhard Beschow
@ 2023-02-04 15:10 ` Bernhard Beschow
2023-02-05 11:16 ` Philippe Mathieu-Daudé
2023-02-04 15:10 ` [PATCH v3 8/9] hw/i386/x86: Make TYPE_X86_MACHINE the owner of smram Bernhard Beschow
2023-02-04 15:10 ` [PATCH v3 9/9] target/i386/tcg/sysemu/tcg-cpu: Avoid own opinion about smram size Bernhard Beschow
8 siblings, 1 reply; 25+ messages in thread
From: Bernhard Beschow @ 2023-02-04 15:10 UTC (permalink / raw)
To: qemu-devel
Cc: Thomas Huth, Richard Henderson, Eduardo Habkost, qemu-trivial,
BALATON Zoltan, Laurent Vivier, Sunil Muthuswamy,
Marcel Apfelbaum, Paolo Bonzini, Michael S. Tsirkin,
Igor Mammedov, Ani Sinha, 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
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH v3 7/9] hw/pci-host/pam: Make init_pam() usage more readable
2023-02-04 15:10 ` [PATCH v3 7/9] hw/pci-host/pam: Make init_pam() usage more readable Bernhard Beschow
@ 2023-02-05 11:16 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 25+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-02-05 11:16 UTC (permalink / raw)
To: Bernhard Beschow, qemu-devel
Cc: Thomas Huth, Richard Henderson, Eduardo Habkost, qemu-trivial,
BALATON Zoltan, Laurent Vivier, Sunil Muthuswamy,
Marcel Apfelbaum, Paolo Bonzini, Michael S. Tsirkin,
Igor Mammedov, Ani Sinha
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>
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v3 8/9] hw/i386/x86: Make TYPE_X86_MACHINE the owner of smram
2023-02-04 15:10 [PATCH v3 0/9] PC cleanups Bernhard Beschow
` (6 preceding siblings ...)
2023-02-04 15:10 ` [PATCH v3 7/9] hw/pci-host/pam: Make init_pam() usage more readable Bernhard Beschow
@ 2023-02-04 15:10 ` Bernhard Beschow
2023-02-05 11:21 ` Philippe Mathieu-Daudé
2023-02-04 15:10 ` [PATCH v3 9/9] target/i386/tcg/sysemu/tcg-cpu: Avoid own opinion about smram size Bernhard Beschow
8 siblings, 1 reply; 25+ messages in thread
From: Bernhard Beschow @ 2023-02-04 15:10 UTC (permalink / raw)
To: qemu-devel
Cc: Thomas Huth, Richard Henderson, Eduardo Habkost, qemu-trivial,
BALATON Zoltan, Laurent Vivier, Sunil Muthuswamy,
Marcel Apfelbaum, Paolo Bonzini, Michael S. Tsirkin,
Igor Mammedov, Ani Sinha, 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
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH v3 8/9] hw/i386/x86: Make TYPE_X86_MACHINE the owner of smram
2023-02-04 15:10 ` [PATCH v3 8/9] hw/i386/x86: Make TYPE_X86_MACHINE the owner of smram Bernhard Beschow
@ 2023-02-05 11:21 ` Philippe Mathieu-Daudé
2023-02-06 10:06 ` Juan Quintela
0 siblings, 1 reply; 25+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-02-05 11:21 UTC (permalink / raw)
To: Bernhard Beschow, qemu-devel
Cc: Thomas Huth, Richard Henderson, Eduardo Habkost, qemu-trivial,
BALATON Zoltan, Laurent Vivier, Sunil Muthuswamy,
Marcel Apfelbaum, Paolo Bonzini, Michael S. Tsirkin,
Igor Mammedov, Ani Sinha, Juan Quintela,
Dr. David Alan Gilbert (git)
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);
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v3 8/9] hw/i386/x86: Make TYPE_X86_MACHINE the owner of smram
2023-02-05 11:21 ` Philippe Mathieu-Daudé
@ 2023-02-06 10:06 ` Juan Quintela
2023-02-07 15:17 ` Bernhard Beschow
0 siblings, 1 reply; 25+ messages in thread
From: Juan Quintela @ 2023-02-06 10:06 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Bernhard Beschow, qemu-devel, Thomas Huth, Richard Henderson,
Eduardo Habkost, qemu-trivial, BALATON Zoltan, Laurent Vivier,
Sunil Muthuswamy, Marcel Apfelbaum, Paolo Bonzini,
Michael S. Tsirkin, Igor Mammedov, Ani Sinha,
Dr. David Alan Gilbert (git)
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.
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v3 8/9] hw/i386/x86: Make TYPE_X86_MACHINE the owner of smram
2023-02-06 10:06 ` Juan Quintela
@ 2023-02-07 15:17 ` Bernhard Beschow
2023-02-07 18:34 ` Juan Quintela
0 siblings, 1 reply; 25+ messages in thread
From: Bernhard Beschow @ 2023-02-07 15:17 UTC (permalink / raw)
To: quintela
Cc: Philippe Mathieu-Daudé, qemu-devel, Thomas Huth,
Richard Henderson, Eduardo Habkost, qemu-trivial, BALATON Zoltan,
Laurent Vivier, Sunil Muthuswamy, Marcel Apfelbaum, Paolo Bonzini,
Michael S. Tsirkin, Igor Mammedov, Ani Sinha,
Dr. David Alan Gilbert (git)
[-- 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 --]
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v3 8/9] hw/i386/x86: Make TYPE_X86_MACHINE the owner of smram
2023-02-07 15:17 ` Bernhard Beschow
@ 2023-02-07 18:34 ` Juan Quintela
2023-02-07 22:43 ` Bernhard Beschow
0 siblings, 1 reply; 25+ messages in thread
From: Juan Quintela @ 2023-02-07 18:34 UTC (permalink / raw)
To: Bernhard Beschow
Cc: Philippe Mathieu-Daudé, qemu-devel, Thomas Huth,
Richard Henderson, Eduardo Habkost, qemu-trivial, BALATON Zoltan,
Laurent Vivier, Sunil Muthuswamy, Marcel Apfelbaum, Paolo Bonzini,
Michael S. Tsirkin, Igor Mammedov, Ani Sinha,
Dr. David Alan Gilbert (git)
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.
>>
>>
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v3 8/9] hw/i386/x86: Make TYPE_X86_MACHINE the owner of smram
2023-02-07 18:34 ` Juan Quintela
@ 2023-02-07 22:43 ` Bernhard Beschow
0 siblings, 0 replies; 25+ messages in thread
From: Bernhard Beschow @ 2023-02-07 22:43 UTC (permalink / raw)
To: quintela, Juan Quintela
Cc: Philippe Mathieu-Daudé, qemu-devel, Thomas Huth,
Richard Henderson, Eduardo Habkost, qemu-trivial, BALATON Zoltan,
Laurent Vivier, Sunil Muthuswamy, Marcel Apfelbaum, Paolo Bonzini,
Michael S. Tsirkin, Igor Mammedov, Ani Sinha,
Dr. David Alan Gilbert (git)
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.
>>>
>>>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v3 9/9] target/i386/tcg/sysemu/tcg-cpu: Avoid own opinion about smram size
2023-02-04 15:10 [PATCH v3 0/9] PC cleanups Bernhard Beschow
` (7 preceding siblings ...)
2023-02-04 15:10 ` [PATCH v3 8/9] hw/i386/x86: Make TYPE_X86_MACHINE the owner of smram Bernhard Beschow
@ 2023-02-04 15:10 ` Bernhard Beschow
2023-02-05 11:26 ` Philippe Mathieu-Daudé
8 siblings, 1 reply; 25+ messages in thread
From: Bernhard Beschow @ 2023-02-04 15:10 UTC (permalink / raw)
To: qemu-devel
Cc: Thomas Huth, Richard Henderson, Eduardo Habkost, qemu-trivial,
BALATON Zoltan, Laurent Vivier, Sunil Muthuswamy,
Marcel Apfelbaum, Paolo Bonzini, Michael S. Tsirkin,
Igor Mammedov, Ani Sinha, 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
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH v3 9/9] target/i386/tcg/sysemu/tcg-cpu: Avoid own opinion about smram size
2023-02-04 15:10 ` [PATCH v3 9/9] target/i386/tcg/sysemu/tcg-cpu: Avoid own opinion about smram size Bernhard Beschow
@ 2023-02-05 11:26 ` Philippe Mathieu-Daudé
2023-02-07 22:46 ` Bernhard Beschow
0 siblings, 1 reply; 25+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-02-05 11:26 UTC (permalink / raw)
To: Bernhard Beschow, qemu-devel
Cc: Thomas Huth, Richard Henderson, Eduardo Habkost, qemu-trivial,
BALATON Zoltan, Laurent Vivier, Sunil Muthuswamy,
Marcel Apfelbaum, Paolo Bonzini, Michael S. Tsirkin,
Igor Mammedov, Ani Sinha
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:").
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v3 9/9] target/i386/tcg/sysemu/tcg-cpu: Avoid own opinion about smram size
2023-02-05 11:26 ` Philippe Mathieu-Daudé
@ 2023-02-07 22:46 ` Bernhard Beschow
0 siblings, 0 replies; 25+ messages in thread
From: Bernhard Beschow @ 2023-02-07 22:46 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Thomas Huth, Richard Henderson, Eduardo Habkost, qemu-trivial,
BALATON Zoltan, Laurent Vivier, Sunil Muthuswamy,
Marcel Apfelbaum, Paolo Bonzini, Michael S. Tsirkin,
Igor Mammedov, Ani Sinha
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.
^ permalink raw reply [flat|nested] 25+ messages in thread