The xtensa frontend calls get_page_addr_code() from its
itlb_hit_test helper function. This function is really part
of the TCG core's internals, and calling it from a target
helper makes it awkward to make changes to that core code.
It also means that we don't pass the correct retaddr to
tlb_fill(), so we won't correctly handle the case where
an exception is generated.

The helper is used for the instructions IHI, IHU and IPFL.

Change it to call cpu_ldb_code_ra() instead.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
This retains the current behaviour that all these insns
may cause exceptions both for MMU permissions checks
failures and also for "resulting physaddr doesn't point
at memory".

My reading of the ISA manual is that this isn't strictly
correct for IHI and IHU, which ought to only cause MMU
exceptions but not actually do a memory access. If we
wanted to fix that, the right thing would be to split
them into their own helper function, which could then
just do a tlb_fill().

Tagged as RFC because I don't have any xtensa test images.

My motivation here is that at some point I'd like us to
support execution from arbitrary MMIO/small MPU regions/etc,
for which purpose get_page_addr_code() will change to return
-1 to mean "this isn't RAM, load a single insn into a
throwaway TB and execute it"...

 target/xtensa/op_helper.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/target/xtensa/op_helper.c b/target/xtensa/op_helper.c
index 8a8c763c631..34dc1eb68fe 100644
--- a/target/xtensa/op_helper.c
+++ b/target/xtensa/op_helper.c
@@ -465,7 +465,11 @@ void HELPER(check_interrupts)(CPUXtensaState *env)
 
 void HELPER(itlb_hit_test)(CPUXtensaState *env, uint32_t vaddr)
 {
-    get_page_addr_code(env, vaddr);
+    /*
+     * Attempt the memory load; we don't care about the result but
+     * only the side-effects (ie any MMU or other exception)
+     */
+    cpu_ldub_code_ra(env, vaddr, GETPC());
 }
 
 /*!
-- 
2.17.1

On 06/22/2018 06:58 AM, Peter Maydell wrote:
> The xtensa frontend calls get_page_addr_code() from its
> itlb_hit_test helper function. This function is really part
> of the TCG core's internals, and calling it from a target
> helper makes it awkward to make changes to that core code.
> It also means that we don't pass the correct retaddr to
> tlb_fill(), so we won't correctly handle the case where
> an exception is generated.
> 
> The helper is used for the instructions IHI, IHU and IPFL.

I think the implementation of these instructions is completely wrong.

(1a) IHI is not invalidating the cacheline within env->config->itlb,
(1b) IHI is not invalidating the qemu TLB that might contain a copy
     of same.

(2a) IPFL is not locking the entry in env->config->itlb,
(2b) IHU is not unlocking the same entry.
(2c) "Xtensa ISA implementations that do not implement cache locking
     must raise an illegal instruction exception when [IPFL or IHU]
     is executed."


r~

On Sat, Jun 30, 2018 at 10:32 AM, Richard Henderson
<richard.henderson@linaro.org> wrote:
> On 06/22/2018 06:58 AM, Peter Maydell wrote:
>> The xtensa frontend calls get_page_addr_code() from its
>> itlb_hit_test helper function. This function is really part
>> of the TCG core's internals, and calling it from a target
>> helper makes it awkward to make changes to that core code.
>> It also means that we don't pass the correct retaddr to
>> tlb_fill(), so we won't correctly handle the case where
>> an exception is generated.
>>
>> The helper is used for the instructions IHI, IHU and IPFL.
>
> I think the implementation of these instructions is completely wrong.
>
> (1a) IHI is not invalidating the cacheline within env->config->itlb,
> (1b) IHI is not invalidating the qemu TLB that might contain a copy
>      of same.
> (2a) IPFL is not locking the entry in env->config->itlb,
> (2b) IHU is not unlocking the same entry.

All the above instructions are meant to invalidate cache, not the TLB.

> (2c) "Xtensa ISA implementations that do not implement cache locking
>      must raise an illegal instruction exception when [IPFL or IHU]
>      is executed."

They will raise an illegal instruction exception, because such CPUs
will not recognize these instructions in the xtensa_opcode_decode.

I believe that the implementation we have currently is rather accurate.

-- 
Thanks.
-- Max

On Fri, Jun 22, 2018 at 6:58 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> The xtensa frontend calls get_page_addr_code() from its
> itlb_hit_test helper function. This function is really part
> of the TCG core's internals, and calling it from a target
> helper makes it awkward to make changes to that core code.
> It also means that we don't pass the correct retaddr to
> tlb_fill(), so we won't correctly handle the case where
> an exception is generated.
>
> The helper is used for the instructions IHI, IHU and IPFL.
>
> Change it to call cpu_ldb_code_ra() instead.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> This retains the current behaviour that all these insns
> may cause exceptions both for MMU permissions checks
> failures and also for "resulting physaddr doesn't point
> at memory".
>
> My reading of the ISA manual is that this isn't strictly
> correct for IHI and IHU, which ought to only cause MMU
> exceptions but not actually do a memory access. If we
> wanted to fix that, the right thing would be to split
> them into their own helper function, which could then
> just do a tlb_fill().
>
> Tagged as RFC because I don't have any xtensa test images.
>
> My motivation here is that at some point I'd like us to
> support execution from arbitrary MMIO/small MPU regions/etc,
> for which purpose get_page_addr_code() will change to return
> -1 to mean "this isn't RAM, load a single insn into a
> throwaway TB and execute it"...

I'm taking this patch, thank you. This test is not worse than
what is done for the data cache. And if it causes problem it'd
be easy to spot, because an exception code for accessing
nonexistent physical memory is very unusual.

-- 
Thanks.
-- Max