See Inline.
On 15/01/2024 18:47, Peter Maydell wrote:
On Mon, 15 Jan 2024 at 13:51, Shlomo Pongratz <shlomopongr...@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.
Actually this is one implementation of one pre-460A and we don't even
knows which.
And QEMU version is 0 which I assume should be some ideal implementation.
So as I suspect there is an implied HW limitation that a region should
never cross
4G boundary e.g. [0x80000000fffff000 0x800000010000efff] will be sent as
0x80000000fffff000 and 0xefff resulting in[0x80000000fffff000
0x800000000000efff]
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).
This was a typo
The + 1 should have been added after masking.
That is the calculation is done in 32 bit.
uint32_t tbase;
uint32_t tsize;
uint64_t size;
uint64_t tlimit;
// Use 32 bit calculation
tbase = base; // Truncate
tsize = limit - tbase;
// Return to 64 bit
size = tsize;
size++;
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;
This will work the only thing is that it will not work if for example
The original range is [0x80000000fffff000 0x800000010000efff]
Stuffing 0x80000000fffff000 0x0efff to your solution yields
size 0xffffffff00010000 and as result a limit of 0x800000000000efff
Note that the 32bit calculation above gives the original values.
Full simulation of all algorithms over some ranges can be found in
my gist
https://gist.github.com/shlomopongartz/f82466b6044f3a1653890b43b046c443
Anyway I assume that in your opinion we should keep the HW limitation
and not try to fix the HW
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).
I'll be glad to see post 460A implementation.
Since until then it will be impossible to use the same device tree in a
project
where the HW are working in a post-A460 controller and there range
crosses 4G boundary.
thanks
-- PMM
------------------------------------------------------------------------
*From:* Peter Maydell [mailto:peter.mayd...@linaro.org]
*Sent:* Monday, January 15, 2024, 6:47 PM
*To:* Shlomo Pongratz
*Cc:* qemu-devel@nongnu.org, andrew.smi...@gmail.com,
peter.mayd...@linaro.com, shlo...@pliops.com
*Subject:* [PATCH V2] Handle wrap around in limit calculation
On Mon, 15 Jan 2024 at 13:51, Shlomo Pongratz <shlomopongr...@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
