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

[Peter Maydell]

On Mon, 15 Jan 2024 at 13:51, Shlomo Pongratz <shlomopongratz@gmail.com> wrote:
>
> On 15/01/2024 12:37, Peter Maydell wrote:
> > For instance, the kernel code suggests that pre-460A
> > there's a 32 bit limit register, and post-460A there
> > is a 64-bit limit (with an "UPPER_LIMIT" register to
> > access the top 32 bits), plus a PCIE_ATU_INCREASE_REGION_SIZE
> > flag bit. That suggests we might either:
> >   (1) implement all that
> >   (2) say we're implementing a pre-460A version with a
> >       32 bit limit field
> > Presumably we're aiming for (2) here, but I find the
> > arithmetic you have in this patch completely confusing:
> > it doesn't look like something hardware would be doing
> > when it has a 64 bit base address register and a 32 bit limit
> > address register. It's also weird as C code, because you
> > add 0x100000000 when calculating tlimit, but this will
> > have no effect because of the AND with 0xffffffff in
> > the following line.
> >
> > My guess at what the hardware is doing is "the actual
> > limit address takes its low 32 bits from the limit address
> > register and the high 32 bits from the high 32 bits of
> > the base address".

> The original code which claims to be based on i.MX6 Applications Processor
> actually fails for the example given there in page 4131 where the lower
> is set to 0xd0000000
> the upper to 0x8000000 and the limit to 0xd000ffff which gives us a size
> of 0x8000000000010000,
> which is a negative number. Therefore some fix need to be done.

Ah, I hadn't realised that this PCI controller was documented
in the i.MX6 applications processor reference manual. Looking
at that I see that the "upper base address register" is documented
as "Forms bits [63:32] of the start (and end) address of the address
region to be translated". That "(and end)" part confirms my
guess that the 64-bit limit address is supposed to be formed by
putting together the upper-base-address with the limit value.

> Your suggestion solve this issue and gives the correct address even if
> the addresses are translated by for example by a multiple of 4G, e.g
> 0x200000000,
> but it won't work if the range is translated in a way that is cross 4G
> boundary (e.g. translate by 0x2ffff000).
>
> After reviewing the mathematics I've found a solution which to my
> humiliation is more simple and it is likely that the HW
> is doing it, and this is just to ignore the high 32 bits of the
> calculation's result. That is:
> const uint64_t size = ((uint64_t)viewport->limit - base + 1) & 0xffffffff;

That gives the wrong answer for the case where the limit register
is set to 0xFFFF_FFFF (it will give you 0 rather than 0x1_0000_0000).

I think my suggestion is:

   uint64_t base = viewport->base;
   uint64_t limit = viewport->limit;
   uint64_t size;

   /*
    * Versions 460A and above of this PCI controller have a
    * 64-bit limit register. We currently model the older type
    * where the limit register is 32 bits, and the upper 32 bits
    * of the end address are the same as the upper 32 bits of
    * the start address.
    */
   limit = deposit64(limit, 32, 32, extract64(base, 32, 32));
   size = limit - base + 1;

That's a fairly clear implementation of what the docs say the
behaviour is, and it gives us a nice easy place to add 64-bit
limit register support if we ever need to (by guarding the setting
of 'limit' with an "if 64-bit limit registers not enabled" check).

thanks
-- PMM