[PATCH v4 0/9] PC cleanups

[PATCH v4 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. Note that the movement of
the SMRAM memory region apparently doesn't change migration ABI for the pc and
q35 machines (see below).

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`
* Confirm that JSON representation of migration files (pc & q35) are empty:
  1. Create 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
     this series.
  2. Run `./scripts/analyze-migration.py -d desc -f *.mig > *.json` on the four
     files
  3. Compare the diffs -> both are empty

v4:
* Remove ram_memory variable in pc_q35 completely (Zoltan)

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                 | 17 +++++++++--------
 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(+), 60 deletions(-)

-- 
2.39.1

[PATCH v4 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

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

[Philippe Mathieu-Daudé]

On 13/2/23 17:19, 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);

To avoid accessing internal fields we should stick to the PCI API:

     memory_region_add_subregion(pci_address_space_io(PCI_DEVICE(dev)),
                                 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);

Now all classes implementing PCI_HOST_BRIDGE register conf/data in I/O
space, so this could be a pattern justifying reworking a bit the
PCIHostBridgeClass or adding an helper in "hw/pci/pci_host.h" to do
that generically.

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

[Bernhard Beschow]

Am 22. Februar 2023 10:58:08 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>On 13/2/23 17:19, 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);
>
>To avoid accessing internal fields we should stick to the PCI API:
>
>    memory_region_add_subregion(pci_address_space_io(PCI_DEVICE(dev)),
>                                0xcf8, &s->conf_mem);

dev is of type PCIHostState which derives from SysBusDevice, not PCIDevice. AFAICS there is no getter implemented on PCIBus.

>
>>       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);
>
>Now all classes implementing PCI_HOST_BRIDGE register conf/data in I/O
>space, so this could be a pattern justifying reworking a bit the
>PCIHostBridgeClass or adding an helper in "hw/pci/pci_host.h" to do
>that generically.

What do you mean exactly? There are PCI hosts spawning two PCI buses and therefore have two such spaces.

Best regards,
Bernhard

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

[Bernhard Beschow]

Am 22. Februar 2023 18:05:51 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>
>
>Am 22. Februar 2023 10:58:08 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>>On 13/2/23 17:19, 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);
>>
>>To avoid accessing internal fields we should stick to the PCI API:
>>
>>    memory_region_add_subregion(pci_address_space_io(PCI_DEVICE(dev)),
>>                                0xcf8, &s->conf_mem);
>
>dev is of type PCIHostState which derives from SysBusDevice, not PCIDevice. AFAICS there is no getter implemented on PCIBus.

Ping

>
>>
>>>       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);
>>
>>Now all classes implementing PCI_HOST_BRIDGE register conf/data in I/O
>>space, so this could be a pattern justifying reworking a bit the
>>PCIHostBridgeClass or adding an helper in "hw/pci/pci_host.h" to do
>>that generically.
>
>What do you mean exactly? There are PCI hosts spawning two PCI buses and therefore have two such spaces.
>
>Best regards,
>Bernhard

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

[Philippe Mathieu-Daudé]

On 6/3/23 07:57, Bernhard Beschow wrote:
> 
> 
> Am 22. Februar 2023 18:05:51 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>>
>>
>> Am 22. Februar 2023 10:58:08 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>>> On 13/2/23 17:19, 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);
>>>
>>> To avoid accessing internal fields we should stick to the PCI API:
>>>
>>>     memory_region_add_subregion(pci_address_space_io(PCI_DEVICE(dev)),
>>>                                 0xcf8, &s->conf_mem);
>>
>> dev is of type PCIHostState which derives from SysBusDevice, not PCIDevice. AFAICS there is no getter implemented on PCIBus.

You are right, there is no getter for PCIBus::address_space_io,
it is accessed directly:

   MemoryRegion *pci_address_space_io(PCIDevice *dev)
   {
       return pci_get_bus(dev)->address_space_io;
   }

If it is considered a property, probably no need for getter.

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

> 
>>
>>>
>>>>        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);
>>>
>>> Now all classes implementing PCI_HOST_BRIDGE register conf/data in I/O
>>> space, so this could be a pattern justifying reworking a bit the
>>> PCIHostBridgeClass or adding an helper in "hw/pci/pci_host.h" to do
>>> that generically.

(this comment is besides the scope of this patch)

>> What do you mean exactly? There are PCI hosts spawning two PCI buses and therefore have two such spaces.

I haven't checked but IIRC each PCI host exposing I/O registers
to access buses via ISA I/O open-code it. Even if it exposes
multiple buses, the same pattern is used (1 time per bus).

Anyway, no need to worry about that now...

[PATCH v4 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 v4 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

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

[Philippe Mathieu-Daudé]

On 13/2/23 17:19, 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(-)
> 
> 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,

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

Long term we should duplicate/extract Q35MachineState from
PCMachineState and add a Q35PCIHost field, then use 
object_initialize_child; removing this object_property_add_child()
call.

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

[Bernhard Beschow]

Am 22. Februar 2023 11:03:38 UTC schrieb "Philippe Mathieu-Daudé"
<philmd@linaro.org>:
>On 13/2/23 17:19, 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(-)
>>
>> 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,
>
>Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
>Long term we should duplicate/extract Q35MachineState from
>PCMachineState and add a Q35PCIHost field, then use object_initialize_child; removing this object_property_add_child()
>call.

The Q35 and PC machines duplicate a lot of code indeed. So I was
thinking more along the lines of consolidating with pc_piix ;) The
idea would be to get a peek preview into a configuration-driven future
where the PCI host bridges (Q35 or I440FX) are dynamically
instantiated based on configuration data. They would also be
configured through their QOM interfaces only.

I've submitted a series where the Q35 host bridge gets a QOM cleanup
[1] and I've got a series locally resolving i440fx_init(). Both series
combined bring these two device models close together regarding their
QOM interface. I've not submitted the i440fx series yet since it is
blocked by this series.

One further step for pc_q35 and pc_piix consolidation would be to
factor ICH9 PCI devices (not functions!) into self-contained models,
like is underway with PIIX(3). I've started with ICH9 cleanup already
[2] and I'm waiting for the PIIX consolidation to land in order to be
able to make more progress here.

Note that pc_q35 and pc_piix consolidation is just an idea for now
which could also turn out to be a bad one. If the two machines just
ended up sharing more code that could IMO be considered a success as
well.

Best regards,
Bernhard

[1] https://patchew.org/QEMU/20230214131441.101760-1-shentey@gmail.com/
[2] https://patchew.org/QEMU/20230213173033.98762-1-shentey@gmail.com/

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

[Michael S. Tsirkin]

On Wed, Feb 22, 2023 at 06:52:02PM +0100, Bernhard Beschow wrote:
> Am 22. Februar 2023 11:03:38 UTC schrieb "Philippe Mathieu-Daudé"
> <philmd@linaro.org>:
> >On 13/2/23 17:19, 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(-)
> >>
> >> 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,
> >
> >Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> >
> >Long term we should duplicate/extract Q35MachineState from
> >PCMachineState and add a Q35PCIHost field, then use object_initialize_child; removing this object_property_add_child()
> >call.
> 
> The Q35 and PC machines duplicate a lot of code indeed. So I was
> thinking more along the lines of consolidating with pc_piix ;)

The reason is that we are trying to limit changes to pc_piix and
focus development on Q35.

> The
> idea would be to get a peek preview into a configuration-driven future
> where the PCI host bridges (Q35 or I440FX) are dynamically
> instantiated based on configuration data. They would also be
> configured through their QOM interfaces only.
> 
> I've submitted a series where the Q35 host bridge gets a QOM cleanup
> [1] and I've got a series locally resolving i440fx_init(). Both series
> combined bring these two device models close together regarding their
> QOM interface. I've not submitted the i440fx series yet since it is
> blocked by this series.
> 
> One further step for pc_q35 and pc_piix consolidation would be to
> factor ICH9 PCI devices (not functions!) into self-contained models,
> like is underway with PIIX(3). I've started with ICH9 cleanup already
> [2] and I'm waiting for the PIIX consolidation to land in order to be
> able to make more progress here.
> 
> Note that pc_q35 and pc_piix consolidation is just an idea for now
> which could also turn out to be a bad one. If the two machines just
> ended up sharing more code that could IMO be considered a success as
> well.
> 
> Best regards,
> Bernhard
> 
> [1] https://patchew.org/QEMU/20230214131441.101760-1-shentey@gmail.com/
> [2] https://patchew.org/QEMU/20230213173033.98762-1-shentey@gmail.com/

[PATCH v4 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 v4 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

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

[Philippe Mathieu-Daudé]

On 13/2/23 17:20, 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>

[PATCH v4 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     | 6 ++----
 4 files changed, 4 insertions(+), 9 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..455cbe11fa 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -129,7 +129,6 @@ static void pc_q35_init(MachineState *machine)
     MemoryRegion *system_io = get_system_io();
     MemoryRegion *pci_memory;
     MemoryRegion *rom_memory;
-    MemoryRegion *ram_memory;
     GSIState *gsi_state;
     ISABus *isa_bus;
     int i;
@@ -216,12 +215,11 @@ 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,
-                             OBJECT(ram_memory), NULL);
+                             OBJECT(machine->ram), 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,
-- 
2.39.1

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

[Xiaoyao Li]

On 2/14/2023 12:20 AM, Bernhard Beschow wrote:
> Going through pc_memory_init() seems quite complicated for a simple
> assignment.
> 

...

> 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);

IMHO, it seems more proper to put

+        ram_memory = machine->ram;

here rather than above.

> +        pc_memory_init(pcms, system_memory, rom_memory, hole64_size);
>       } else {
>           pc_system_flash_cleanup_unused(pcms);
>           if (machine->kernel_filename != NULL) {

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

[Bernhard Beschow]

Am 22. Februar 2023 02:38:04 UTC schrieb Xiaoyao Li <xiaoyao.li@intel.com>:
>On 2/14/2023 12:20 AM, Bernhard Beschow wrote:
>> Going through pc_memory_init() seems quite complicated for a simple
>> assignment.
>> 
>
>...
>
>> 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);

In Xen mode, ram_memory is initialized here and machine->ram isn't used at all.

>>       } else {
>> +        ram_memory = machine->ram;

The idea of moving the assignment here is to make the code more symmetric to Xen and to establish the invariant of ram_memory being initialized after this if-else block. IOW one shouldn't need to use machine->ram directly after this point.

>>           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);
>
>IMHO, it seems more proper to put
>
>+        ram_memory = machine->ram;
>
>here rather than above.

This patch allowed for further cleanup (establishing above invariant) which I inlined for simplicity.

Do you still see issues here?

Thanks,
Bernhard
>
>> +        pc_memory_init(pcms, system_memory, rom_memory, hole64_size);
>>       } else {
>>           pc_system_flash_cleanup_unused(pcms);
>>           if (machine->kernel_filename != NULL) {
>

[PATCH v4 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>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 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 v4 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 455cbe11fa..a5ffb77ed8 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -220,6 +220,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(machine->ram), 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 v4 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 v4 0/9] PC cleanups

[Bernhard Beschow]

Am 13. Februar 2023 16:19:55 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>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. Note that the movement of
>
>the SMRAM memory region apparently doesn't change migration ABI for the pc and
>
>q35 machines (see below).
>
>
>
>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`
>
>* Confirm that JSON representation of migration files (pc & q35) are empty:
>
>  1. Create 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
>
>     this series.
>
>  2. Run `./scripts/analyze-migration.py -d desc -f *.mig > *.json` on the four
>
>     files
>
>  3. Compare the diffs -> both are empty
>
>
>
>v4:
>
>* Remove ram_memory variable in pc_q35 completely (Zoltan)
>

The last two patches still need review. Comments welcome!

>
>
>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                 | 17 +++++++++--------
>
> 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(+), 60 deletions(-)
>
>
>
>-- >
>2.39.1
>
>
>

Re: [PATCH v4 0/9] PC cleanups

[Bernhard Beschow]

Am 13. Februar 2023 16:45:05 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>
>
>Am 13. Februar 2023 16:19:55 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>>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. Note that the movement of
>>
>>the SMRAM memory region apparently doesn't change migration ABI for the pc and
>>
>>q35 machines (see below).
>>
>>
>>
>>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`
>>
>>* Confirm that JSON representation of migration files (pc & q35) are empty:
>>
>>  1. Create 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
>>
>>     this series.
>>
>>  2. Run `./scripts/analyze-migration.py -d desc -f *.mig > *.json` on the four
>>
>>     files
>>
>>  3. Compare the diffs -> both are empty
>>
>>
>>
>>v4:
>>
>>* Remove ram_memory variable in pc_q35 completely (Zoltan)
>>
>
>The last two patches still need review. Comments welcome!

Ping

Can we queue the reviewed patches (all but the last two) already?

Thanks,
Bernhard

>
>>
>>
>>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                 | 17 +++++++++--------
>>
>> 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(+), 60 deletions(-)
>>
>>
>>
>>-- >
>>2.39.1
>>
>>
>>

Re: [PATCH v4 0/9] PC cleanups

[Michael S. Tsirkin]

On Sun, Mar 05, 2023 at 07:45:55AM +0000, Bernhard Beschow wrote:
> 
> 
> Am 13. Februar 2023 16:45:05 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
> >
> >
> >Am 13. Februar 2023 16:19:55 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
> >>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. Note that the movement of
> >>
> >>the SMRAM memory region apparently doesn't change migration ABI for the pc and
> >>
> >>q35 machines (see below).
> >>
> >>
> >>
> >>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`
> >>
> >>* Confirm that JSON representation of migration files (pc & q35) are empty:
> >>
> >>  1. Create 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
> >>
> >>     this series.
> >>
> >>  2. Run `./scripts/analyze-migration.py -d desc -f *.mig > *.json` on the four
> >>
> >>     files
> >>
> >>  3. Compare the diffs -> both are empty
> >>
> >>
> >>
> >>v4:
> >>
> >>* Remove ram_memory variable in pc_q35 completely (Zoltan)
> >>
> >
> >The last two patches still need review. Comments welcome!
> 
> Ping
> 
> Can we queue the reviewed patches (all but the last two) already?
> 
> Thanks,
> Bernhard

Philippe objected to patch 1 - he suggested using
pci_address_space_io.




> >
> >>
> >>
> >>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                 | 17 +++++++++--------
> >>
> >> 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(+), 60 deletions(-)
> >>
> >>
> >>
> >>-- >
> >>2.39.1
> >>
> >>
> >>

Re: [PATCH v4 0/9] PC cleanups

[Philippe Mathieu-Daudé]

On 5/3/23 11:09, Michael S. Tsirkin wrote:
> On Sun, Mar 05, 2023 at 07:45:55AM +0000, Bernhard Beschow wrote:
>>
>>
>> Am 13. Februar 2023 16:45:05 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>>>
>>>
>>> Am 13. Februar 2023 16:19:55 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>>>> 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. Note that the movement of
>>>>
>>>> the SMRAM memory region apparently doesn't change migration ABI for the pc and
>>>>
>>>> q35 machines (see below).


>>> The last two patches still need review. Comments welcome!
>>
>> Ping
>>
>> Can we queue the reviewed patches (all but the last two) already?
>>
>> Thanks,
>> Bernhard
> 
> Philippe objected to patch 1 - he suggested using
> pci_address_space_io.

I was wrong...

Re: [PATCH v4 0/9] PC cleanups

[Bernhard Beschow]

Am 5. März 2023 07:45:55 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>
>
>Am 13. Februar 2023 16:45:05 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>>
>>
>>Am 13. Februar 2023 16:19:55 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>>>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. Note that the movement of
>>>
>>>the SMRAM memory region apparently doesn't change migration ABI for the pc and
>>>
>>>q35 machines (see below).
>>>
>>>
>>>
>>>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`
>>>
>>>* Confirm that JSON representation of migration files (pc & q35) are empty:
>>>
>>>  1. Create 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
>>>
>>>     this series.
>>>
>>>  2. Run `./scripts/analyze-migration.py -d desc -f *.mig > *.json` on the four
>>>
>>>     files
>>>
>>>  3. Compare the diffs -> both are empty
>>>
>>>
>>>
>>>v4:
>>>
>>>* Remove ram_memory variable in pc_q35 completely (Zoltan)
>>>
>>
>>The last two patches still need review. Comments welcome!
>
>Ping
>
>Can we queue the reviewed patches (all but the last two) already?

Ping 2

>
>Thanks,
>Bernhard
>
>>
>>>
>>>
>>>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                 | 17 +++++++++--------
>>>
>>> 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(+), 60 deletions(-)
>>>
>>>
>>>
>>>-- >
>>>2.39.1
>>>
>>>
>>>

Re: [PATCH v4 0/9] PC cleanups

[Michael S. Tsirkin]

On Wed, May 10, 2023 at 06:26:54PM +0000, Bernhard Beschow wrote:
> 
> 
> Am 5. März 2023 07:45:55 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
> >
> >
> >Am 13. Februar 2023 16:45:05 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
> >>
> >>
> >>Am 13. Februar 2023 16:19:55 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
> >>>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. Note that the movement of
> >>>
> >>>the SMRAM memory region apparently doesn't change migration ABI for the pc and
> >>>
> >>>q35 machines (see below).
> >>>
> >>>
> >>>
> >>>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`
> >>>
> >>>* Confirm that JSON representation of migration files (pc & q35) are empty:
> >>>
> >>>  1. Create 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
> >>>
> >>>     this series.
> >>>
> >>>  2. Run `./scripts/analyze-migration.py -d desc -f *.mig > *.json` on the four
> >>>
> >>>     files
> >>>
> >>>  3. Compare the diffs -> both are empty
> >>>
> >>>
> >>>
> >>>v4:
> >>>
> >>>* Remove ram_memory variable in pc_q35 completely (Zoltan)
> >>>
> >>
> >>The last two patches still need review. Comments welcome!
> >
> >Ping
> >
> >Can we queue the reviewed patches (all but the last two) already?
> 
> Ping 2


queued 1-7

> >
> >Thanks,
> >Bernhard
> >
> >>
> >>>
> >>>
> >>>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                 | 17 +++++++++--------
> >>>
> >>> 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(+), 60 deletions(-)
> >>>
> >>>
> >>>
> >>>-- >
> >>>2.39.1
> >>>
> >>>
> >>>

Re: [PATCH v4 0/9] PC cleanups

[Bernhard Beschow]

Am 10. Mai 2023 19:20:59 UTC schrieb "Michael S. Tsirkin" <mst@redhat.com>:
>On Wed, May 10, 2023 at 06:26:54PM +0000, Bernhard Beschow wrote:
>> 
>> 
>> Am 5. März 2023 07:45:55 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>> >
>> >
>> >Am 13. Februar 2023 16:45:05 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>> >>
>> >>
>> >>Am 13. Februar 2023 16:19:55 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>> >>>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. Note that the movement of
>> >>>
>> >>>the SMRAM memory region apparently doesn't change migration ABI for the pc and
>> >>>
>> >>>q35 machines (see below).
>> >>>
>> >>>
>> >>>
>> >>>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`
>> >>>
>> >>>* Confirm that JSON representation of migration files (pc & q35) are empty:
>> >>>
>> >>>  1. Create 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
>> >>>
>> >>>     this series.
>> >>>
>> >>>  2. Run `./scripts/analyze-migration.py -d desc -f *.mig > *.json` on the four
>> >>>
>> >>>     files
>> >>>
>> >>>  3. Compare the diffs -> both are empty
>> >>>
>> >>>
>> >>>
>> >>>v4:
>> >>>
>> >>>* Remove ram_memory variable in pc_q35 completely (Zoltan)
>> >>>
>> >>
>> >>The last two patches still need review. Comments welcome!
>> >
>> >Ping
>> >
>> >Can we queue the reviewed patches (all but the last two) already?
>> 
>> Ping 2
>
>
>queued 1-7

Thanks -- also for the RTC ones!

Best regards,
Bernhard

>
>> >
>> >Thanks,
>> >Bernhard
>> >
>> >>
>> >>>
>> >>>
>> >>>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                 | 17 +++++++++--------
>> >>>
>> >>> 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(+), 60 deletions(-)
>> >>>
>> >>>
>> >>>
>> >>>-- >
>> >>>2.39.1
>> >>>
>> >>>
>> >>>
>

Re: [PATCH v4 0/9] PC cleanups

[Bernhard Beschow]

Am 13. Februar 2023 16:19:55 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>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. Note that the movement of
>
>the SMRAM memory region apparently doesn't change migration ABI for the pc and
>
>q35 machines (see below).
>

Ping

>
>
>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`
>
>* Confirm that JSON representation of migration files (pc & q35) are empty:
>
>  1. Create 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
>
>     this series.
>
>  2. Run `./scripts/analyze-migration.py -d desc -f *.mig > *.json` on the four
>
>     files
>
>  3. Compare the diffs -> both are empty
>
>
>
>v4:
>
>* Remove ram_memory variable in pc_q35 completely (Zoltan)
>
>
>
>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                 | 17 +++++++++--------
>
> 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(+), 60 deletions(-)
>
>
>
>-- >
>2.39.1
>
>
>