From: "Philippe Mathieu-Daudé" <philmd@linaro.org>
To: Bernhard Beschow <shentey@gmail.com>, qemu-devel@nongnu.org
Cc: Thomas Huth <thuth@redhat.com>,
Richard Henderson <richard.henderson@linaro.org>,
Eduardo Habkost <eduardo@habkost.net>,
qemu-trivial@nongnu.org, BALATON Zoltan <balaton@eik.bme.hu>,
Laurent Vivier <lvivier@redhat.com>,
Sunil Muthuswamy <sunilmut@microsoft.com>,
Marcel Apfelbaum <marcel.apfelbaum@gmail.com>,
Paolo Bonzini <pbonzini@redhat.com>,
"Michael S. Tsirkin" <mst@redhat.com>,
Igor Mammedov <imammedo@redhat.com>, Ani Sinha <ani@anisinha.ca>,
Juan Quintela <quintela@redhat.com>,
"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com>
Subject: Re: [PATCH v3 8/9] hw/i386/x86: Make TYPE_X86_MACHINE the owner of smram
Date: Sun, 5 Feb 2023 12:21:45 +0100 [thread overview]
Message-ID: <10bf125e-85a4-72cc-07de-0d6206941f62@linaro.org> (raw)
In-Reply-To: <20230204151027.39007-9-shentey@gmail.com>
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);
next prev parent reply other threads:[~2023-02-05 11:22 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-04 15:10 [PATCH v3 0/9] PC cleanups Bernhard Beschow
2023-02-04 15:10 ` [PATCH v3 1/9] hw/pci-host/i440fx: Inline sysbus_add_io() Bernhard Beschow
2023-02-05 11:05 ` Philippe Mathieu-Daudé
2023-02-04 15:10 ` [PATCH v3 2/9] hw/pci-host/q35: " Bernhard Beschow
2023-02-05 11:12 ` Philippe Mathieu-Daudé
2023-02-06 0:15 ` Bernhard Beschow
2023-02-04 15:10 ` [PATCH v3 3/9] hw/i386/pc_q35: Reuse machine parameter Bernhard Beschow
2023-02-05 11:13 ` Philippe Mathieu-Daudé
2023-02-04 15:10 ` [PATCH v3 4/9] hw/i386/pc_{q35, piix}: Reuse MachineClass::desc as SMB product name Bernhard Beschow
2023-02-04 15:10 ` [PATCH v3 5/9] hw/i386/pc_{q35, piix}: Minimize usage of get_system_memory() Bernhard Beschow
2023-02-05 11:13 ` Philippe Mathieu-Daudé
2023-02-04 15:10 ` [PATCH v3 6/9] hw/i386/pc: Initialize ram_memory variable directly Bernhard Beschow
2023-02-04 15:26 ` BALATON Zoltan
2023-02-06 0:07 ` Bernhard Beschow
2023-02-04 15:10 ` [PATCH v3 7/9] hw/pci-host/pam: Make init_pam() usage more readable Bernhard Beschow
2023-02-05 11:16 ` Philippe Mathieu-Daudé
2023-02-04 15:10 ` [PATCH v3 8/9] hw/i386/x86: Make TYPE_X86_MACHINE the owner of smram Bernhard Beschow
2023-02-05 11:21 ` Philippe Mathieu-Daudé [this message]
2023-02-06 10:06 ` Juan Quintela
2023-02-07 15:17 ` Bernhard Beschow
2023-02-07 18:34 ` Juan Quintela
2023-02-07 22:43 ` Bernhard Beschow
2023-02-04 15:10 ` [PATCH v3 9/9] target/i386/tcg/sysemu/tcg-cpu: Avoid own opinion about smram size Bernhard Beschow
2023-02-05 11:26 ` Philippe Mathieu-Daudé
2023-02-07 22:46 ` Bernhard Beschow
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=10bf125e-85a4-72cc-07de-0d6206941f62@linaro.org \
--to=philmd@linaro.org \
--cc=ani@anisinha.ca \
--cc=balaton@eik.bme.hu \
--cc=dgilbert@redhat.com \
--cc=eduardo@habkost.net \
--cc=imammedo@redhat.com \
--cc=lvivier@redhat.com \
--cc=marcel.apfelbaum@gmail.com \
--cc=mst@redhat.com \
--cc=pbonzini@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=qemu-trivial@nongnu.org \
--cc=quintela@redhat.com \
--cc=richard.henderson@linaro.org \
--cc=shentey@gmail.com \
--cc=sunilmut@microsoft.com \
--cc=thuth@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).