qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: "Emilio G. Cota" <cota@braap.org>
To: "Alex Bennée" <alex.bennee@linaro.org>
Cc: robert.foley@linaro.org, qemu-devel@nongnu.org,
	robhenry@microsoft.com, aaron@os.amperecomputing.com,
	Paolo Bonzini <pbonzini@redhat.com>,
	kuhn.chenqun@huawei.com, peter.puhov@linaro.org,
	Richard Henderson <rth@twiddle.net>
Subject: Re: [PATCH v2 3/6] cputlb: ensure we save the IOTLB data in case of reset
Date: Sun, 21 Jun 2020 16:33:07 -0400	[thread overview]
Message-ID: <20200621203307.GA168836@sff> (raw)
In-Reply-To: <20200610155509.12850-4-alex.bennee@linaro.org>

On Wed, Jun 10, 2020 at 16:55:06 +0100, 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. To
> prevent this we save the IOTLB data in case it is later needed by a
> plugin doing a lookup.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> ---
> v2
>   - save the entry instead of re-running the tlb_fill.
> 
> squash! cputlb: ensure we save the IOTLB entry in case of reset
> ---
>  accel/tcg/cputlb.c | 63 ++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 61 insertions(+), 2 deletions(-)
> 
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index eb2cf9de5e6..9bf9e479c7c 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -1058,6 +1058,47 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
>      return val;
>  }
>  
> +#ifdef CONFIG_PLUGIN
> +
> +typedef struct SavedIOTLB {
> +    struct rcu_head rcu;
> +    struct SavedIOTLB **save_loc;
> +    MemoryRegionSection *section;
> +    hwaddr mr_offset;
> +} SavedIOTLB;
> +
> +static void clean_saved_entry(SavedIOTLB *s)
> +{
> +    atomic_rcu_set(s->save_loc, NULL);

This will race with the CPU thread that sets saved_for_plugin in
save_iotlb_data().

> +    g_free(s);
> +}
> +
> +static __thread SavedIOTLB *saved_for_plugin;

Apologies if this has been discussed, but why is this using TLS
variables and not state embedded in CPUState?
I see that qemu_plugin_get_hwaddr does not take a cpu_index, but
maybe it should? We could then just embed the RCU pointer in CPUState.

> +
> +/*
> + * Save a potentially trashed IOTLB entry for later lookup by plugin.
> + *
> + * We also need to track the thread storage address because the RCU
> + * cleanup that runs when we leave the critical region (the current
> + * execution) is actually in a different thread.
> + */
> +static void save_iotlb_data(MemoryRegionSection *section, hwaddr mr_offset)
> +{
> +    SavedIOTLB *s = g_new(SavedIOTLB, 1);
> +    s->save_loc = &saved_for_plugin;
> +    s->section = section;
> +    s->mr_offset = mr_offset;
> +    atomic_rcu_set(&saved_for_plugin, s);
> +    call_rcu(s, clean_saved_entry, rcu);

Here we could just publish the new pointer and g_free_rcu the old
one, if any.

> +}
> +
> +#else
> +static void save_iotlb_data(MemoryRegionSection *section, hwaddr mr_offset)
> +{
> +    /* do nothing */
> +}
> +#endif
> +
>  static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
>                        int mmu_idx, uint64_t val, target_ulong addr,
>                        uintptr_t retaddr, MemOp op)
> @@ -1077,6 +1118,12 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
>      }
>      cpu->mem_io_pc = retaddr;
>  
> +    /*
> +     * The memory_region_dispatch may trigger a flush/resize
> +     * so for plugins we save the iotlb_data just in case.
> +     */
> +    save_iotlb_data(section, mr_offset);
> +
>      if (mr->global_locking && !qemu_mutex_iothread_locked()) {
>          qemu_mutex_lock_iothread();
>          locked = true;
> @@ -1091,6 +1138,7 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
>                                 MMU_DATA_STORE, mmu_idx, iotlbentry->attrs, r,
>                                 retaddr);
>      }
> +

Stray whitespace change.

>      if (locked) {
>          qemu_mutex_unlock_iothread();
>      }
> @@ -1366,8 +1414,11 @@ void *tlb_vaddr_to_host(CPUArchState *env, abi_ptr addr,
>   * in the softmmu lookup code (or helper). We don't handle re-fills or
>   * checking the victim table. This is purely informational.
>   *
> - * This should never fail as the memory access being instrumented
> - * should have just filled the TLB.
> + * This almost never fails as the memory access being instrumented
> + * should have just filled the TLB. The one corner case is io_writex
> + * which can cause TLB flushes and potential resizing of the TLBs
> + * loosing the information we need. In those cases we need to recover
> + * data from a thread local copy of the io_tlb entry.
>   */
>  
>  bool tlb_plugin_lookup(CPUState *cpu, target_ulong addr, int mmu_idx,
> @@ -1391,6 +1442,14 @@ bool tlb_plugin_lookup(CPUState *cpu, target_ulong addr, int mmu_idx,
>              data->v.ram.hostaddr = addr + tlbe->addend;
>          }
>          return true;
> +    } else {
> +        SavedIOTLB *saved = atomic_rcu_read(&saved_for_plugin);
> +        if (saved) {
> +            data->is_io = true;
> +            data->v.io.section = saved->section;
> +            data->v.io.offset = saved->mr_offset;
> +            return true;
> +        }

Shouldn't we check that the contents of the saved IOTLB match the
parameters of the lookup? Otherwise passing a random address is likely
to land here.

Thanks,
		Emilio


  reply	other threads:[~2020-06-21 20:33 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-10 15:55 [PATCH v2 0/6] plugins/next (lockstep, api, hwprofile) Alex Bennée
2020-06-10 15:55 ` [PATCH v2 1/6] iotests: 194: wait migration completion on target too Alex Bennée
2020-06-10 16:38   ` Alex Bennée
2020-06-10 15:55 ` [PATCH v2 2/6] plugins: new lockstep plugin for debugging TCG changes Alex Bennée
2020-06-11 17:04   ` Robert Foley
2020-06-10 15:55 ` [PATCH v2 3/6] cputlb: ensure we save the IOTLB data in case of reset Alex Bennée
2020-06-21 20:33   ` Emilio G. Cota [this message]
2020-06-22  9:02     ` Alex Bennée
2020-06-23  1:54       ` Emilio G. Cota
2020-06-10 15:55 ` [PATCH v2 4/6] hw/virtio/pci: include vdev name in registered PCI sections Alex Bennée
2020-06-10 15:55 ` [PATCH v2 5/6] plugins: add API to return a name for a IO device Alex Bennée
2020-06-10 15:55 ` [PATCH v2 6/6] plugins: new hwprofile plugin Alex Bennée

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200621203307.GA168836@sff \
    --to=cota@braap.org \
    --cc=aaron@os.amperecomputing.com \
    --cc=alex.bennee@linaro.org \
    --cc=kuhn.chenqun@huawei.com \
    --cc=pbonzini@redhat.com \
    --cc=peter.puhov@linaro.org \
    --cc=qemu-devel@nongnu.org \
    --cc=robert.foley@linaro.org \
    --cc=robhenry@microsoft.com \
    --cc=rth@twiddle.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).