Re: [Qemu-devel] [RFC v1 14/22] memory: manage coalesced mmio via a MemoryListener

[Anthony Liguori]

Avi Kivity <address@hidden> writes:

> Instead of calling a global function on coalesced mmio changes, which
> routes the call to kvm if enabled, add coalesced mmio hooks to
> MemoryListener and make kvm use that instead.
>
> The motivation is support for multiple address spaces (which means we
> we need to filter the call on the right address space) but the result
> is cleaner as well.
>
> Signed-off-by: Avi Kivity <address@hidden>
> ---
>  exec.c     | 13 -------------
>  kvm-all.c  | 20 ++++++++++----------
>  kvm-stub.c | 10 ----------
>  kvm.h      |  2 --
>  memory.c   | 24 ++++++++++++++++++++----
>  memory.h   | 12 +++++++++++-
>  6 files changed, 41 insertions(+), 40 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index 1fd6a10..51a32e7 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2313,19 +2313,6 @@ void 
> cpu_register_physical_memory_log(MemoryRegionSection *section,
>      }
>  }
>  
> -
> -void qemu_register_coalesced_mmio(target_phys_addr_t addr, ram_addr_t size)
> -{
> -    if (kvm_enabled())
> -        kvm_coalesce_mmio_region(addr, size);
> -}
> -
> -void qemu_unregister_coalesced_mmio(target_phys_addr_t addr, ram_addr_t size)
> -{
> -    if (kvm_enabled())
> -        kvm_uncoalesce_mmio_region(addr, size);
> -}
> -
>  void qemu_flush_coalesced_mmio_buffer(void)
>  {
>      if (kvm_enabled())
> diff --git a/kvm-all.c b/kvm-all.c
> index 5e9215d..25ca202 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -454,9 +454,10 @@ static int 
> kvm_physical_sync_dirty_bitmap(MemoryRegionSection *section)
>      return ret;
>  }
>  
> -int kvm_coalesce_mmio_region(target_phys_addr_t start, ram_addr_t size)
> +static void kvm_coalesce_mmio_region(MemoryListener *listener,
> +                                     MemoryRegionSection *secion,
> +                                     target_phys_addr_t start, ram_addr_t 
> size)
>  {
> -    int ret = -ENOSYS;
>      KVMState *s = kvm_state;
>  
>      if (s->coalesced_mmio) {
> @@ -466,15 +467,14 @@ int kvm_coalesce_mmio_region(target_phys_addr_t start, 
> ram_addr_t size)
>          zone.size = size;
>          zone.pad = 0;
>  
> -        ret = kvm_vm_ioctl(s, KVM_REGISTER_COALESCED_MMIO, &zone);
> +        (void)kvm_vm_ioctl(s, KVM_REGISTER_COALESCED_MMIO, &zone);

g_assert on error instead of ignoring it.

>      }
> -
> -    return ret;
>  }
>  
> -int kvm_uncoalesce_mmio_region(target_phys_addr_t start, ram_addr_t size)
> +static void kvm_uncoalesce_mmio_region(MemoryListener *listener,
> +                                       MemoryRegionSection *secion,
> +                                       target_phys_addr_t start, ram_addr_t 
> size)
>  {
> -    int ret = -ENOSYS;
>      KVMState *s = kvm_state;
>  
>      if (s->coalesced_mmio) {
> @@ -484,10 +484,8 @@ int kvm_uncoalesce_mmio_region(target_phys_addr_t start, 
> ram_addr_t size)
>          zone.size = size;
>          zone.pad = 0;
>  
> -        ret = kvm_vm_ioctl(s, KVM_UNREGISTER_COALESCED_MMIO, &zone);
> +        (void)kvm_vm_ioctl(s, KVM_UNREGISTER_COALESCED_MMIO, &zone);
>      }
> -
> -    return ret;
>  }
>  
>  int kvm_check_extension(KVMState *s, unsigned int extension)
> @@ -818,6 +816,8 @@ static void kvm_io_ioeventfd_del(MemoryListener *listener,
>      .log_global_stop = kvm_log_global_stop,
>      .eventfd_add = kvm_mem_ioeventfd_add,
>      .eventfd_del = kvm_mem_ioeventfd_del,
> +    .coalesced_mmio_add = kvm_coalesce_mmio_region,
> +    .coalesced_mmio_del = kvm_uncoalesce_mmio_region,
>      .priority = 10,
>  };
>  
> diff --git a/kvm-stub.c b/kvm-stub.c
> index 3c52eb5..a3455e2 100644
> --- a/kvm-stub.c
> +++ b/kvm-stub.c
> @@ -29,16 +29,6 @@ int kvm_init_vcpu(CPUArchState *env)
>      return -ENOSYS;
>  }
>  
> -int kvm_coalesce_mmio_region(target_phys_addr_t start, ram_addr_t size)
> -{
> -    return -ENOSYS;
> -}
> -
> -int kvm_uncoalesce_mmio_region(target_phys_addr_t start, ram_addr_t size)
> -{
> -    return -ENOSYS;
> -}
> -
>  int kvm_init(void)
>  {
>      return -ENOSYS;
> diff --git a/kvm.h b/kvm.h
> index dea2998..eefcb49 100644
> --- a/kvm.h
> +++ b/kvm.h
> @@ -129,8 +129,6 @@ void *kvm_vmalloc(ram_addr_t size);
>  void *kvm_arch_vmalloc(ram_addr_t size);
>  void kvm_setup_guest_memory(void *start, size_t size);
>  
> -int kvm_coalesce_mmio_region(target_phys_addr_t start, ram_addr_t size);
> -int kvm_uncoalesce_mmio_region(target_phys_addr_t start, ram_addr_t size);
>  void kvm_flush_coalesced_mmio_buffer(void);
>  #endif
>  
> diff --git a/memory.c b/memory.c
> index efefcb8..eb75349 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1130,11 +1130,19 @@ static void 
> memory_region_update_coalesced_range_as(MemoryRegion *mr, AddressSpa
>      FlatRange *fr;
>      CoalescedMemoryRange *cmr;
>      AddrRange tmp;
> +    MemoryRegionSection section;
>  
>      FOR_EACH_FLAT_RANGE(fr, as->current_map) {
>          if (fr->mr == mr) {
> -            qemu_unregister_coalesced_mmio(int128_get64(fr->addr.start),
> -                                           int128_get64(fr->addr.size));
> +            section = (MemoryRegionSection) {
> +                .address_space = as->root,
> +                .offset_within_address_space = int128_get64(fr->addr.start),
> +                .size = int128_get64(fr->addr.size),
> +            };

I think this is a bit too clever.  You can move the definition of
section into this block with a zero initializer, and then just set the
fields directly.  You'll end up losing 2 lines of code in the process
too.

Regards,

Anthony Liguori

> +
> +            MEMORY_LISTENER_CALL(coalesced_mmio_del, Reverse, &section,
> +                                 int128_get64(fr->addr.start),
> +                                 int128_get64(fr->addr.size));
>              QTAILQ_FOREACH(cmr, &mr->coalesced, link) {
>                  tmp = addrrange_shift(cmr->addr,
>                                        int128_sub(fr->addr.start,
> @@ -1143,8 +1151,9 @@ static void 
> memory_region_update_coalesced_range_as(MemoryRegion *mr, AddressSpa
>                      continue;
>                  }
>                  tmp = addrrange_intersection(tmp, fr->addr);
> -                qemu_register_coalesced_mmio(int128_get64(tmp.start),
> -                                             int128_get64(tmp.size));
> +                MEMORY_LISTENER_CALL(coalesced_mmio_add, Forward, &section,
> +                                     int128_get64(tmp.start),
> +                                     int128_get64(tmp.size));
>              }
>          }
>      }
> @@ -1529,6 +1538,13 @@ void memory_listener_default_eventfd(MemoryListener 
> *listener,
>  {
>  }
>  
> +void memory_listener_default_coalesced_mmio(MemoryListener *listener,
> +                                            MemoryRegionSection *section,
> +                                            target_phys_addr_t addr,
> +                                            target_phys_addr_t len)
> +{
> +}
> +
>  void address_space_init(AddressSpace *as, MemoryRegion *root)
>  {
>      memory_region_transaction_begin();
> diff --git a/memory.h b/memory.h
> index 0ef95cb..5f50bce 100644
> --- a/memory.h
> +++ b/memory.h
> @@ -217,6 +217,10 @@ struct MemoryListener {
>                          bool match_data, uint64_t data, EventNotifier *e);
>      void (*eventfd_del)(MemoryListener *listener, MemoryRegionSection 
> *section,
>                          bool match_data, uint64_t data, EventNotifier *e);
> +    void (*coalesced_mmio_add)(MemoryListener *listener, MemoryRegionSection 
> *section,
> +                               target_phys_addr_t addr, target_phys_addr_t 
> len);
> +    void (*coalesced_mmio_del)(MemoryListener *listener, MemoryRegionSection 
> *section,
> +                               target_phys_addr_t addr, target_phys_addr_t 
> len);
>      /* Lower = earlier (during add), later (during del) */
>      unsigned priority;
>      MemoryRegion *address_space_filter;
> @@ -235,7 +239,9 @@ struct MemoryListener {
>      .log_global_start = memory_listener_default_global,     \
>      .log_global_stop = memory_listener_default_global,      \
>      .eventfd_add = memory_listener_default_eventfd,         \
> -    .eventfd_del = memory_listener_default_eventfd          \
> +    .eventfd_del = memory_listener_default_eventfd,         \
> +    .coalesced_mmio_add = memory_listener_default_coalesced_mmio,  \
> +    .coalesced_mmio_del = memory_listener_default_coalesced_mmio   \
>  
>  void memory_listener_default_global(MemoryListener *listener);
>  void memory_listener_default_section(MemoryListener *listener,
> @@ -243,6 +249,10 @@ void memory_listener_default_section(MemoryListener 
> *listener,
>  void memory_listener_default_eventfd(MemoryListener *listener,
>                                       MemoryRegionSection *section,
>                                       bool match_data, uint64_t data, 
> EventNotifier *e);
> +void memory_listener_default_coalesced_mmio(MemoryListener *listener,
> +                                            MemoryRegionSection *section,
> +                                            target_phys_addr_t addr,
> +                                            target_phys_addr_t len);
>  
>  /**
>   * memory_region_init: Initialize a memory region
> -- 
> 1.7.12