Patchwork [17/23] xen: convert to MemoryListener API

login
register
mail settings
Submitter Avi Kivity
Date Dec. 19, 2011, 2:13 p.m.
Message ID <1324304024-11220-18-git-send-email-avi@redhat.com>
Download mbox | patch
Permalink /patch/132255/
State New
Headers show

Comments

Avi Kivity - Dec. 19, 2011, 2:13 p.m.
Signed-off-by: Avi Kivity <avi@redhat.com>
---
 trace-events |    2 +-
 xen-all.c    |  137 ++++++++++++++++++++++++++++++++++------------------------
 2 files changed, 81 insertions(+), 58 deletions(-)
Stefano Stabellini - Jan. 4, 2012, 6:06 p.m.
On Mon, 19 Dec 2011, Avi Kivity wrote:
> -static int xen_log_start(CPUPhysMemoryClient *client, target_phys_addr_t phys_addr, ram_addr_t size)
> +static void xen_log_start(MemoryListener *listener,
> +                          MemoryRegionSection *section)
>  {
> -    XenIOState *state = container_of(client, XenIOState, client);
> +    XenIOState *state = container_of(listener, XenIOState, memory_listener);
> +    int r;
>  
> -    return xen_sync_dirty_bitmap(state, phys_addr, size);
> +    r = xen_sync_dirty_bitmap(state, section->offset_within_address_space,
> +                              section->size);
> +    assert(r >= 0);
>  }

I really feel I should thank you for your work because you did a very
good job porting xen to the new api. In fact apart from the dirty bitmap
(Anthony is about to send a patch to fix the issue:
xen_sync_dirty_bitmap can actually fail sometimes), everything else
is done right and works correctly.

However I would have appreciated if you could have given us more time to
review the four patches you wrote: considering the time of the year both
Anthony and I were on vacation and didn't have a chance to read them
until today.
Avi Kivity - Jan. 4, 2012, 7:42 p.m.
On 01/04/2012 08:06 PM, Stefano Stabellini wrote:
> On Mon, 19 Dec 2011, Avi Kivity wrote:
> > -static int xen_log_start(CPUPhysMemoryClient *client, target_phys_addr_t phys_addr, ram_addr_t size)
> > +static void xen_log_start(MemoryListener *listener,
> > +                          MemoryRegionSection *section)
> >  {
> > -    XenIOState *state = container_of(client, XenIOState, client);
> > +    XenIOState *state = container_of(listener, XenIOState, memory_listener);
> > +    int r;
> >  
> > -    return xen_sync_dirty_bitmap(state, phys_addr, size);
> > +    r = xen_sync_dirty_bitmap(state, section->offset_within_address_space,
> > +                              section->size);
> > +    assert(r >= 0);
> >  }
>
> I really feel I should thank you for your work because you did a very
> good job porting xen to the new api. In fact apart from the dirty bitmap
> (Anthony is about to send a patch to fix the issue:
> xen_sync_dirty_bitmap can actually fail sometimes), everything else
> is done right and works correctly.

Thanks.

> However I would have appreciated if you could have given us more time to
> review the four patches you wrote: considering the time of the year both
> Anthony and I were on vacation and didn't have a chance to read them
> until today.

I realize that I bypassed the normal protocol here, but I had to choose
one of several bad choices:

- continue developing without merging, and risk large rebases in case
the patches (or something else in qemu) had to be changed
- stop developing until you returned from your (undoubtedly well
deserved) vacations
- merge and look away while whistling innocently

I chose the third, since I still have quite a lot of work with the
memory API.  Of course I will help with fixing the fallout if needed,
and since you're back online, we can go back to the normal way of
reviewing and testing patches before merging.

Patch

diff --git a/trace-events b/trace-events
index bf1cf57..728df97 100644
--- a/trace-events
+++ b/trace-events
@@ -464,7 +464,7 @@  mipsnet_irq(uint32_t isr, uint32_t intctl) "set irq to %d (%02x)"
 
 # xen-all.c
 xen_ram_alloc(unsigned long ram_addr, unsigned long size) "requested: %#lx, size %#lx"
-xen_client_set_memory(uint64_t start_addr, unsigned long size, unsigned long phys_offset, bool log_dirty) "%#"PRIx64" size %#lx, offset %#lx, log_dirty %i"
+xen_client_set_memory(uint64_t start_addr, unsigned long size, bool log_dirty) "%#"PRIx64" size %#lx, log_dirty %i"
 
 # xen-mapcache.c
 xen_map_cache(uint64_t phys_addr) "want %#"PRIx64
diff --git a/xen-all.c b/xen-all.c
index 51315ce..e662dc5 100644
--- a/xen-all.c
+++ b/xen-all.c
@@ -63,6 +63,7 @@  static inline uint32_t xen_vcpu_eport(shared_iopage_t *shared_page, int i)
 typedef struct XenPhysmap {
     target_phys_addr_t start_addr;
     ram_addr_t size;
+    MemoryRegion *mr;
     target_phys_addr_t phys_offset;
 
     QLIST_ENTRY(XenPhysmap) list;
@@ -80,8 +81,9 @@  static inline uint32_t xen_vcpu_eport(shared_iopage_t *shared_page, int i)
     int send_vcpu;
 
     struct xs_handle *xenstore;
-    CPUPhysMemoryClient client;
+    MemoryListener memory_listener;
     QLIST_HEAD(, XenPhysmap) physmap;
+    target_phys_addr_t free_phys_offset;
     const XenPhysmap *log_for_dirtybit;
 
     Notifier exit;
@@ -226,13 +228,14 @@  void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, MemoryRegion *mr)
 static int xen_add_to_physmap(XenIOState *state,
                               target_phys_addr_t start_addr,
                               ram_addr_t size,
-                              target_phys_addr_t phys_offset)
+                              MemoryRegion *mr,
+                              target_phys_addr_t offset_within_region)
 {
     unsigned long i = 0;
     int rc = 0;
     XenPhysmap *physmap = NULL;
     target_phys_addr_t pfn, start_gpfn;
-    RAMBlock *block;
+    target_phys_addr_t phys_offset = memory_region_get_ram_addr(mr);
 
     if (get_physmapping(state, start_addr, size)) {
         return 0;
@@ -245,17 +248,13 @@  static int xen_add_to_physmap(XenIOState *state,
      * the linear framebuffer to be that region.
      * Avoid tracking any regions that is not videoram and avoid tracking
      * the legacy vga region. */
-    QLIST_FOREACH(block, &ram_list.blocks, next) {
-        if (!strcmp(block->idstr, "vga.vram") && block->offset == phys_offset
-                && start_addr > 0xbffff) {
-            goto go_physmap;
-        }
+    if (mr == framebuffer && start_addr > 0xbffff) {
+        goto go_physmap;
     }
     return -1;
 
 go_physmap:
-    DPRINTF("mapping vram to %llx - %llx, from %llx\n",
-            start_addr, start_addr + size, phys_offset);
+    DPRINTF("mapping vram to %llx - %llx\n", start_addr, start_addr + size);
 
     pfn = phys_offset >> TARGET_PAGE_BITS;
     start_gpfn = start_addr >> TARGET_PAGE_BITS;
@@ -347,49 +346,62 @@  static int xen_remove_from_physmap(XenIOState *state,
 }
 #endif
 
-static void xen_client_set_memory(struct CPUPhysMemoryClient *client,
-                                  target_phys_addr_t start_addr,
-                                  ram_addr_t size,
-                                  ram_addr_t phys_offset,
-                                  bool log_dirty)
+static void xen_set_memory(struct MemoryListener *listener,
+                           MemoryRegionSection *section,
+                           bool add)
 {
-    XenIOState *state = container_of(client, XenIOState, client);
-    ram_addr_t flags = phys_offset & ~TARGET_PAGE_MASK;
+    XenIOState *state = container_of(listener, XenIOState, memory_listener);
+    target_phys_addr_t start_addr = section->offset_within_address_space;
+    ram_addr_t size = section->size;
+    bool log_dirty = memory_region_is_logging(section->mr);
     hvmmem_type_t mem_type;
 
-    if (!(start_addr != phys_offset
-          && ( (log_dirty && flags < IO_MEM_UNASSIGNED)
-               || (!log_dirty && flags == IO_MEM_UNASSIGNED)))) {
+    if (!memory_region_is_ram(section->mr)) {
+        return;
+    }
+
+    if (!(section->mr != &ram_memory
+          && ( (log_dirty && add) || (!log_dirty && !add)))) {
         return;
     }
 
-    trace_xen_client_set_memory(start_addr, size, phys_offset, log_dirty);
+    trace_xen_client_set_memory(start_addr, size, log_dirty);
 
     start_addr &= TARGET_PAGE_MASK;
     size = TARGET_PAGE_ALIGN(size);
-    phys_offset &= TARGET_PAGE_MASK;
-
-    switch (flags) {
-    case IO_MEM_RAM:
-        xen_add_to_physmap(state, start_addr, size, phys_offset);
-        break;
-    case IO_MEM_ROM:
-        mem_type = HVMMEM_ram_ro;
-        if (xc_hvm_set_mem_type(xen_xc, xen_domid, mem_type,
-                                start_addr >> TARGET_PAGE_BITS,
-                                size >> TARGET_PAGE_BITS)) {
-            DPRINTF("xc_hvm_set_mem_type error, addr: "TARGET_FMT_plx"\n",
-                    start_addr);
+
+    if (add) {
+        if (!memory_region_is_rom(section->mr)) {
+            xen_add_to_physmap(state, start_addr, size,
+                               section->mr, section->offset_within_region);
+        } else {
+            mem_type = HVMMEM_ram_ro;
+            if (xc_hvm_set_mem_type(xen_xc, xen_domid, mem_type,
+                                    start_addr >> TARGET_PAGE_BITS,
+                                    size >> TARGET_PAGE_BITS)) {
+                DPRINTF("xc_hvm_set_mem_type error, addr: "TARGET_FMT_plx"\n",
+                        start_addr);
+            }
         }
-        break;
-    case IO_MEM_UNASSIGNED:
+    } else {
         if (xen_remove_from_physmap(state, start_addr, size) < 0) {
             DPRINTF("physmapping does not exist at "TARGET_FMT_plx"\n", start_addr);
         }
-        break;
     }
 }
 
+static void xen_region_add(MemoryListener *listener,
+                           MemoryRegionSection *section)
+{
+    xen_set_memory(listener, section, true);
+}
+
+static void xen_region_del(MemoryListener *listener,
+                           MemoryRegionSection *section)
+{
+    xen_set_memory(listener, section, false);
+}
+
 static int xen_sync_dirty_bitmap(XenIOState *state,
                                  target_phys_addr_t start_addr,
                                  ram_addr_t size)
@@ -433,43 +445,54 @@  static int xen_sync_dirty_bitmap(XenIOState *state,
     return 0;
 }
 
-static int xen_log_start(CPUPhysMemoryClient *client, target_phys_addr_t phys_addr, ram_addr_t size)
+static void xen_log_start(MemoryListener *listener,
+                          MemoryRegionSection *section)
 {
-    XenIOState *state = container_of(client, XenIOState, client);
+    XenIOState *state = container_of(listener, XenIOState, memory_listener);
+    int r;
 
-    return xen_sync_dirty_bitmap(state, phys_addr, size);
+    r = xen_sync_dirty_bitmap(state, section->offset_within_address_space,
+                              section->size);
+    assert(r >= 0);
 }
 
-static int xen_log_stop(CPUPhysMemoryClient *client, target_phys_addr_t phys_addr, ram_addr_t size)
+static void xen_log_stop(MemoryListener *listener, MemoryRegionSection *section)
 {
-    XenIOState *state = container_of(client, XenIOState, client);
+    XenIOState *state = container_of(listener, XenIOState, memory_listener);
+    int r;
 
     state->log_for_dirtybit = NULL;
     /* Disable dirty bit tracking */
-    return xc_hvm_track_dirty_vram(xen_xc, xen_domid, 0, 0, NULL);
+    r = xc_hvm_track_dirty_vram(xen_xc, xen_domid, 0, 0, NULL);
+    assert(r >= 0);
 }
 
-static int xen_client_sync_dirty_bitmap(struct CPUPhysMemoryClient *client,
-                                        target_phys_addr_t start_addr,
-                                        target_phys_addr_t end_addr)
+static void xen_log_sync(MemoryListener *listener, MemoryRegionSection *section)
 {
-    XenIOState *state = container_of(client, XenIOState, client);
+    XenIOState *state = container_of(listener, XenIOState, memory_listener);
+    int r;
 
-    return xen_sync_dirty_bitmap(state, start_addr, end_addr - start_addr);
+    r = xen_sync_dirty_bitmap(state, section->offset_within_address_space,
+                              section->size);
+    assert(r >= 0);
 }
 
-static int xen_client_migration_log(struct CPUPhysMemoryClient *client,
-                                    int enable)
+static void xen_log_global_start(MemoryListener *listener)
+{
+}
+
+static void xen_log_global_stop(MemoryListener *listener)
 {
-    return 0;
 }
 
-static CPUPhysMemoryClient xen_cpu_phys_memory_client = {
-    .set_memory = xen_client_set_memory,
-    .sync_dirty_bitmap = xen_client_sync_dirty_bitmap,
-    .migration_log = xen_client_migration_log,
+static MemoryListener xen_memory_listener = {
+    .region_add = xen_region_add,
+    .region_del = xen_region_del,
     .log_start = xen_log_start,
     .log_stop = xen_log_stop,
+    .log_sync = xen_log_sync,
+    .log_global_start = xen_log_global_start,
+    .log_global_stop = xen_log_global_stop,
 };
 
 /* VCPU Operations, MMIO, IO ring ... */
@@ -947,9 +970,9 @@  int xen_hvm_init(void)
 
     qemu_add_vm_change_state_handler(xen_hvm_change_state_handler, state);
 
-    state->client = xen_cpu_phys_memory_client;
+    state->memory_listener = xen_memory_listener;
     QLIST_INIT(&state->physmap);
-    cpu_register_phys_memory_client(&state->client);
+    memory_listener_register(&state->memory_listener);
     state->log_for_dirtybit = NULL;
 
     /* Initialize backend core & drivers */