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