Re: [Qemu-devel] [patch v5 7/8] memory: introduce tls context to record nested dma

[liu ping fan <qemulist@gmail.com>]

On Fri, Nov 2, 2012 at 6:39 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2012-10-29 00:48, Liu Ping Fan wrote:
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> ---
>>  cpus.c        |    3 ++
>>  exec.c        |   58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  qemu-thread.h |    8 +++++++
>>  vl.c          |    1 +
>>  4 files changed, 70 insertions(+), 0 deletions(-)
>>
>> diff --git a/cpus.c b/cpus.c
>> index 191cbf5..e67d80f 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -733,6 +733,7 @@ static void *qemu_kvm_cpu_thread_fn(void *arg)
>>
>>      qemu_mutex_lock(&qemu_global_mutex);
>>      qemu_thread_get_self(cpu->thread);
>> +    qemu_thread_init_context();
>>      env->thread_id = qemu_get_thread_id();
>>      cpu_single_env = env;
>>
>> @@ -774,6 +775,7 @@ static void *qemu_dummy_cpu_thread_fn(void *arg)
>>
>>      qemu_mutex_lock_iothread();
>>      qemu_thread_get_self(cpu->thread);
>> +    qemu_thread_init_context();
>>      env->thread_id = qemu_get_thread_id();
>>
>>      sigemptyset(&waitset);
>> @@ -813,6 +815,7 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
>>
>>      qemu_tcg_init_cpu_signals();
>>      qemu_thread_get_self(cpu->thread);
>> +    qemu_thread_init_context();
>>
>>      /* signal CPU creation */
>>      qemu_mutex_lock(&qemu_global_mutex);
>> diff --git a/exec.c b/exec.c
>> index 46da08c..ea672c6 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -3449,6 +3449,49 @@ static bool address_space_section_lookup_ref(AddressSpace *as,
>>      return safe_ref;
>>  }
>>
>> +typedef struct ThreadContext {
>> +  DispatchType dispatch_type;
>> +  unsigned int mmio_req_pending;
>> +} ThreadContext;
>> +
>> +static __thread ThreadContext *thread_context;
>> +
>> +void qemu_thread_init_context(void)
>> +{
>> +    thread_context = g_new(ThreadContext, 1);
>> +    thread_context->dispatch_type = DISPATCH_INIT;
>> +    thread_context->mmio_req_pending = 0;
>> +}
>> +
>> +void qemu_thread_set_dispatch_type(DispatchType type)
>> +{
>> +    thread_context->dispatch_type = type;
>> +}
>> +
>> +void qemu_thread_reset_dispatch_type(void)
>> +{
>> +    thread_context->dispatch_type = DISPATCH_INIT;
>> +}
>> +
>> +static bool address_space_inc_req_pending(void)
>> +{
>> +    bool nested = false;
>> +
>> +    /* currently, only mmio out of big lock, and need this to avoid dead lock */
>> +    if (thread_context->dispatch_type == DISPATCH_MMIO) {
>> +        nested = ++thread_context->mmio_req_pending > 1 ? true : false;
>> +    }
>> +
>> +    return nested;
>> +}
>> +
>> +static void address_space_dec_req_pending(void)
>> +{
>> +    if (thread_context->dispatch_type == DISPATCH_MMIO) {
>> +        thread_context->mmio_req_pending--;
>> +    }
>> +}
>> +
>>  void address_space_rw(AddressSpace *as, target_phys_addr_t addr, uint8_t *buf,
>>                        int len, bool is_write)
>>  {
>> @@ -3459,6 +3502,7 @@ void address_space_rw(AddressSpace *as, target_phys_addr_t addr, uint8_t *buf,
>>      target_phys_addr_t page;
>>      bool safe_ref = false;
>>      MemoryRegionSection *section, obj_mrs;
>> +    bool nested_dma = false;
>>
>>      while (len > 0) {
>>          page = addr & TARGET_PAGE_MASK;
>> @@ -3485,10 +3529,17 @@ void address_space_rw(AddressSpace *as, target_phys_addr_t addr, uint8_t *buf,
>>              memory_region_section_lookup_ref(d, page, &obj_mrs);
>>          }
>>          section = &obj_mrs;
>> +        nested_dma = address_space_inc_req_pending();
>>
>>          if (is_write) {
>>              if (!memory_region_is_ram(section->mr)) {
>>                  target_phys_addr_t addr1;
>> +
>> +                /* To fix, will filter iommu case */
>> +                if (nested_dma) {
>> +                    fprintf(stderr, "can not support nested DMA");
>> +                    abort();
>> +                }
>>                  addr1 = memory_region_section_addr(section, addr);
>>                  /* XXX: could force cpu_single_env to NULL to avoid
>>                     potential bugs */
>> @@ -3522,6 +3573,12 @@ void address_space_rw(AddressSpace *as, target_phys_addr_t addr, uint8_t *buf,
>>              if (!(memory_region_is_ram(section->mr) ||
>>                    memory_region_is_romd(section->mr))) {
>>                  target_phys_addr_t addr1;
>> +
>> +                /* To fix, will filter iommu case */
>> +                if (nested_dma) {
>> +                    fprintf(stderr, "can not support nested DMA");
>> +                    abort();
>> +                }
>>                  /* I/O case */
>>                  addr1 = memory_region_section_addr(section, addr);
>>                  if (l >= 4 && ((addr1 & 3) == 0)) {
>> @@ -3549,6 +3606,7 @@ void address_space_rw(AddressSpace *as, target_phys_addr_t addr, uint8_t *buf,
>>                  qemu_put_ram_ptr(ptr);
>>              }
>>          }
>> +        address_space_dec_req_pending();
>>          memory_region_section_unref(&obj_mrs);
>>          len -= l;
>>          buf += l;
>> diff --git a/qemu-thread.h b/qemu-thread.h
>> index 05fdaaf..bb9535e 100644
>> --- a/qemu-thread.h
>> +++ b/qemu-thread.h
>> @@ -7,6 +7,11 @@
>>  typedef struct QemuMutex QemuMutex;
>>  typedef struct QemuCond QemuCond;
>>  typedef struct QemuThread QemuThread;
>> +typedef enum {
>> +  DISPATCH_INIT = 0,
>> +  DISPATCH_MMIO,
>> +  DISPATCH_IO,
>> +} DispatchType;
>>
>>  #ifdef _WIN32
>>  #include "qemu-thread-win32.h"
>> @@ -46,4 +51,7 @@ void qemu_thread_get_self(QemuThread *thread);
>>  bool qemu_thread_is_self(QemuThread *thread);
>>  void qemu_thread_exit(void *retval);
>>
>> +void qemu_thread_init_context(void);
>> +void qemu_thread_set_dispatch_type(DispatchType type);
>> +void qemu_thread_reset_dispatch_type(void);
>>  #endif
>> diff --git a/vl.c b/vl.c
>> index ee3c43a..be8d825 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -3439,6 +3439,7 @@ int main(int argc, char **argv, char **envp)
>>              add_device_config(DEV_VIRTCON, "vc:80Cx24C");
>>      }
>>
>> +    qemu_thread_init_context();
>>      socket_init();
>>
>>      if (qemu_opts_foreach(qemu_find_opts("chardev"), chardev_init_func, NULL, 1) != 0)
>>
>
> In fact, this is not targeting nested DMA but nesting of memory region
> callbacks, and there potential deadlock issue. Triggering a DMA with RAM
> as target from within an MMIO (or later PIO) handler is perfectly fine
> and not rejected here. You should clarify the scope and purpose of this
> patch in its changelog.
>
Applied, thanks.


> Jan
>
> --
> Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
> Corporate Competence Center Embedded Linux
>