[Qemu-devel] Re: [PATCH 7/7] Fake dirty loggin when it's not there

[Alexander Graf]

On 17.07.2009, at 16:37, Jan Kiszka wrote:

> Alexander Graf wrote:
> > Some KVM platforms don't support dirty logging yet, like IA64 and  
> > PPC,
> > so in order to still have screen updates on those, we need to fake  
> > it.
> >
> > This patch just tells the getter function for dirty bitmaps, that all
> > pages within a slot are dirty when the slot has dirty logging  
> > enabled.
> >
> > That way we can implement dirty logging on those platforms sometime  
> > when
> > it drags down performance, but share the rest of the code with dirty
> > logging capable platforms.
> >
> > Signed-off-by: Alexander Graf <address@hidden>
> > ---
> > kvm-all.c |   13 ++++++++++++-
> > 1 files changed, 12 insertions(+), 1 deletions(-)
> >
> > diff --git a/kvm-all.c b/kvm-all.c
> > index 60b76cf..72b7935 100644
> > --- a/kvm-all.c
> > +++ b/kvm-all.c
> > @@ -300,6 +300,7 @@ int  
> > kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
> >     KVMDirtyLog d;
> >     KVMSlot *mem;
> >     int ret = 0;
> > +    int r;
> >
> >     d.dirty_bitmap = NULL;
> >     while (start_addr < end_addr) {
> > @@ -308,6 +309,11 @@ int  
> > kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
> >             break;
> >         }
> >
> > +        /* We didn't activate dirty logging? Don't care then. */
> > +        if(!(mem->flags & KVM_MEM_LOG_DIRTY_PAGES)) {
> > +            continue;
> > +        }
> > +
>
> Isn't this worth a separate patch? Do we already run into this patch  
> on
> dirty-logging archs (ie. x86)? I'm still wondering why it's needed.

We don't. I'm just being paranoid.

> >         size = ((mem->memory_size >> TARGET_PAGE_BITS) + 7) / 8;
> >         if (!d.dirty_bitmap) {
> >             d.dirty_bitmap = qemu_malloc(size);
> > @@ -319,7 +325,8 @@ int  
> > kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
> >
> >         d.slot = mem->slot;
> >
> > -        if (kvm_vm_ioctl(s, KVM_GET_DIRTY_LOG, &d) == -1) {
> > +        r = kvm_vm_ioctl(s, KVM_GET_DIRTY_LOG, &d);
> > +        if (r == -EINVAL) {
>
> I would say, better exclude 0 and the error you for missing support  
> here
> than to assume only EINVAL is a "real" error.

Hum, this was the logic that was there before and it makes sense. - 
EINVAL is used for errors whole -ENOTSUPP is used for N/A.  
Unfortunately -ENOTSUPP is only exported when __KERNEL__ is defined.

Alex