[Qemu-devel] [PATCH] target-arm: Fix descriptor address masking in ARM address translation

[Qemu-devel] [PATCH] target-arm: Fix descriptor address masking in ARM address translation

[Sergey Sorokin]

There is a bug in ARM address translation regime with a long-descriptor
format. On the descriptor reading its address is formed from an index
which is a part of the input address. And on the first iteration this index
is incorrectly masked with 'grainsize' mask. But it can be wider according
to pseudo-code.
On the other hand on the iterations other than first the descriptor address
is formed from the previous level descriptor by masking with 'descaddrmask'
value. It always clears just 12 lower bits, but it must clear 'grainsize'
lower bits instead according to pseudo-code.
The patch fixes both cases.

Signed-off-by: Sergey Sorokin <afarallax@yandex.ru>
---
 target-arm/helper.c | 29 ++++++++++-------------------
 1 file changed, 10 insertions(+), 19 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index dec8e8b..b5f289c 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -7243,7 +7243,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
     uint32_t tg;
     uint64_t ttbr;
     int ttbr_select;
-    hwaddr descaddr, descmask;
+    hwaddr descaddr, indexmask, indexmask_grainsize;
     uint32_t tableattrs;
     target_ulong page_size;
     uint32_t attrs;
@@ -7431,28 +7431,18 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
         }
     }
 
-    /* Clear the vaddr bits which aren't part of the within-region address,
-     * so that we don't have to special case things when calculating the
-     * first descriptor address.
-     */
-    if (va_size != inputsize) {
-        address &= (1ULL << inputsize) - 1;
-    }
-
-    descmask = (1ULL << (stride + 3)) - 1;
+    indexmask_grainsize = (1ULL << (stride + 3)) - 1;
+    indexmask = (1ULL << (inputsize - (stride * (4 - level)))) - 1;
 
     /* Now we can extract the actual base address from the TTBR */
     descaddr = extract64(ttbr, 0, 48);
-    descaddr &= ~((1ULL << (inputsize - (stride * (4 - level)))) - 1);
+    descaddr &= ~indexmask;
 
-    /* The address field in the descriptor goes up to bit 39 for ARMv7
-     * but up to bit 47 for ARMv8.
+    /* The address field in the descriptor goes up to bit 39 for AArch32
+     * but up to bit 47 for AArch64.
      */
-    if (arm_feature(env, ARM_FEATURE_V8)) {
-        descaddrmask = 0xfffffffff000ULL;
-    } else {
-        descaddrmask = 0xfffffff000ULL;
-    }
+    descaddrmask = ((1ull << (va_size == 64 ? 48 : 40)) - 1) &
+                   ~indexmask_grainsize;
 
     /* Secure accesses start with the page table in secure memory and
      * can be downgraded to non-secure at any step. Non-secure accesses
@@ -7464,7 +7454,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
         uint64_t descriptor;
         bool nstable;
 
-        descaddr |= (address >> (stride * (4 - level))) & descmask;
+        descaddr |= (address >> (stride * (4 - level))) & indexmask;
         descaddr &= ~7ULL;
         nstable = extract32(tableattrs, 4, 1);
         descriptor = arm_ldq_ptw(cs, descaddr, !nstable, mmu_idx, fsr, fi);
@@ -7487,6 +7477,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
              */
             tableattrs |= extract64(descriptor, 59, 5);
             level++;
+            indexmask = indexmask_grainsize;
             continue;
         }
         /* Block entry at level 1 or 2, or page entry at level 3.
-- 
1.9.3

Re: [Qemu-devel] [PATCH] target-arm: Fix descriptor address masking in ARM address translation

[Peter Maydell]

On 4 March 2016 at 23:04, Sergey Sorokin <afarallax@yandex.ru> wrote:
> There is a bug in ARM address translation regime with a long-descriptor
> format. On the descriptor reading its address is formed from an index
> which is a part of the input address. And on the first iteration this index
> is incorrectly masked with 'grainsize' mask. But it can be wider according
> to pseudo-code.
> On the other hand on the iterations other than first the descriptor address
> is formed from the previous level descriptor by masking with 'descaddrmask'
> value. It always clears just 12 lower bits, but it must clear 'grainsize'
> lower bits instead according to pseudo-code.
> The patch fixes both cases.

This is pretty confusing to understand -- it might help if you
could give an example.

Is this something that only causes problems for the "concatenated
translation tables at the initial level" case (which is only
possible for S2 tables) ?

> Signed-off-by: Sergey Sorokin <afarallax@yandex.ru>
> ---
>  target-arm/helper.c | 29 ++++++++++-------------------
>  1 file changed, 10 insertions(+), 19 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index dec8e8b..b5f289c 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -7243,7 +7243,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>      uint32_t tg;
>      uint64_t ttbr;
>      int ttbr_select;
> -    hwaddr descaddr, descmask;
> +    hwaddr descaddr, indexmask, indexmask_grainsize;
>      uint32_t tableattrs;
>      target_ulong page_size;
>      uint32_t attrs;
> @@ -7431,28 +7431,18 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>          }
>      }
>
> -    /* Clear the vaddr bits which aren't part of the within-region address,
> -     * so that we don't have to special case things when calculating the
> -     * first descriptor address.
> -     */
> -    if (va_size != inputsize) {
> -        address &= (1ULL << inputsize) - 1;
> -    }
> -
> -    descmask = (1ULL << (stride + 3)) - 1;
> +    indexmask_grainsize = (1ULL << (stride + 3)) - 1;
> +    indexmask = (1ULL << (inputsize - (stride * (4 - level)))) - 1;
>
>      /* Now we can extract the actual base address from the TTBR */
>      descaddr = extract64(ttbr, 0, 48);
> -    descaddr &= ~((1ULL << (inputsize - (stride * (4 - level)))) - 1);
> +    descaddr &= ~indexmask;
>
> -    /* The address field in the descriptor goes up to bit 39 for ARMv7
> -     * but up to bit 47 for ARMv8.
> +    /* The address field in the descriptor goes up to bit 39 for AArch32
> +     * but up to bit 47 for AArch64.
>       */

This is not correct -- the descriptor field widths are as the comment
states before your patch:
 * up to bit 39 for ARMv7
 * up to bit 47 for ARMv8 (whether AArch32 or AArch64)

See the v8 ARM ARM AArch32.TranslationTableWalkLD pseudocode and in
particular note the width which it uses for AddressSizeFault checks.

> -    if (arm_feature(env, ARM_FEATURE_V8)) {
> -        descaddrmask = 0xfffffffff000ULL;
> -    } else {
> -        descaddrmask = 0xfffffff000ULL;
> -    }
> +    descaddrmask = ((1ull << (va_size == 64 ? 48 : 40)) - 1) &
> +                   ~indexmask_grainsize;

...so this part of the patch is wrong.

>
>      /* Secure accesses start with the page table in secure memory and
>       * can be downgraded to non-secure at any step. Non-secure accesses
> @@ -7464,7 +7454,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>          uint64_t descriptor;
>          bool nstable;
>
> -        descaddr |= (address >> (stride * (4 - level))) & descmask;
> +        descaddr |= (address >> (stride * (4 - level))) & indexmask;
>          descaddr &= ~7ULL;
>          nstable = extract32(tableattrs, 4, 1);
>          descriptor = arm_ldq_ptw(cs, descaddr, !nstable, mmu_idx, fsr, fi);
> @@ -7487,6 +7477,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>               */
>              tableattrs |= extract64(descriptor, 59, 5);
>              level++;
> +            indexmask = indexmask_grainsize;
>              continue;
>          }
>          /* Block entry at level 1 or 2, or page entry at level 3.
> --
> 1.9.3
>

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target-arm: Fix descriptor address masking in ARM address translation

[Sergey Sorokin]

11.03.2016, 11:41, "Peter Maydell" <peter.maydell@linaro.org>:
>On 4 March 2016 at 23:04, Sergey Sorokin <afarallax@yandex.ru> wrote:
>> There is a bug in ARM address translation regime with a long-descriptor
>> format. On the descriptor reading its address is formed from an index
>> which is a part of the input address. And on the first iteration this index
>> is incorrectly masked with 'grainsize' mask. But it can be wider according
>> to pseudo-code.
>> On the other hand on the iterations other than first the descriptor address
>> is formed from the previous level descriptor by masking with 'descaddrmask'
>> value. It always clears just 12 lower bits, but it must clear 'grainsize'
>> lower bits instead according to pseudo-code.
>> The patch fixes both cases.
>
>This is pretty confusing to understand -- it might help if you
>could give an example.

According to documentation (ARMv8 ARM DDI 0487A.i J1.1.5:
aarch64/translation/walk/AArch64.TranslationTableWalk):

bits(48) index = ZeroExtend(inputaddr<addrselecttop:addrselectbottom>:'000');
descaddr.paddress.physicaladdress = baseaddress OR index;

For a first iteration of the descriptor reading:

addrselecttop = inputsize - 1;
addrselectbottom = (3-level)*stride + grainsize;

Let's assume grainsize == 12 (so stride == 9), level == 1, inputsize == 43.
Then index is
inputaddr<42:30>:'000';

But currently QEMU incorrecly masks an input address with descmask value:

descmask = (1ULL << (stride + 3)) - 1;
...
descaddr |= (address >> (stride * (4 - level))) & descmask;
descaddr &= ~7ULL;

Then the index in my example in QEMU is:
address<38:30>:'000';

And my patch fixes this bug.

>
>Is this something that only causes problems for the "concatenated
>translation tables at the initial level" case (which is only
>possible for S2 tables) ?
>
>> Signed-off-by: Sergey Sorokin <afarallax@yandex.ru>
>> ---
>>  target-arm/helper.c | 29 ++++++++++-------------------
>>  1 file changed, 10 insertions(+), 19 deletions(-)
>>
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index dec8e8b..b5f289c 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -7243,7 +7243,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>>      uint32_t tg;
>>      uint64_t ttbr;
>>      int ttbr_select;
>> -    hwaddr descaddr, descmask;
>> +    hwaddr descaddr, indexmask, indexmask_grainsize;
>>      uint32_t tableattrs;
>>      target_ulong page_size;
>>      uint32_t attrs;
>> @@ -7431,28 +7431,18 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>>          }
>>      }
>>
>> -    /* Clear the vaddr bits which aren't part of the within-region address,
>> -     * so that we don't have to special case things when calculating the
>> -     * first descriptor address.
>> -     */
>> -    if (va_size != inputsize) {
>> -        address &= (1ULL << inputsize) - 1;
>> -    }
>> -
>> -    descmask = (1ULL << (stride + 3)) - 1;
>> +    indexmask_grainsize = (1ULL << (stride + 3)) - 1;
>> +    indexmask = (1ULL << (inputsize - (stride * (4 - level)))) - 1;
>>
>>      /* Now we can extract the actual base address from the TTBR */
>>      descaddr = extract64(ttbr, 0, 48);
>> -    descaddr &= ~((1ULL << (inputsize - (stride * (4 - level)))) - 1);
>> +    descaddr &= ~indexmask;
>>
>> -    /* The address field in the descriptor goes up to bit 39 for ARMv7
>> -     * but up to bit 47 for ARMv8.
>> +    /* The address field in the descriptor goes up to bit 39 for AArch32
>> +     * but up to bit 47 for AArch64.
>>       */
>
>This is not correct -- the descriptor field widths are as the comment
>states before your patch:
> * up to bit 39 for ARMv7
> * up to bit 47 for ARMv8 (whether AArch32 or AArch64)
>
>See the v8 ARM ARM AArch32.TranslationTableWalkLD pseudocode and in
>particular note the width which it uses for AddressSizeFault checks.

I see in ARMv8 ARM DDI 0487A.i J1.2.4
aarch32/translation/walk/AArch32.TranslationTableWalkLD:

Before 'repeat' cycle:
baseaddress = baseregister<39:baselowerbound>:Zeros(baselowerbound);

Inside the cycle:
baseaddress = desc<39:grainsize>:Zeros(grainsize);

We use 'descaddrmask' in QEMU to get this 'baseaddress' from a descriptor.
So my patch is correct and fixes the bug.

>
>> -    if (arm_feature(env, ARM_FEATURE_V8)) {
>> -        descaddrmask = 0xfffffffff000ULL;
>> -    } else {
>> -        descaddrmask = 0xfffffff000ULL;
>> -    }
>> +    descaddrmask = ((1ull << (va_size == 64 ? 48 : 40)) - 1) &
>> +                   ~indexmask_grainsize;
>
>...so this part of the patch is wrong.
>
>>
>>      /* Secure accesses start with the page table in secure memory and
>>       * can be downgraded to non-secure at any step. Non-secure accesses
>> @@ -7464,7 +7454,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>>          uint64_t descriptor;
>>          bool nstable;
>>
>> -        descaddr |= (address >> (stride * (4 - level))) & descmask;
>> +        descaddr |= (address >> (stride * (4 - level))) & indexmask;
>>          descaddr &= ~7ULL;
>>          nstable = extract32(tableattrs, 4, 1);
>>          descriptor = arm_ldq_ptw(cs, descaddr, !nstable, mmu_idx, fsr, fi);
>> @@ -7487,6 +7477,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>>               */
>>              tableattrs |= extract64(descriptor, 59, 5);
>>              level++;
>> +            indexmask = indexmask_grainsize;
>>              continue;
>>          }
>>          /* Block entry at level 1 or 2, or page entry at level 3.
>> --
>> 1.9.3
>>
>
>thanks
>-- PMM

Re: [Qemu-devel] [PATCH] target-arm: Fix descriptor address masking in ARM address translation

[Peter Maydell]

On 12 March 2016 at 06:44, Sergey Sorokin <afarallax@yandex.ru> wrote:
> 11.03.2016, 11:41, "Peter Maydell" <peter.maydell@linaro.org>:
>>On 4 March 2016 at 23:04, Sergey Sorokin <afarallax@yandex.ru> wrote:
>>> There is a bug in ARM address translation regime with a long-descriptor
>>> format. On the descriptor reading its address is formed from an index
>>> which is a part of the input address. And on the first iteration this index
>>> is incorrectly masked with 'grainsize' mask. But it can be wider according
>>> to pseudo-code.
>>> On the other hand on the iterations other than first the descriptor address
>>> is formed from the previous level descriptor by masking with 'descaddrmask'
>>> value. It always clears just 12 lower bits, but it must clear 'grainsize'
>>> lower bits instead according to pseudo-code.
>>> The patch fixes both cases.
>>
>>This is pretty confusing to understand -- it might help if you
>>could give an example.
>
> According to documentation (ARMv8 ARM DDI 0487A.i J1.1.5:
> aarch64/translation/walk/AArch64.TranslationTableWalk):
>
> bits(48) index = ZeroExtend(inputaddr<addrselecttop:addrselectbottom>:'000');
> descaddr.paddress.physicaladdress = baseaddress OR index;
>
> For a first iteration of the descriptor reading:
>
> addrselecttop = inputsize - 1;
> addrselectbottom = (3-level)*stride + grainsize;
>
> Let's assume grainsize == 12 (so stride == 9), level == 1, inputsize == 43.
> Then index is
> inputaddr<42:30>:'000';

...which is more than 9 bits, so when does this happen?
I think this can only happen for the Stage-2 only
concatenated translation-tables case...

(I agree we have a bug here, I'm just trying to work out when it
can trigger; if it's only possible for S2 page tables then it's
not a visible bug yet because no CPUs have EL2 support enabled.)

>>> -    /* The address field in the descriptor goes up to bit 39 for ARMv7
>>> -     * but up to bit 47 for ARMv8.
>>> +    /* The address field in the descriptor goes up to bit 39 for AArch32
>>> +     * but up to bit 47 for AArch64.
>>>       */
>>
>>This is not correct -- the descriptor field widths are as the comment
>>states before your patch:
>> * up to bit 39 for ARMv7
>> * up to bit 47 for ARMv8 (whether AArch32 or AArch64)
>>
>>See the v8 ARM ARM AArch32.TranslationTableWalkLD pseudocode and in
>>particular note the width which it uses for AddressSizeFault checks.
>
> I see in ARMv8 ARM DDI 0487A.i J1.2.4
> aarch32/translation/walk/AArch32.TranslationTableWalkLD:
>
> Before 'repeat' cycle:
> baseaddress = baseregister<39:baselowerbound>:Zeros(baselowerbound);
>
> Inside the cycle:
> baseaddress = desc<39:grainsize>:Zeros(grainsize);

Yes, but this happens only after we have done the check:

  if !IsZero(desc<47:40>) then
     [take the AddressSize fault]

which tells us that the descriptor field really is up to bit 48.
We just haven't yet implemented the check in QEMU which will
generate the AddressSize fault if the top bits are nonzero.
(In contrast, in ARMv7 there really are only 40 bits there.)

If you want to implement the AddressSize checks that's fine,
but otherwise please leave this bit of the code alone.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target-arm: Fix descriptor address masking in ARM address translation

[Sergey Sorokin]

12.03.2016, 03:18, "Peter Maydell" <peter.maydell@linaro.org>:
>On 12 March 2016 at 06:44, Sergey Sorokin <afarallax@yandex.ru> wrote:
>> 11.03.2016, 11:41, "Peter Maydell" <peter.maydell@linaro.org>:
>>>On 4 March 2016 at 23:04, Sergey Sorokin <afarallax@yandex.ru> wrote:
>>>> There is a bug in ARM address translation regime with a long-descriptor
>>>> format. On the descriptor reading its address is formed from an index
>>>> which is a part of the input address. And on the first iteration this index
>>>> is incorrectly masked with 'grainsize' mask. But it can be wider according
>>>> to pseudo-code.
>>>> On the other hand on the iterations other than first the descriptor address
>>>> is formed from the previous level descriptor by masking with 'descaddrmask'
>>>> value. It always clears just 12 lower bits, but it must clear 'grainsize'
>>>> lower bits instead according to pseudo-code.
>>>> The patch fixes both cases.
>>>
>>>This is pretty confusing to understand -- it might help if you
>>>could give an example.
>>
>> According to documentation (ARMv8 ARM DDI 0487A.i J1.1.5:
>> aarch64/translation/walk/AArch64.TranslationTableWalk):
>>
>> bits(48) index = ZeroExtend(inputaddr<addrselecttop:addrselectbottom>:'000');
>> descaddr.paddress.physicaladdress = baseaddress OR index;
>>
>> For a first iteration of the descriptor reading:
>>
>> addrselecttop = inputsize - 1;
>> addrselectbottom = (3-level)*stride + grainsize;
>>
>> Let's assume grainsize == 12 (so stride == 9), level == 1, inputsize == 43.
>> Then index is
>> inputaddr<42:30>:'000';
>
>...which is more than 9 bits, so when does this happen?
>I think this can only happen for the Stage-2 only
>concatenated translation-tables case...
>
>(I agree we have a bug here, I'm just trying to work out when it
>can trigger; if it's only possible for S2 page tables then it's
>not a visible bug yet because no CPUs have EL2 support enabled.)

I can not anwer you to this question. The bug was found by our internal
corporate test suite with EL2 enabled.

>
>>>> -    /* The address field in the descriptor goes up to bit 39 for ARMv7
>>>> -     * but up to bit 47 for ARMv8.
>>>> +    /* The address field in the descriptor goes up to bit 39 for AArch32
>>>> +     * but up to bit 47 for AArch64.
>>>>       */
>>>
>>>This is not correct -- the descriptor field widths are as the comment
>>>states before your patch:
>>> * up to bit 39 for ARMv7
>>> * up to bit 47 for ARMv8 (whether AArch32 or AArch64)
>>>
>>>See the v8 ARM ARM AArch32.TranslationTableWalkLD pseudocode and in
>>>particular note the width which it uses for AddressSizeFault checks.
>>
>> I see in ARMv8 ARM DDI 0487A.i J1.2.4
>> aarch32/translation/walk/AArch32.TranslationTableWalkLD:
>>
>> Before 'repeat' cycle:
>> baseaddress = baseregister<39:baselowerbound>:Zeros(baselowerbound);
>>
>> Inside the cycle:
>> baseaddress = desc<39:grainsize>:Zeros(grainsize);
>
>Yes, but this happens only after we have done the check:
>
>  if !IsZero(desc<47:40>) then
>     [take the AddressSize fault]
>
>which tells us that the descriptor field really is up to bit 48.
>We just haven't yet implemented the check in QEMU which will
>generate the AddressSize fault if the top bits are nonzero.
>(In contrast, in ARMv7 there really are only 40 bits there.)
>
>If you want to implement the AddressSize checks that's fine,
>but otherwise please leave this bit of the code alone.

You said me that my code is not correct, I have proved that it conforms
to the documentation.
It's a bit obfuscating when the doc explicitly says to take bits up to 39
from the descriptor, but in QEMU we take bits up to 47 relying on the check in
another part of the code, even if both ways are correct.

Nevertheless there is another bug in descaddrmask in QEMU.
>From ARM ARM:
baseaddress = desc<39:grainsize>:Zeros(grainsize);

But currently QEMU does:
descaddrmask = 0xfffffffff000ULL;

It assumes that grainsize is always 12, but it can be greater
in AArch64 translation regime.
The patch fixes the bug,
and completely conforms to the doc, doesn't it?

Re: [Qemu-devel] [PATCH] target-arm: Fix descriptor address masking in ARM address translation

[Peter Maydell]

On 13 March 2016 at 18:28, Sergey Sorokin <afarallax@yandex.ru> wrote:
> 12.03.2016, 03:18, "Peter Maydell" <peter.maydell@linaro.org>:
>>(I agree we have a bug here, I'm just trying to work out when it
>>can trigger; if it's only possible for S2 page tables then it's
>>not a visible bug yet because no CPUs have EL2 support enabled.)
>
> I can not anwer you to this question. The bug was found by our internal
> corporate test suite with EL2 enabled.

OK, sounds like it is the stage-2 only stuff. Thanks for
helping to flush out the bugs.

>>If you want to implement the AddressSize checks that's fine,
>>but otherwise please leave this bit of the code alone.
>
> You said me that my code is not correct, I have proved that it conforms
> to the documentation.
> It's a bit obfuscating when the doc explicitly says to take bits up to 39
> from the descriptor, but in QEMU we take bits up to 47 relying on the check in
> another part of the code, even if both ways are correct.

The way the code in QEMU is structured is that we extract the
descriptor field in one go and then will operate on it
(checking for need to AddressSize fault, etc) as a second
action. The field descriptors themselves are the sizes I said.

> Nevertheless there is another bug in descaddrmask in QEMU.
> From ARM ARM:
> baseaddress = desc<39:grainsize>:Zeros(grainsize);
>
> But currently QEMU does:
> descaddrmask = 0xfffffffff000ULL;
>
> It assumes that grainsize is always 12, but it can be greater
> in AArch64 translation regime.

Yes, we get that wrong at the moment and should fix it.

> The patch fixes the bug,
> and completely conforms to the doc, doesn't it?

It doesn't structure the code the way I would prefer it to
be structured though.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target-arm: Fix descriptor address masking in ARM address translation

[Sergey Sorokin]

17.03.2016, 14:40, "Peter Maydell" <peter.maydell@linaro.org>:
> On 13 March 2016 at 18:28, Sergey Sorokin <afarallax@yandex.ru> wrote:
>>> If you want to implement the AddressSize checks that's fine,
>>> but otherwise please leave this bit of the code alone.
>>
>>  You said me that my code is not correct, I have proved that it conforms
>>  to the documentation.
>>  It's a bit obfuscating when the doc explicitly says to take bits up to 39
>>  from the descriptor, but in QEMU we take bits up to 47 relying on the check in
>>  another part of the code, even if both ways are correct.
>
> The way the code in QEMU is structured is that we extract the
> descriptor field in one go and then will operate on it
> (checking for need to AddressSize fault, etc) as a second
> action. The field descriptors themselves are the sizes I said.

Well, may be it's enough just to change this comment as you intend:

>> -    /* The address field in the descriptor goes up to bit 39 for ARMv7
>> -     * but up to bit 47 for ARMv8.
>> +    /* The address field in the descriptor goes up to bit 39 for AArch32
>> +     * but up to bit 47 for AArch64.
>>       */
>
>This is not correct -- the descriptor field widths are as the comment
>states before your patch:
> * up to bit 39 for ARMv7
> * up to bit 47 for ARMv8 (whether AArch32 or AArch64)

I could describe there, that the descriptor field is up to bit 47 for ARMv8,
but we use the descaddrmask up to bit 39 for AArch32,
because we don't need other bits in that case to construct next descriptor address,
as it is described in the doc.

Re: [Qemu-devel] [PATCH] target-arm: Fix descriptor address masking in ARM address translation

[Peter Maydell]

On 17 March 2016 at 15:21, Sergey Sorokin <afarallax@yandex.ru> wrote:
> 17.03.2016, 14:40, "Peter Maydell" <peter.maydell@linaro.org>:
>> On 13 March 2016 at 18:28, Sergey Sorokin <afarallax@yandex.ru> wrote:
>>>> If you want to implement the AddressSize checks that's fine,
>>>> but otherwise please leave this bit of the code alone.
>>>
>>>  You said me that my code is not correct, I have proved that it conforms
>>>  to the documentation.
>>>  It's a bit obfuscating when the doc explicitly says to take bits up to 39
>>>  from the descriptor, but in QEMU we take bits up to 47 relying on the check in
>>>  another part of the code, even if both ways are correct.
>>
>> The way the code in QEMU is structured is that we extract the
>> descriptor field in one go and then will operate on it
>> (checking for need to AddressSize fault, etc) as a second
>> action. The field descriptors themselves are the sizes I said.
>
> Well, may be it's enough just to change this comment as you intend:
>
>>> -    /* The address field in the descriptor goes up to bit 39 for ARMv7
>>> -     * but up to bit 47 for ARMv8.
>>> +    /* The address field in the descriptor goes up to bit 39 for AArch32
>>> +     * but up to bit 47 for AArch64.
>>>       */

The comment is correct as it stands.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target-arm: Fix descriptor address masking in ARM address translation

[Sergey Sorokin]

17.03.2016, 18:24, "Peter Maydell" <peter.maydell@linaro.org>:
>  On 17 March 2016 at 15:21, Sergey Sorokin <afarallax@yandex.ru> wrote:
>>   17.03.2016, 14:40, "Peter Maydell" <peter.maydell@linaro.org>:
>>>   On 13 March 2016 at 18:28, Sergey Sorokin <afarallax@yandex.ru> wrote:
>>>>>   If you want to implement the AddressSize checks that's fine,
>>>>>   but otherwise please leave this bit of the code alone.
>>>>
>>>>    You said me that my code is not correct, I have proved that it conforms
>>>>    to the documentation.
>>>>    It's a bit obfuscating when the doc explicitly says to take bits up to 39
>>>>    from the descriptor, but in QEMU we take bits up to 47 relying on the check in
>>>>    another part of the code, even if both ways are correct.
>>>
>>>   The way the code in QEMU is structured is that we extract the
>>>   descriptor field in one go and then will operate on it
>>>   (checking for need to AddressSize fault, etc) as a second
>>>   action. The field descriptors themselves are the sizes I said.
>>
>>   Well, may be it's enough just to change this comment as you intend:
>>
>>>>   - /* The address field in the descriptor goes up to bit 39 for ARMv7
>>>>   - * but up to bit 47 for ARMv8.
>>>>   + /* The address field in the descriptor goes up to bit 39 for AArch32
>>>>   + * but up to bit 47 for AArch64.
>>>>         */
>
>  The comment is correct as it stands.
>
>  thanks
>  -- PMM

I mean in the patch.
We need to fix lower bits in descaddrmask anyway.
So:

I could describe in the comment, that the descriptor field is up to bit 47 for ARMv8 (as long as you want it),
but we use the descaddrmask up to bit 39 for AArch32,
because we don't need other bits in that case to construct next descriptor address.
It is clearly described in the ARM pseudo-code.
Why should we keep in the mask bits from 40 up to 47 if we don't need them? Even if they are all zeroes.
It is a bit obfuscating, as I said.

Re: [Qemu-devel] [Qemu-arm] [PATCH] target-arm: Fix descriptor address masking in ARM address translation

[Tom Hanson]

On 03/21/2016 09:56 AM, Sergey Sorokin wrote:
> 17.03.2016, 18:24, "Peter Maydell" <peter.maydell@linaro.org>:
>>   On 17 March 2016 at 15:21, Sergey Sorokin <afarallax@yandex.ru> wrote:
>>>    17.03.2016, 14:40, "Peter Maydell" <peter.maydell@linaro.org>:
>>>>    On 13 March 2016 at 18:28, Sergey Sorokin <afarallax@yandex.ru> wrote:
>>>>>>    If you want to implement the AddressSize checks that's fine,
>>>>>>    but otherwise please leave this bit of the code alone.
>>>>>
>>>>>     You said me that my code is not correct, I have proved that it conforms
>>>>>     to the documentation.
>>>>>     It's a bit obfuscating when the doc explicitly says to take bits up to 39
>>>>>     from the descriptor, but in QEMU we take bits up to 47 relying on the check in
>>>>>     another part of the code, even if both ways are correct.
>>>>
>>>>    The way the code in QEMU is structured is that we extract the
>>>>    descriptor field in one go and then will operate on it
>>>>    (checking for need to AddressSize fault, etc) as a second
>>>>    action. The field descriptors themselves are the sizes I said.
>>>
>>>    Well, may be it's enough just to change this comment as you intend:
>>>
>>>>>    - /* The address field in the descriptor goes up to bit 39 for ARMv7
>>>>>    - * but up to bit 47 for ARMv8.
>>>>>    + /* The address field in the descriptor goes up to bit 39 for AArch32
>>>>>    + * but up to bit 47 for AArch64.
>>>>>          */
>>
>>   The comment is correct as it stands.
>>
>>   thanks
>>   -- PMM
>
> I mean in the patch.
> We need to fix lower bits in descaddrmask anyway.
> So:
>
> I could describe in the comment, that the descriptor field is up to bit 47 for ARMv8 (as long as you want it),
> but we use the descaddrmask up to bit 39 for AArch32,
> because we don't need other bits in that case to construct next descriptor address.
> It is clearly described in the ARM pseudo-code.
> Why should we keep in the mask bits from 40 up to 47 if we don't need them? Even if they are all zeroes.
> It is a bit obfuscating, as I said.
>
  I agree with Peter.  The original comment is correct.

Looking at the TLBRecord AArch32.TranslationTableWalkLD pseudocode, it is treating the AArch32 address as 48 bits long.  For example:
     if !IsZero(baseregister<47:40>) then
         level = 0;
         result.addrdesc.fault = AArch32.AddressSizeFault(ipaddress, domain, level, acctype, iswrite,
                                                          secondstage, s2fs1walk);
     return result;

This requires that an AArch32 address have specific values up through bit 47.

Re: [Qemu-devel] [Qemu-arm] [PATCH] target-arm: Fix descriptor address masking in ARM address translation

[Sergey Sorokin]

[-- Attachment #1: Type: text/html, Size: 3326 bytes --]