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

[Peter Maydell]

On Sun, 21 Jan 2024 at 16:48, Shlomo Pongratz <shlomopongratz@gmail.com> 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

"documentation".

Multiline comments should have the /* on a line of its own.

> +         * "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;
>          break;

Better to calculate the effective limit address when we need it,
rather than when the register is written. It's not a very
expensive calculation.

> diff --git a/include/hw/pci-host/designware.h 
> b/include/hw/pci-host/designware.h
> index 908f3d946b..51052683b7 100644
> --- a/include/hw/pci-host/designware.h
> +++ b/include/hw/pci-host/designware.h
> @@ -41,7 +41,7 @@ typedef struct DesignwarePCIEViewport {
>
>      uint64_t base;
>      uint64_t target;
> -    uint32_t limit;
> +    uint64_t limit;
>      uint32_t cr[2];

Making this field 64 bits makes the code to read and write
the register more complicated, and introduces a migration
compat break that we need to deal with. Why bother?
You can fix the problem that you're trying to solve in
the way that I suggested to you without introducing this
extra complication to this patch.

thanks
-- PMM