diff mbox series

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

Message ID 20230204151027.39007-9-shentey@gmail.com
State New
Headers show
Series PC cleanups | expand

Commit Message

Bernhard Beschow Feb. 4, 2023, 3:10 p.m. UTC
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(-)

Comments

Philippe Mathieu-Daudé Feb. 5, 2023, 11:21 a.m. UTC | #1
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);
Juan Quintela Feb. 6, 2023, 10:06 a.m. UTC | #2
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.
Bernhard Beschow Feb. 7, 2023, 3:17 p.m. UTC | #3
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.
>
>
Juan Quintela Feb. 7, 2023, 6:34 p.m. UTC | #4
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.
>>
>>
Bernhard Beschow Feb. 7, 2023, 10:43 p.m. UTC | #5
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.
>>>
>>>
>
diff mbox series

Patch

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