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

[Jan Kiszka <jan.kiszka@siemens.com>]

Alexander Graf wrote:
> 
> 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 <agraf@suse.de>
>>> ---
>>> 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.

Uuh, kvm returns a private error code to user space? That would be a
bug. So we need a check for the to-be-fixed error code and >= 512.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux