qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v3] Handle wrap around in limit calculation

From: Shlomo Pongratz
Subject: Re: [PATCH v3] Handle wrap around in limit calculation
Date: Mon, 22 Jan 2024 10:37:44 +0200
User-agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.13.1 
Please see inline

On 22/01/2024 01:17, Philippe Mathieu-Daudé wrote:

Hi Shlomo,

On 21/1/24 17:47, Shlomo Pongratz wrote:

From: Shlomo Pongratz <shlomopongratz@gmail.com>

     Hanlde wrap around when calculating the viewport size
     caused by the fact that perior to version 460A the limit variable
     was 32bit quantity and not 64 bits quantity.
     In the i.MX 6Dual/6Quad Applications Processor Reference Manual
     document on which the original code was based upon in the
     description of the iATU Region Upper Base Address Register it is
     written:
     Forms bits [63:32] of the start (and end) address of the address region to be 
     translated.
     That is in this register is the upper of both base and the limit.
     In the current implementation this value was ignored for the limit
     which caused a wrap around of the size calculaiton.
     Using the documnet example:
     Base HI: 0x80000000 Base LO: 0xd0000000 Limit LO: 0xd000ffff
     The correct size is 0x80000000d000ffff - 0x80000000d0000000 + 1 =
0x010000
     The wrong result is 0xd000ffff - 0x80000000d0000000 + 1 = 0x8000000000010000 

     Signed-off-by: Shlomo Pongratz <shlomop@pliops.com>

     ----

     Changes since v2:
      * Don't try to fix the calculation.
      * Change the limit variable from 32bit to 64 bit
      * Set the limit bits [63:32] same as the base according to
        the specification on which the original code was base upon.

     Changes since v1:
      * Seperate subject and description
---
  hw/pci-host/designware.c         | 19 ++++++++++++++-----
  include/hw/pci-host/designware.h |  2 +-
  2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
index dd9e389c07..43cba9432f 100644
--- a/hw/pci-host/designware.c
+++ b/hw/pci-host/designware.c
@@ -269,7 +269,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;
@@ -351,6 +351,14 @@ static void designware_pcie_root_config_write(PCIDevice *d, uint32_t address, 
      case DESIGNWARE_PCIE_ATU_UPPER_BASE:
          viewport->base &= 0x00000000FFFFFFFFULL;
          viewport->base |= (uint64_t)val << 32;
+        /* The documentatoin states that the value of this register
+         * "Forms bits [63:32] of the start (and end) address
+         * of the address region to be translated.
+         * Note that from version 406A there is a sperate
+         * register fot the upper end address
+         */
+        viewport->limit &= 0x00000000FFFFFFFFULL;
+        viewport->limit |= (uint64_t)val << 32;


This code is easier to review using:

          viewport->limit = deposit64(viewport->limit, 32, 32, val);


It will be strange to have
viewport->base &= 0x00000000FFFFFFFFULL;
    viewport->base |= (uint64_t)val << 32;
and then
viewport->limit = deposit64(viewport->limit, 32, 32, val);
I think that the code for base and limit should be the same.
And I don't think the original for base should be change to
viewport->base = deposit64(viewport->base, 32, 32, val);
SO everything will look the same.

          break;
        case DESIGNWARE_PCIE_ATU_LOWER_TARGET:
@@ -364,7 +372,8 @@ static void designware_pcie_root_config_write(PCIDevice *d, uint32_t address, 
          break;
        case DESIGNWARE_PCIE_ATU_LIMIT:
-        viewport->limit = val;
+        viewport->limit &= 0xFFFFFFFF00000000ULL;
+        viewport->limit |= val;


Here:

          viewport->limit = deposit64(viewport->limit, 0, 32, val);

My opinion is that the code should be identical to
case DESIGNWARE_PCIE_ATU_LOWER_BASE:
        viewport->base &= 0xFFFFFFFF00000000ULL;
        viewport->base |= val;
        break;
I don't think it is good to mix two styles.

          break;
        case DESIGNWARE_PCIE_ATU_CR1:
@@ -429,7 +438,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   = 0x00000000FFFFFFFFULL;


Previous code is easier to review IMHO.

Just want to make it clear that this is 64 bit values and that the
upper value is the same as the base's upper value, and according to spec.

          viewport->cr[0]   = DESIGNWARE_PCIE_ATU_TYPE_MEM;
            source      = &host->pci.address_space_root;
@@ -453,7 +462,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   = 0x00000000FFFFFFFFULL;


Ditto.

Ditto.



          viewport->cr[0]   = DESIGNWARE_PCIE_ATU_TYPE_MEM;
            destination = &host->pci.memory;
@@ -560,7 +569,7 @@ static const VMStateDescription vmstate_designware_pcie_viewport = { 
      .fields = (const VMStateField[]) {
          VMSTATE_UINT64(base, DesignwarePCIEViewport),
          VMSTATE_UINT64(target, DesignwarePCIEViewport),
-        VMSTATE_UINT32(limit, DesignwarePCIEViewport),
+        VMSTATE_UINT64(limit, DesignwarePCIEViewport),


Unfortunately this breaks the migration stream. I'm not sure
what is the best way to deal with it (Cc'ing migration
maintainers).

My bad forgot to update version_id and minimum_version_id.
I'll consult Peter Xu's document.



          VMSTATE_UINT32_ARRAY(cr, DesignwarePCIEViewport, 2),
          VMSTATE_END_OF_LIST()
      }

Regards,

Phil.



------------------------------------------------------------------------
*From:* Philippe Mathieu-Daudé [mailto:philmd@linaro.org]
*Sent:* Monday, January 22, 2024, 1:17 AM
*To:* Shlomo Pongratz; qemu-devel@nongnu.org
*Cc:* andrew.sminov@gmail.com, peter.maydell@linaro.com, shlomop@pliops.com, Peter Xu; Fabiano Rosas 
*Subject:* [PATCH v3] Handle wrap around in limit calculation


Hi Shlomo,

On 21/1/24 17:47, Shlomo Pongratz wrote:

From: Shlomo Pongratz <shlomopongratz@gmail.com>

     Hanlde wrap around when calculating the viewport size
     caused by the fact that perior to version 460A the limit variable
     was 32bit quantity and not 64 bits quantity.
     In the i.MX 6Dual/6Quad Applications Processor Reference Manual
     document on which the original code was based upon in the
     description of the iATU Region Upper Base Address Register it is
     written:
     Forms bits [63:32] of the start (and end) address of the address region to be 
     translated.
     That is in this register is the upper of both base and the limit.
     In the current implementation this value was ignored for the limit
     which caused a wrap around of the size calculaiton.
     Using the documnet example:
     Base HI: 0x80000000 Base LO: 0xd0000000 Limit LO: 0xd000ffff
     The correct size is 0x80000000d000ffff - 0x80000000d0000000 + 1 =
0x010000
     The wrong result is 0xd000ffff - 0x80000000d0000000 + 1 = 0x8000000000010000 

     Signed-off-by: Shlomo Pongratz <shlomop@pliops.com>

     ----

     Changes since v2:
      * Don't try to fix the calculation.
      * Change the limit variable from 32bit to 64 bit
      * Set the limit bits [63:32] same as the base according to
        the specification on which the original code was base upon.

     Changes since v1:
      * Seperate subject and description
---
  hw/pci-host/designware.c         | 19 ++++++++++++++-----
  include/hw/pci-host/designware.h |  2 +-
  2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
index dd9e389c07..43cba9432f 100644
--- a/hw/pci-host/designware.c
+++ b/hw/pci-host/designware.c
@@ -269,7 +269,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;
@@ -351,6 +351,14 @@ static void designware_pcie_root_config_write(PCIDevice *d, uint32_t address, 
      case DESIGNWARE_PCIE_ATU_UPPER_BASE:
          viewport->base &= 0x00000000FFFFFFFFULL;
          viewport->base |= (uint64_t)val << 32;
+        /* The documentatoin states that the value of this register
+         * "Forms bits [63:32] of the start (and end) address
+         * of the address region to be translated.
+         * Note that from version 406A there is a sperate
+         * register fot the upper end address
+         */
+        viewport->limit &= 0x00000000FFFFFFFFULL;
+        viewport->limit |= (uint64_t)val << 32;


This code is easier to review using:

          viewport->limit = deposit64(viewport->limit, 32, 32, val);


          break;
        case DESIGNWARE_PCIE_ATU_LOWER_TARGET:
@@ -364,7 +372,8 @@ static void designware_pcie_root_config_write(PCIDevice *d, uint32_t address, 
          break;
        case DESIGNWARE_PCIE_ATU_LIMIT:
-        viewport->limit = val;
+        viewport->limit &= 0xFFFFFFFF00000000ULL;
+        viewport->limit |= val;


Here:

          viewport->limit = deposit64(viewport->limit, 0, 32, val);


          break;
        case DESIGNWARE_PCIE_ATU_CR1:
@@ -429,7 +438,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   = 0x00000000FFFFFFFFULL;


Previous code is easier to review IMHO.


          viewport->cr[0]   = DESIGNWARE_PCIE_ATU_TYPE_MEM;
            source      = &host->pci.address_space_root;
@@ -453,7 +462,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   = 0x00000000FFFFFFFFULL;


Ditto.


          viewport->cr[0]   = DESIGNWARE_PCIE_ATU_TYPE_MEM;
            destination = &host->pci.memory;
@@ -560,7 +569,7 @@ static const VMStateDescription vmstate_designware_pcie_viewport = { 
      .fields = (const VMStateField[]) {
          VMSTATE_UINT64(base, DesignwarePCIEViewport),
          VMSTATE_UINT64(target, DesignwarePCIEViewport),
-        VMSTATE_UINT32(limit, DesignwarePCIEViewport),
+        VMSTATE_UINT64(limit, DesignwarePCIEViewport),


Unfortunately this breaks the migration stream. I'm not sure
what is the best way to deal with it (Cc'ing migration
maintainers).


          VMSTATE_UINT32_ARRAY(cr, DesignwarePCIEViewport, 2),
          VMSTATE_END_OF_LIST()
      }

Regards,

Phil.
reply via email to
[Prev in Thread] Current Thread [Next in Thread]