[RFC,3/5] softmmu: add a tlb_vaddr_to_host_fill function

The softmmu code already provides a tlb_vaddr_to_host function, which
returns the host address corresponding to a guest virtual address,
*if it is already in the QEMU MMU TLB*.

This patch is an attempt to have a function which try to fill the TLB
entry if it is not already in the QEMU MMU TLB, possibly trigger a guest
fault. It can be used directly in helpers. For that it creates a common
function with a boolean to tell if the TLB needs to be filled or not. If
yes, it causes tlb_fill, which might trigger an exception or succeed in
which case the tlbentry pointer need to be reloaded.

I also had to change the MMIO test part. It seems that in write mode
some TLB entries are filled with TLB_NOTDIRTY. They are caught by the
MMIO test and a NULL pointer is returned instead. I am not sure of my
change, but I guess the current softmmu code has the same issue.

At the same time, it defines the same function for the user mode, so
that we can write helpers using the same code for softmmu and user mode,
just like cpu_ldxx_data() functions works for both.

It also replaces hard-coded values for the access-type by the
corresponding constants.

Cc: Richard Henderson <rth@twiddle.net>
Cc: Alexander Graf <agraf@suse.de>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Yongbok Kim <yongbok.kim@imgtec.com>
Cc: Leon Alrae <leon.alrae@imgtec.com>
Cc: Andreas Färber <afaerber@suse.de>
Cc: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 include/exec/cpu_ldst.h | 100 +++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 77 insertions(+), 23 deletions(-)

On 2015-06-02 13:26, Aurelien Jarno wrote:
> The softmmu code already provides a tlb_vaddr_to_host function, which
> returns the host address corresponding to a guest virtual address,
> *if it is already in the QEMU MMU TLB*.
> 
> This patch is an attempt to have a function which try to fill the TLB
> entry if it is not already in the QEMU MMU TLB, possibly trigger a guest
> fault. It can be used directly in helpers. For that it creates a common
> function with a boolean to tell if the TLB needs to be filled or not. If
> yes, it causes tlb_fill, which might trigger an exception or succeed in
> which case the tlbentry pointer need to be reloaded.
> 
> I also had to change the MMIO test part. It seems that in write mode
> some TLB entries are filled with TLB_NOTDIRTY. They are caught by the
> MMIO test and a NULL pointer is returned instead. I am not sure of my
> change, but I guess the current softmmu code has the same issue.

It looks like we have to go through the MMIO functions to get the
TLB_NOTDIRTY bit cleaned correctly. This is something we don't want for
probe_write, so we definitely want two different functions.

On 06/02/2015 04:26 AM, Aurelien Jarno wrote:
>      int index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
> -    CPUTLBEntry *tlbentry = &env->tlb_table[mmu_idx][index];
> +    CPUTLBEntry *tlbentry;
>      target_ulong tlb_addr;
>      uintptr_t haddr;
>  
> +again:
> +    tlbentry = &env->tlb_table[mmu_idx][index];
> +
>      switch (access_type) {
> -    case 0:
> +    case MMU_DATA_LOAD:
>          tlb_addr = tlbentry->addr_read;
>          break;
> -    case 1:
> +    case MMU_DATA_STORE:
>          tlb_addr = tlbentry->addr_write;
>          break;
> -    case 2:
> +    case MMU_INST_FETCH:
>          tlb_addr = tlbentry->addr_code;
>          break;
>      default:
> @@ -347,10 +350,14 @@ static inline void *tlb_vaddr_to_host(CPUArchState *env, target_ulong addr,
>      if ((addr & TARGET_PAGE_MASK)
>          != (tlb_addr & (TARGET_PAGE_MASK | TLB_INVALID_MASK))) {
>          /* TLB entry is for a different page */
> +        if (fill) {
> +            tlb_fill(ENV_GET_CPU(env), addr, access_type, mmu_idx, retaddr);
> +            goto again;
> +        }
>          return NULL;
>      }

To properly perform a fill, you also ought to check the victim cache.
There's a macro to do that in softmmu_template.h, which is why I
placed probe_write there.  It's not so convenient to use with a
variable type though.

In addition, the address of tlbentry cannot change, so there's no
point in recomputing that.  Indeed, you'd probably be better off
saving &addr_foo so that you only have to go through the switch once.

  switch (access_type) {
  case N:
    tlb_addr_ptr = &tlbentry->addr_foo;
    break;
  }
  tlb_addr = *tlb_addr_ptr;
  if (...) {
    if (!VICTIM_TLB_HIT(...)) {
      if (!fill) {
        return NULL;
      }
      tlb_fill(...);
    }
    tlb_addr = *tlb_addr_ptr;
  }

and thus there's no loop to be mis-predicted.


r~

On 06/02/2015 01:10 PM, Aurelien Jarno wrote:
> It looks like we have to go through the MMIO functions to get the
> TLB_NOTDIRTY bit cleaned correctly. This is something we don't want for
> probe_write, so we definitely want two different functions.

I think that's why target-arm does it's somewhat convoluted loop in which it
stores one byte to the page and then tries again to use tlb_vaddr_to_host.

If the page isn't in the tlb, we perform a complete store and thus both pull
the page into the tlb as well as mark it dirty.  Thus if the page still isn't
present for the second vaddr_to_host, it really is I/O, or is being watched by
the debugger, or something equally unlikely.


r~

On 2 June 2015 at 21:58, Richard Henderson <rth@twiddle.net> wrote:
> On 06/02/2015 01:10 PM, Aurelien Jarno wrote:
>> It looks like we have to go through the MMIO functions to get the
>> TLB_NOTDIRTY bit cleaned correctly. This is something we don't want for
>> probe_write, so we definitely want two different functions.
>
> I think that's why target-arm does it's somewhat convoluted loop in which it
> stores one byte to the page and then tries again to use tlb_vaddr_to_host.

Also if we take a fault we must do so with the fault address set
to the exact address passed in by the guest in the register,
even if that isn't the first (QEMU) page in the region being cleared.
So we must test that exact byte first.

-- PMM

On 2015-06-02 13:54, Richard Henderson wrote:
> On 06/02/2015 04:26 AM, Aurelien Jarno wrote:
> >      int index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
> > -    CPUTLBEntry *tlbentry = &env->tlb_table[mmu_idx][index];
> > +    CPUTLBEntry *tlbentry;
> >      target_ulong tlb_addr;
> >      uintptr_t haddr;
> >  
> > +again:
> > +    tlbentry = &env->tlb_table[mmu_idx][index];
> > +
> >      switch (access_type) {
> > -    case 0:
> > +    case MMU_DATA_LOAD:
> >          tlb_addr = tlbentry->addr_read;
> >          break;
> > -    case 1:
> > +    case MMU_DATA_STORE:
> >          tlb_addr = tlbentry->addr_write;
> >          break;
> > -    case 2:
> > +    case MMU_INST_FETCH:
> >          tlb_addr = tlbentry->addr_code;
> >          break;
> >      default:
> > @@ -347,10 +350,14 @@ static inline void *tlb_vaddr_to_host(CPUArchState *env, target_ulong addr,
> >      if ((addr & TARGET_PAGE_MASK)
> >          != (tlb_addr & (TARGET_PAGE_MASK | TLB_INVALID_MASK))) {
> >          /* TLB entry is for a different page */
> > +        if (fill) {
> > +            tlb_fill(ENV_GET_CPU(env), addr, access_type, mmu_idx, retaddr);
> > +            goto again;
> > +        }
> >          return NULL;
> >      }
> 
> To properly perform a fill, you also ought to check the victim cache.
> There's a macro to do that in softmmu_template.h, which is why I
> placed probe_write there.  It's not so convenient to use with a
> variable type though.
> 

Unfortunately that means we can't cleanly provide a probe_write function
doing nothing for the user-mode case. That would allow to avoid to many
#ifdef in the helper code. For me the softmmu_template.h is supposed to
only contain the code called by the helpers or by the glue in
cpu_ldst*.h

That also means the current tlb_vaddr_to_host code doesn't look in the
victim cache and that there is no easy way to fix that, though that's
less problematic.

On 2015-06-02 13:58, Richard Henderson wrote:
> On 06/02/2015 01:10 PM, Aurelien Jarno wrote:
> > It looks like we have to go through the MMIO functions to get the
> > TLB_NOTDIRTY bit cleaned correctly. This is something we don't want for
> > probe_write, so we definitely want two different functions.
> 
> I think that's why target-arm does it's somewhat convoluted loop in which it
> stores one byte to the page and then tries again to use tlb_vaddr_to_host.
> 
> If the page isn't in the tlb, we perform a complete store and thus both pull
> the page into the tlb as well as mark it dirty.  Thus if the page still isn't
> present for the second vaddr_to_host, it really is I/O, or is being watched by
> the debugger, or something equally unlikely.

Unfortunately it seems there is no way to guarantee that the full page
can be marked as dirty at the same time, even on s390x without MMIO.

I will try to rewrite the code to have a fallback code for the initial
TLB filling, that could also be used in the case the whole page can't be
marked as dirty. That's relatively easy when it deals with memset like
functions, but it becomes more tricky for memcpy or string functions.