updates for designware pci-host

updates for designware pci-host

[Ben Dooks]

As part of a project we have been looking at using the DesignWare
PCIe host. We found a few issues of missing features or small bugs
when using this with a recent Linux kernel (v5.17.x)

Whilst doing this we also made a start on some tracing events.

[PATCH 1/7] pci: designware: add 64-bit viewport limit

[Ben Dooks]

Versions 4 and above add support for 64-bit viewport
limit. Add support for the DESIGNWARE_PCIE_ATU_UPPER_LIMIT
regiser where supported.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 hw/pci-host/designware.c         | 22 +++++++++++++++++-----
 include/hw/pci-host/designware.h |  2 +-
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
index bde3a343a2..296f1b9760 100644
--- a/hw/pci-host/designware.c
+++ b/hw/pci-host/designware.c
@@ -54,6 +54,7 @@
 #define DESIGNWARE_PCIE_ATU_BUS(x)                 (((x) >> 24) & 0xff)
 #define DESIGNWARE_PCIE_ATU_DEVFN(x)               (((x) >> 16) & 0xff)
 #define DESIGNWARE_PCIE_ATU_UPPER_TARGET           0x91C
+#define DESIGNWARE_PCIE_ATU_UPPER_LIMIT            0x924
 
 #define DESIGNWARE_PCIE_IRQ_MSI                    3
 
@@ -196,6 +197,10 @@ designware_pcie_root_config_read(PCIDevice *d, uint32_t address, int len)
         val = viewport->target >> 32;
         break;
 
+    case DESIGNWARE_PCIE_ATU_UPPER_LIMIT:
+        val = viewport->limit >> 32;
+        break;
+
     case DESIGNWARE_PCIE_ATU_LIMIT:
         val = viewport->limit;
         break;
@@ -269,7 +274,7 @@ static void designware_pcie_update_viewport(DesignwarePCIERoot *root,
 {
     const uint64_t target = viewport->target;
     const uint64_t base   = viewport->base;
-    const uint64_t size   = (uint64_t)viewport->limit - base + 1;
+    const uint64_t size   = viewport->limit - base + 1;
     const bool enabled    = viewport->cr[1] & DESIGNWARE_PCIE_ATU_ENABLE;
 
     MemoryRegion *current, *other;
@@ -363,14 +368,21 @@ static void designware_pcie_root_config_write(PCIDevice *d, uint32_t address,
         viewport->target |= val;
         break;
 
+    case DESIGNWARE_PCIE_ATU_UPPER_LIMIT:
+        viewport->limit &= 0x00000000FFFFFFFFUL;
+        viewport->limit |= (uint64_t)val << 32;
+        break;
+
     case DESIGNWARE_PCIE_ATU_LIMIT:
-        viewport->limit = val;
+        viewport->limit = 0xFFFFFFFF00000000ULL;
+        viewport->limit |= val;
         break;
 
     case DESIGNWARE_PCIE_ATU_CR1:
         viewport->cr[0] = val;
         break;
     case DESIGNWARE_PCIE_ATU_CR2:
+        //printf("CR2: value %08x\n", val);
         viewport->cr[1] = val;
         designware_pcie_update_viewport(root, viewport);
         break;
@@ -429,7 +441,7 @@ static void designware_pcie_root_realize(PCIDevice *dev, Error **errp)
         viewport->inbound = true;
         viewport->base    = 0x0000000000000000ULL;
         viewport->target  = 0x0000000000000000ULL;
-        viewport->limit   = UINT32_MAX;
+        viewport->limit   = UINT64_MAX-1;
         viewport->cr[0]   = DESIGNWARE_PCIE_ATU_TYPE_MEM;
 
         source      = &host->pci.address_space_root;
@@ -453,7 +465,7 @@ static void designware_pcie_root_realize(PCIDevice *dev, Error **errp)
         viewport->inbound = false;
         viewport->base    = 0x0000000000000000ULL;
         viewport->target  = 0x0000000000000000ULL;
-        viewport->limit   = UINT32_MAX;
+        viewport->limit   = UINT64_MAX-1;
         viewport->cr[0]   = DESIGNWARE_PCIE_ATU_TYPE_MEM;
 
         destination = &host->pci.memory;
@@ -560,7 +572,7 @@ static const VMStateDescription vmstate_designware_pcie_viewport = {
     .fields = (VMStateField[]) {
         VMSTATE_UINT64(base, DesignwarePCIEViewport),
         VMSTATE_UINT64(target, DesignwarePCIEViewport),
-        VMSTATE_UINT32(limit, DesignwarePCIEViewport),
+        VMSTATE_UINT64(limit, DesignwarePCIEViewport),
         VMSTATE_UINT32_ARRAY(cr, DesignwarePCIEViewport, 2),
         VMSTATE_END_OF_LIST()
     }
diff --git a/include/hw/pci-host/designware.h b/include/hw/pci-host/designware.h
index 6d9b51ae67..bd4dd49aec 100644
--- a/include/hw/pci-host/designware.h
+++ b/include/hw/pci-host/designware.h
@@ -44,7 +44,7 @@ typedef struct DesignwarePCIEViewport {
 
     uint64_t base;
     uint64_t target;
-    uint32_t limit;
+    uint64_t limit;
     uint32_t cr[2];
 
     bool inbound;
-- 
2.35.1

[PATCH 2/7] pci: designware: fix DESIGNWARE_PCIE_ATU_UPPER_TARGET

[Ben Dooks]

By inspection DESIGNWARE_PCIE_ATU_UPPER_TARGET should be writing to
the upper 32-bits of viewport->target, so fix this by shifting the
32-bit value before the or.

Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
---
 hw/pci-host/designware.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
index 296f1b9760..d213d7324c 100644
--- a/hw/pci-host/designware.c
+++ b/hw/pci-host/designware.c
@@ -365,7 +365,7 @@ static void designware_pcie_root_config_write(PCIDevice *d, uint32_t address,
 
     case DESIGNWARE_PCIE_ATU_UPPER_TARGET:
         viewport->target &= 0x00000000FFFFFFFFULL;
-        viewport->target |= val;
+        viewport->target |= (uint64_t)val << 32;
         break;
 
     case DESIGNWARE_PCIE_ATU_UPPER_LIMIT:
-- 
2.35.1

[PATCH 3/7] pci: designware: clamp viewport index

[Ben Dooks]

The current Linux driver for this assumes it can write the 255 into
this register and then read back the value to work out how many
viewports are supported.

Clamp the value so that the probe works and does not cause memory
corruption as the value is not well clamped elsewhere in the driver.

Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
---
 hw/pci-host/designware.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
index d213d7324c..6403416634 100644
--- a/hw/pci-host/designware.c
+++ b/hw/pci-host/designware.c
@@ -345,6 +345,10 @@ static void designware_pcie_root_config_write(PCIDevice *d, uint32_t address,
         break;
 
     case DESIGNWARE_PCIE_ATU_VIEWPORT:
+        /* clamp this value, linux uses it to calculate the
+         * available number of viewports */
+        if (val >= DESIGNWARE_PCIE_NUM_VIEWPORTS)
+            val = DESIGNWARE_PCIE_NUM_VIEWPORTS-1;
         root->atu_viewport = val;
         break;
 
-- 
2.35.1

[PATCH 4/7] pci: designware: ignore new bits in ATU CR1

[Ben Dooks]

In version 4 and anver ATU CR1 has more bits in it than just the
viewport type. Make a guess at masking these out to avoid issues
where Linux writes these bits and fails to enable memory ATUs.

Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
---
 hw/pci-host/designware.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
index 6403416634..947547d153 100644
--- a/hw/pci-host/designware.c
+++ b/hw/pci-host/designware.c
@@ -276,10 +276,10 @@ static void designware_pcie_update_viewport(DesignwarePCIERoot *root,
     const uint64_t base   = viewport->base;
     const uint64_t size   = viewport->limit - base + 1;
     const bool enabled    = viewport->cr[1] & DESIGNWARE_PCIE_ATU_ENABLE;
-
+    uint32_t cr0          = viewport->cr[0];
     MemoryRegion *current, *other;
 
-    if (viewport->cr[0] == DESIGNWARE_PCIE_ATU_TYPE_MEM) {
+    if ((cr0 & 0xFF) == DESIGNWARE_PCIE_ATU_TYPE_MEM) {
         current = &viewport->mem;
         other   = &viewport->cfg;
         memory_region_set_alias_offset(current, target);
-- 
2.35.1

[PATCH 5/7] pci: designware: move msi to entry 5

[Ben Dooks]

The driver should leave irq[0..3] for INT[A..D] but seems to put the
MSI IRQ at entry 3 which should also be INT_D. Extend the irqs[] array
to 5 entires and put the MSI at entry irqs[4].

Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
---
 hw/pci-host/designware.c         | 2 +-
 include/hw/pci-host/designware.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
index 947547d153..b5d5b2b8a5 100644
--- a/hw/pci-host/designware.c
+++ b/hw/pci-host/designware.c
@@ -56,7 +56,7 @@
 #define DESIGNWARE_PCIE_ATU_UPPER_TARGET           0x91C
 #define DESIGNWARE_PCIE_ATU_UPPER_LIMIT            0x924
 
-#define DESIGNWARE_PCIE_IRQ_MSI                    3
+#define DESIGNWARE_PCIE_IRQ_MSI                    4
 
 static DesignwarePCIEHost *
 designware_pcie_root_to_host(DesignwarePCIERoot *root)
diff --git a/include/hw/pci-host/designware.h b/include/hw/pci-host/designware.h
index bd4dd49aec..37f90c5000 100644
--- a/include/hw/pci-host/designware.h
+++ b/include/hw/pci-host/designware.h
@@ -90,7 +90,7 @@ struct DesignwarePCIEHost {
         MemoryRegion memory;
         MemoryRegion io;
 
-        qemu_irq     irqs[4];
+        qemu_irq     irqs[5];
     } pci;
 
     MemoryRegion mmio;
-- 
2.35.1

[PATCH 6/7] pci: designware: correct host's class_id

[Ben Dooks]

This is a host to pcie bridge, so use PCI_CLASS_BRIDGE_HOST
for the class.

Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
---
 hw/pci-host/designware.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
index b5d5b2b8a5..a47ae48071 100644
--- a/hw/pci-host/designware.c
+++ b/hw/pci-host/designware.c
@@ -615,7 +615,7 @@ static void designware_pcie_root_class_init(ObjectClass *klass, void *data)
     k->vendor_id = PCI_VENDOR_ID_SYNOPSYS;
     k->device_id = 0xABCD;
     k->revision = 0;
-    k->class_id = PCI_CLASS_BRIDGE_PCI;
+    k->class_id = PCI_CLASS_BRIDGE_HOST;
     k->is_bridge = true;
     k->exit = pci_bridge_exitfn;
     k->realize = designware_pcie_root_realize;
-- 
2.35.1

[PATCH 7/7] pci: designware: add initial tracing events

[Ben Dooks]

Add a couple of tracing events for internal driver updates

Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
---
 hw/pci-host/designware.c | 4 ++++
 hw/pci-host/trace-events | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
index a47ae48071..489959513f 100644
--- a/hw/pci-host/designware.c
+++ b/hw/pci-host/designware.c
@@ -30,6 +30,7 @@
 #include "migration/vmstate.h"
 #include "hw/irq.h"
 #include "hw/pci-host/designware.h"
+#include "trace.h"
 
 #define DESIGNWARE_PCIE_PORT_LINK_CONTROL          0x710
 #define DESIGNWARE_PCIE_PHY_DEBUG_R1               0x72C
@@ -112,6 +113,7 @@ static void designware_pcie_root_update_msi_mapping(DesignwarePCIERoot *root)
     const uint64_t base = root->msi.base;
     const bool enable   = root->msi.intr[0].enable;
 
+    trace_dw_pcie_msi_update(base, enable);
     memory_region_set_address(mem, base);
     memory_region_set_enabled(mem, enable);
 }
@@ -279,6 +281,8 @@ static void designware_pcie_update_viewport(DesignwarePCIERoot *root,
     uint32_t cr0          = viewport->cr[0];
     MemoryRegion *current, *other;
 
+    trace_dw_pcie_viewport_update(target, base, size, cr0, enabled);
+
     if ((cr0 & 0xFF) == DESIGNWARE_PCIE_ATU_TYPE_MEM) {
         current = &viewport->mem;
         other   = &viewport->cfg;
diff --git a/hw/pci-host/trace-events b/hw/pci-host/trace-events
index 437e66ff50..6b064d3c74 100644
--- a/hw/pci-host/trace-events
+++ b/hw/pci-host/trace-events
@@ -3,6 +3,10 @@
 # bonito.c
 bonito_spciconf_small_access(uint64_t addr, unsigned size) "PCI config address is smaller then 32-bit, addr: 0x%"PRIx64", size: %u"
 
+# designware.c
+dw_pcie_msi_update(uint64_t base, int enable) "base 0x%" PRIx64 " enable %d"
+dw_pcie_viewport_update(uint64_t target, uint64_t base, uint64_t limit, uint32_t cr0, int enabled) "target 0x%" PRIx64 " base 0x%" PRIx64 " limit 0x%" PRIx64 " cr0 0x%" PRIx32 " enabled %d"
+
 # grackle.c
 grackle_set_irq(int irq_num, int level) "set_irq num %d level %d"
 
-- 
2.35.1

Re: [PATCH 1/7] pci: designware: add 64-bit viewport limit

[Ben Dooks]

On 13/07/2022 17:54, Ben Dooks wrote:
> Versions 4 and above add support for 64-bit viewport
> limit. Add support for the DESIGNWARE_PCIE_ATU_UPPER_LIMIT
> regiser where supported.
> 
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>

Whoops, just noticed this was my old ct address.

> ---
>   hw/pci-host/designware.c         | 22 +++++++++++++++++-----
>   include/hw/pci-host/designware.h |  2 +-
>   2 files changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
> index bde3a343a2..296f1b9760 100644
> --- a/hw/pci-host/designware.c
> +++ b/hw/pci-host/designware.c
> @@ -54,6 +54,7 @@
>   #define DESIGNWARE_PCIE_ATU_BUS(x)                 (((x) >> 24) & 0xff)
>   #define DESIGNWARE_PCIE_ATU_DEVFN(x)               (((x) >> 16) & 0xff)
>   #define DESIGNWARE_PCIE_ATU_UPPER_TARGET           0x91C
> +#define DESIGNWARE_PCIE_ATU_UPPER_LIMIT            0x924
>   
>   #define DESIGNWARE_PCIE_IRQ_MSI                    3
>   
> @@ -196,6 +197,10 @@ designware_pcie_root_config_read(PCIDevice *d, uint32_t address, int len)
>           val = viewport->target >> 32;
>           break;
>   
> +    case DESIGNWARE_PCIE_ATU_UPPER_LIMIT:
> +        val = viewport->limit >> 32;
> +        break;
> +
>       case DESIGNWARE_PCIE_ATU_LIMIT:
>           val = viewport->limit;
>           break;
> @@ -269,7 +274,7 @@ static void designware_pcie_update_viewport(DesignwarePCIERoot *root,
>   {
>       const uint64_t target = viewport->target;
>       const uint64_t base   = viewport->base;
> -    const uint64_t size   = (uint64_t)viewport->limit - base + 1;
> +    const uint64_t size   = viewport->limit - base + 1;
>       const bool enabled    = viewport->cr[1] & DESIGNWARE_PCIE_ATU_ENABLE;
>   
>       MemoryRegion *current, *other;
> @@ -363,14 +368,21 @@ static void designware_pcie_root_config_write(PCIDevice *d, uint32_t address,
>           viewport->target |= val;
>           break;
>   
> +    case DESIGNWARE_PCIE_ATU_UPPER_LIMIT:
> +        viewport->limit &= 0x00000000FFFFFFFFUL;
> +        viewport->limit |= (uint64_t)val << 32;
> +        break;
> +
>       case DESIGNWARE_PCIE_ATU_LIMIT:
> -        viewport->limit = val;
> +        viewport->limit = 0xFFFFFFFF00000000ULL;
> +        viewport->limit |= val;
>           break;
>   
>       case DESIGNWARE_PCIE_ATU_CR1:
>           viewport->cr[0] = val;
>           break;
>       case DESIGNWARE_PCIE_ATU_CR2:
> +        //printf("CR2: value %08x\n", val);
>           viewport->cr[1] = val;
>           designware_pcie_update_viewport(root, viewport);
>           break;
> @@ -429,7 +441,7 @@ static void designware_pcie_root_realize(PCIDevice *dev, Error **errp)
>           viewport->inbound = true;
>           viewport->base    = 0x0000000000000000ULL;
>           viewport->target  = 0x0000000000000000ULL;
> -        viewport->limit   = UINT32_MAX;
> +        viewport->limit   = UINT64_MAX-1;
>           viewport->cr[0]   = DESIGNWARE_PCIE_ATU_TYPE_MEM;
>   
>           source      = &host->pci.address_space_root;
> @@ -453,7 +465,7 @@ static void designware_pcie_root_realize(PCIDevice *dev, Error **errp)
>           viewport->inbound = false;
>           viewport->base    = 0x0000000000000000ULL;
>           viewport->target  = 0x0000000000000000ULL;
> -        viewport->limit   = UINT32_MAX;
> +        viewport->limit   = UINT64_MAX-1;
>           viewport->cr[0]   = DESIGNWARE_PCIE_ATU_TYPE_MEM;
>   
>           destination = &host->pci.memory;
> @@ -560,7 +572,7 @@ static const VMStateDescription vmstate_designware_pcie_viewport = {
>       .fields = (VMStateField[]) {
>           VMSTATE_UINT64(base, DesignwarePCIEViewport),
>           VMSTATE_UINT64(target, DesignwarePCIEViewport),
> -        VMSTATE_UINT32(limit, DesignwarePCIEViewport),
> +        VMSTATE_UINT64(limit, DesignwarePCIEViewport),
>           VMSTATE_UINT32_ARRAY(cr, DesignwarePCIEViewport, 2),
>           VMSTATE_END_OF_LIST()
>       }
> diff --git a/include/hw/pci-host/designware.h b/include/hw/pci-host/designware.h
> index 6d9b51ae67..bd4dd49aec 100644
> --- a/include/hw/pci-host/designware.h
> +++ b/include/hw/pci-host/designware.h
> @@ -44,7 +44,7 @@ typedef struct DesignwarePCIEViewport {
>   
>       uint64_t base;
>       uint64_t target;
> -    uint32_t limit;
> +    uint64_t limit;
>       uint32_t cr[2];
>   
>       bool inbound;

Re: updates for designware pci-host

[Ben Dooks]

On Wed, Jul 13, 2022 at 05:54:42PM +0100, Ben Dooks wrote:
> As part of a project we have been looking at using the DesignWare
> PCIe host. We found a few issues of missing features or small bugs
> when using this with a recent Linux kernel (v5.17.x)
> 
> Whilst doing this we also made a start on some tracing events.

Hi, has anyone had a chance to review these. If so can this series
get applied? If not should anyone else be added to the review list?

If it would be easier I can try and find a git tree to publish this
branch on if a pull request would be easier.

-- 
Ben Dooks, ben@fluff.org, http://www.fluff.org/ben/

Large Hadron Colada: A large Pina Colada that makes the universe disappear.

Re: updates for designware pci-host

[Peter Maydell]

On Wed, 27 Jul 2022 at 12:15, Ben Dooks <qemu@ben.fluff.org> wrote:
>
> On Wed, Jul 13, 2022 at 05:54:42PM +0100, Ben Dooks wrote:
> > As part of a project we have been looking at using the DesignWare
> > PCIe host. We found a few issues of missing features or small bugs
> > when using this with a recent Linux kernel (v5.17.x)
> >
> > Whilst doing this we also made a start on some tracing events.
>
> Hi, has anyone had a chance to review these. If so can this series
> get applied? If not should anyone else be added to the review list?
>
> If it would be easier I can try and find a git tree to publish this
> branch on if a pull request would be easier.

Is there a public spec for the hardware? There isn't anything
listed in the source file in the tree. Without the h/w specs
it's pretty difficult to review changes.

thanks
-- PMM

Re: updates for designware pci-host

[Ben Dooks]

On 27/07/2022 13:56, Peter Maydell wrote:
> On Wed, 27 Jul 2022 at 12:15, Ben Dooks <qemu@ben.fluff.org> wrote:
>>
>> On Wed, Jul 13, 2022 at 05:54:42PM +0100, Ben Dooks wrote:
>>> As part of a project we have been looking at using the DesignWare
>>> PCIe host. We found a few issues of missing features or small bugs
>>> when using this with a recent Linux kernel (v5.17.x)
>>>
>>> Whilst doing this we also made a start on some tracing events.
>>
>> Hi, has anyone had a chance to review these. If so can this series
>> get applied? If not should anyone else be added to the review list?
>>
>> If it would be easier I can try and find a git tree to publish this
>> branch on if a pull request would be easier.
> 
> Is there a public spec for the hardware? There isn't anything
> listed in the source file in the tree. Without the h/w specs
> it's pretty difficult to review changes.

Some of this I did against the Linux driver from the v5.17 kernel
which didn't work until these fixes were added (not sure when the
Linux kernel got the ATU size autodetection in, etc).

I think everything described in this set was from Linux, but I
do have access to the Synopsys v5 PCIe core documentation so have
not gone to see if there is anything publicly available.

-- 
Ben

Re: updates for designware pci-host

[Ben Dooks]

On 27/07/2022 13:56, Peter Maydell wrote:
> On Wed, 27 Jul 2022 at 12:15, Ben Dooks <qemu@ben.fluff.org> wrote:
>>
>> On Wed, Jul 13, 2022 at 05:54:42PM +0100, Ben Dooks wrote:
>>> As part of a project we have been looking at using the DesignWare
>>> PCIe host. We found a few issues of missing features or small bugs
>>> when using this with a recent Linux kernel (v5.17.x)
>>>
>>> Whilst doing this we also made a start on some tracing events.
>>
>> Hi, has anyone had a chance to review these. If so can this series
>> get applied? If not should anyone else be added to the review list?
>>
>> If it would be easier I can try and find a git tree to publish this
>> branch on if a pull request would be easier.
> 
> Is there a public spec for the hardware? There isn't anything
> listed in the source file in the tree. Without the h/w specs
> it's pretty difficult to review changes.

Would it be helpful if i put out a v2 series with pointers into the
Linux tree to where the info is available there?