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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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.