[Qemu-devel] [PATCH] target-arm: Fix translation level on early translation faults

[Qemu-devel] [PATCH] target-arm: Fix translation level on early translation faults

[Sergey Sorokin]

Qemu reports translation fault on 1st level instead of 0th level in case of
AArch64 address translation if the translation table walk is disabled or
the address is in the gap between the two regions.

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

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 18c8296..09f920c 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -7238,6 +7238,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
      * support for those page table walks.
      */
     if (arm_el_is_aa64(env, el)) {
+        level = 0;
         va_size = 64;
         if (el > 1) {
             if (mmu_idx != ARMMMUIdx_S2NS) {
-- 
1.9.3

Re: [Qemu-devel] [PATCH] target-arm: Fix translation level on early translation faults

[Sergey Fedorov]

On 02.03.2016 21:04, Sergey Sorokin wrote:
> Qemu reports translation fault on 1st level instead of 0th level in case of
> AArch64 address translation if the translation table walk is disabled or
> the address is in the gap between the two regions.

It's probably not a very clear description in the commit message. IIUC,
level 0 fault is reported in case of any fault from TTBR in AArch64 state.

Best regards,
Sergey

>
> Signed-off-by: Sergey Sorokin <afarallax@yandex.ru>
> ---
>  target-arm/helper.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 18c8296..09f920c 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -7238,6 +7238,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>       * support for those page table walks.
>       */
>      if (arm_el_is_aa64(env, el)) {
> +        level = 0;
>          va_size = 64;
>          if (el > 1) {
>              if (mmu_idx != ARMMMUIdx_S2NS) {

Re: [Qemu-devel] [PATCH] target-arm: Fix translation level on early translation faults

[Peter Maydell]

On 2 March 2016 at 19:19, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
> On 02.03.2016 21:04, Sergey Sorokin wrote:
>> Qemu reports translation fault on 1st level instead of 0th level in case of
>> AArch64 address translation if the translation table walk is disabled or
>> the address is in the gap between the two regions.
>
> It's probably not a very clear description in the commit message. IIUC,
> level 0 fault is reported in case of any fault from TTBR in AArch64 state.

Yes (though you mean "under an AArch64 translation regime"). Conversely, the
only fault reported at level 0 under an AArch32 translation regime is
the AddressSize fault (for bad addresses in TTBR0/1), which we don't
currently implement.

There's also a code path later in the function that does
    level = va_size == 64 ? 0 : 1;

but I'm not sure it's worth rearranging that code to avoid the
duplication of "what level do we report this kind of fault at?".

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target-arm: Fix translation level on early translation faults

[Sergey Fedorov]

On 03.03.2016 16:49, Peter Maydell wrote:
> On 2 March 2016 at 19:19, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>> On 02.03.2016 21:04, Sergey Sorokin wrote:
>>> Qemu reports translation fault on 1st level instead of 0th level in case of
>>> AArch64 address translation if the translation table walk is disabled or
>>> the address is in the gap between the two regions.
>> It's probably not a very clear description in the commit message. IIUC,
>> level 0 fault is reported in case of any fault from TTBR in AArch64 state.
> Yes (though you mean "under an AArch64 translation regime"). Conversely, the
> only fault reported at level 0 under an AArch32 translation regime is
> the AddressSize fault (for bad addresses in TTBR0/1), which we don't
> currently implement.
>
> There's also a code path later in the function that does
>     level = va_size == 64 ? 0 : 1;
>
> but I'm not sure it's worth rearranging that code to avoid the
> duplication of "what level do we report this kind of fault at?".

Right, but actually I think this patch is going to fix the two "goto
do_fault" cases which can happen before this "level = va_size == 64 ? 0
: 1", namely the EDP check and the check for virtual address which is in
the gap between TTBR0 and TTBR1 regions.

Best regards,
Sergey

Re: [Qemu-devel] [Qemu-arm] [PATCH] target-arm: Fix translation level on early translation faults

[Peter Maydell]

On 3 March 2016 at 14:48, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
> On 03.03.2016 16:49, Peter Maydell wrote:
>> On 2 March 2016 at 19:19, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>>> On 02.03.2016 21:04, Sergey Sorokin wrote:
>>>> Qemu reports translation fault on 1st level instead of 0th level in case of
>>>> AArch64 address translation if the translation table walk is disabled or
>>>> the address is in the gap between the two regions.
>>> It's probably not a very clear description in the commit message. IIUC,
>>> level 0 fault is reported in case of any fault from TTBR in AArch64 state.
>> Yes (though you mean "under an AArch64 translation regime"). Conversely, the
>> only fault reported at level 0 under an AArch32 translation regime is
>> the AddressSize fault (for bad addresses in TTBR0/1), which we don't
>> currently implement.
>>
>> There's also a code path later in the function that does
>>     level = va_size == 64 ? 0 : 1;
>>
>> but I'm not sure it's worth rearranging that code to avoid the
>> duplication of "what level do we report this kind of fault at?".
>
> Right, but actually I think this patch is going to fix the two "goto
> do_fault" cases which can happen before this "level = va_size == 64 ? 0
> : 1", namely the EDP check and the check for virtual address which is in
> the gap between TTBR0 and TTBR1 regions.

Yes, this patch is definitely fixing a bug; I'm just mentioning that other
code path because it seems to be the result of previously fixing the bug
for a particular special case...

-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH] target-arm: Fix translation level on early translation faults

[Sergey Fedorov]

On 03.03.2016 17:55, Peter Maydell wrote:
> On 3 March 2016 at 14:48, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>> On 03.03.2016 16:49, Peter Maydell wrote:
>>> On 2 March 2016 at 19:19, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>>>> On 02.03.2016 21:04, Sergey Sorokin wrote:
>>>>> Qemu reports translation fault on 1st level instead of 0th level in case of
>>>>> AArch64 address translation if the translation table walk is disabled or
>>>>> the address is in the gap between the two regions.
>>>> It's probably not a very clear description in the commit message. IIUC,
>>>> level 0 fault is reported in case of any fault from TTBR in AArch64 state.
>>> Yes (though you mean "under an AArch64 translation regime"). Conversely, the
>>> only fault reported at level 0 under an AArch32 translation regime is
>>> the AddressSize fault (for bad addresses in TTBR0/1), which we don't
>>> currently implement.
>>>
>>> There's also a code path later in the function that does
>>>     level = va_size == 64 ? 0 : 1;
>>>
>>> but I'm not sure it's worth rearranging that code to avoid the
>>> duplication of "what level do we report this kind of fault at?".
>> Right, but actually I think this patch is going to fix the two "goto
>> do_fault" cases which can happen before this "level = va_size == 64 ? 0
>> : 1", namely the EDP check and the check for virtual address which is in
>> the gap between TTBR0 and TTBR1 regions.
> Yes, this patch is definitely fixing a bug; I'm just mentioning that other
> code path because it seems to be the result of previously fixing the bug
> for a particular special case...
>
>

Ah, right, I think I understand you :) So we'd better remove these lines:

            /* AArch64 reports these as level 0 faults.
             * AArch32 reports these as level 1 faults.
             */
            level = va_size == 64 ? 0 : 1;
            fault_type = translation_fault;

Kind regards,
Sergey

Re: [Qemu-devel] [Qemu-arm] [PATCH] target-arm: Fix translation level on early translation faults

[Peter Maydell]

On 3 March 2016 at 16:37, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
> On 03.03.2016 17:55, Peter Maydell wrote:
>> Yes, this patch is definitely fixing a bug; I'm just mentioning that other
>> code path because it seems to be the result of previously fixing the bug
>> for a particular special case...
>>
>>
>
> Ah, right, I think I understand you :) So we'd better remove these lines:
>
>             /* AArch64 reports these as level 0 faults.
>              * AArch32 reports these as level 1 faults.
>              */
>             level = va_size == 64 ? 0 : 1;
>             fault_type = translation_fault;


Those lines come after some code which has set level to something
else, so you'd need to rearrange that code a bit so it didn't
set level before it had determined that there wasn't a fault.

thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH] target-arm: Fix translation level on early translation faults

[Sergey Sorokin]

03.03.2016, 19:54, "Peter Maydell" <peter.maydell@linaro.org>:
>  On 3 March 2016 at 16:37, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>>   On 03.03.2016 17:55, Peter Maydell wrote:
>>>   Yes, this patch is definitely fixing a bug; I'm just mentioning that other
>>>   code path because it seems to be the result of previously fixing the bug
>>>   for a particular special case...
>>
>>   Ah, right, I think I understand you :) So we'd better remove these lines:
>>
>>               /* AArch64 reports these as level 0 faults.
>>                * AArch32 reports these as level 1 faults.
>>                */
>>               level = va_size == 64 ? 0 : 1;
>>               fault_type = translation_fault;
>
>  Those lines come after some code which has set level to something
>  else, so you'd need to rearrange that code a bit so it didn't
>  set level before it had determined that there wasn't a fault.
>
>  thanks
>  -- PMM

Do we really need to rearrange the code?
In pseudo-code the level is set explicitly as well, in case of a fault.

Re: [Qemu-devel] [Qemu-arm] [PATCH] target-arm: Fix translation level on early translation faults

[Peter Maydell]

On 4 March 2016 at 15:03, Sergey Sorokin <afarallax@yandex.ru> wrote:
> Do we really need to rearrange the code?
> In pseudo-code the level is set explicitly as well, in case of a fault.

That's why when I originally brought the point up I said
"but I'm not sure it's worth rearranging that code"...
If you want to neaten things up so that we pick the initial
level for faults (0 or 1 depending on va_size) in one place,
then you need to rearrange the code. If you don't want to
rearrange that bit of code you can leave it all as it is, but
you don't get the neat "only one place makes this decision"
structure. You could argue either way and I don't much mind
which we do.

(Incidentally the pseudocode is not always a good guide
for the structure of code; sometimes it makes clear choices,
but sometimes it makes confusing choices.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target-arm: Fix translation level on early translation faults

[Sergey Fedorov]

On 02.03.2016 21:04, Sergey Sorokin wrote:
> Qemu reports translation fault on 1st level instead of 0th level in case of
> AArch64 address translation if the translation table walk is disabled or
> the address is in the gap between the two regions.
>
> Signed-off-by: Sergey Sorokin <afarallax@yandex.ru>
> ---
>   target-arm/helper.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 18c8296..09f920c 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -7238,6 +7238,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>        * support for those page table walks.
>        */
>       if (arm_el_is_aa64(env, el)) {
> +        level = 0;
>           va_size = 64;
>           if (el > 1) {
>               if (mmu_idx != ARMMMUIdx_S2NS) {

I think we'd better set the level variable to 1 for AArch32 in the else 
clause explicitly and drop its initialization in the beginning of the 
function. Otherwise it looks like AArch64 is a kind of special case.

Best regards,
Sergey