Patchwork [RFC,v1,03/22] kvm: use separate MemoryListeners for memory and I/O

login
register
mail settings
Submitter Avi Kivity
Date Oct. 3, 2012, 4:03 p.m.
Message ID <1349280245-16341-4-git-send-email-avi@redhat.com>
Download mbox | patch
Permalink /patch/188819/
State New
Headers show

Comments

Avi Kivity - Oct. 3, 2012, 4:03 p.m.
The construct

   if (address_space == get_system_memory()) {
       // memory thing
   } else {
       // io thing
   }

fails if we have more than two address spaces.  Use a separate listener
for memory and I/O, and utilize MemoryListener's address space filtering to
fix this.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 kvm-all.c | 83 +++++++++++++++++++++++++++++++++------------------------------
 1 file changed, 44 insertions(+), 39 deletions(-)
Blue Swirl - Oct. 3, 2012, 8:16 p.m.
On Wed, Oct 3, 2012 at 4:03 PM, Avi Kivity <avi@redhat.com> wrote:
> The construct
>
>    if (address_space == get_system_memory()) {
>        // memory thing
>    } else {
>        // io thing
>    }
>
> fails if we have more than two address spaces.  Use a separate listener
> for memory and I/O, and utilize MemoryListener's address space filtering to
> fix this.
>
> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
>  kvm-all.c | 83 +++++++++++++++++++++++++++++++++------------------------------
>  1 file changed, 44 insertions(+), 39 deletions(-)
>
> diff --git a/kvm-all.c b/kvm-all.c
> index 92a7137..c69e012 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -755,9 +755,16 @@ static void kvm_log_global_stop(struct MemoryListener *listener)
>      assert(r >= 0);
>  }
>
> -static void kvm_mem_ioeventfd_add(MemoryRegionSection *section,
> -                                  bool match_data, uint64_t data, int fd)
> +static void kvm_log_nop(struct MemoryListener *listener)
>  {
> +}
> +
> +static void kvm_mem_ioeventfd_add(MemoryListener *listener,
> +                                  MemoryRegionSection *section,
> +                                  bool match_data, uint64_t data,
> +                                  EventNotifier *e)
> +{
> +    int fd = event_notifier_get_fd(e);
>      int r;
>
>      assert(match_data && section->size <= 8);
> @@ -769,9 +776,12 @@ static void kvm_mem_ioeventfd_add(MemoryRegionSection *section,
>      }
>  }
>
> -static void kvm_mem_ioeventfd_del(MemoryRegionSection *section,
> -                                  bool match_data, uint64_t data, int fd)
> +static void kvm_mem_ioeventfd_del(MemoryListener *listener,
> +                                  MemoryRegionSection *section,
> +                                  bool match_data, uint64_t data,
> +                                  EventNotifier *e)
>  {
> +    int fd = event_notifier_get_fd(e);
>      int r;
>
>      r = kvm_set_ioeventfd_mmio(fd, section->offset_within_address_space,
> @@ -781,9 +791,12 @@ static void kvm_mem_ioeventfd_del(MemoryRegionSection *section,
>      }
>  }
>
> -static void kvm_io_ioeventfd_add(MemoryRegionSection *section,
> -                                 bool match_data, uint64_t data, int fd)
> +static void kvm_io_ioeventfd_add(MemoryListener *listener,
> +                                 MemoryRegionSection *section,
> +                                 bool match_data, uint64_t data,
> +                                 EventNotifier *e)
>  {
> +    int fd = event_notifier_get_fd(e);
>      int r;
>
>      assert(match_data && section->size == 2);
> @@ -795,10 +808,13 @@ static void kvm_io_ioeventfd_add(MemoryRegionSection *section,
>      }
>  }
>
> -static void kvm_io_ioeventfd_del(MemoryRegionSection *section,
> -                                 bool match_data, uint64_t data, int fd)
> +static void kvm_io_ioeventfd_del(MemoryListener *listener,
> +                                 MemoryRegionSection *section,
> +                                 bool match_data, uint64_t data,
> +                                 EventNotifier *e)
>
>  {
> +    int fd = event_notifier_get_fd(e);
>      int r;
>
>      r = kvm_set_ioeventfd_pio_word(fd, section->offset_within_address_space,
> @@ -808,34 +824,6 @@ static void kvm_io_ioeventfd_del(MemoryRegionSection *section,
>      }
>  }
>
> -static void kvm_eventfd_add(MemoryListener *listener,
> -                            MemoryRegionSection *section,
> -                            bool match_data, uint64_t data,
> -                            EventNotifier *e)
> -{
> -    if (section->address_space == get_system_memory()) {
> -        kvm_mem_ioeventfd_add(section, match_data, data,
> -                             event_notifier_get_fd(e));
> -    } else {
> -        kvm_io_ioeventfd_add(section, match_data, data,
> -                            event_notifier_get_fd(e));
> -    }
> -}
> -
> -static void kvm_eventfd_del(MemoryListener *listener,
> -                            MemoryRegionSection *section,
> -                            bool match_data, uint64_t data,
> -                            EventNotifier *e)
> -{
> -    if (section->address_space == get_system_memory()) {
> -        kvm_mem_ioeventfd_del(section, match_data, data,
> -                             event_notifier_get_fd(e));
> -    } else {
> -        kvm_io_ioeventfd_del(section, match_data, data,
> -                            event_notifier_get_fd(e));
> -    }
> -}
> -
>  static MemoryListener kvm_memory_listener = {
>      .begin = kvm_begin,
>      .commit = kvm_commit,
> @@ -847,8 +835,24 @@ static void kvm_eventfd_del(MemoryListener *listener,
>      .log_sync = kvm_log_sync,
>      .log_global_start = kvm_log_global_start,
>      .log_global_stop = kvm_log_global_stop,
> -    .eventfd_add = kvm_eventfd_add,
> -    .eventfd_del = kvm_eventfd_del,
> +    .eventfd_add = kvm_mem_ioeventfd_add,
> +    .eventfd_del = kvm_mem_ioeventfd_del,
> +    .priority = 10,
> +};
> +
> +static MemoryListener kvm_io_listener = {

const

> +    .begin = kvm_begin,
> +    .commit = kvm_commit,
> +    .region_add = kvm_region_nop,
> +    .region_del = kvm_region_nop,
> +    .region_nop = kvm_region_nop,
> +    .log_start = kvm_region_nop,
> +    .log_stop = kvm_region_nop,
> +    .log_sync = kvm_region_nop,
> +    .log_global_start = kvm_log_nop,
> +    .log_global_stop = kvm_log_nop,
> +    .eventfd_add = kvm_io_ioeventfd_add,
> +    .eventfd_del = kvm_io_ioeventfd_del,
>      .priority = 10,
>  };
>
> @@ -1401,7 +1405,8 @@ int kvm_init(void)
>      }
>
>      kvm_state = s;
> -    memory_listener_register(&kvm_memory_listener, NULL);
> +    memory_listener_register(&kvm_memory_listener, get_system_memory());
> +    memory_listener_register(&kvm_io_listener, get_system_io());
>
>      s->many_ioeventfds = kvm_check_many_ioeventfds();
>
> --
> 1.7.12
>
Avi Kivity - Oct. 4, 2012, 6:33 a.m.
On 10/03/2012 10:16 PM, Blue Swirl wrote:
> > +static MemoryListener kvm_io_listener = {
>
> const
>

There is a list link field in there.  It's a mixed data/ops structure
(perhaps we should make it a traditional ->ops-> pointer).
Blue Swirl - Oct. 4, 2012, 4:44 p.m.
On Thu, Oct 4, 2012 at 6:33 AM, Avi Kivity <avi@redhat.com> wrote:
> On 10/03/2012 10:16 PM, Blue Swirl wrote:
>> > +static MemoryListener kvm_io_listener = {
>>
>> const
>>
>
> There is a list link field in there.  It's a mixed data/ops structure
> (perhaps we should make it a traditional ->ops-> pointer).

I grepped for MemoryListener and in hw/xen_pt.c 'const' already
exists, so it looked OK but the code actually copies the structure to
writable memory.

>
> --
> I have a truly marvellous patch that fixes the bug which this
> signature is too narrow to contain.
>
Avi Kivity - Oct. 4, 2012, 4:58 p.m.
On 10/04/2012 06:44 PM, Blue Swirl wrote:
> On Thu, Oct 4, 2012 at 6:33 AM, Avi Kivity <avi@redhat.com> wrote:
>> On 10/03/2012 10:16 PM, Blue Swirl wrote:
>>> > +static MemoryListener kvm_io_listener = {
>>>
>>> const
>>>
>>
>> There is a list link field in there.  It's a mixed data/ops structure
>> (perhaps we should make it a traditional ->ops-> pointer).
> 
> I grepped for MemoryListener and in hw/xen_pt.c 'const' already
> exists, so it looked OK but the code actually copies the structure to
> writable memory.

Yes.  We should probably switch to the standard object->ops->callback
pattern.  I'll write a patch after this series is merged.

Patch

diff --git a/kvm-all.c b/kvm-all.c
index 92a7137..c69e012 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -755,9 +755,16 @@  static void kvm_log_global_stop(struct MemoryListener *listener)
     assert(r >= 0);
 }
 
-static void kvm_mem_ioeventfd_add(MemoryRegionSection *section,
-                                  bool match_data, uint64_t data, int fd)
+static void kvm_log_nop(struct MemoryListener *listener)
 {
+}
+
+static void kvm_mem_ioeventfd_add(MemoryListener *listener,
+                                  MemoryRegionSection *section,
+                                  bool match_data, uint64_t data,
+                                  EventNotifier *e)
+{
+    int fd = event_notifier_get_fd(e);
     int r;
 
     assert(match_data && section->size <= 8);
@@ -769,9 +776,12 @@  static void kvm_mem_ioeventfd_add(MemoryRegionSection *section,
     }
 }
 
-static void kvm_mem_ioeventfd_del(MemoryRegionSection *section,
-                                  bool match_data, uint64_t data, int fd)
+static void kvm_mem_ioeventfd_del(MemoryListener *listener,
+                                  MemoryRegionSection *section,
+                                  bool match_data, uint64_t data,
+                                  EventNotifier *e)
 {
+    int fd = event_notifier_get_fd(e);
     int r;
 
     r = kvm_set_ioeventfd_mmio(fd, section->offset_within_address_space,
@@ -781,9 +791,12 @@  static void kvm_mem_ioeventfd_del(MemoryRegionSection *section,
     }
 }
 
-static void kvm_io_ioeventfd_add(MemoryRegionSection *section,
-                                 bool match_data, uint64_t data, int fd)
+static void kvm_io_ioeventfd_add(MemoryListener *listener,
+                                 MemoryRegionSection *section,
+                                 bool match_data, uint64_t data,
+                                 EventNotifier *e)
 {
+    int fd = event_notifier_get_fd(e);
     int r;
 
     assert(match_data && section->size == 2);
@@ -795,10 +808,13 @@  static void kvm_io_ioeventfd_add(MemoryRegionSection *section,
     }
 }
 
-static void kvm_io_ioeventfd_del(MemoryRegionSection *section,
-                                 bool match_data, uint64_t data, int fd)
+static void kvm_io_ioeventfd_del(MemoryListener *listener,
+                                 MemoryRegionSection *section,
+                                 bool match_data, uint64_t data,
+                                 EventNotifier *e)
 
 {
+    int fd = event_notifier_get_fd(e);
     int r;
 
     r = kvm_set_ioeventfd_pio_word(fd, section->offset_within_address_space,
@@ -808,34 +824,6 @@  static void kvm_io_ioeventfd_del(MemoryRegionSection *section,
     }
 }
 
-static void kvm_eventfd_add(MemoryListener *listener,
-                            MemoryRegionSection *section,
-                            bool match_data, uint64_t data,
-                            EventNotifier *e)
-{
-    if (section->address_space == get_system_memory()) {
-        kvm_mem_ioeventfd_add(section, match_data, data,
-			      event_notifier_get_fd(e));
-    } else {
-        kvm_io_ioeventfd_add(section, match_data, data,
-			     event_notifier_get_fd(e));
-    }
-}
-
-static void kvm_eventfd_del(MemoryListener *listener,
-                            MemoryRegionSection *section,
-                            bool match_data, uint64_t data,
-                            EventNotifier *e)
-{
-    if (section->address_space == get_system_memory()) {
-        kvm_mem_ioeventfd_del(section, match_data, data,
-			      event_notifier_get_fd(e));
-    } else {
-        kvm_io_ioeventfd_del(section, match_data, data,
-			     event_notifier_get_fd(e));
-    }
-}
-
 static MemoryListener kvm_memory_listener = {
     .begin = kvm_begin,
     .commit = kvm_commit,
@@ -847,8 +835,24 @@  static void kvm_eventfd_del(MemoryListener *listener,
     .log_sync = kvm_log_sync,
     .log_global_start = kvm_log_global_start,
     .log_global_stop = kvm_log_global_stop,
-    .eventfd_add = kvm_eventfd_add,
-    .eventfd_del = kvm_eventfd_del,
+    .eventfd_add = kvm_mem_ioeventfd_add,
+    .eventfd_del = kvm_mem_ioeventfd_del,
+    .priority = 10,
+};
+
+static MemoryListener kvm_io_listener = {
+    .begin = kvm_begin,
+    .commit = kvm_commit,
+    .region_add = kvm_region_nop,
+    .region_del = kvm_region_nop,
+    .region_nop = kvm_region_nop,
+    .log_start = kvm_region_nop,
+    .log_stop = kvm_region_nop,
+    .log_sync = kvm_region_nop,
+    .log_global_start = kvm_log_nop,
+    .log_global_stop = kvm_log_nop,
+    .eventfd_add = kvm_io_ioeventfd_add,
+    .eventfd_del = kvm_io_ioeventfd_del,
     .priority = 10,
 };
 
@@ -1401,7 +1405,8 @@  int kvm_init(void)
     }
 
     kvm_state = s;
-    memory_listener_register(&kvm_memory_listener, NULL);
+    memory_listener_register(&kvm_memory_listener, get_system_memory());
+    memory_listener_register(&kvm_io_listener, get_system_io());
 
     s->many_ioeventfds = kvm_check_many_ioeventfds();