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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Mon, 17 Jan 2011, Jan Kiszka wrote:

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

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel