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

[Jan Kiszka <jan.kiszka@web.de>]

[-- 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 --]