[Qemu-devel] Re: [PATCH] kvm: Fix error detection for KVM_GET_DIRTY_LOG

[Alexander Graf]

Am 22.07.2009 um 23:14 schrieb Jan Kiszka <address@hidden>:

> Alexander Graf wrote:
> > Am 22.07.2009 um 22:07 schrieb Jan Kiszka <address@hidden>:
> >
> > > Anthony Liguori wrote:
> > > > Jan Kiszka wrote:
> > > > > > That makes me uncomfortable.  Shouldn't we make kvm return  
> > > > > > something
> > > > > > that's exposed to userspace?
> > > > >
> > > > > Yes, but we can't do this from user space :) (or in other words:  
> > > > > there
> > > > > are already kernels out there which return this invalid code).
> > > >
> > > > But since it's not symbolic, at some point in time the meaning of  
> > > > 524
> > > > can potentially change and introduce a very, very subtle bug.
> > >
> > > I won't change in old kernel versions, and I expect someone from the
> > > PowerPC folks to fix it for new version fairly soon.
> > >
> > > > > The situation would only be different if Alex said that it takes
> > > > > further
> > > > > kernel patches anyway to make his PowerPC targets work. Dunno.
> > > >
> > > > Certainly, his PPC target is not in any released kernel version so
> > > > there's time to fix things properly.
> > >
> > > Ah, ok. Then let's do this (Alex can carry a temporary workaround
> > > locally IMHO):
> > >
> > > ------------->
> > >
> > > Signed-off-by: Jan Kiszka <address@hidden>
> > > ---
> > >
> > > kvm-all.c |    2 +-
> > > 1 files changed, 1 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/kvm-all.c b/kvm-all.c
> > > index 824bb4c..5fb8dba 100644
> > > --- a/kvm-all.c
> > > +++ b/kvm-all.c
> > > @@ -348,7 +348,7 @@ int
> > > kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
> > >        d.slot = mem->slot;
> > >
> > >        r = kvm_vm_ioctl(s, KVM_GET_DIRTY_LOG, &d);
> > > -        if (r == -EINVAL) {
> > > +        if (r < 0) {
> >
> > That's worse as it essentially removes the not implemented case.
>
> You can add the valid return code of a fixed powerpc version of
> kvm_vm_ioctl_get_dirty_log here.
>
> > The only thing I'd agree with as a good idea instead of the version I
> > sent is an explicit switch statement with an include of errno.h with
> > __KERNEL__ set.
>
> Sorry, that's also horrible.
>
> Again the question: Is there already some official, unpatched kernel
> version which would work with QEMU if we only handle that borken
> ENOTSUPP case?

No, but I haven't implemented dirty logging at all yet.

I guess the real question is what does IA64 do and check for that.

Alex

> If not, I see no point in handling it here. Please fix KVM and  
> provide a
> patch that checks for the fixed, valid error code.
>
> If there is such a kernel, I see no problem with hard-coding  
> ENOTSUPP's
> value now, as we are working around a hard-coded bug that will no  
> longer
> exist in the future (where ENOTSUPP may change its value -  
> theoretically).
>
> Jan