Re: [PATCH 06/12] hw/pci-host/q35: Initialize properties just once

[Michael S. Tsirkin]

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