[Qemu-devel] Re: [COMMIT bd83677] Fake dirty loggin when it's not there

[Qemu-devel] Re: [COMMIT bd83677] Fake dirty loggin when it's not there

[Jan Kiszka]

Anthony Liguori wrote:
> From: Alexander Graf <agraf-l3A5Bk7waGM@public.gmane.org>
> 
> 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.

Sorry to disturb the merge, but my remarks remained unpatched. Would be
nice if they are considered in some follow-up patch... :)

> 
> Signed-off-by: Alexander Graf <agraf-l3A5Bk7waGM@public.gmane.org>
> Signed-off-by: Anthony Liguori <aliguori-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> 
> diff --git a/kvm-all.c b/kvm-all.c
> index 961fa32..2032949 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;
> +        }
> +

According to Alex' reply [1], I think this is probably better expressed
as an assert() than a silent skip. It indicates an error at higher
level, no?

>          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) {
>              dprintf("ioctl failed %d\n", errno);
>              ret = -1;
>              break;

My remark still stands: This is an "optimistic" assumption that only
EINVAL is a "real" error. If KVM is buggy /wrt a specific invalid return
value, work around that particular bug (and we should also fix the
kernel, but that's a different topic).

Jan

[1] http://permalink.gmane.org/gmane.comp.emulators.qemu/47996

Re: [Qemu-devel] Re: [COMMIT bd83677] Fake dirty loggin when it's not there

[Anthony Liguori]

Jan Kiszka wrote:
>> Signed-off-by: Alexander Graf <agraf-l3A5Bk7waGM@public.gmane.org>
>> Signed-off-by: Anthony Liguori <aliguori-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
>>
>> diff --git a/kvm-all.c b/kvm-all.c
>> index 961fa32..2032949 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;
>> +        }
>> +
>>     
>
> According to Alex' reply [1], I think this is probably better expressed
> as an assert() than a silent skip. It indicates an error at higher
> level, no?
>   

I don't have a strong opinion either way.

>>          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) {
>>              dprintf("ioctl failed %d\n", errno);
>>              ret = -1;
>>              break;
>>     
>
> My remark still stands: This is an "optimistic" assumption that only
> EINVAL is a "real" error. If KVM is buggy /wrt a specific invalid return
> value, work around that particular bug (and we should also fix the
> kernel, but that's a different topic).
>   

The behavior is identical to what it was before.  It's just a stylistic 
change.

I agree that this needs to be properly fixed in KVM and then in QEMU but 
I also think that that is best done as a separate set of patches.

Regards,

Anthony Liguori

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2882 bytes --]

Anthony Liguori wrote:
> Jan Kiszka wrote:
>>> Signed-off-by: Alexander Graf <agraf-l3A5Bk7waGM@public.gmane.org>
>>> Signed-off-by: Anthony Liguori
>>> <aliguori-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
>>>
>>> diff --git a/kvm-all.c b/kvm-all.c
>>> index 961fa32..2032949 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;
>>> +        }
>>> +
>>>     
>>
>> According to Alex' reply [1], I think this is probably better expressed
>> as an assert() than a silent skip. It indicates an error at higher
>> level, no?
>>   
> 
> I don't have a strong opinion either way.
> 
>>>          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) {
>>>              dprintf("ioctl failed %d\n", errno);
>>>              ret = -1;
>>>              break;
>>>     
>>
>> My remark still stands: This is an "optimistic" assumption that only
>> EINVAL is a "real" error. If KVM is buggy /wrt a specific invalid return
>> value, work around that particular bug (and we should also fix the
>> kernel, but that's a different topic).
>>   
> 
> The behavior is identical to what it was before.  It's just a stylistic
> change.

Not really (EINVAL != 1). But, yes, it's incorrect in similar way. :)

Jan

----------->

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 kvm-all.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index 824bb4c..53925be 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -348,7 +348,9 @@ 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) {
+        /* KVM for PowerPC returns illicit -ENOTSUPP (-524) which we handle
+         * below. */
+        if (r < 0 && r != -524) {
             dprintf("ioctl failed %d\n", errno);
             ret = -1;
             break;


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

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

[Anthony Liguori]

Jan Kiszka wrote:
> ----------->
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>
>  kvm-all.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/kvm-all.c b/kvm-all.c
> index 824bb4c..53925be 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -348,7 +348,9 @@ 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) {
> +        /* KVM for PowerPC returns illicit -ENOTSUPP (-524) which we handle
> +         * below. */
> +        if (r < 0 && r != -524) {
>   

That makes me uncomfortable.  Shouldn't we make kvm return something 
that's exposed to userspace?

-- 
Regards,

Anthony Liguori

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1110 bytes --]

Anthony Liguori wrote:
> Jan Kiszka wrote:
>> ----------->
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>
>>  kvm-all.c |    4 +++-
>>  1 files changed, 3 insertions(+), 1 deletions(-)
>>
>> diff --git a/kvm-all.c b/kvm-all.c
>> index 824bb4c..53925be 100644
>> --- a/kvm-all.c
>> +++ b/kvm-all.c
>> @@ -348,7 +348,9 @@ 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) {
>> +        /* KVM for PowerPC returns illicit -ENOTSUPP (-524) which we
>> handle
>> +         * below. */
>> +        if (r < 0 && r != -524) {
>>   
> 
> 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).

The situation would only be different if Alex said that it takes further
kernel patches anyway to make his PowerPC targets work. Dunno.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

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

[Anthony Liguori]

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.

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

> Jan
>
>   


-- 
Regards,

Anthony Liguori

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1524 bytes --]

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 <jan.kiszka@siemens.com>
---

 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) {
             dprintf("ioctl failed %d\n", errno);
             ret = -1;
             break;


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

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

[Alexander Graf]

Am 22.07.2009 um 22:07 schrieb Jan Kiszka <jan.kiszka@web.de>:

> 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 <jan.kiszka@siemens.com>
> ---
>
> 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.

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.

Alex


>             dprintf("ioctl failed %d\n", errno);
>             ret = -1;
>             break;
>

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2529 bytes --]

Alexander Graf wrote:
> 
> 
> 
> 
> Am 22.07.2009 um 22:07 schrieb Jan Kiszka <jan.kiszka@web.de>:
> 
>> 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 <jan.kiszka@siemens.com>
>> ---
>>
>> 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?

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[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 <jan.kiszka@web.de>:

> Alexander Graf wrote:
>>
>>
>>
>>
>> Am 22.07.2009 um 22:07 schrieb Jan Kiszka <jan.kiszka@web.de>:
>>
>>> 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 <jan.kiszka@siemens.com>
>>> ---
>>>
>>> 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
>

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

[Anthony Liguori]

Alexander Graf wrote:
>
> 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.

No way :-)

What's the problem with fixing the kernel to return a proper errno?

We can introduce a CAP if necessary to gracefully detect support for 
proper errno return.

-- 
Regards,

Anthony Liguori

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

[Alexander Graf]

Am 22.07.2009 um 23:15 schrieb Anthony Liguori <aliguori@us.ibm.com>:

> Alexander Graf wrote:
>>
>> 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.
>
> No way :-)
>
> What's the problem with fixing the kernel to return a proper errno?

I'd rather do a proper dirty log implementation than that :). But yes,  
you convinced me. I'll just do dirtylogging for ppc.

Now what about ia64?

Alex

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

[Anthony Liguori]

Alexander Graf wrote:
> I'd rather do a proper dirty log implementation than that :). But yes, 
> you convinced me. I'll just do dirtylogging for ppc.

Okay, then I'll take Jan's patch that just checks for if (ret < 0).

>
> Now what about ia64?

Out of sight, out of mind.

I have no idea what they do in qemu-kvm.  There is no ia64 support in 
upstream QEMU.

-- 
Regards,

Anthony Liguori

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

[Alexander Graf]

On 22.07.2009, at 23:25, Anthony Liguori wrote:

> Alexander Graf wrote:
>> I'd rather do a proper dirty log implementation than that :). But  
>> yes, you convinced me. I'll just do dirtylogging for ppc.
>
> Okay, then I'll take Jan's patch that just checks for if (ret < 0).

Sounds good. If anyone needs it they get fake dirty logging without  
needing to implement it then.

>>
>> Now what about ia64?
>
> Out of sight, out of mind.
>
> I have no idea what they do in qemu-kvm.  There is no ia64 support  
> in upstream QEMU.

*shrug*. I just figured it might be worth considering, since the plan  
was to get qemu as capable as qemu-kvm, no?

Alex

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

[Anthony Liguori]

Alexander Graf wrote:
>>>
>>> Now what about ia64?
>>
>> Out of sight, out of mind.
>>
>> I have no idea what they do in qemu-kvm.  There is no ia64 support in 
>> upstream QEMU.
>
> *shrug*. I just figured it might be worth considering, since the plan 
> was to get qemu as capable as qemu-kvm, no?

I do, but there's a lot of work to get ia64 there primarily due to the 
fact that there is no target support for ia64 in qemu.  The upstream 
implementation is going to end up significantly different than the 
qemu-kvm implementation.

> Alex
>


Regards,

Anthony Liguori

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 742 bytes --]

Alexander Graf wrote:
> 
> Am 22.07.2009 um 23:15 schrieb Anthony Liguori <aliguori@us.ibm.com>:
> 
>> Alexander Graf wrote:
>>>
>>> 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.
>>
>> No way :-)
>>
>> What's the problem with fixing the kernel to return a proper errno?
> 
> I'd rather do a proper dirty log implementation than that :). But yes,
> you convinced me. I'll just do dirtylogging for ppc.

Puh... :)

> 
> Now what about ia64?

There is a non-empty kvm_vm_ioctl_get_dirty_log() in kvm-ia64.c.

So I think we can revert your patch and just merge the tiny error-check
fix, right?

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]