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
|
Home