From: "Michael S. Tsirkin" <mst@redhat.com>
To: Bernhard Beschow <shentey@gmail.com>
Cc: qemu-devel@nongnu.org,
Richard Henderson <richard.henderson@linaro.org>,
Marcel Apfelbaum <marcel.apfelbaum@gmail.com>,
Paolo Bonzini <pbonzini@redhat.com>,
Eduardo Habkost <eduardo@habkost.net>
Subject: Re: [PATCH 06/12] hw/pci-host/q35: Initialize properties just once
Date: Wed, 1 Mar 2023 16:47:20 -0500 [thread overview]
Message-ID: <20230301164701-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <20230214131441.101760-7-shentey@gmail.com>
On Tue, Feb 14, 2023 at 02:14:35PM +0100, Bernhard Beschow wrote:
> Although not used there, the attributes for Q35's "pci-hole64-size" and
> "short_root_bus" properties currently reside in its child device. This
> causes the default values to be overwritten during the child's
> object_initialize() phase.
pls add explanation why this is a problem.
> Avoid this by moving both attributes into the
> host device.
>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> include/hw/pci-host/q35.h | 5 +++--
> hw/pci-host/q35.c | 20 +++++---------------
> 2 files changed, 8 insertions(+), 17 deletions(-)
>
> diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
> index fcbe57b42d..93e41ffbee 100644
> --- a/include/hw/pci-host/q35.h
> +++ b/include/hw/pci-host/q35.h
> @@ -54,8 +54,6 @@ struct MCHPCIState {
> Range pci_hole;
> uint64_t below_4g_mem_size;
> uint64_t above_4g_mem_size;
> - uint64_t pci_hole64_size;
> - uint32_t short_root_bus;
> uint16_t ext_tseg_mbytes;
> };
>
> @@ -64,7 +62,10 @@ struct Q35PCIHost {
> PCIExpressHost parent_obj;
> /*< public >*/
>
> + uint64_t pci_hole64_size;
> + uint32_t short_root_bus;
> bool pci_hole64_fix;
> +
> MCHPCIState mch;
> };
>
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index 0e198f97a7..03aa08dae5 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -76,7 +76,7 @@ static const char *q35_host_root_bus_path(PCIHostState *host_bridge,
> Q35PCIHost *s = Q35_HOST_DEVICE(host_bridge);
>
> /* For backwards compat with old device paths */
> - if (s->mch.short_root_bus) {
> + if (s->short_root_bus) {
> return "0000";
> }
> return "0000:00";
> @@ -161,27 +161,19 @@ static void q35_host_get_pci_hole64_end(Object *obj, Visitor *v,
>
> pci_bus_get_w64_range(h->bus, &w64);
> value = range_is_empty(&w64) ? 0 : range_upb(&w64) + 1;
> - hole64_end = ROUND_UP(hole64_start + s->mch.pci_hole64_size, 1ULL << 30);
> + hole64_end = ROUND_UP(hole64_start + s->pci_hole64_size, 1ULL << 30);
> if (s->pci_hole64_fix && value < hole64_end) {
> value = hole64_end;
> }
> visit_type_uint64(v, name, &value, errp);
> }
>
> -/*
> - * NOTE: setting defaults for the mch.* fields in this table
> - * doesn't work, because mch is a separate QOM object that is
> - * zeroed by the object_initialize(&s->mch, ...) call inside
> - * q35_host_initfn(). The default values for those
> - * properties need to be initialized manually by
> - * q35_host_initfn() after the object_initialize() call.
> - */
> static Property q35_host_props[] = {
> DEFINE_PROP_UINT64(PCIE_HOST_MCFG_BASE, Q35PCIHost, parent_obj.base_addr,
> MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT),
> DEFINE_PROP_SIZE(PCI_HOST_PROP_PCI_HOLE64_SIZE, Q35PCIHost,
> - mch.pci_hole64_size, Q35_PCI_HOST_HOLE64_SIZE_DEFAULT),
> - DEFINE_PROP_UINT32("short_root_bus", Q35PCIHost, mch.short_root_bus, 0),
> + pci_hole64_size, Q35_PCI_HOST_HOLE64_SIZE_DEFAULT),
> + DEFINE_PROP_UINT32("short_root_bus", Q35PCIHost, short_root_bus, 0),
> DEFINE_PROP_SIZE(PCI_HOST_BELOW_4G_MEM_SIZE, Q35PCIHost,
> mch.below_4g_mem_size, 0),
> DEFINE_PROP_SIZE(PCI_HOST_ABOVE_4G_MEM_SIZE, Q35PCIHost,
> @@ -218,9 +210,7 @@ static void q35_host_initfn(Object *obj)
> object_initialize_child(OBJECT(s), "mch", &s->mch, TYPE_MCH_PCI_DEVICE);
> qdev_prop_set_int32(DEVICE(&s->mch), "addr", PCI_DEVFN(0, 0));
> qdev_prop_set_bit(DEVICE(&s->mch), "multifunction", false);
> - /* mch's object_initialize resets the default value, set it again */
> - qdev_prop_set_uint64(DEVICE(s), PCI_HOST_PROP_PCI_HOLE64_SIZE,
> - Q35_PCI_HOST_HOLE64_SIZE_DEFAULT);
> +
> object_property_add(obj, PCI_HOST_PROP_PCI_HOLE_START, "uint32",
> q35_host_get_pci_hole_start,
> NULL, NULL, NULL);
> --
> 2.39.1
next prev parent reply other threads:[~2023-03-01 21:47 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-14 13:14 [PATCH 00/12] Q35 PCI host fixes and QOM cleanup Bernhard Beschow
2023-02-14 13:14 ` [PATCH 01/12] hw/i386/pc_q35: Resolve redundant q35_host variable Bernhard Beschow
2023-02-14 13:14 ` [PATCH 02/12] hw/pci-host/q35: Fix contradicting .endianness assignment Bernhard Beschow
2023-03-01 21:43 ` Michael S. Tsirkin
2023-03-01 21:44 ` Michael S. Tsirkin
2023-02-14 13:14 ` [PATCH 03/12] hw/pci-host/q35: Use memory_region_set_address() also for tseg_blackhole Bernhard Beschow
2023-03-01 21:45 ` Michael S. Tsirkin
2023-02-14 13:14 ` [PATCH 04/12] hw/pci-host/q35: Initialize PCMachineState::bus in board code Bernhard Beschow
2023-02-14 13:14 ` [PATCH 05/12] hw/pci-host/q35: Initialize "bypass-iommu" property from " Bernhard Beschow
2023-03-01 21:46 ` Michael S. Tsirkin
2023-02-14 13:14 ` [PATCH 06/12] hw/pci-host/q35: Initialize properties just once Bernhard Beschow
2023-03-01 21:47 ` Michael S. Tsirkin [this message]
2023-02-14 13:14 ` [PATCH 07/12] hw/pci-host/q35: Initialize PCI hole boundaries " Bernhard Beschow
2023-02-14 13:14 ` [PATCH 08/12] hw/pci-host/q35: Turn PCI hole properties into class properties Bernhard Beschow
2023-02-14 13:14 ` [PATCH 09/12] hw/pci-host/q35: Rename local variable to more idiomatic "phb" Bernhard Beschow
2023-02-14 13:14 ` [PATCH 10/12] hw/pci-host/q35: Propagate to errp rather than doing error_fatal Bernhard Beschow
2023-02-14 13:14 ` [PATCH 11/12] hw/pci-host/q35: Merge mch_realize() into q35_host_realize() Bernhard Beschow
2023-02-14 13:14 ` [PATCH 12/12] hw/pci-host/q35: Move MemoryRegion pointers to host device Bernhard Beschow
2023-02-21 15:39 ` [PATCH 00/12] Q35 PCI host fixes and QOM cleanup Bernhard Beschow
2023-03-01 21:49 ` Michael S. Tsirkin
2023-03-02 21:54 ` Bernhard Beschow
2023-03-05 7:41 ` Bernhard Beschow
2023-03-01 21:50 ` Michael S. Tsirkin
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=20230301164701-mutt-send-email-mst@kernel.org \
--to=mst@redhat.com \
--cc=eduardo@habkost.net \
--cc=marcel.apfelbaum@gmail.com \
--cc=pbonzini@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=richard.henderson@linaro.org \
--cc=shentey@gmail.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.