Patchwork [V2,1/3] Introduce log_start/log_stop in CPUPhysMemoryClient

login
register
mail settings
Submitter Anthony PERARD
Date Jan. 14, 2011, 6:10 p.m.
Message ID <1295028613-28237-2-git-send-email-anthony.perard@citrix.com>
Download mbox | patch
Permalink /patch/78990/
State New
Headers show

Comments

Anthony PERARD - Jan. 14, 2011, 6:10 p.m.
From: Anthony PERARD <anthony.perard@citrix.com>

In order to use log_start/log_stop with Xen as well in the vga code,
this two operations have been put in CPUPhysMemoryClient.

The two new functions cpu_physical_log_start,cpu_physical_log_stop are
used in hw/vga.c and replace the kvm_log_start/stop. With this, vga does
no longer depends on kvm header.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 cpu-all.h    |    6 ++++++
 cpu-common.h |    4 ++++
 exec.c       |   42 ++++++++++++++++++++++++++++++++++++++++++
 hw/vga.c     |   31 ++++++++++++++++---------------
 hw/vhost.c   |   16 ++++++++++++++++
 kvm-all.c    |    8 ++++++--
 kvm-stub.c   |   10 ----------
 kvm.h        |    3 ---
 8 files changed, 90 insertions(+), 30 deletions(-)
Jan Kiszka - Jan. 17, 2011, 3:54 p.m.
On 2011-01-14 19:10, anthony.perard@citrix.com wrote:
> From: Anthony PERARD <anthony.perard@citrix.com>
> 
> In order to use log_start/log_stop with Xen as well in the vga code,
> this two operations have been put in CPUPhysMemoryClient.
> 
> The two new functions cpu_physical_log_start,cpu_physical_log_stop are
> used in hw/vga.c and replace the kvm_log_start/stop. With this, vga does
> no longer depends on kvm header.

Basically, this looks good to me. Two remarks below.

> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  cpu-all.h    |    6 ++++++
>  cpu-common.h |    4 ++++
>  exec.c       |   42 ++++++++++++++++++++++++++++++++++++++++++
>  hw/vga.c     |   31 ++++++++++++++++---------------
>  hw/vhost.c   |   16 ++++++++++++++++
>  kvm-all.c    |    8 ++++++--
>  kvm-stub.c   |   10 ----------
>  kvm.h        |    3 ---
>  8 files changed, 90 insertions(+), 30 deletions(-)
> 
> diff --git a/cpu-all.h b/cpu-all.h
> index 30ae17d..aaf5442 100644
> --- a/cpu-all.h
> +++ b/cpu-all.h
> @@ -957,6 +957,12 @@ int cpu_physical_memory_get_dirty_tracking(void);
>  int cpu_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
>                                     target_phys_addr_t end_addr);
>  
> +int cpu_physical_log_start(target_phys_addr_t start_addr,
> +                           ram_addr_t size);
> +
> +int cpu_physical_log_stop(target_phys_addr_t start_addr,
> +                           ram_addr_t size);
> +
>  void dump_exec_info(FILE *f, fprintf_function cpu_fprintf);
>  #endif /* !CONFIG_USER_ONLY */
>  
> diff --git a/cpu-common.h b/cpu-common.h
> index 8ec01f4..2344842 100644
> --- a/cpu-common.h
> +++ b/cpu-common.h
> @@ -88,6 +88,10 @@ struct CPUPhysMemoryClient {
>                               target_phys_addr_t end_addr);
>      int (*migration_log)(struct CPUPhysMemoryClient *client,
>                           int enable);
> +    int (*log_start)(struct CPUPhysMemoryClient *client,
> +                     target_phys_addr_t phys_addr, ram_addr_t size);
> +    int (*log_stop)(struct CPUPhysMemoryClient *client,
> +                    target_phys_addr_t phys_addr, ram_addr_t size);
>      QLIST_ENTRY(CPUPhysMemoryClient) list;
>  };
>  
> diff --git a/exec.c b/exec.c
> index c6ed96d..609ec88 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1734,6 +1734,30 @@ static int cpu_notify_migration_log(int enable)
>      return 0;
>  }
>  
> +static int cpu_notify_log_start(target_phys_addr_t start,
> +                                ram_addr_t size)
> +{
> +    CPUPhysMemoryClient *client;
> +    QLIST_FOREACH(client, &memory_client_list, list) {
> +        int r = client->log_start(client, start, size);
> +        if (r < 0)
> +            return r;
> +    }
> +    return 0;
> +}
> +
> +static int cpu_notify_log_stop(target_phys_addr_t start,
> +                               ram_addr_t size)
> +{
> +    CPUPhysMemoryClient *client;
> +    QLIST_FOREACH(client, &memory_client_list, list) {
> +        int r = client->log_stop(client, start, size);
> +        if (r < 0)
> +            return r;
> +    }
> +    return 0;
> +}
> +

I would make the new callbacks optional, i.e. only invoke them if the
handler is non-NULL. Avoids stubs and branching to vhost.

>  static void phys_page_for_each_1(CPUPhysMemoryClient *client,
>                                   int level, void **lp)
>  {
> @@ -2073,6 +2097,24 @@ int cpu_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
>      return ret;
>  }
>  
> +int cpu_physical_log_start(target_phys_addr_t start_addr,
> +                           ram_addr_t size)
> +{
> +    int ret;
> +
> +    ret = cpu_notify_log_start(start_addr, size);
> +    return ret;
> +}
> +
> +int cpu_physical_log_stop(target_phys_addr_t start_addr,
> +                          ram_addr_t size)
> +{
> +    int ret;
> +
> +    ret = cpu_notify_log_stop(start_addr, size);
> +    return ret;
> +}
> +

I consider this split-up between API frontend and cpu_notify_* backend
as unneeded. But that's not your fault, you just follow the pre-existing
pattern. Unless someone feels strong about this, you may just keep it.

Jan
Anthony PERARD - Jan. 17, 2011, 6:25 p.m.
On Mon, 17 Jan 2011, Jan Kiszka wrote:

> On 2011-01-14 19:10, anthony.perard@citrix.com wrote:
> > From: Anthony PERARD <anthony.perard@citrix.com>
> >
> > In order to use log_start/log_stop with Xen as well in the vga code,
> > this two operations have been put in CPUPhysMemoryClient.
> >
> > The two new functions cpu_physical_log_start,cpu_physical_log_stop are
> > used in hw/vga.c and replace the kvm_log_start/stop. With this, vga does
> > no longer depends on kvm header.
>
> Basically, this looks good to me. Two remarks below.
>
> >
> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> > ---
> >  cpu-all.h    |    6 ++++++
> >  cpu-common.h |    4 ++++
> >  exec.c       |   42 ++++++++++++++++++++++++++++++++++++++++++
> >  hw/vga.c     |   31 ++++++++++++++++---------------
> >  hw/vhost.c   |   16 ++++++++++++++++
> >  kvm-all.c    |    8 ++++++--
> >  kvm-stub.c   |   10 ----------
> >  kvm.h        |    3 ---
> >  8 files changed, 90 insertions(+), 30 deletions(-)
> >
> > diff --git a/exec.c b/exec.c
> > index c6ed96d..609ec88 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -1734,6 +1734,30 @@ static int cpu_notify_migration_log(int enable)
> >      return 0;
> >  }
> >
> > +static int cpu_notify_log_start(target_phys_addr_t start,
> > +                                ram_addr_t size)
> > +{
> > +    CPUPhysMemoryClient *client;
> > +    QLIST_FOREACH(client, &memory_client_list, list) {
> > +        int r = client->log_start(client, start, size);
> > +        if (r < 0)
> > +            return r;
> > +    }
> > +    return 0;
> > +}
> > +
> > +static int cpu_notify_log_stop(target_phys_addr_t start,
> > +                               ram_addr_t size)
> > +{
> > +    CPUPhysMemoryClient *client;
> > +    QLIST_FOREACH(client, &memory_client_list, list) {
> > +        int r = client->log_stop(client, start, size);
> > +        if (r < 0)
> > +            return r;
> > +    }
> > +    return 0;
> > +}
> > +
>
> I would make the new callbacks optional, i.e. only invoke them if the
> handler is non-NULL. Avoids stubs and branching to vhost.

Ok, I will do that.

> >  static void phys_page_for_each_1(CPUPhysMemoryClient *client,
> >                                   int level, void **lp)
> >  {
> > @@ -2073,6 +2097,24 @@ int cpu_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
> >      return ret;
> >  }
> >
> > +int cpu_physical_log_start(target_phys_addr_t start_addr,
> > +                           ram_addr_t size)
> > +{
> > +    int ret;
> > +
> > +    ret = cpu_notify_log_start(start_addr, size);
> > +    return ret;
> > +}
> > +
> > +int cpu_physical_log_stop(target_phys_addr_t start_addr,
> > +                          ram_addr_t size)
> > +{
> > +    int ret;
> > +
> > +    ret = cpu_notify_log_stop(start_addr, size);
> > +    return ret;
> > +}
> > +
>
> I consider this split-up between API frontend and cpu_notify_* backend
> as unneeded. But that's not your fault, you just follow the pre-existing
> pattern. Unless someone feels strong about this, you may just keep it.

Actually, I will remove it, and keep only the frontend fonction.

Thanks,

Patch

diff --git a/cpu-all.h b/cpu-all.h
index 30ae17d..aaf5442 100644
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -957,6 +957,12 @@  int cpu_physical_memory_get_dirty_tracking(void);
 int cpu_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
                                    target_phys_addr_t end_addr);
 
+int cpu_physical_log_start(target_phys_addr_t start_addr,
+                           ram_addr_t size);
+
+int cpu_physical_log_stop(target_phys_addr_t start_addr,
+                           ram_addr_t size);
+
 void dump_exec_info(FILE *f, fprintf_function cpu_fprintf);
 #endif /* !CONFIG_USER_ONLY */
 
diff --git a/cpu-common.h b/cpu-common.h
index 8ec01f4..2344842 100644
--- a/cpu-common.h
+++ b/cpu-common.h
@@ -88,6 +88,10 @@  struct CPUPhysMemoryClient {
                              target_phys_addr_t end_addr);
     int (*migration_log)(struct CPUPhysMemoryClient *client,
                          int enable);
+    int (*log_start)(struct CPUPhysMemoryClient *client,
+                     target_phys_addr_t phys_addr, ram_addr_t size);
+    int (*log_stop)(struct CPUPhysMemoryClient *client,
+                    target_phys_addr_t phys_addr, ram_addr_t size);
     QLIST_ENTRY(CPUPhysMemoryClient) list;
 };
 
diff --git a/exec.c b/exec.c
index c6ed96d..609ec88 100644
--- a/exec.c
+++ b/exec.c
@@ -1734,6 +1734,30 @@  static int cpu_notify_migration_log(int enable)
     return 0;
 }
 
+static int cpu_notify_log_start(target_phys_addr_t start,
+                                ram_addr_t size)
+{
+    CPUPhysMemoryClient *client;
+    QLIST_FOREACH(client, &memory_client_list, list) {
+        int r = client->log_start(client, start, size);
+        if (r < 0)
+            return r;
+    }
+    return 0;
+}
+
+static int cpu_notify_log_stop(target_phys_addr_t start,
+                               ram_addr_t size)
+{
+    CPUPhysMemoryClient *client;
+    QLIST_FOREACH(client, &memory_client_list, list) {
+        int r = client->log_stop(client, start, size);
+        if (r < 0)
+            return r;
+    }
+    return 0;
+}
+
 static void phys_page_for_each_1(CPUPhysMemoryClient *client,
                                  int level, void **lp)
 {
@@ -2073,6 +2097,24 @@  int cpu_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
     return ret;
 }
 
+int cpu_physical_log_start(target_phys_addr_t start_addr,
+                           ram_addr_t size)
+{
+    int ret;
+
+    ret = cpu_notify_log_start(start_addr, size);
+    return ret;
+}
+
+int cpu_physical_log_stop(target_phys_addr_t start_addr,
+                          ram_addr_t size)
+{
+    int ret;
+
+    ret = cpu_notify_log_stop(start_addr, size);
+    return ret;
+}
+
 static inline void tlb_update_dirty(CPUTLBEntry *tlb_entry)
 {
     ram_addr_t ram_addr;
diff --git a/hw/vga.c b/hw/vga.c
index c057f4f..a9bf172 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -28,7 +28,6 @@ 
 #include "vga_int.h"
 #include "pixel_ops.h"
 #include "qemu-timer.h"
-#include "kvm.h"
 
 //#define DEBUG_VGA
 //#define DEBUG_VGA_MEM
@@ -1597,34 +1596,36 @@  static void vga_sync_dirty_bitmap(VGACommonState *s)
 
 void vga_dirty_log_start(VGACommonState *s)
 {
-    if (kvm_enabled() && s->map_addr)
-        kvm_log_start(s->map_addr, s->map_end - s->map_addr);
+    if (s->map_addr) {
+        cpu_physical_log_start(s->map_addr, s->map_end - s->map_addr);
+    }
 
-    if (kvm_enabled() && s->lfb_vram_mapped) {
-        kvm_log_start(isa_mem_base + 0xa0000, 0x8000);
-        kvm_log_start(isa_mem_base + 0xa8000, 0x8000);
+    if (s->lfb_vram_mapped) {
+        cpu_physical_log_start(isa_mem_base + 0xa0000, 0x8000);
+        cpu_physical_log_start(isa_mem_base + 0xa8000, 0x8000);
     }
 
 #ifdef CONFIG_BOCHS_VBE
-    if (kvm_enabled() && s->vbe_mapped) {
-        kvm_log_start(VBE_DISPI_LFB_PHYSICAL_ADDRESS, s->vram_size);
+    if (s->vbe_mapped) {
+        cpu_physical_log_start(VBE_DISPI_LFB_PHYSICAL_ADDRESS, s->vram_size);
     }
 #endif
 }
 
 void vga_dirty_log_stop(VGACommonState *s)
 {
-    if (kvm_enabled() && s->map_addr)
-	kvm_log_stop(s->map_addr, s->map_end - s->map_addr);
+    if (s->map_addr) {
+	cpu_physical_log_stop(s->map_addr, s->map_end - s->map_addr);
+    }
 
-    if (kvm_enabled() && s->lfb_vram_mapped) {
-	kvm_log_stop(isa_mem_base + 0xa0000, 0x8000);
-	kvm_log_stop(isa_mem_base + 0xa8000, 0x8000);
+    if (s->lfb_vram_mapped) {
+	cpu_physical_log_stop(isa_mem_base + 0xa0000, 0x8000);
+	cpu_physical_log_stop(isa_mem_base + 0xa8000, 0x8000);
     }
 
 #ifdef CONFIG_BOCHS_VBE
-    if (kvm_enabled() && s->vbe_mapped) {
-	kvm_log_stop(VBE_DISPI_LFB_PHYSICAL_ADDRESS, s->vram_size);
+    if (s->vbe_mapped) {
+	cpu_physical_log_stop(VBE_DISPI_LFB_PHYSICAL_ADDRESS, s->vram_size);
     }
 #endif
 }
diff --git a/hw/vhost.c b/hw/vhost.c
index 8586f66..5335178 100644
--- a/hw/vhost.c
+++ b/hw/vhost.c
@@ -439,6 +439,20 @@  static int vhost_client_migration_log(CPUPhysMemoryClient *client,
     return 0;
 }
 
+static int vhost_client_log_start(CPUPhysMemoryClient *client,
+                                  target_phys_addr_t start_addr,
+                                  ram_addr_t size)
+{
+    return 0;
+}
+
+static int vhost_client_log_stop(CPUPhysMemoryClient *client,
+                                 target_phys_addr_t start_addr,
+                                 ram_addr_t size)
+{
+    return 0;
+}
+
 static int vhost_virtqueue_init(struct vhost_dev *dev,
                                 struct VirtIODevice *vdev,
                                 struct vhost_virtqueue *vq,
@@ -606,6 +620,8 @@  int vhost_dev_init(struct vhost_dev *hdev, int devfd)
     hdev->client.set_memory = vhost_client_set_memory;
     hdev->client.sync_dirty_bitmap = vhost_client_sync_dirty_bitmap;
     hdev->client.migration_log = vhost_client_migration_log;
+    hdev->client.log_start = vhost_client_log_start;
+    hdev->client.log_stop = vhost_client_log_stop;
     hdev->mem = qemu_mallocz(offsetof(struct vhost_memory, regions));
     hdev->log = NULL;
     hdev->log_size = 0;
diff --git a/kvm-all.c b/kvm-all.c
index 37b99c7..aedfe33 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -267,14 +267,16 @@  static int kvm_dirty_pages_log_change(target_phys_addr_t phys_addr,
     return kvm_set_user_memory_region(s, mem);
 }
 
-int kvm_log_start(target_phys_addr_t phys_addr, ram_addr_t size)
+static int kvm_log_start(CPUPhysMemoryClient *client,
+                         target_phys_addr_t phys_addr, ram_addr_t size)
 {
         return kvm_dirty_pages_log_change(phys_addr, size,
                                           KVM_MEM_LOG_DIRTY_PAGES,
                                           KVM_MEM_LOG_DIRTY_PAGES);
 }
 
-int kvm_log_stop(target_phys_addr_t phys_addr, ram_addr_t size)
+static int kvm_log_stop(CPUPhysMemoryClient *client,
+                        target_phys_addr_t phys_addr, ram_addr_t size)
 {
         return kvm_dirty_pages_log_change(phys_addr, size,
                                           0,
@@ -596,6 +598,8 @@  static CPUPhysMemoryClient kvm_cpu_phys_memory_client = {
 	.set_memory = kvm_client_set_memory,
 	.sync_dirty_bitmap = kvm_client_sync_dirty_bitmap,
 	.migration_log = kvm_client_migration_log,
+        .log_start = kvm_log_start,
+        .log_stop = kvm_log_stop,
 };
 
 int kvm_init(int smp_cpus)
diff --git a/kvm-stub.c b/kvm-stub.c
index 5384a4b..41324e4 100644
--- a/kvm-stub.c
+++ b/kvm-stub.c
@@ -33,16 +33,6 @@  int kvm_init_vcpu(CPUState *env)
     return -ENOSYS;
 }
 
-int kvm_log_start(target_phys_addr_t phys_addr, ram_addr_t size)
-{
-    return -ENOSYS;
-}
-
-int kvm_log_stop(target_phys_addr_t phys_addr, ram_addr_t size)
-{
-    return -ENOSYS;
-}
-
 int kvm_coalesce_mmio_region(target_phys_addr_t start, ram_addr_t size)
 {
     return -ENOSYS;
diff --git a/kvm.h b/kvm.h
index 60a9b42..fc77b81 100644
--- a/kvm.h
+++ b/kvm.h
@@ -49,9 +49,6 @@  int kvm_init_vcpu(CPUState *env);
 int kvm_cpu_exec(CPUState *env);
 
 #if !defined(CONFIG_USER_ONLY)
-int kvm_log_start(target_phys_addr_t phys_addr, ram_addr_t size);
-int kvm_log_stop(target_phys_addr_t phys_addr, 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);