[Qemu-devel] Re: [PATCH V2 1/3] Introduce log_start/log_stop in CPUPhysMemoryClient

[Jan Kiszka <jan.kiszka@siemens.com>]

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

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux