From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=57558 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PerPd-0006dz-Tm for qemu-devel@nongnu.org; Mon, 17 Jan 2011 10:54:35 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PerPc-0002K2-92 for qemu-devel@nongnu.org; Mon, 17 Jan 2011 10:54:33 -0500 Received: from goliath.siemens.de ([192.35.17.28]:20033) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PerPc-0002Jn-0V for qemu-devel@nongnu.org; Mon, 17 Jan 2011 10:54:32 -0500 Message-ID: <4D34662F.7050904@siemens.com> Date: Mon, 17 Jan 2011 16:54:23 +0100 From: Jan Kiszka MIME-Version: 1.0 References: <1295028613-28237-1-git-send-email-anthony.perard@citrix.com> <1295028613-28237-2-git-send-email-anthony.perard@citrix.com> In-Reply-To: <1295028613-28237-2-git-send-email-anthony.perard@citrix.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH V2 1/3] Introduce log_start/log_stop in CPUPhysMemoryClient List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: anthony.perard@citrix.com Cc: Xen Devel , QEMU-devel On 2011-01-14 19:10, anthony.perard@citrix.com wrote: > From: Anthony PERARD > > 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 > --- > 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 -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kiszka Subject: Re: [PATCH V2 1/3] Introduce log_start/log_stop in CPUPhysMemoryClient Date: Mon, 17 Jan 2011 16:54:23 +0100 Message-ID: <4D34662F.7050904@siemens.com> References: <1295028613-28237-1-git-send-email-anthony.perard@citrix.com> <1295028613-28237-2-git-send-email-anthony.perard@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1295028613-28237-2-git-send-email-anthony.perard@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: anthony.perard@citrix.com Cc: Xen Devel , QEMU-devel List-Id: xen-devel@lists.xenproject.org On 2011-01-14 19:10, anthony.perard@citrix.com wrote: > From: Anthony PERARD > > 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 > --- > 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 -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux