| Message ID | 1456941872-8791-1-git-send-email-afarallax@yandex.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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) {
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
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
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
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
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
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
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.
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
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) {
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(+)