On 2011-01-14 19:10, anthony.perard@xxxxxxxxxx wrote:
> From: Anthony PERARD <anthony.perard@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home