Message ID | 20221121174326.68520-1-ardb@kernel.org |
---|---|
State | New |
Headers | show |
Series | target/arm: Use signed quantity to represent VMSAv8-64 translation level | expand |
On Mon, 21 Nov 2022 at 17:43, Ard Biesheuvel <ardb@kernel.org> wrote: > > The LPA2 extension implements 52-bit virtual addressing for 4k and 16k > translation granules, and for the former, this means an additional level > of translation is needed. This means we start counting at -1 instead of > 0 when doing a walk, and so 'level' is now a signed quantity, and should > be typed as such. So turn it from uint32_t into int32_t. > Does this cause any visible wrong behaviour, or is it just a cleanup thing ? thanks -- PMM
On Mon, 21 Nov 2022 at 19:51, Peter Maydell <peter.maydell@linaro.org> wrote: > > On Mon, 21 Nov 2022 at 17:43, Ard Biesheuvel <ardb@kernel.org> wrote: > > > > The LPA2 extension implements 52-bit virtual addressing for 4k and 16k > > translation granules, and for the former, this means an additional level > > of translation is needed. This means we start counting at -1 instead of > > 0 when doing a walk, and so 'level' is now a signed quantity, and should > > be typed as such. So turn it from uint32_t into int32_t. > > > > Does this cause any visible wrong behaviour, or is it just > a cleanup thing ? > No, 5 level paging is completely broken because of this, given that the 'level < 3' tests give the wrong result for (uint32_t)-1
On Mon, 21 Nov 2022 at 19:02, Ard Biesheuvel <ardb@kernel.org> wrote: > > On Mon, 21 Nov 2022 at 19:51, Peter Maydell <peter.maydell@linaro.org> wrote: > > > > On Mon, 21 Nov 2022 at 17:43, Ard Biesheuvel <ardb@kernel.org> wrote: > > > > > > The LPA2 extension implements 52-bit virtual addressing for 4k and 16k > > > translation granules, and for the former, this means an additional level > > > of translation is needed. This means we start counting at -1 instead of > > > 0 when doing a walk, and so 'level' is now a signed quantity, and should > > > be typed as such. So turn it from uint32_t into int32_t. > > > > > > > Does this cause any visible wrong behaviour, or is it just > > a cleanup thing ? > > > > No, 5 level paging is completely broken because of this, given that > the 'level < 3' tests give the wrong result for (uint32_t)-1 Right, thanks. This seems like a bug worth fixing for 7.2. We should make 'uint32_t startlevel' also an int32_t for consistency, I think, given that it is also sometimes negative, though in that case it doesn't get used in any comparisons so it's not going to cause wrong behaviour. thanks -- PMM
On Tue, 22 Nov 2022 at 14:21, Peter Maydell <peter.maydell@linaro.org> wrote: > > On Mon, 21 Nov 2022 at 19:02, Ard Biesheuvel <ardb@kernel.org> wrote: > > > > On Mon, 21 Nov 2022 at 19:51, Peter Maydell <peter.maydell@linaro.org> wrote: > > > > > > On Mon, 21 Nov 2022 at 17:43, Ard Biesheuvel <ardb@kernel.org> wrote: > > > > > > > > The LPA2 extension implements 52-bit virtual addressing for 4k and 16k > > > > translation granules, and for the former, this means an additional level > > > > of translation is needed. This means we start counting at -1 instead of > > > > 0 when doing a walk, and so 'level' is now a signed quantity, and should > > > > be typed as such. So turn it from uint32_t into int32_t. > > > > > > > > > > Does this cause any visible wrong behaviour, or is it just > > > a cleanup thing ? > > > > > > > No, 5 level paging is completely broken because of this, given that > > the 'level < 3' tests give the wrong result for (uint32_t)-1 > > Right, thanks. This seems like a bug worth fixing for 7.2. > Indeed. And the other patch I sent is needed too if you want to run with LPA2 'target/arm: Limit LPA2 effective output address when TCR.DS == 0' In case it is useful, I have a WIP kernel branch here which can be built with 52-bit virtual addressing for 4k or 16k pages. https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=arm64-4k-lpa2 > We should make 'uint32_t startlevel' also an int32_t > for consistency, I think, given that it is also sometimes > negative, though in that case it doesn't get used in any > comparisons so it's not going to cause wrong behaviour. > Indeed. I'll send a v2 and fold that in.
diff --git a/target/arm/ptw.c b/target/arm/ptw.c index 3745ac9723..6d6992580a 100644 --- a/target/arm/ptw.c +++ b/target/arm/ptw.c @@ -1172,7 +1172,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw, ARMCPU *cpu = env_archcpu(env); ARMMMUIdx mmu_idx = ptw->in_mmu_idx; bool is_secure = ptw->in_secure; - uint32_t level; + int32_t level; ARMVAParameters param; uint64_t ttbr; hwaddr descaddr, indexmask, indexmask_grainsize;
The LPA2 extension implements 52-bit virtual addressing for 4k and 16k translation granules, and for the former, this means an additional level of translation is needed. This means we start counting at -1 instead of 0 when doing a walk, and so 'level' is now a signed quantity, and should be typed as such. So turn it from uint32_t into int32_t. Cc: Peter Maydell <peter.maydell@linaro.org> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org> Cc: Richard Henderson <richard.henderson@linaro.org> Signed-off-by: Ard Biesheuvel <ardb@kernel.org> --- target/arm/ptw.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)