[PATCH 0/8] hw/pci-host/designware: QOM shuffling (Host bridge <-> Root function)

[PATCH 0/8] hw/pci-host/designware: QOM shuffling (Host bridge <-> Root function)

[Philippe Mathieu-Daudé]

Hi,

While trying this PCI host bridge in a hegerogeneous setup
I noticed few discrepancies due to the fact that host bridge
pieces were managed by the root function.

This series move these pieces (ViewPort and MSI regs) to the
host bridge side where they belong. Unfortunately this is
a migration breakage.

I recommend reviewing using 'git-diff --color-moved=dimmed-zebra'.

Regards,

Phil.

Philippe Mathieu-Daudé (8):
  hw/pci-host/designware: Declare CPU QOM types using DEFINE_TYPES()
    macro
  hw/pci-host/designware: Initialize root function in host bridge
    realize
  hw/pci-host/designware: Add 'host_mem' variable for clarity
  hw/pci-host/designware: Hoist host controller in root function #0
  hw/pci-host/designware: Keep host reference in DesignwarePCIEViewport
  hw/pci-host/designware: Move viewports from root func to host bridge
  hw/pci-host/designware: Move MSI registers from root func to host
    bridge
  hw/pci-host/designware: Create ViewPorts during host bridge
    realization

 include/hw/pci-host/designware.h |  20 +-
 hw/pci-host/designware.c         | 376 +++++++++++++++----------------
 2 files changed, 187 insertions(+), 209 deletions(-)

-- 
2.41.0

[PATCH 1/8] hw/pci-host/designware: Declare CPU QOM types using DEFINE_TYPES() macro

[Philippe Mathieu-Daudé]

When multiple QOM types are registered in the same file,
it is simpler to use the the DEFINE_TYPES() macro. In
particular because type array declared with such macro
are easier to review.

Remove a pointless structure declaration in "designware.h".

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/pci-host/designware.h |  2 --
 hw/pci-host/designware.c         | 39 ++++++++++++++------------------
 2 files changed, 17 insertions(+), 24 deletions(-)

diff --git a/include/hw/pci-host/designware.h b/include/hw/pci-host/designware.h
index 908f3d946b..c484e377a8 100644
--- a/include/hw/pci-host/designware.h
+++ b/include/hw/pci-host/designware.h
@@ -31,8 +31,6 @@ OBJECT_DECLARE_SIMPLE_TYPE(DesignwarePCIEHost, DESIGNWARE_PCIE_HOST)
 #define TYPE_DESIGNWARE_PCIE_ROOT "designware-pcie-root"
 OBJECT_DECLARE_SIMPLE_TYPE(DesignwarePCIERoot, DESIGNWARE_PCIE_ROOT)
 
-struct DesignwarePCIERoot;
-
 typedef struct DesignwarePCIEViewport {
     DesignwarePCIERoot *root;
 
diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
index 6f5442f108..304eca1b5c 100644
--- a/hw/pci-host/designware.c
+++ b/hw/pci-host/designware.c
@@ -746,28 +746,23 @@ static void designware_pcie_host_init(Object *obj)
     qdev_prop_set_bit(DEVICE(root), "multifunction", false);
 }
 
-static const TypeInfo designware_pcie_root_info = {
-    .name = TYPE_DESIGNWARE_PCIE_ROOT,
-    .parent = TYPE_PCI_BRIDGE,
-    .instance_size = sizeof(DesignwarePCIERoot),
-    .class_init = designware_pcie_root_class_init,
-    .interfaces = (InterfaceInfo[]) {
-        { INTERFACE_PCIE_DEVICE },
-        { }
+static const TypeInfo designware_pcie_types[] = {
+    {
+        .name           = TYPE_DESIGNWARE_PCIE_HOST,
+        .parent         = TYPE_PCI_HOST_BRIDGE,
+        .instance_size  = sizeof(DesignwarePCIEHost),
+        .instance_init  = designware_pcie_host_init,
+        .class_init     = designware_pcie_host_class_init,
+    }, {
+        .name           = TYPE_DESIGNWARE_PCIE_ROOT,
+        .parent         = TYPE_PCI_BRIDGE,
+        .instance_size  = sizeof(DesignwarePCIERoot),
+        .class_init     = designware_pcie_root_class_init,
+        .interfaces     = (InterfaceInfo[]) {
+            { INTERFACE_PCIE_DEVICE },
+            { }
+        },
     },
 };
 
-static const TypeInfo designware_pcie_host_info = {
-    .name       = TYPE_DESIGNWARE_PCIE_HOST,
-    .parent     = TYPE_PCI_HOST_BRIDGE,
-    .instance_size = sizeof(DesignwarePCIEHost),
-    .instance_init = designware_pcie_host_init,
-    .class_init = designware_pcie_host_class_init,
-};
-
-static void designware_pcie_register(void)
-{
-    type_register_static(&designware_pcie_root_info);
-    type_register_static(&designware_pcie_host_info);
-}
-type_init(designware_pcie_register)
+DEFINE_TYPES(designware_pcie_types)
-- 
2.41.0

Re: [PATCH 1/8] hw/pci-host/designware: Declare CPU QOM types using DEFINE_TYPES() macro

[Peter Maydell]

On Thu, 12 Oct 2023 at 13:19, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> When multiple QOM types are registered in the same file,
> it is simpler to use the the DEFINE_TYPES() macro. In
> particular because type array declared with such macro
> are easier to review.
>
> Remove a pointless structure declaration in "designware.h".
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 1/8] hw/pci-host/designware: Declare CPU QOM types using DEFINE_TYPES() macro

[Gustavo Romero]

Hi Phil,

On 10/12/23 9:18 AM, Philippe Mathieu-Daudé wrote:
> When multiple QOM types are registered in the same file,
> it is simpler to use the the DEFINE_TYPES() macro. In
> particular because type array declared with such macro
> are easier to review.
> 
> Remove a pointless structure declaration in "designware.h".
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   include/hw/pci-host/designware.h |  2 --
>   hw/pci-host/designware.c         | 39 ++++++++++++++------------------
>   2 files changed, 17 insertions(+), 24 deletions(-)
> 
> diff --git a/include/hw/pci-host/designware.h b/include/hw/pci-host/designware.h
> index 908f3d946b..c484e377a8 100644
> --- a/include/hw/pci-host/designware.h
> +++ b/include/hw/pci-host/designware.h
> @@ -31,8 +31,6 @@ OBJECT_DECLARE_SIMPLE_TYPE(DesignwarePCIEHost, DESIGNWARE_PCIE_HOST)
>   #define TYPE_DESIGNWARE_PCIE_ROOT "designware-pcie-root"
>   OBJECT_DECLARE_SIMPLE_TYPE(DesignwarePCIERoot, DESIGNWARE_PCIE_ROOT)
>   
> -struct DesignwarePCIERoot;
> -
>   typedef struct DesignwarePCIEViewport {
>       DesignwarePCIERoot *root;
>   
> diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
> index 6f5442f108..304eca1b5c 100644
> --- a/hw/pci-host/designware.c
> +++ b/hw/pci-host/designware.c
> @@ -746,28 +746,23 @@ static void designware_pcie_host_init(Object *obj)
>       qdev_prop_set_bit(DEVICE(root), "multifunction", false);
>   }
>   
> -static const TypeInfo designware_pcie_root_info = {
> -    .name = TYPE_DESIGNWARE_PCIE_ROOT,
> -    .parent = TYPE_PCI_BRIDGE,
> -    .instance_size = sizeof(DesignwarePCIERoot),
> -    .class_init = designware_pcie_root_class_init,
> -    .interfaces = (InterfaceInfo[]) {
> -        { INTERFACE_PCIE_DEVICE },
> -        { }
> +static const TypeInfo designware_pcie_types[] = {
> +    {
> +        .name           = TYPE_DESIGNWARE_PCIE_HOST,
> +        .parent         = TYPE_PCI_HOST_BRIDGE,
> +        .instance_size  = sizeof(DesignwarePCIEHost),
> +        .instance_init  = designware_pcie_host_init,
> +        .class_init     = designware_pcie_host_class_init,
> +    }, {
> +        .name           = TYPE_DESIGNWARE_PCIE_ROOT,
> +        .parent         = TYPE_PCI_BRIDGE,
> +        .instance_size  = sizeof(DesignwarePCIERoot),
> +        .class_init     = designware_pcie_root_class_init,
> +        .interfaces     = (InterfaceInfo[]) {
> +            { INTERFACE_PCIE_DEVICE },
> +            { }
> +        },
>       },
>   };
>   
> -static const TypeInfo designware_pcie_host_info = {
> -    .name       = TYPE_DESIGNWARE_PCIE_HOST,
> -    .parent     = TYPE_PCI_HOST_BRIDGE,
> -    .instance_size = sizeof(DesignwarePCIEHost),
> -    .instance_init = designware_pcie_host_init,
> -    .class_init = designware_pcie_host_class_init,
> -};
> -
> -static void designware_pcie_register(void)
> -{
> -    type_register_static(&designware_pcie_root_info);
> -    type_register_static(&designware_pcie_host_info);
> -}
> -type_init(designware_pcie_register)
> +DEFINE_TYPES(designware_pcie_types)
> 

Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org>

This patch can get merged independently of this series.


Cheers,
Gustavo

Re: [PATCH 1/8] hw/pci-host/designware: Declare CPU QOM types using DEFINE_TYPES() macro

[Philippe Mathieu-Daudé]

On 19/8/24 06:21, Gustavo Romero wrote:
> Hi Phil,
> 
> On 10/12/23 9:18 AM, Philippe Mathieu-Daudé wrote:
>> When multiple QOM types are registered in the same file,
>> it is simpler to use the the DEFINE_TYPES() macro. In
>> particular because type array declared with such macro
>> are easier to review.
>>
>> Remove a pointless structure declaration in "designware.h".
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>   include/hw/pci-host/designware.h |  2 --
>>   hw/pci-host/designware.c         | 39 ++++++++++++++------------------
>>   2 files changed, 17 insertions(+), 24 deletions(-)


> Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org>
> 
> This patch can get merged independently of this series.

OK, thank you!

[PATCH 2/8] hw/pci-host/designware: Initialize root function in host bridge realize

[Philippe Mathieu-Daudé]

There are no root function properties exposed by the host
bridge, so using a 2-step QOM creation isn't really useful.

Simplify by creating the root function when the host bridge
is realized.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/pci-host/designware.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
index 304eca1b5c..692e0731cd 100644
--- a/hw/pci-host/designware.c
+++ b/hw/pci-host/designware.c
@@ -707,6 +707,10 @@ static void designware_pcie_host_realize(DeviceState *dev, Error **errp)
                        "pcie-bus-address-space");
     pci_setup_iommu(pci->bus, designware_pcie_host_set_iommu, s);
 
+    object_initialize_child(OBJECT(dev), "root", &s->root,
+                            TYPE_DESIGNWARE_PCIE_ROOT);
+    qdev_prop_set_int32(DEVICE(&s->root), "addr", PCI_DEVFN(0, 0));
+    qdev_prop_set_bit(DEVICE(&s->root), "multifunction", false);
     qdev_realize(DEVICE(&s->root), BUS(pci->bus), &error_fatal);
 }
 
@@ -736,22 +740,11 @@ static void designware_pcie_host_class_init(ObjectClass *klass, void *data)
     dc->vmsd = &vmstate_designware_pcie_host;
 }
 
-static void designware_pcie_host_init(Object *obj)
-{
-    DesignwarePCIEHost *s = DESIGNWARE_PCIE_HOST(obj);
-    DesignwarePCIERoot *root = &s->root;
-
-    object_initialize_child(obj, "root", root, TYPE_DESIGNWARE_PCIE_ROOT);
-    qdev_prop_set_int32(DEVICE(root), "addr", PCI_DEVFN(0, 0));
-    qdev_prop_set_bit(DEVICE(root), "multifunction", false);
-}
-
 static const TypeInfo designware_pcie_types[] = {
     {
         .name           = TYPE_DESIGNWARE_PCIE_HOST,
         .parent         = TYPE_PCI_HOST_BRIDGE,
         .instance_size  = sizeof(DesignwarePCIEHost),
-        .instance_init  = designware_pcie_host_init,
         .class_init     = designware_pcie_host_class_init,
     }, {
         .name           = TYPE_DESIGNWARE_PCIE_ROOT,
-- 
2.41.0

Re: [PATCH 2/8] hw/pci-host/designware: Initialize root function in host bridge realize

[Peter Maydell]

On Thu, 12 Oct 2023 at 13:19, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> There are no root function properties exposed by the host
> bridge, so using a 2-step QOM creation isn't really useful.
>
> Simplify by creating the root function when the host bridge
> is realized.

It's not necessary, but on the other hand "init child objects
in init; realize child objects in realize" is the standard
way to do this. Does not moving this to the realize method
block anything in the rest of the patchset?

thanks
-- PMM

Re: [PATCH 2/8] hw/pci-host/designware: Initialize root function in host bridge realize

[Philippe Mathieu-Daudé]

Hi Peter,

On 17/10/23 18:32, Peter Maydell wrote:
> On Thu, 12 Oct 2023 at 13:19, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> There are no root function properties exposed by the host
>> bridge, so using a 2-step QOM creation isn't really useful.
>>
>> Simplify by creating the root function when the host bridge
>> is realized.
> 
> It's not necessary, but on the other hand "init child objects
> in init; realize child objects in realize" is the standard
> way to do this. Does not moving this to the realize method
> block anything in the rest of the patchset?

I thought it was only recommended to use the init/realize
pair when properties could be consumed. Otherwise using
a single handler makes a model simpler.

No problem if I can drop this patch.

Re: [PATCH 2/8] hw/pci-host/designware: Initialize root function in host bridge realize

[Gustavo Romero]

Hi Phil,

On 10/12/23 9:18 AM, Philippe Mathieu-Daudé wrote:
> There are no root function properties exposed by the host
> bridge, so using a 2-step QOM creation isn't really useful.
> 
> Simplify by creating the root function when the host bridge
> is realized.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/pci-host/designware.c | 15 ++++-----------
>   1 file changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
> index 304eca1b5c..692e0731cd 100644
> --- a/hw/pci-host/designware.c
> +++ b/hw/pci-host/designware.c
> @@ -707,6 +707,10 @@ static void designware_pcie_host_realize(DeviceState *dev, Error **errp)
>                          "pcie-bus-address-space");
>       pci_setup_iommu(pci->bus, designware_pcie_host_set_iommu, s);
>   
> +    object_initialize_child(OBJECT(dev), "root", &s->root,
> +                            TYPE_DESIGNWARE_PCIE_ROOT);
> +    qdev_prop_set_int32(DEVICE(&s->root), "addr", PCI_DEVFN(0, 0));
> +    qdev_prop_set_bit(DEVICE(&s->root), "multifunction", false);
>       qdev_realize(DEVICE(&s->root), BUS(pci->bus), &error_fatal);
>   }
>   
> @@ -736,22 +740,11 @@ static void designware_pcie_host_class_init(ObjectClass *klass, void *data)
>       dc->vmsd = &vmstate_designware_pcie_host;
>   }
>   
> -static void designware_pcie_host_init(Object *obj)
> -{
> -    DesignwarePCIEHost *s = DESIGNWARE_PCIE_HOST(obj);
> -    DesignwarePCIERoot *root = &s->root;
> -
> -    object_initialize_child(obj, "root", root, TYPE_DESIGNWARE_PCIE_ROOT);
> -    qdev_prop_set_int32(DEVICE(root), "addr", PCI_DEVFN(0, 0));
> -    qdev_prop_set_bit(DEVICE(root), "multifunction", false);
> -}
> -
>   static const TypeInfo designware_pcie_types[] = {
>       {
>           .name           = TYPE_DESIGNWARE_PCIE_HOST,
>           .parent         = TYPE_PCI_HOST_BRIDGE,
>           .instance_size  = sizeof(DesignwarePCIEHost),
> -        .instance_init  = designware_pcie_host_init,
>           .class_init     = designware_pcie_host_class_init,
>       }, {
>           .name           = TYPE_DESIGNWARE_PCIE_ROOT,
> 

I could not find any mention in the docs recommending the use of
init/realize pair only when properties could be consumed. Anyways,
you agreed with Peter's comment (I agree too), so I understand this
patch will be drop since it doesn't affect the other patches in the
series.


Cheers,
Gustavo

[PATCH 3/8] hw/pci-host/designware: Add 'host_mem' variable for clarity

[Philippe Mathieu-Daudé]

designware_pcie_root_realize() uses get_system_memory()
as the "host side memory region", as opposed to the "PCI
side" one. Introduce the 'host_mem' variable for clarity.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/pci-host/designware.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
index 692e0731cd..bacb2bdb2d 100644
--- a/hw/pci-host/designware.c
+++ b/hw/pci-host/designware.c
@@ -393,6 +393,7 @@ static void designware_pcie_root_realize(PCIDevice *dev, Error **errp)
 {
     DesignwarePCIERoot *root = DESIGNWARE_PCIE_ROOT(dev);
     DesignwarePCIEHost *host = designware_pcie_root_to_host(root);
+    MemoryRegion *host_mem = get_system_memory();
     MemoryRegion *address_space = &host->pci.memory;
     PCIBridge *br = PCI_BRIDGE(dev);
     DesignwarePCIEViewport *viewport;
@@ -433,7 +434,7 @@ static void designware_pcie_root_realize(PCIDevice *dev, Error **errp)
         viewport->cr[0]   = DESIGNWARE_PCIE_ATU_TYPE_MEM;
 
         source      = &host->pci.address_space_root;
-        destination = get_system_memory();
+        destination = host_mem;
         direction   = "Inbound";
 
         /*
@@ -458,7 +459,7 @@ static void designware_pcie_root_realize(PCIDevice *dev, Error **errp)
 
         destination = &host->pci.memory;
         direction   = "Outbound";
-        source      = get_system_memory();
+        source      = host_mem;
 
         /*
          * Configure MemoryRegion implementing CPU -> PCI memory
-- 
2.41.0

Re: [PATCH 3/8] hw/pci-host/designware: Add 'host_mem' variable for clarity

[Peter Maydell]

On Thu, 12 Oct 2023 at 13:19, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> designware_pcie_root_realize() uses get_system_memory()
> as the "host side memory region", as opposed to the "PCI
> side" one. Introduce the 'host_mem' variable for clarity.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 3/8] hw/pci-host/designware: Add 'host_mem' variable for clarity

[Gustavo Romero]

Hi Phil,

On 10/12/23 9:18 AM, Philippe Mathieu-Daudé wrote:
> designware_pcie_root_realize() uses get_system_memory()
> as the "host side memory region", as opposed to the "PCI
> side" one. Introduce the 'host_mem' variable for clarity.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/pci-host/designware.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
> index 692e0731cd..bacb2bdb2d 100644
> --- a/hw/pci-host/designware.c
> +++ b/hw/pci-host/designware.c
> @@ -393,6 +393,7 @@ static void designware_pcie_root_realize(PCIDevice *dev, Error **errp)
>   {
>       DesignwarePCIERoot *root = DESIGNWARE_PCIE_ROOT(dev);
>       DesignwarePCIEHost *host = designware_pcie_root_to_host(root);
> +    MemoryRegion *host_mem = get_system_memory();
>       MemoryRegion *address_space = &host->pci.memory;
>       PCIBridge *br = PCI_BRIDGE(dev);
>       DesignwarePCIEViewport *viewport;
> @@ -433,7 +434,7 @@ static void designware_pcie_root_realize(PCIDevice *dev, Error **errp)
>           viewport->cr[0]   = DESIGNWARE_PCIE_ATU_TYPE_MEM;
>   
>           source      = &host->pci.address_space_root;
> -        destination = get_system_memory();
> +        destination = host_mem;
>           direction   = "Inbound";
>   
>           /*
> @@ -458,7 +459,7 @@ static void designware_pcie_root_realize(PCIDevice *dev, Error **errp)
>   
>           destination = &host->pci.memory;
>           direction   = "Outbound";
> -        source      = get_system_memory();
> +        source      = host_mem;
>   
>           /*
>            * Configure MemoryRegion implementing CPU -> PCI memory
> 

Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org>

This patch can get merged independently of this series.


Cheers,
Gustavo

[PATCH 4/8] hw/pci-host/designware: Hoist host controller in root function #0

[Philippe Mathieu-Daudé]

There is always an unique root function for the host bridge
controller. We create this function when the controller is
realized, in designware_pcie_host_realize().

No need to call qdev_get_parent_bus() each time the root function
want to resolve its host part. Hoist a pointer in its state. Set
the pointer once when the function is realized.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/pci-host/designware.h |  1 +
 hw/pci-host/designware.c         | 15 +++++----------
 2 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/include/hw/pci-host/designware.h b/include/hw/pci-host/designware.h
index c484e377a8..9e2caa04e9 100644
--- a/include/hw/pci-host/designware.h
+++ b/include/hw/pci-host/designware.h
@@ -71,6 +71,7 @@ struct DesignwarePCIERoot {
 
     DesignwarePCIEViewport viewports[2][DESIGNWARE_PCIE_NUM_VIEWPORTS];
     DesignwarePCIEMSI msi;
+    DesignwarePCIEHost *host;
 };
 
 struct DesignwarePCIEHost {
diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
index bacb2bdb2d..fb46493a05 100644
--- a/hw/pci-host/designware.c
+++ b/hw/pci-host/designware.c
@@ -57,13 +57,6 @@
 
 #define DESIGNWARE_PCIE_IRQ_MSI                    3
 
-static DesignwarePCIEHost *
-designware_pcie_root_to_host(DesignwarePCIERoot *root)
-{
-    BusState *bus = qdev_get_parent_bus(DEVICE(root));
-    return DESIGNWARE_PCIE_HOST(bus->parent);
-}
-
 static uint64_t designware_pcie_root_msi_read(void *opaque, hwaddr addr,
                                               unsigned size)
 {
@@ -85,7 +78,7 @@ static void designware_pcie_root_msi_write(void *opaque, hwaddr addr,
                                            uint64_t val, unsigned len)
 {
     DesignwarePCIERoot *root = DESIGNWARE_PCIE_ROOT(opaque);
-    DesignwarePCIEHost *host = designware_pcie_root_to_host(root);
+    DesignwarePCIEHost *host = root->host;
 
     root->msi.intr[0].status |= BIT(val) & root->msi.intr[0].enable;
 
@@ -300,7 +293,7 @@ static void designware_pcie_root_config_write(PCIDevice *d, uint32_t address,
                                               uint32_t val, int len)
 {
     DesignwarePCIERoot *root = DESIGNWARE_PCIE_ROOT(d);
-    DesignwarePCIEHost *host = designware_pcie_root_to_host(root);
+    DesignwarePCIEHost *host = root->host;
     DesignwarePCIEViewport *viewport =
         designware_pcie_root_get_current_viewport(root);
 
@@ -392,7 +385,8 @@ static char *designware_pcie_viewport_name(const char *direction,
 static void designware_pcie_root_realize(PCIDevice *dev, Error **errp)
 {
     DesignwarePCIERoot *root = DESIGNWARE_PCIE_ROOT(dev);
-    DesignwarePCIEHost *host = designware_pcie_root_to_host(root);
+    DesignwarePCIEHost *host = DESIGNWARE_PCIE_HOST(
+                                    qdev_get_parent_bus(DEVICE(dev))->parent);
     MemoryRegion *host_mem = get_system_memory();
     MemoryRegion *address_space = &host->pci.memory;
     PCIBridge *br = PCI_BRIDGE(dev);
@@ -406,6 +400,7 @@ static void designware_pcie_root_realize(PCIDevice *dev, Error **errp)
     size_t i;
 
     br->bus_name  = "dw-pcie";
+    root->host = host;
 
     pci_set_word(dev->config + PCI_COMMAND,
                  PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER);
-- 
2.41.0

Re: [PATCH 4/8] hw/pci-host/designware: Hoist host controller in root function #0

[Gustavo Romero]

Hi Phil,

I think the title of this patch is bit misleading. You're not
moving host into root, but rather adding a reference of host
inside the root.

Maybe change it to something like:

  "Add a back-pointer to the host in the root"?


On 10/12/23 9:18 AM, Philippe Mathieu-Daudé wrote:
> There is always an unique root function for the host bridge
> controller. We create this function when the controller is
> realized, in designware_pcie_host_realize().
> 
> No need to call qdev_get_parent_bus() each time the root function
> want to resolve its host part. Hoist a pointer in its state. Set
> the pointer once when the function is realized.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/hw/pci-host/designware.h |  1 +
>   hw/pci-host/designware.c         | 15 +++++----------
>   2 files changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/include/hw/pci-host/designware.h b/include/hw/pci-host/designware.h
> index c484e377a8..9e2caa04e9 100644
> --- a/include/hw/pci-host/designware.h
> +++ b/include/hw/pci-host/designware.h
> @@ -71,6 +71,7 @@ struct DesignwarePCIERoot {
>   
>       DesignwarePCIEViewport viewports[2][DESIGNWARE_PCIE_NUM_VIEWPORTS];
>       DesignwarePCIEMSI msi;
> +    DesignwarePCIEHost *host;

Because root is still defined in host and you're actually adding a
reference of host inside root this is a back-pointer to host, so do
you mind adding a comment above this like saying something like:

/* Convenient back-pointer for easy access to the host interface. */

Otherwise:

Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org>


Cheers,
Gustavo


>   };
>   
>   struct DesignwarePCIEHost {
> diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
> index bacb2bdb2d..fb46493a05 100644
> --- a/hw/pci-host/designware.c
> +++ b/hw/pci-host/designware.c
> @@ -57,13 +57,6 @@
>   
>   #define DESIGNWARE_PCIE_IRQ_MSI                    3
>   
> -static DesignwarePCIEHost *
> -designware_pcie_root_to_host(DesignwarePCIERoot *root)
> -{
> -    BusState *bus = qdev_get_parent_bus(DEVICE(root));
> -    return DESIGNWARE_PCIE_HOST(bus->parent);
> -}
> -
>   static uint64_t designware_pcie_root_msi_read(void *opaque, hwaddr addr,
>                                                 unsigned size)
>   {
> @@ -85,7 +78,7 @@ static void designware_pcie_root_msi_write(void *opaque, hwaddr addr,
>                                              uint64_t val, unsigned len)
>   {
>       DesignwarePCIERoot *root = DESIGNWARE_PCIE_ROOT(opaque);
> -    DesignwarePCIEHost *host = designware_pcie_root_to_host(root);
> +    DesignwarePCIEHost *host = root->host;
>   
>       root->msi.intr[0].status |= BIT(val) & root->msi.intr[0].enable;
>   
> @@ -300,7 +293,7 @@ static void designware_pcie_root_config_write(PCIDevice *d, uint32_t address,
>                                                 uint32_t val, int len)
>   {
>       DesignwarePCIERoot *root = DESIGNWARE_PCIE_ROOT(d);
> -    DesignwarePCIEHost *host = designware_pcie_root_to_host(root);
> +    DesignwarePCIEHost *host = root->host;
>       DesignwarePCIEViewport *viewport =
>           designware_pcie_root_get_current_viewport(root);
>   
> @@ -392,7 +385,8 @@ static char *designware_pcie_viewport_name(const char *direction,
>   static void designware_pcie_root_realize(PCIDevice *dev, Error **errp)
>   {
>       DesignwarePCIERoot *root = DESIGNWARE_PCIE_ROOT(dev);
> -    DesignwarePCIEHost *host = designware_pcie_root_to_host(root);
> +    DesignwarePCIEHost *host = DESIGNWARE_PCIE_HOST(
> +                                    qdev_get_parent_bus(DEVICE(dev))->parent);
>       MemoryRegion *host_mem = get_system_memory();
>       MemoryRegion *address_space = &host->pci.memory;
>       PCIBridge *br = PCI_BRIDGE(dev);
> @@ -406,6 +400,7 @@ static void designware_pcie_root_realize(PCIDevice *dev, Error **errp)
>       size_t i;
>   
>       br->bus_name  = "dw-pcie";
> +    root->host = host;
>   
>       pci_set_word(dev->config + PCI_COMMAND,
>                    PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER);
> 

[PATCH 5/8] hw/pci-host/designware: Keep host reference in DesignwarePCIEViewport

[Philippe Mathieu-Daudé]

The PCI root function is irrelevant for the ViewPort; only
a reference to the host bridge is required. Since we can
directly access the PCI bus, remove the pci_get_bus() call.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/pci-host/designware.h | 2 +-
 hw/pci-host/designware.c         | 7 +++----
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/include/hw/pci-host/designware.h b/include/hw/pci-host/designware.h
index 9e2caa04e9..e1952ad324 100644
--- a/include/hw/pci-host/designware.h
+++ b/include/hw/pci-host/designware.h
@@ -32,7 +32,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(DesignwarePCIEHost, DESIGNWARE_PCIE_HOST)
 OBJECT_DECLARE_SIMPLE_TYPE(DesignwarePCIERoot, DESIGNWARE_PCIE_ROOT)
 
 typedef struct DesignwarePCIEViewport {
-    DesignwarePCIERoot *root;
+    DesignwarePCIEHost *host;
 
     MemoryRegion cfg;
     MemoryRegion mem;
diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
index fb46493a05..d12a36b628 100644
--- a/hw/pci-host/designware.c
+++ b/hw/pci-host/designware.c
@@ -211,12 +211,11 @@ static uint64_t designware_pcie_root_data_access(void *opaque, hwaddr addr,
                                                  uint64_t *val, unsigned len)
 {
     DesignwarePCIEViewport *viewport = opaque;
-    DesignwarePCIERoot *root = viewport->root;
+    PCIHostState *pci = PCI_HOST_BRIDGE(viewport->host);
 
     const uint8_t busnum = DESIGNWARE_PCIE_ATU_BUS(viewport->target);
     const uint8_t devfn  = DESIGNWARE_PCIE_ATU_DEVFN(viewport->target);
-    PCIBus    *pcibus    = pci_get_bus(PCI_DEVICE(root));
-    PCIDevice *pcidev    = pci_find_device(pcibus, busnum, devfn);
+    PCIDevice *pcidev    = pci_find_device(pci->bus, busnum, devfn);
 
     if (pcidev) {
         addr &= pci_config_size(pcidev) - 1;
@@ -445,7 +444,7 @@ static void designware_pcie_root_realize(PCIDevice *dev, Error **errp)
         g_free(name);
 
         viewport = &root->viewports[DESIGNWARE_PCIE_VIEWPORT_OUTBOUND][i];
-        viewport->root    = root;
+        viewport->host    = host;
         viewport->inbound = false;
         viewport->base    = 0x0000000000000000ULL;
         viewport->target  = 0x0000000000000000ULL;
-- 
2.41.0

Re: [PATCH 5/8] hw/pci-host/designware: Keep host reference in DesignwarePCIEViewport

[Gustavo Romero]

Hi Phil,

I think the title of this patch is also a bit misleading.
The host reference is not present only in DesigwarePCIEViewport,
it's also present, for instance, in DesignwarePCIERoot after the
previous patch. This is also a back-pointer to host, so how about
something like:

"Add a back-pointer to the host in viewport"

Anyways,

Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org>


Cheers,
Gustavo


On 10/12/23 9:18 AM, Philippe Mathieu-Daudé wrote:
> The PCI root function is irrelevant for the ViewPort; only
> a reference to the host bridge is required. Since we can
> directly access the PCI bus, remove the pci_get_bus() call.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/hw/pci-host/designware.h | 2 +-
>   hw/pci-host/designware.c         | 7 +++----
>   2 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/include/hw/pci-host/designware.h b/include/hw/pci-host/designware.h
> index 9e2caa04e9..e1952ad324 100644
> --- a/include/hw/pci-host/designware.h
> +++ b/include/hw/pci-host/designware.h
> @@ -32,7 +32,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(DesignwarePCIEHost, DESIGNWARE_PCIE_HOST)
>   OBJECT_DECLARE_SIMPLE_TYPE(DesignwarePCIERoot, DESIGNWARE_PCIE_ROOT)
>   
>   typedef struct DesignwarePCIEViewport {
> -    DesignwarePCIERoot *root;
> +    DesignwarePCIEHost *host;
>   
>       MemoryRegion cfg;
>       MemoryRegion mem;
> diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
> index fb46493a05..d12a36b628 100644
> --- a/hw/pci-host/designware.c
> +++ b/hw/pci-host/designware.c
> @@ -211,12 +211,11 @@ static uint64_t designware_pcie_root_data_access(void *opaque, hwaddr addr,
>                                                    uint64_t *val, unsigned len)
>   {
>       DesignwarePCIEViewport *viewport = opaque;
> -    DesignwarePCIERoot *root = viewport->root;
> +    PCIHostState *pci = PCI_HOST_BRIDGE(viewport->host);
>   
>       const uint8_t busnum = DESIGNWARE_PCIE_ATU_BUS(viewport->target);
>       const uint8_t devfn  = DESIGNWARE_PCIE_ATU_DEVFN(viewport->target);
> -    PCIBus    *pcibus    = pci_get_bus(PCI_DEVICE(root));
> -    PCIDevice *pcidev    = pci_find_device(pcibus, busnum, devfn);
> +    PCIDevice *pcidev    = pci_find_device(pci->bus, busnum, devfn);
>   
>       if (pcidev) {
>           addr &= pci_config_size(pcidev) - 1;
> @@ -445,7 +444,7 @@ static void designware_pcie_root_realize(PCIDevice *dev, Error **errp)
>           g_free(name);
>   
>           viewport = &root->viewports[DESIGNWARE_PCIE_VIEWPORT_OUTBOUND][i];
> -        viewport->root    = root;
> +        viewport->host    = host;
>           viewport->inbound = false;
>           viewport->base    = 0x0000000000000000ULL;
>           viewport->target  = 0x0000000000000000ULL;
> 

[PATCH 6/8] hw/pci-host/designware: Move viewports from root func to host bridge

[Philippe Mathieu-Daudé]

As mentioned in previous commit, the PCI root function is
irrelevant for the ViewPorts. Move the fields to the host
bridge state.

This is a migration compatibility break for the machines
using the i.MX7 SoC (currently the mcimx7d-sabre machine).

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/pci-host/designware.h | 13 ++++-----
 hw/pci-host/designware.c         | 47 ++++++++++++++++----------------
 2 files changed, 30 insertions(+), 30 deletions(-)

diff --git a/include/hw/pci-host/designware.h b/include/hw/pci-host/designware.h
index e1952ad324..702777ab17 100644
--- a/include/hw/pci-host/designware.h
+++ b/include/hw/pci-host/designware.h
@@ -63,13 +63,6 @@ typedef struct DesignwarePCIEMSI {
 struct DesignwarePCIERoot {
     PCIBridge parent_obj;
 
-    uint32_t atu_viewport;
-
-#define DESIGNWARE_PCIE_VIEWPORT_OUTBOUND    0
-#define DESIGNWARE_PCIE_VIEWPORT_INBOUND     1
-#define DESIGNWARE_PCIE_NUM_VIEWPORTS        4
-
-    DesignwarePCIEViewport viewports[2][DESIGNWARE_PCIE_NUM_VIEWPORTS];
     DesignwarePCIEMSI msi;
     DesignwarePCIEHost *host;
 };
@@ -79,6 +72,12 @@ struct DesignwarePCIEHost {
 
     DesignwarePCIERoot root;
 
+    uint32_t atu_viewport;
+#define DESIGNWARE_PCIE_VIEWPORT_OUTBOUND    0
+#define DESIGNWARE_PCIE_VIEWPORT_INBOUND     1
+#define DESIGNWARE_PCIE_NUM_VIEWPORTS        4
+    DesignwarePCIEViewport viewports[2][DESIGNWARE_PCIE_NUM_VIEWPORTS];
+
     struct {
         AddressSpace address_space;
         MemoryRegion address_space_root;
diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
index d12a36b628..2ef17137e2 100644
--- a/hw/pci-host/designware.c
+++ b/hw/pci-host/designware.c
@@ -109,20 +109,21 @@ static void designware_pcie_root_update_msi_mapping(DesignwarePCIERoot *root)
 }
 
 static DesignwarePCIEViewport *
-designware_pcie_root_get_current_viewport(DesignwarePCIERoot *root)
+designware_pcie_host_get_current_viewport(DesignwarePCIEHost *host)
 {
-    const unsigned int idx = root->atu_viewport & 0xF;
+    const unsigned int idx = host->atu_viewport & 0xF;
     const unsigned int dir =
-        !!(root->atu_viewport & DESIGNWARE_PCIE_ATU_REGION_INBOUND);
-    return &root->viewports[dir][idx];
+        !!(host->atu_viewport & DESIGNWARE_PCIE_ATU_REGION_INBOUND);
+    return &host->viewports[dir][idx];
 }
 
 static uint32_t
 designware_pcie_root_config_read(PCIDevice *d, uint32_t address, int len)
 {
     DesignwarePCIERoot *root = DESIGNWARE_PCIE_ROOT(d);
+    DesignwarePCIEHost *host = root->host;
     DesignwarePCIEViewport *viewport =
-        designware_pcie_root_get_current_viewport(root);
+        designware_pcie_host_get_current_viewport(host);
 
     uint32_t val;
 
@@ -170,7 +171,7 @@ designware_pcie_root_config_read(PCIDevice *d, uint32_t address, int len)
         break;
 
     case DESIGNWARE_PCIE_ATU_VIEWPORT:
-        val = root->atu_viewport;
+        val = host->atu_viewport;
         break;
 
     case DESIGNWARE_PCIE_ATU_LOWER_BASE:
@@ -294,7 +295,7 @@ static void designware_pcie_root_config_write(PCIDevice *d, uint32_t address,
     DesignwarePCIERoot *root = DESIGNWARE_PCIE_ROOT(d);
     DesignwarePCIEHost *host = root->host;
     DesignwarePCIEViewport *viewport =
-        designware_pcie_root_get_current_viewport(root);
+        designware_pcie_host_get_current_viewport(host);
 
     switch (address) {
     case DESIGNWARE_PCIE_PORT_LINK_CONTROL:
@@ -332,7 +333,7 @@ static void designware_pcie_root_config_write(PCIDevice *d, uint32_t address,
         break;
 
     case DESIGNWARE_PCIE_ATU_VIEWPORT:
-        root->atu_viewport = val;
+        host->atu_viewport = val;
         break;
 
     case DESIGNWARE_PCIE_ATU_LOWER_BASE:
@@ -420,7 +421,7 @@ static void designware_pcie_root_realize(PCIDevice *dev, Error **errp)
         const char *direction;
         char *name;
 
-        viewport = &root->viewports[DESIGNWARE_PCIE_VIEWPORT_INBOUND][i];
+        viewport = &host->viewports[DESIGNWARE_PCIE_VIEWPORT_INBOUND][i];
         viewport->inbound = true;
         viewport->base    = 0x0000000000000000ULL;
         viewport->target  = 0x0000000000000000ULL;
@@ -443,7 +444,7 @@ static void designware_pcie_root_realize(PCIDevice *dev, Error **errp)
         memory_region_set_enabled(mem, false);
         g_free(name);
 
-        viewport = &root->viewports[DESIGNWARE_PCIE_VIEWPORT_OUTBOUND][i];
+        viewport = &host->viewports[DESIGNWARE_PCIE_VIEWPORT_OUTBOUND][i];
         viewport->host    = host;
         viewport->inbound = false;
         viewport->base    = 0x0000000000000000ULL;
@@ -490,7 +491,7 @@ static void designware_pcie_root_realize(PCIDevice *dev, Error **errp)
      * NOTE: This will not work correctly for the case when first
      * configured inbound window is window 0
      */
-    viewport = &root->viewports[DESIGNWARE_PCIE_VIEWPORT_INBOUND][0];
+    viewport = &host->viewports[DESIGNWARE_PCIE_VIEWPORT_INBOUND][0];
     viewport->cr[1] = DESIGNWARE_PCIE_ATU_ENABLE;
     designware_pcie_update_viewport(root, viewport);
 
@@ -563,18 +564,10 @@ static const VMStateDescription vmstate_designware_pcie_viewport = {
 
 static const VMStateDescription vmstate_designware_pcie_root = {
     .name = "designware-pcie-root",
-    .version_id = 1,
-    .minimum_version_id = 1,
+    .version_id = 2,
+    .minimum_version_id = 2,
     .fields = (VMStateField[]) {
         VMSTATE_PCI_DEVICE(parent_obj, PCIBridge),
-        VMSTATE_UINT32(atu_viewport, DesignwarePCIERoot),
-        VMSTATE_STRUCT_2DARRAY(viewports,
-                               DesignwarePCIERoot,
-                               2,
-                               DESIGNWARE_PCIE_NUM_VIEWPORTS,
-                               1,
-                               vmstate_designware_pcie_viewport,
-                               DesignwarePCIEViewport),
         VMSTATE_STRUCT(msi,
                        DesignwarePCIERoot,
                        1,
@@ -711,14 +704,22 @@ static void designware_pcie_host_realize(DeviceState *dev, Error **errp)
 
 static const VMStateDescription vmstate_designware_pcie_host = {
     .name = "designware-pcie-host",
-    .version_id = 1,
-    .minimum_version_id = 1,
+    .version_id = 2,
+    .minimum_version_id = 2,
     .fields = (VMStateField[]) {
         VMSTATE_STRUCT(root,
                        DesignwarePCIEHost,
                        1,
                        vmstate_designware_pcie_root,
                        DesignwarePCIERoot),
+        VMSTATE_UINT32(atu_viewport, DesignwarePCIEHost),
+        VMSTATE_STRUCT_2DARRAY(viewports,
+                               DesignwarePCIEHost,
+                               2,
+                               DESIGNWARE_PCIE_NUM_VIEWPORTS,
+                               1,
+                               vmstate_designware_pcie_viewport,
+                               DesignwarePCIEViewport),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
2.41.0

Re: [PATCH 6/8] hw/pci-host/designware: Move viewports from root func to host bridge

[Gustavo Romero]

Hi Phil,

On 10/12/23 9:18 AM, Philippe Mathieu-Daudé wrote:
> As mentioned in previous commit, the PCI root function is
> irrelevant for the ViewPorts. Move the fields to the host
> bridge state.
> 
> This is a migration compatibility break for the machines
> using the i.MX7 SoC (currently the mcimx7d-sabre machine).
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

I also see the ATU Viewport registers closer to the host
bridge, so from a modeling perspective I think that makes
sense, even if it's hard to tell that for sure when looking
at the DW IP core docs (but of course QEMU model doesn't
have to mimic any verilog code etc).

Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org>


Cheers,
Gustavo


> ---
>   include/hw/pci-host/designware.h | 13 ++++-----
>   hw/pci-host/designware.c         | 47 ++++++++++++++++----------------
>   2 files changed, 30 insertions(+), 30 deletions(-)
> 
> diff --git a/include/hw/pci-host/designware.h b/include/hw/pci-host/designware.h
> index e1952ad324..702777ab17 100644
> --- a/include/hw/pci-host/designware.h
> +++ b/include/hw/pci-host/designware.h
> @@ -63,13 +63,6 @@ typedef struct DesignwarePCIEMSI {
>   struct DesignwarePCIERoot {
>       PCIBridge parent_obj;
>   
> -    uint32_t atu_viewport;
> -
> -#define DESIGNWARE_PCIE_VIEWPORT_OUTBOUND    0
> -#define DESIGNWARE_PCIE_VIEWPORT_INBOUND     1
> -#define DESIGNWARE_PCIE_NUM_VIEWPORTS        4
> -
> -    DesignwarePCIEViewport viewports[2][DESIGNWARE_PCIE_NUM_VIEWPORTS];
>       DesignwarePCIEMSI msi;
>       DesignwarePCIEHost *host;
>   };
> @@ -79,6 +72,12 @@ struct DesignwarePCIEHost {
>   
>       DesignwarePCIERoot root;
>   
> +    uint32_t atu_viewport;
> +#define DESIGNWARE_PCIE_VIEWPORT_OUTBOUND    0
> +#define DESIGNWARE_PCIE_VIEWPORT_INBOUND     1
> +#define DESIGNWARE_PCIE_NUM_VIEWPORTS        4
> +    DesignwarePCIEViewport viewports[2][DESIGNWARE_PCIE_NUM_VIEWPORTS];
> +
>       struct {
>           AddressSpace address_space;
>           MemoryRegion address_space_root;
> diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
> index d12a36b628..2ef17137e2 100644
> --- a/hw/pci-host/designware.c
> +++ b/hw/pci-host/designware.c
> @@ -109,20 +109,21 @@ static void designware_pcie_root_update_msi_mapping(DesignwarePCIERoot *root)
>   }
>   
>   static DesignwarePCIEViewport *
> -designware_pcie_root_get_current_viewport(DesignwarePCIERoot *root)
> +designware_pcie_host_get_current_viewport(DesignwarePCIEHost *host)
>   {
> -    const unsigned int idx = root->atu_viewport & 0xF;
> +    const unsigned int idx = host->atu_viewport & 0xF;
>       const unsigned int dir =
> -        !!(root->atu_viewport & DESIGNWARE_PCIE_ATU_REGION_INBOUND);
> -    return &root->viewports[dir][idx];
> +        !!(host->atu_viewport & DESIGNWARE_PCIE_ATU_REGION_INBOUND);
> +    return &host->viewports[dir][idx];
>   }
>   
>   static uint32_t
>   designware_pcie_root_config_read(PCIDevice *d, uint32_t address, int len)
>   {
>       DesignwarePCIERoot *root = DESIGNWARE_PCIE_ROOT(d);
> +    DesignwarePCIEHost *host = root->host;
>       DesignwarePCIEViewport *viewport =
> -        designware_pcie_root_get_current_viewport(root);
> +        designware_pcie_host_get_current_viewport(host);
>   
>       uint32_t val;
>   
> @@ -170,7 +171,7 @@ designware_pcie_root_config_read(PCIDevice *d, uint32_t address, int len)
>           break;
>   
>       case DESIGNWARE_PCIE_ATU_VIEWPORT:
> -        val = root->atu_viewport;
> +        val = host->atu_viewport;
>           break;
>   
>       case DESIGNWARE_PCIE_ATU_LOWER_BASE:
> @@ -294,7 +295,7 @@ static void designware_pcie_root_config_write(PCIDevice *d, uint32_t address,
>       DesignwarePCIERoot *root = DESIGNWARE_PCIE_ROOT(d);
>       DesignwarePCIEHost *host = root->host;
>       DesignwarePCIEViewport *viewport =
> -        designware_pcie_root_get_current_viewport(root);
> +        designware_pcie_host_get_current_viewport(host);
>   
>       switch (address) {
>       case DESIGNWARE_PCIE_PORT_LINK_CONTROL:
> @@ -332,7 +333,7 @@ static void designware_pcie_root_config_write(PCIDevice *d, uint32_t address,
>           break;
>   
>       case DESIGNWARE_PCIE_ATU_VIEWPORT:
> -        root->atu_viewport = val;
> +        host->atu_viewport = val;
>           break;
>   
>       case DESIGNWARE_PCIE_ATU_LOWER_BASE:
> @@ -420,7 +421,7 @@ static void designware_pcie_root_realize(PCIDevice *dev, Error **errp)
>           const char *direction;
>           char *name;
>   
> -        viewport = &root->viewports[DESIGNWARE_PCIE_VIEWPORT_INBOUND][i];
> +        viewport = &host->viewports[DESIGNWARE_PCIE_VIEWPORT_INBOUND][i];
>           viewport->inbound = true;
>           viewport->base    = 0x0000000000000000ULL;
>           viewport->target  = 0x0000000000000000ULL;
> @@ -443,7 +444,7 @@ static void designware_pcie_root_realize(PCIDevice *dev, Error **errp)
>           memory_region_set_enabled(mem, false);
>           g_free(name);
>   
> -        viewport = &root->viewports[DESIGNWARE_PCIE_VIEWPORT_OUTBOUND][i];
> +        viewport = &host->viewports[DESIGNWARE_PCIE_VIEWPORT_OUTBOUND][i];
>           viewport->host    = host;
>           viewport->inbound = false;
>           viewport->base    = 0x0000000000000000ULL;
> @@ -490,7 +491,7 @@ static void designware_pcie_root_realize(PCIDevice *dev, Error **errp)
>        * NOTE: This will not work correctly for the case when first
>        * configured inbound window is window 0
>        */
> -    viewport = &root->viewports[DESIGNWARE_PCIE_VIEWPORT_INBOUND][0];
> +    viewport = &host->viewports[DESIGNWARE_PCIE_VIEWPORT_INBOUND][0];
>       viewport->cr[1] = DESIGNWARE_PCIE_ATU_ENABLE;
>       designware_pcie_update_viewport(root, viewport);
>   
> @@ -563,18 +564,10 @@ static const VMStateDescription vmstate_designware_pcie_viewport = {
>   
>   static const VMStateDescription vmstate_designware_pcie_root = {
>       .name = "designware-pcie-root",
> -    .version_id = 1,
> -    .minimum_version_id = 1,
> +    .version_id = 2,
> +    .minimum_version_id = 2,
>       .fields = (VMStateField[]) {
>           VMSTATE_PCI_DEVICE(parent_obj, PCIBridge),
> -        VMSTATE_UINT32(atu_viewport, DesignwarePCIERoot),
> -        VMSTATE_STRUCT_2DARRAY(viewports,
> -                               DesignwarePCIERoot,
> -                               2,
> -                               DESIGNWARE_PCIE_NUM_VIEWPORTS,
> -                               1,
> -                               vmstate_designware_pcie_viewport,
> -                               DesignwarePCIEViewport),
>           VMSTATE_STRUCT(msi,
>                          DesignwarePCIERoot,
>                          1,
> @@ -711,14 +704,22 @@ static void designware_pcie_host_realize(DeviceState *dev, Error **errp)
>   
>   static const VMStateDescription vmstate_designware_pcie_host = {
>       .name = "designware-pcie-host",
> -    .version_id = 1,
> -    .minimum_version_id = 1,
> +    .version_id = 2,
> +    .minimum_version_id = 2,
>       .fields = (VMStateField[]) {
>           VMSTATE_STRUCT(root,
>                          DesignwarePCIEHost,
>                          1,
>                          vmstate_designware_pcie_root,
>                          DesignwarePCIERoot),
> +        VMSTATE_UINT32(atu_viewport, DesignwarePCIEHost),
> +        VMSTATE_STRUCT_2DARRAY(viewports,
> +                               DesignwarePCIEHost,
> +                               2,
> +                               DESIGNWARE_PCIE_NUM_VIEWPORTS,
> +                               1,
> +                               vmstate_designware_pcie_viewport,
> +                               DesignwarePCIEViewport),
>           VMSTATE_END_OF_LIST()
>       }
>   };
> 

[PATCH 7/8] hw/pci-host/designware: Move MSI registers from root func to host bridge

[Philippe Mathieu-Daudé]

The MSI registers belong the the host bridge. Move the
DesignwarePCIEMSI field to the host bridge state.

This is a migration compatibility break for the machines
using the i.MX7 SoC (currently the mcimx7d-sabre machine).

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/pci-host/designware.h |  2 +-
 hw/pci-host/designware.c         | 79 ++++++++++++++++----------------
 2 files changed, 40 insertions(+), 41 deletions(-)

diff --git a/include/hw/pci-host/designware.h b/include/hw/pci-host/designware.h
index 702777ab17..fe8e8a9f24 100644
--- a/include/hw/pci-host/designware.h
+++ b/include/hw/pci-host/designware.h
@@ -63,7 +63,6 @@ typedef struct DesignwarePCIEMSI {
 struct DesignwarePCIERoot {
     PCIBridge parent_obj;
 
-    DesignwarePCIEMSI msi;
     DesignwarePCIEHost *host;
 };
 
@@ -71,6 +70,7 @@ struct DesignwarePCIEHost {
     PCIHostState parent_obj;
 
     DesignwarePCIERoot root;
+    DesignwarePCIEMSI msi;
 
     uint32_t atu_viewport;
 #define DESIGNWARE_PCIE_VIEWPORT_OUTBOUND    0
diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
index 2ef17137e2..6cb8655a75 100644
--- a/hw/pci-host/designware.c
+++ b/hw/pci-host/designware.c
@@ -57,7 +57,7 @@
 
 #define DESIGNWARE_PCIE_IRQ_MSI                    3
 
-static uint64_t designware_pcie_root_msi_read(void *opaque, hwaddr addr,
+static uint64_t designware_pcie_host_msi_read(void *opaque, hwaddr addr,
                                               unsigned size)
 {
     /*
@@ -74,22 +74,21 @@ static uint64_t designware_pcie_root_msi_read(void *opaque, hwaddr addr,
     return 0;
 }
 
-static void designware_pcie_root_msi_write(void *opaque, hwaddr addr,
+static void designware_pcie_host_msi_write(void *opaque, hwaddr addr,
                                            uint64_t val, unsigned len)
 {
-    DesignwarePCIERoot *root = DESIGNWARE_PCIE_ROOT(opaque);
-    DesignwarePCIEHost *host = root->host;
+    DesignwarePCIEHost *host = opaque;
 
-    root->msi.intr[0].status |= BIT(val) & root->msi.intr[0].enable;
+    host->msi.intr[0].status |= BIT(val) & host->msi.intr[0].enable;
 
-    if (root->msi.intr[0].status & ~root->msi.intr[0].mask) {
+    if (host->msi.intr[0].status & ~host->msi.intr[0].mask) {
         qemu_set_irq(host->pci.irqs[DESIGNWARE_PCIE_IRQ_MSI], 1);
     }
 }
 
 static const MemoryRegionOps designware_pci_host_msi_ops = {
-    .read = designware_pcie_root_msi_read,
-    .write = designware_pcie_root_msi_write,
+    .read = designware_pcie_host_msi_read,
+    .write = designware_pcie_host_msi_write,
     .endianness = DEVICE_LITTLE_ENDIAN,
     .valid = {
         .min_access_size = 4,
@@ -97,12 +96,12 @@ static const MemoryRegionOps designware_pci_host_msi_ops = {
     },
 };
 
-static void designware_pcie_root_update_msi_mapping(DesignwarePCIERoot *root)
+static void designware_pcie_host_update_msi_mapping(DesignwarePCIEHost *host)
 
 {
-    MemoryRegion *mem   = &root->msi.iomem;
-    const uint64_t base = root->msi.base;
-    const bool enable   = root->msi.intr[0].enable;
+    MemoryRegion *mem   = &host->msi.iomem;
+    const uint64_t base = host->msi.base;
+    const bool enable   = host->msi.intr[0].enable;
 
     memory_region_set_address(mem, base);
     memory_region_set_enabled(mem, enable);
@@ -147,23 +146,23 @@ designware_pcie_root_config_read(PCIDevice *d, uint32_t address, int len)
         break;
 
     case DESIGNWARE_PCIE_MSI_ADDR_LO:
-        val = root->msi.base;
+        val = host->msi.base;
         break;
 
     case DESIGNWARE_PCIE_MSI_ADDR_HI:
-        val = root->msi.base >> 32;
+        val = host->msi.base >> 32;
         break;
 
     case DESIGNWARE_PCIE_MSI_INTR0_ENABLE:
-        val = root->msi.intr[0].enable;
+        val = host->msi.intr[0].enable;
         break;
 
     case DESIGNWARE_PCIE_MSI_INTR0_MASK:
-        val = root->msi.intr[0].mask;
+        val = host->msi.intr[0].mask;
         break;
 
     case DESIGNWARE_PCIE_MSI_INTR0_STATUS:
-        val = root->msi.intr[0].status;
+        val = host->msi.intr[0].status;
         break;
 
     case DESIGNWARE_PCIE_PHY_DEBUG_R1:
@@ -305,29 +304,29 @@ static void designware_pcie_root_config_write(PCIDevice *d, uint32_t address,
         break;
 
     case DESIGNWARE_PCIE_MSI_ADDR_LO:
-        root->msi.base &= 0xFFFFFFFF00000000ULL;
-        root->msi.base |= val;
-        designware_pcie_root_update_msi_mapping(root);
+        host->msi.base &= 0xFFFFFFFF00000000ULL;
+        host->msi.base |= val;
+        designware_pcie_host_update_msi_mapping(host);
         break;
 
     case DESIGNWARE_PCIE_MSI_ADDR_HI:
-        root->msi.base &= 0x00000000FFFFFFFFULL;
-        root->msi.base |= (uint64_t)val << 32;
-        designware_pcie_root_update_msi_mapping(root);
+        host->msi.base &= 0x00000000FFFFFFFFULL;
+        host->msi.base |= (uint64_t)val << 32;
+        designware_pcie_host_update_msi_mapping(host);
         break;
 
     case DESIGNWARE_PCIE_MSI_INTR0_ENABLE:
-        root->msi.intr[0].enable = val;
-        designware_pcie_root_update_msi_mapping(root);
+        host->msi.intr[0].enable = val;
+        designware_pcie_host_update_msi_mapping(host);
         break;
 
     case DESIGNWARE_PCIE_MSI_INTR0_MASK:
-        root->msi.intr[0].mask = val;
+        host->msi.intr[0].mask = val;
         break;
 
     case DESIGNWARE_PCIE_MSI_INTR0_STATUS:
-        root->msi.intr[0].status ^= val;
-        if (!root->msi.intr[0].status) {
+        host->msi.intr[0].status ^= val;
+        if (!host->msi.intr[0].status) {
             qemu_set_irq(host->pci.irqs[DESIGNWARE_PCIE_IRQ_MSI], 0);
         }
         break;
@@ -495,7 +494,7 @@ static void designware_pcie_root_realize(PCIDevice *dev, Error **errp)
     viewport->cr[1] = DESIGNWARE_PCIE_ATU_ENABLE;
     designware_pcie_update_viewport(root, viewport);
 
-    memory_region_init_io(&root->msi.iomem, OBJECT(root),
+    memory_region_init_io(&host->msi.iomem, OBJECT(root),
                           &designware_pci_host_msi_ops,
                           root, "pcie-msi", 0x4);
     /*
@@ -504,8 +503,8 @@ static void designware_pcie_root_realize(PCIDevice *dev, Error **errp)
      * in designware_pcie_root_update_msi_mapping() as a part of
      * initialization done by guest OS
      */
-    memory_region_add_subregion(address_space, dummy_offset, &root->msi.iomem);
-    memory_region_set_enabled(&root->msi.iomem, false);
+    memory_region_add_subregion(address_space, dummy_offset, &host->msi.iomem);
+    memory_region_set_enabled(&host->msi.iomem, false);
 }
 
 static void designware_pcie_set_irq(void *opaque, int irq_num, int level)
@@ -564,15 +563,10 @@ static const VMStateDescription vmstate_designware_pcie_viewport = {
 
 static const VMStateDescription vmstate_designware_pcie_root = {
     .name = "designware-pcie-root",
-    .version_id = 2,
-    .minimum_version_id = 2,
+    .version_id = 3,
+    .minimum_version_id = 3,
     .fields = (VMStateField[]) {
         VMSTATE_PCI_DEVICE(parent_obj, PCIBridge),
-        VMSTATE_STRUCT(msi,
-                       DesignwarePCIERoot,
-                       1,
-                       vmstate_designware_pcie_msi,
-                       DesignwarePCIEMSI),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -704,8 +698,8 @@ static void designware_pcie_host_realize(DeviceState *dev, Error **errp)
 
 static const VMStateDescription vmstate_designware_pcie_host = {
     .name = "designware-pcie-host",
-    .version_id = 2,
-    .minimum_version_id = 2,
+    .version_id = 3,
+    .minimum_version_id = 3,
     .fields = (VMStateField[]) {
         VMSTATE_STRUCT(root,
                        DesignwarePCIEHost,
@@ -720,6 +714,11 @@ static const VMStateDescription vmstate_designware_pcie_host = {
                                1,
                                vmstate_designware_pcie_viewport,
                                DesignwarePCIEViewport),
+        VMSTATE_STRUCT(msi,
+                       DesignwarePCIEHost,
+                       1,
+                       vmstate_designware_pcie_msi,
+                       DesignwarePCIEMSI),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
2.41.0

Re: [PATCH 7/8] hw/pci-host/designware: Move MSI registers from root func to host bridge

[Gustavo Romero]

Hi Phil

On 10/12/23 9:18 AM, Philippe Mathieu-Daudé wrote:
> The MSI registers belong the the host bridge. Move the
> DesignwarePCIEMSI field to the host bridge state.

I would say MSI registers are more tied to the PCI/PCIe network
side than to the host side. The MSI registers control if an
interrupt will be delivered to a host by looking at MSI data
payload to determine which end point sent the MSI packet and
which interrupt vector it corresponds to.

Why do you say the belong to the host bridge?

In 0/8 you mentioned you "noticed few discrepancies due to the
fact that host bridge pieces were managed by the root function",
is this patch addressing some of these discrepancies? What are
them exactly?

This patch also needs a rebase onto master.


Cheers,
Gustavo


> This is a migration compatibility break for the machines
> using the i.MX7 SoC (currently the mcimx7d-sabre machine).
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/hw/pci-host/designware.h |  2 +-
>   hw/pci-host/designware.c         | 79 ++++++++++++++++----------------
>   2 files changed, 40 insertions(+), 41 deletions(-)
> 
> diff --git a/include/hw/pci-host/designware.h b/include/hw/pci-host/designware.h
> index 702777ab17..fe8e8a9f24 100644
> --- a/include/hw/pci-host/designware.h
> +++ b/include/hw/pci-host/designware.h
> @@ -63,7 +63,6 @@ typedef struct DesignwarePCIEMSI {
>   struct DesignwarePCIERoot {
>       PCIBridge parent_obj;
>   
> -    DesignwarePCIEMSI msi;
>       DesignwarePCIEHost *host;
>   };
>   
> @@ -71,6 +70,7 @@ struct DesignwarePCIEHost {
>       PCIHostState parent_obj;
>   
>       DesignwarePCIERoot root;
> +    DesignwarePCIEMSI msi;
>   
>       uint32_t atu_viewport;
>   #define DESIGNWARE_PCIE_VIEWPORT_OUTBOUND    0
> diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
> index 2ef17137e2..6cb8655a75 100644
> --- a/hw/pci-host/designware.c
> +++ b/hw/pci-host/designware.c
> @@ -57,7 +57,7 @@
>   
>   #define DESIGNWARE_PCIE_IRQ_MSI                    3
>   
> -static uint64_t designware_pcie_root_msi_read(void *opaque, hwaddr addr,
> +static uint64_t designware_pcie_host_msi_read(void *opaque, hwaddr addr,
>                                                 unsigned size)
>   {
>       /*
> @@ -74,22 +74,21 @@ static uint64_t designware_pcie_root_msi_read(void *opaque, hwaddr addr,
>       return 0;
>   }
>   
> -static void designware_pcie_root_msi_write(void *opaque, hwaddr addr,
> +static void designware_pcie_host_msi_write(void *opaque, hwaddr addr,
>                                              uint64_t val, unsigned len)
>   {
> -    DesignwarePCIERoot *root = DESIGNWARE_PCIE_ROOT(opaque);
> -    DesignwarePCIEHost *host = root->host;
> +    DesignwarePCIEHost *host = opaque;
>   
> -    root->msi.intr[0].status |= BIT(val) & root->msi.intr[0].enable;
> +    host->msi.intr[0].status |= BIT(val) & host->msi.intr[0].enable;
>   
> -    if (root->msi.intr[0].status & ~root->msi.intr[0].mask) {
> +    if (host->msi.intr[0].status & ~host->msi.intr[0].mask) {
>           qemu_set_irq(host->pci.irqs[DESIGNWARE_PCIE_IRQ_MSI], 1);
>       }
>   }
>   
>   static const MemoryRegionOps designware_pci_host_msi_ops = {
> -    .read = designware_pcie_root_msi_read,
> -    .write = designware_pcie_root_msi_write,
> +    .read = designware_pcie_host_msi_read,
> +    .write = designware_pcie_host_msi_write,
>       .endianness = DEVICE_LITTLE_ENDIAN,
>       .valid = {
>           .min_access_size = 4,
> @@ -97,12 +96,12 @@ static const MemoryRegionOps designware_pci_host_msi_ops = {
>       },
>   };
>   
> -static void designware_pcie_root_update_msi_mapping(DesignwarePCIERoot *root)
> +static void designware_pcie_host_update_msi_mapping(DesignwarePCIEHost *host)
>   
>   {
> -    MemoryRegion *mem   = &root->msi.iomem;
> -    const uint64_t base = root->msi.base;
> -    const bool enable   = root->msi.intr[0].enable;
> +    MemoryRegion *mem   = &host->msi.iomem;
> +    const uint64_t base = host->msi.base;
> +    const bool enable   = host->msi.intr[0].enable;
>   
>       memory_region_set_address(mem, base);
>       memory_region_set_enabled(mem, enable);
> @@ -147,23 +146,23 @@ designware_pcie_root_config_read(PCIDevice *d, uint32_t address, int len)
>           break;
>   
>       case DESIGNWARE_PCIE_MSI_ADDR_LO:
> -        val = root->msi.base;
> +        val = host->msi.base;
>           break;
>   
>       case DESIGNWARE_PCIE_MSI_ADDR_HI:
> -        val = root->msi.base >> 32;
> +        val = host->msi.base >> 32;
>           break;
>   
>       case DESIGNWARE_PCIE_MSI_INTR0_ENABLE:
> -        val = root->msi.intr[0].enable;
> +        val = host->msi.intr[0].enable;
>           break;
>   
>       case DESIGNWARE_PCIE_MSI_INTR0_MASK:
> -        val = root->msi.intr[0].mask;
> +        val = host->msi.intr[0].mask;
>           break;
>   
>       case DESIGNWARE_PCIE_MSI_INTR0_STATUS:
> -        val = root->msi.intr[0].status;
> +        val = host->msi.intr[0].status;
>           break;
>   
>       case DESIGNWARE_PCIE_PHY_DEBUG_R1:
> @@ -305,29 +304,29 @@ static void designware_pcie_root_config_write(PCIDevice *d, uint32_t address,
>           break;
>   
>       case DESIGNWARE_PCIE_MSI_ADDR_LO:
> -        root->msi.base &= 0xFFFFFFFF00000000ULL;
> -        root->msi.base |= val;
> -        designware_pcie_root_update_msi_mapping(root);
> +        host->msi.base &= 0xFFFFFFFF00000000ULL;
> +        host->msi.base |= val;
> +        designware_pcie_host_update_msi_mapping(host);
>           break;
>   
>       case DESIGNWARE_PCIE_MSI_ADDR_HI:
> -        root->msi.base &= 0x00000000FFFFFFFFULL;
> -        root->msi.base |= (uint64_t)val << 32;
> -        designware_pcie_root_update_msi_mapping(root);
> +        host->msi.base &= 0x00000000FFFFFFFFULL;
> +        host->msi.base |= (uint64_t)val << 32;
> +        designware_pcie_host_update_msi_mapping(host);
>           break;
>   
>       case DESIGNWARE_PCIE_MSI_INTR0_ENABLE:
> -        root->msi.intr[0].enable = val;
> -        designware_pcie_root_update_msi_mapping(root);
> +        host->msi.intr[0].enable = val;
> +        designware_pcie_host_update_msi_mapping(host);
>           break;
>   
>       case DESIGNWARE_PCIE_MSI_INTR0_MASK:
> -        root->msi.intr[0].mask = val;
> +        host->msi.intr[0].mask = val;
>           break;
>   
>       case DESIGNWARE_PCIE_MSI_INTR0_STATUS:
> -        root->msi.intr[0].status ^= val;
> -        if (!root->msi.intr[0].status) {
> +        host->msi.intr[0].status ^= val;
> +        if (!host->msi.intr[0].status) {
>               qemu_set_irq(host->pci.irqs[DESIGNWARE_PCIE_IRQ_MSI], 0);
>           }
>           break;
> @@ -495,7 +494,7 @@ static void designware_pcie_root_realize(PCIDevice *dev, Error **errp)
>       viewport->cr[1] = DESIGNWARE_PCIE_ATU_ENABLE;
>       designware_pcie_update_viewport(root, viewport);
>   
> -    memory_region_init_io(&root->msi.iomem, OBJECT(root),
> +    memory_region_init_io(&host->msi.iomem, OBJECT(root),
>                             &designware_pci_host_msi_ops,
>                             root, "pcie-msi", 0x4);
>       /*
> @@ -504,8 +503,8 @@ static void designware_pcie_root_realize(PCIDevice *dev, Error **errp)
>        * in designware_pcie_root_update_msi_mapping() as a part of
>        * initialization done by guest OS
>        */
> -    memory_region_add_subregion(address_space, dummy_offset, &root->msi.iomem);
> -    memory_region_set_enabled(&root->msi.iomem, false);
> +    memory_region_add_subregion(address_space, dummy_offset, &host->msi.iomem);
> +    memory_region_set_enabled(&host->msi.iomem, false);
>   }
>   
>   static void designware_pcie_set_irq(void *opaque, int irq_num, int level)
> @@ -564,15 +563,10 @@ static const VMStateDescription vmstate_designware_pcie_viewport = {
>   
>   static const VMStateDescription vmstate_designware_pcie_root = {
>       .name = "designware-pcie-root",
> -    .version_id = 2,
> -    .minimum_version_id = 2,
> +    .version_id = 3,
> +    .minimum_version_id = 3,
>       .fields = (VMStateField[]) {
>           VMSTATE_PCI_DEVICE(parent_obj, PCIBridge),
> -        VMSTATE_STRUCT(msi,
> -                       DesignwarePCIERoot,
> -                       1,
> -                       vmstate_designware_pcie_msi,
> -                       DesignwarePCIEMSI),
>           VMSTATE_END_OF_LIST()
>       }
>   };
> @@ -704,8 +698,8 @@ static void designware_pcie_host_realize(DeviceState *dev, Error **errp)
>   
>   static const VMStateDescription vmstate_designware_pcie_host = {
>       .name = "designware-pcie-host",
> -    .version_id = 2,
> -    .minimum_version_id = 2,
> +    .version_id = 3,
> +    .minimum_version_id = 3,
>       .fields = (VMStateField[]) {
>           VMSTATE_STRUCT(root,
>                          DesignwarePCIEHost,
> @@ -720,6 +714,11 @@ static const VMStateDescription vmstate_designware_pcie_host = {
>                                  1,
>                                  vmstate_designware_pcie_viewport,
>                                  DesignwarePCIEViewport),
> +        VMSTATE_STRUCT(msi,
> +                       DesignwarePCIEHost,
> +                       1,
> +                       vmstate_designware_pcie_msi,
> +                       DesignwarePCIEMSI),
>           VMSTATE_END_OF_LIST()
>       }
>   };
> 

[PATCH 8/8] hw/pci-host/designware: Create ViewPorts during host bridge realization

[Philippe Mathieu-Daudé]

ViewPorts are managed by the host bridge part, so create them
when the host bridge is realized. The host bridge become the
owner of the memory regions.

The PCI root function realize() method now only contains PCI
specific code.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/pci-host/designware.c | 207 +++++++++++++++++++--------------------
 1 file changed, 102 insertions(+), 105 deletions(-)

diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
index 6cb8655a75..e5dc9b4b8d 100644
--- a/hw/pci-host/designware.c
+++ b/hw/pci-host/designware.c
@@ -384,22 +384,10 @@ static char *designware_pcie_viewport_name(const char *direction,
 static void designware_pcie_root_realize(PCIDevice *dev, Error **errp)
 {
     DesignwarePCIERoot *root = DESIGNWARE_PCIE_ROOT(dev);
-    DesignwarePCIEHost *host = DESIGNWARE_PCIE_HOST(
-                                    qdev_get_parent_bus(DEVICE(dev))->parent);
-    MemoryRegion *host_mem = get_system_memory();
-    MemoryRegion *address_space = &host->pci.memory;
     PCIBridge *br = PCI_BRIDGE(dev);
-    DesignwarePCIEViewport *viewport;
-    /*
-     * Dummy values used for initial configuration of MemoryRegions
-     * that belong to a given viewport
-     */
-    const hwaddr dummy_offset = 0;
-    const uint64_t dummy_size = 4;
-    size_t i;
 
     br->bus_name  = "dw-pcie";
-    root->host = host;
+    root->host = DESIGNWARE_PCIE_HOST(qdev_get_parent_bus(DEVICE(dev))->parent);
 
     pci_set_word(dev->config + PCI_COMMAND,
                  PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER);
@@ -414,97 +402,6 @@ static void designware_pcie_root_realize(PCIDevice *dev, Error **errp)
 
     msi_nonbroken = true;
     msi_init(dev, 0x50, 32, true, true, &error_fatal);
-
-    for (i = 0; i < DESIGNWARE_PCIE_NUM_VIEWPORTS; i++) {
-        MemoryRegion *source, *destination, *mem;
-        const char *direction;
-        char *name;
-
-        viewport = &host->viewports[DESIGNWARE_PCIE_VIEWPORT_INBOUND][i];
-        viewport->inbound = true;
-        viewport->base    = 0x0000000000000000ULL;
-        viewport->target  = 0x0000000000000000ULL;
-        viewport->limit   = UINT32_MAX;
-        viewport->cr[0]   = DESIGNWARE_PCIE_ATU_TYPE_MEM;
-
-        source      = &host->pci.address_space_root;
-        destination = host_mem;
-        direction   = "Inbound";
-
-        /*
-         * Configure MemoryRegion implementing PCI -> CPU memory
-         * access
-         */
-        mem  = &viewport->mem;
-        name = designware_pcie_viewport_name(direction, i, "MEM");
-        memory_region_init_alias(mem, OBJECT(root), name, destination,
-                                 dummy_offset, dummy_size);
-        memory_region_add_subregion_overlap(source, dummy_offset, mem, -1);
-        memory_region_set_enabled(mem, false);
-        g_free(name);
-
-        viewport = &host->viewports[DESIGNWARE_PCIE_VIEWPORT_OUTBOUND][i];
-        viewport->host    = host;
-        viewport->inbound = false;
-        viewport->base    = 0x0000000000000000ULL;
-        viewport->target  = 0x0000000000000000ULL;
-        viewport->limit   = UINT32_MAX;
-        viewport->cr[0]   = DESIGNWARE_PCIE_ATU_TYPE_MEM;
-
-        destination = &host->pci.memory;
-        direction   = "Outbound";
-        source      = host_mem;
-
-        /*
-         * Configure MemoryRegion implementing CPU -> PCI memory
-         * access
-         */
-        mem  = &viewport->mem;
-        name = designware_pcie_viewport_name(direction, i, "MEM");
-        memory_region_init_alias(mem, OBJECT(root), name, destination,
-                                 dummy_offset, dummy_size);
-        memory_region_add_subregion(source, dummy_offset, mem);
-        memory_region_set_enabled(mem, false);
-        g_free(name);
-
-        /*
-         * Configure MemoryRegion implementing access to configuration
-         * space
-         */
-        mem  = &viewport->cfg;
-        name = designware_pcie_viewport_name(direction, i, "CFG");
-        memory_region_init_io(&viewport->cfg, OBJECT(root),
-                              &designware_pci_host_conf_ops,
-                              viewport, name, dummy_size);
-        memory_region_add_subregion(source, dummy_offset, mem);
-        memory_region_set_enabled(mem, false);
-        g_free(name);
-    }
-
-    /*
-     * If no inbound iATU windows are configured, HW defaults to
-     * letting inbound TLPs to pass in. We emulate that by explicitly
-     * configuring first inbound window to cover all of target's
-     * address space.
-     *
-     * NOTE: This will not work correctly for the case when first
-     * configured inbound window is window 0
-     */
-    viewport = &host->viewports[DESIGNWARE_PCIE_VIEWPORT_INBOUND][0];
-    viewport->cr[1] = DESIGNWARE_PCIE_ATU_ENABLE;
-    designware_pcie_update_viewport(root, viewport);
-
-    memory_region_init_io(&host->msi.iomem, OBJECT(root),
-                          &designware_pci_host_msi_ops,
-                          root, "pcie-msi", 0x4);
-    /*
-     * We initially place MSI interrupt I/O region at address 0 and
-     * disable it. It'll be later moved to correct offset and enabled
-     * in designware_pcie_root_update_msi_mapping() as a part of
-     * initialization done by guest OS
-     */
-    memory_region_add_subregion(address_space, dummy_offset, &host->msi.iomem);
-    memory_region_set_enabled(&host->msi.iomem, false);
 }
 
 static void designware_pcie_set_irq(void *opaque, int irq_num, int level)
@@ -590,7 +487,7 @@ static void designware_pcie_root_class_init(ObjectClass *klass, void *data)
     dc->reset = pci_bridge_reset;
     /*
      * PCI-facing part of the host bridge, not usable without the
-     * host-facing part, which can't be device_add'ed, yet.
+     * host-facing part.
      */
     dc->user_creatable = false;
     dc->vmsd = &vmstate_designware_pcie_root;
@@ -650,8 +547,17 @@ static void designware_pcie_host_realize(DeviceState *dev, Error **errp)
     PCIHostState *pci = PCI_HOST_BRIDGE(dev);
     DesignwarePCIEHost *s = DESIGNWARE_PCIE_HOST(dev);
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+    MemoryRegion *host_mem = get_system_memory();
+    DesignwarePCIEViewport *viewport;
     size_t i;
 
+    /*
+     * Dummy values used for initial configuration of MemoryRegions
+     * that belong to a given viewport
+     */
+    const hwaddr dummy_offset = 0;
+    const uint64_t dummy_size = 4;
+
     for (i = 0; i < ARRAY_SIZE(s->pci.irqs); i++) {
         sysbus_init_irq(sbd, &s->pci.irqs[i]);
     }
@@ -694,6 +600,97 @@ static void designware_pcie_host_realize(DeviceState *dev, Error **errp)
     qdev_prop_set_int32(DEVICE(&s->root), "addr", PCI_DEVFN(0, 0));
     qdev_prop_set_bit(DEVICE(&s->root), "multifunction", false);
     qdev_realize(DEVICE(&s->root), BUS(pci->bus), &error_fatal);
+
+    memory_region_init_io(&s->msi.iomem, OBJECT(s),
+                          &designware_pci_host_msi_ops,
+                          s, "pcie-msi", 0x4);
+    /*
+     * We initially place MSI interrupt I/O region at address 0 and
+     * disable it. It'll be later moved to correct offset and enabled
+     * in designware_pcie_host_update_msi_mapping() as a part of
+     * initialization done by guest OS
+     */
+    memory_region_add_subregion(&s->pci.memory, dummy_offset, &s->msi.iomem);
+    memory_region_set_enabled(&s->msi.iomem, false);
+
+    for (i = 0; i < DESIGNWARE_PCIE_NUM_VIEWPORTS; i++) {
+        MemoryRegion *source, *destination, *mem;
+        const char *direction;
+        char *name;
+
+        viewport = &s->viewports[DESIGNWARE_PCIE_VIEWPORT_INBOUND][i];
+        viewport->inbound = true;
+        viewport->base    = 0x0000000000000000ULL;
+        viewport->target  = 0x0000000000000000ULL;
+        viewport->limit   = UINT32_MAX;
+        viewport->cr[0]   = DESIGNWARE_PCIE_ATU_TYPE_MEM;
+
+        source      = &s->pci.address_space_root;
+        destination = host_mem;
+        direction   = "Inbound";
+
+        /*
+         * Configure MemoryRegion implementing PCI -> CPU memory
+         * access
+         */
+        mem  = &viewport->mem;
+        name = designware_pcie_viewport_name(direction, i, "MEM");
+        memory_region_init_alias(mem, OBJECT(s), name, destination,
+                                 dummy_offset, dummy_size);
+        memory_region_add_subregion_overlap(source, dummy_offset, mem, -1);
+        memory_region_set_enabled(mem, false);
+        g_free(name);
+
+        viewport = &s->viewports[DESIGNWARE_PCIE_VIEWPORT_OUTBOUND][i];
+        viewport->host    = s;
+        viewport->inbound = false;
+        viewport->base    = 0x0000000000000000ULL;
+        viewport->target  = 0x0000000000000000ULL;
+        viewport->limit   = UINT32_MAX;
+        viewport->cr[0]   = DESIGNWARE_PCIE_ATU_TYPE_MEM;
+
+        destination = &s->pci.memory;
+        direction   = "Outbound";
+        source      = host_mem;
+
+        /*
+         * Configure MemoryRegion implementing CPU -> PCI memory
+         * access
+         */
+        mem  = &viewport->mem;
+        name = designware_pcie_viewport_name(direction, i, "MEM");
+        memory_region_init_alias(mem, OBJECT(s), name, destination,
+                                 dummy_offset, dummy_size);
+        memory_region_add_subregion(source, dummy_offset, mem);
+        memory_region_set_enabled(mem, false);
+        g_free(name);
+
+        /*
+         * Configure MemoryRegion implementing access to configuration
+         * space
+         */
+        mem  = &viewport->cfg;
+        name = designware_pcie_viewport_name(direction, i, "CFG");
+        memory_region_init_io(&viewport->cfg, OBJECT(s),
+                              &designware_pci_host_conf_ops,
+                              viewport, name, dummy_size);
+        memory_region_add_subregion(source, dummy_offset, mem);
+        memory_region_set_enabled(mem, false);
+        g_free(name);
+    }
+
+    /*
+     * If no inbound iATU windows are configured, HW defaults to
+     * letting inbound TLPs to pass in. We emulate that by explicitly
+     * configuring first inbound window to cover all of target's
+     * address space.
+     *
+     * NOTE: This will not work correctly for the case when first
+     * configured inbound window is window 0
+     */
+    viewport = &s->viewports[DESIGNWARE_PCIE_VIEWPORT_INBOUND][0];
+    viewport->cr[1] = DESIGNWARE_PCIE_ATU_ENABLE;
+    designware_pcie_update_viewport(&s->root, viewport);
 }
 
 static const VMStateDescription vmstate_designware_pcie_host = {
-- 
2.41.0

Re: [PATCH 8/8] hw/pci-host/designware: Create ViewPorts during host bridge realization

[Gustavo Romero]

Hi Phil,

On 10/12/23 9:18 AM, Philippe Mathieu-Daudé wrote:
> ViewPorts are managed by the host bridge part, so create them
> when the host bridge is realized. The host bridge become the

nit: becomes


> owner of the memory regions.
> 
> The PCI root function realize() method now only contains PCI
> specific code.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org>


Cheers,
Gustavo

> ---
>   hw/pci-host/designware.c | 207 +++++++++++++++++++--------------------
>   1 file changed, 102 insertions(+), 105 deletions(-)
> 
> diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
> index 6cb8655a75..e5dc9b4b8d 100644
> --- a/hw/pci-host/designware.c
> +++ b/hw/pci-host/designware.c
> @@ -384,22 +384,10 @@ static char *designware_pcie_viewport_name(const char *direction,
>   static void designware_pcie_root_realize(PCIDevice *dev, Error **errp)
>   {
>       DesignwarePCIERoot *root = DESIGNWARE_PCIE_ROOT(dev);
> -    DesignwarePCIEHost *host = DESIGNWARE_PCIE_HOST(
> -                                    qdev_get_parent_bus(DEVICE(dev))->parent);
> -    MemoryRegion *host_mem = get_system_memory();
> -    MemoryRegion *address_space = &host->pci.memory;
>       PCIBridge *br = PCI_BRIDGE(dev);
> -    DesignwarePCIEViewport *viewport;
> -    /*
> -     * Dummy values used for initial configuration of MemoryRegions
> -     * that belong to a given viewport
> -     */
> -    const hwaddr dummy_offset = 0;
> -    const uint64_t dummy_size = 4;
> -    size_t i;
>   
>       br->bus_name  = "dw-pcie";
> -    root->host = host;
> +    root->host = DESIGNWARE_PCIE_HOST(qdev_get_parent_bus(DEVICE(dev))->parent);
>   
>       pci_set_word(dev->config + PCI_COMMAND,
>                    PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER);
> @@ -414,97 +402,6 @@ static void designware_pcie_root_realize(PCIDevice *dev, Error **errp)
>   
>       msi_nonbroken = true;
>       msi_init(dev, 0x50, 32, true, true, &error_fatal);
> -
> -    for (i = 0; i < DESIGNWARE_PCIE_NUM_VIEWPORTS; i++) {
> -        MemoryRegion *source, *destination, *mem;
> -        const char *direction;
> -        char *name;
> -
> -        viewport = &host->viewports[DESIGNWARE_PCIE_VIEWPORT_INBOUND][i];
> -        viewport->inbound = true;
> -        viewport->base    = 0x0000000000000000ULL;
> -        viewport->target  = 0x0000000000000000ULL;
> -        viewport->limit   = UINT32_MAX;
> -        viewport->cr[0]   = DESIGNWARE_PCIE_ATU_TYPE_MEM;
> -
> -        source      = &host->pci.address_space_root;
> -        destination = host_mem;
> -        direction   = "Inbound";
> -
> -        /*
> -         * Configure MemoryRegion implementing PCI -> CPU memory
> -         * access
> -         */
> -        mem  = &viewport->mem;
> -        name = designware_pcie_viewport_name(direction, i, "MEM");
> -        memory_region_init_alias(mem, OBJECT(root), name, destination,
> -                                 dummy_offset, dummy_size);
> -        memory_region_add_subregion_overlap(source, dummy_offset, mem, -1);
> -        memory_region_set_enabled(mem, false);
> -        g_free(name);
> -
> -        viewport = &host->viewports[DESIGNWARE_PCIE_VIEWPORT_OUTBOUND][i];
> -        viewport->host    = host;
> -        viewport->inbound = false;
> -        viewport->base    = 0x0000000000000000ULL;
> -        viewport->target  = 0x0000000000000000ULL;
> -        viewport->limit   = UINT32_MAX;
> -        viewport->cr[0]   = DESIGNWARE_PCIE_ATU_TYPE_MEM;
> -
> -        destination = &host->pci.memory;
> -        direction   = "Outbound";
> -        source      = host_mem;
> -
> -        /*
> -         * Configure MemoryRegion implementing CPU -> PCI memory
> -         * access
> -         */
> -        mem  = &viewport->mem;
> -        name = designware_pcie_viewport_name(direction, i, "MEM");
> -        memory_region_init_alias(mem, OBJECT(root), name, destination,
> -                                 dummy_offset, dummy_size);
> -        memory_region_add_subregion(source, dummy_offset, mem);
> -        memory_region_set_enabled(mem, false);
> -        g_free(name);
> -
> -        /*
> -         * Configure MemoryRegion implementing access to configuration
> -         * space
> -         */
> -        mem  = &viewport->cfg;
> -        name = designware_pcie_viewport_name(direction, i, "CFG");
> -        memory_region_init_io(&viewport->cfg, OBJECT(root),
> -                              &designware_pci_host_conf_ops,
> -                              viewport, name, dummy_size);
> -        memory_region_add_subregion(source, dummy_offset, mem);
> -        memory_region_set_enabled(mem, false);
> -        g_free(name);
> -    }
> -
> -    /*
> -     * If no inbound iATU windows are configured, HW defaults to
> -     * letting inbound TLPs to pass in. We emulate that by explicitly
> -     * configuring first inbound window to cover all of target's
> -     * address space.
> -     *
> -     * NOTE: This will not work correctly for the case when first
> -     * configured inbound window is window 0
> -     */
> -    viewport = &host->viewports[DESIGNWARE_PCIE_VIEWPORT_INBOUND][0];
> -    viewport->cr[1] = DESIGNWARE_PCIE_ATU_ENABLE;
> -    designware_pcie_update_viewport(root, viewport);
> -
> -    memory_region_init_io(&host->msi.iomem, OBJECT(root),
> -                          &designware_pci_host_msi_ops,
> -                          root, "pcie-msi", 0x4);
> -    /*
> -     * We initially place MSI interrupt I/O region at address 0 and
> -     * disable it. It'll be later moved to correct offset and enabled
> -     * in designware_pcie_root_update_msi_mapping() as a part of
> -     * initialization done by guest OS
> -     */
> -    memory_region_add_subregion(address_space, dummy_offset, &host->msi.iomem);
> -    memory_region_set_enabled(&host->msi.iomem, false);
>   }
>   
>   static void designware_pcie_set_irq(void *opaque, int irq_num, int level)
> @@ -590,7 +487,7 @@ static void designware_pcie_root_class_init(ObjectClass *klass, void *data)
>       dc->reset = pci_bridge_reset;
>       /*
>        * PCI-facing part of the host bridge, not usable without the
> -     * host-facing part, which can't be device_add'ed, yet.
> +     * host-facing part.
>        */
>       dc->user_creatable = false;
>       dc->vmsd = &vmstate_designware_pcie_root;
> @@ -650,8 +547,17 @@ static void designware_pcie_host_realize(DeviceState *dev, Error **errp)
>       PCIHostState *pci = PCI_HOST_BRIDGE(dev);
>       DesignwarePCIEHost *s = DESIGNWARE_PCIE_HOST(dev);
>       SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +    MemoryRegion *host_mem = get_system_memory();
> +    DesignwarePCIEViewport *viewport;
>       size_t i;
>   
> +    /*
> +     * Dummy values used for initial configuration of MemoryRegions
> +     * that belong to a given viewport
> +     */
> +    const hwaddr dummy_offset = 0;
> +    const uint64_t dummy_size = 4;
> +
>       for (i = 0; i < ARRAY_SIZE(s->pci.irqs); i++) {
>           sysbus_init_irq(sbd, &s->pci.irqs[i]);
>       }
> @@ -694,6 +600,97 @@ static void designware_pcie_host_realize(DeviceState *dev, Error **errp)
>       qdev_prop_set_int32(DEVICE(&s->root), "addr", PCI_DEVFN(0, 0));
>       qdev_prop_set_bit(DEVICE(&s->root), "multifunction", false);
>       qdev_realize(DEVICE(&s->root), BUS(pci->bus), &error_fatal);
> +
> +    memory_region_init_io(&s->msi.iomem, OBJECT(s),
> +                          &designware_pci_host_msi_ops,
> +                          s, "pcie-msi", 0x4);
> +    /*
> +     * We initially place MSI interrupt I/O region at address 0 and
> +     * disable it. It'll be later moved to correct offset and enabled
> +     * in designware_pcie_host_update_msi_mapping() as a part of
> +     * initialization done by guest OS
> +     */
> +    memory_region_add_subregion(&s->pci.memory, dummy_offset, &s->msi.iomem);
> +    memory_region_set_enabled(&s->msi.iomem, false);
> +
> +    for (i = 0; i < DESIGNWARE_PCIE_NUM_VIEWPORTS; i++) {
> +        MemoryRegion *source, *destination, *mem;
> +        const char *direction;
> +        char *name;
> +
> +        viewport = &s->viewports[DESIGNWARE_PCIE_VIEWPORT_INBOUND][i];
> +        viewport->inbound = true;
> +        viewport->base    = 0x0000000000000000ULL;
> +        viewport->target  = 0x0000000000000000ULL;
> +        viewport->limit   = UINT32_MAX;
> +        viewport->cr[0]   = DESIGNWARE_PCIE_ATU_TYPE_MEM;
> +
> +        source      = &s->pci.address_space_root;
> +        destination = host_mem;
> +        direction   = "Inbound";
> +
> +        /*
> +         * Configure MemoryRegion implementing PCI -> CPU memory
> +         * access
> +         */
> +        mem  = &viewport->mem;
> +        name = designware_pcie_viewport_name(direction, i, "MEM");
> +        memory_region_init_alias(mem, OBJECT(s), name, destination,
> +                                 dummy_offset, dummy_size);
> +        memory_region_add_subregion_overlap(source, dummy_offset, mem, -1);
> +        memory_region_set_enabled(mem, false);
> +        g_free(name);
> +
> +        viewport = &s->viewports[DESIGNWARE_PCIE_VIEWPORT_OUTBOUND][i];
> +        viewport->host    = s;
> +        viewport->inbound = false;
> +        viewport->base    = 0x0000000000000000ULL;
> +        viewport->target  = 0x0000000000000000ULL;
> +        viewport->limit   = UINT32_MAX;
> +        viewport->cr[0]   = DESIGNWARE_PCIE_ATU_TYPE_MEM;
> +
> +        destination = &s->pci.memory;
> +        direction   = "Outbound";
> +        source      = host_mem;
> +
> +        /*
> +         * Configure MemoryRegion implementing CPU -> PCI memory
> +         * access
> +         */
> +        mem  = &viewport->mem;
> +        name = designware_pcie_viewport_name(direction, i, "MEM");
> +        memory_region_init_alias(mem, OBJECT(s), name, destination,
> +                                 dummy_offset, dummy_size);
> +        memory_region_add_subregion(source, dummy_offset, mem);
> +        memory_region_set_enabled(mem, false);
> +        g_free(name);
> +
> +        /*
> +         * Configure MemoryRegion implementing access to configuration
> +         * space
> +         */
> +        mem  = &viewport->cfg;
> +        name = designware_pcie_viewport_name(direction, i, "CFG");
> +        memory_region_init_io(&viewport->cfg, OBJECT(s),
> +                              &designware_pci_host_conf_ops,
> +                              viewport, name, dummy_size);
> +        memory_region_add_subregion(source, dummy_offset, mem);
> +        memory_region_set_enabled(mem, false);
> +        g_free(name);
> +    }
> +
> +    /*
> +     * If no inbound iATU windows are configured, HW defaults to
> +     * letting inbound TLPs to pass in. We emulate that by explicitly
> +     * configuring first inbound window to cover all of target's
> +     * address space.
> +     *
> +     * NOTE: This will not work correctly for the case when first
> +     * configured inbound window is window 0
> +     */
> +    viewport = &s->viewports[DESIGNWARE_PCIE_VIEWPORT_INBOUND][0];
> +    viewport->cr[1] = DESIGNWARE_PCIE_ATU_ENABLE;
> +    designware_pcie_update_viewport(&s->root, viewport);
>   }
>   
>   static const VMStateDescription vmstate_designware_pcie_host = {
> 

Re: [PATCH 0/8] hw/pci-host/designware: QOM shuffling (Host bridge <-> Root function)

[Peter Maydell]

On Thu, 12 Oct 2023 at 13:19, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Hi,
>
> While trying this PCI host bridge in a hegerogeneous setup
> I noticed few discrepancies due to the fact that host bridge
> pieces were managed by the root function.
>
> This series move these pieces (ViewPort and MSI regs) to the
> host bridge side where they belong. Unfortunately this is
> a migration breakage.
>
> I recommend reviewing using 'git-diff --color-moved=dimmed-zebra'.
>
> Regards,
>

I had a go at reviewing this series, but I ran into the
problem that I don't know enough PCI to know whether
the various things being moved between root and bridge
here really do belong logically in one place rather
than the other.

Is there somebody with more PCI experience who could
look at this series?

thanks
-- PMM

Re: [PATCH-for-9.0 0/8] hw/pci-host/designware: QOM shuffling (Host bridge <-> Root function)

[Philippe Mathieu-Daudé]

Cc'ing Sifive developers :)

On 12/10/23 14:18, Philippe Mathieu-Daudé wrote:
> Hi,
> 
> While trying this PCI host bridge in a hegerogeneous setup
> I noticed few discrepancies due to the fact that host bridge
> pieces were managed by the root function.
> 
> This series move these pieces (ViewPort and MSI regs) to the
> host bridge side where they belong. Unfortunately this is
> a migration breakage.
> 
> I recommend reviewing using 'git-diff --color-moved=dimmed-zebra'.
> 
> Regards,
> 
> Phil.
> 
> Philippe Mathieu-Daudé (8):
>    hw/pci-host/designware: Declare CPU QOM types using DEFINE_TYPES()
>      macro
>    hw/pci-host/designware: Initialize root function in host bridge
>      realize
>    hw/pci-host/designware: Add 'host_mem' variable for clarity
>    hw/pci-host/designware: Hoist host controller in root function #0
>    hw/pci-host/designware: Keep host reference in DesignwarePCIEViewport
>    hw/pci-host/designware: Move viewports from root func to host bridge
>    hw/pci-host/designware: Move MSI registers from root func to host
>      bridge
>    hw/pci-host/designware: Create ViewPorts during host bridge
>      realization
> 
>   include/hw/pci-host/designware.h |  20 +-
>   hw/pci-host/designware.c         | 376 +++++++++++++++----------------
>   2 files changed, 187 insertions(+), 209 deletions(-)
>