Any write to a device might cause a re-arrangement of memory
triggering a TLB flush and potential re-size of the TLB invalidating
previous entries. This would cause users of qemu_plugin_get_hwaddr()
to see the warning:

  invalid use of qemu_plugin_get_hwaddr

because of the failed tlb_lookup which should always succeed. We catch
this case by checking to see if the list of entries has been cleared
and if so triggering a re-fill.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 accel/tcg/cputlb.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index eb2cf9de5e6..b7d329f7155 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1091,6 +1091,20 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
                                MMU_DATA_STORE, mmu_idx, iotlbentry->attrs, r,
                                retaddr);
     }
+
+    /*
+     * The memory_region_dispatch may have triggered a flush/resize
+     * so for plugins we need to ensure we have reset the tlb_entry
+     * so any later lookup is correct.
+     */
+#ifdef CONFIG_PLUGIN
+    if (env_tlb(env)->d[mmu_idx].n_used_entries == 0) {
+        int size = op & MO_SIZE;
+        tlb_fill(env_cpu(env), addr, size, MMU_DATA_STORE,
+                 mmu_idx, retaddr);
+    }
+#endif
+
     if (locked) {
         qemu_mutex_unlock_iothread();
     }
-- 
2.20.1

On 6/2/20 8:46 AM, Alex Bennée wrote:
> Any write to a device might cause a re-arrangement of memory
> triggering a TLB flush and potential re-size of the TLB invalidating
> previous entries. This would cause users of qemu_plugin_get_hwaddr()
> to see the warning:
> 
>   invalid use of qemu_plugin_get_hwaddr
> 
> because of the failed tlb_lookup which should always succeed. We catch
> this case by checking to see if the list of entries has been cleared
> and if so triggering a re-fill.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  accel/tcg/cputlb.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index eb2cf9de5e6..b7d329f7155 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -1091,6 +1091,20 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
>                                 MMU_DATA_STORE, mmu_idx, iotlbentry->attrs, r,
>                                 retaddr);
>      }
> +
> +    /*
> +     * The memory_region_dispatch may have triggered a flush/resize
> +     * so for plugins we need to ensure we have reset the tlb_entry
> +     * so any later lookup is correct.
> +     */
> +#ifdef CONFIG_PLUGIN
> +    if (env_tlb(env)->d[mmu_idx].n_used_entries == 0) {
> +        int size = op & MO_SIZE;
> +        tlb_fill(env_cpu(env), addr, size, MMU_DATA_STORE,
> +                 mmu_idx, retaddr);

Ouch.  What if the target has a soft tlb fill, so this requires a call into the
OS, so this fill actually raises another exception?  This will not be happy fun
making.

I recall I had objections to recording this translation, saying that "we can
always get it back again".  Clearly I was wrong, and we should just preserve
the required CPUTLBEntry details before they're lost by a device.


r~

Richard Henderson <richard.henderson@linaro.org> writes:

> On 6/2/20 8:46 AM, Alex Bennée wrote:
>> Any write to a device might cause a re-arrangement of memory
>> triggering a TLB flush and potential re-size of the TLB invalidating
>> previous entries. This would cause users of qemu_plugin_get_hwaddr()
>> to see the warning:
>> 
>>   invalid use of qemu_plugin_get_hwaddr
>> 
>> because of the failed tlb_lookup which should always succeed. We catch
>> this case by checking to see if the list of entries has been cleared
>> and if so triggering a re-fill.
>> 
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  accel/tcg/cputlb.c | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>> 
>> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
>> index eb2cf9de5e6..b7d329f7155 100644
>> --- a/accel/tcg/cputlb.c
>> +++ b/accel/tcg/cputlb.c
>> @@ -1091,6 +1091,20 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
>>                                 MMU_DATA_STORE, mmu_idx, iotlbentry->attrs, r,
>>                                 retaddr);
>>      }
>> +
>> +    /*
>> +     * The memory_region_dispatch may have triggered a flush/resize
>> +     * so for plugins we need to ensure we have reset the tlb_entry
>> +     * so any later lookup is correct.
>> +     */
>> +#ifdef CONFIG_PLUGIN
>> +    if (env_tlb(env)->d[mmu_idx].n_used_entries == 0) {
>> +        int size = op & MO_SIZE;
>> +        tlb_fill(env_cpu(env), addr, size, MMU_DATA_STORE,
>> +                 mmu_idx, retaddr);
>
> Ouch.  What if the target has a soft tlb fill, so this requires a call into the
> OS, so this fill actually raises another exception?  This will not be happy fun
> making.
>
> I recall I had objections to recording this translation, saying that "we can
> always get it back again".  Clearly I was wrong, and we should just preserve
> the required CPUTLBEntry details before they're lost by a device.

Maybe we could just RCU the old TLB if it gets flushed thus ensuring the
whole TLB is preserved until after the critical section (i.e. between
the actual store and looking it up). However I don't know if the
MemoryRegion will be similarly preserved.

Paolo?

>
>
> r~


-- 
Alex Bennée