diff mbox

[17/23] xen: convert to MemoryListener API

Message ID 1324304024-11220-18-git-send-email-avi@redhat.com
State New
Headers show

Commit Message

Avi Kivity Dec. 19, 2011, 2:13 p.m. UTC 
Signed-off-by: Avi Kivity <avi@redhat.com>
---
 trace-events |    2 +-
 xen-all.c    |  137 ++++++++++++++++++++++++++++++++++------------------------
 2 files changed, 81 insertions(+), 58 deletions(-)

Comments

Stefano Stabellini Jan. 4, 2012, 6:06 p.m. UTC | #1 
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. UTC | #2 
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.
diff mbox

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 */