[PATCH] target/arm: Use signed quantity to represent VMSAv8-64 translation level

[PATCH] target/arm: Use signed quantity to represent VMSAv8-64 translation level

[Ard Biesheuvel]

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(-)

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;
-- 
2.35.1

Re: [PATCH] target/arm: Use signed quantity to represent VMSAv8-64 translation level

[Peter Maydell]

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

Re: [PATCH] target/arm: Use signed quantity to represent VMSAv8-64 translation level

[Ard Biesheuvel]

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

Re: [PATCH] target/arm: Use signed quantity to represent VMSAv8-64 translation level

[Peter Maydell]

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

Re: [PATCH] target/arm: Use signed quantity to represent VMSAv8-64 translation level

[Ard Biesheuvel]

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.