Re: [Qemu-devel] [PATCH 5/6] Introduce kvm logging interface.

[Glauber Costa]

On Wed, Nov 19, 2008 at 1:18 PM, Anthony Liguori <address@hidden> wrote:
> Glauber Costa wrote:
>>
>> Introduce functions to stop and start logging of memory regions.
>> We select region based on its start address.
>>
>> Signed-off-by: Glauber Costa <address@hidden>
>> ---
>>  kvm-all.c |  153
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  kvm.h     |    5 ++
>>  2 files changed, 158 insertions(+), 0 deletions(-)
>>
>> diff --git a/kvm-all.c b/kvm-all.c
>> index 9700e50..c522205 100644
>> --- a/kvm-all.c
>> +++ b/kvm-all.c
>> @@ -32,8 +32,13 @@
>>     do { } while (0)
>>  #endif
>>  +#define warning(fmt, ...) \
>> +    do { dprintf("%s:%d" fmt, __func__, __LINE__, ## __VA_ARGS__); }
>> while (0)
>>
>
> I don't think this macro really serves a purpose.  You can just add "%s:%d:
> " to the beginning of dprintf() if you want.

Are you okay with all dprintfs having these info? This is very
valuable info, so I'd
like to have it. I  can send a separate patch.

>>
>
> Note that start and stop are identical except for a different printf().  The
> call a common helper function.  Why not fold everything into
> kvm_dirty_pages_log_change() and make that the public interface
> (kvm_set_log).

I personally believe kvm_set_log is a very bad interface. It's nicer
to read "start"
and "stop" instead, but I can definitely do this internally.

>
>> +static inline int lookup_bitmap_phys(unsigned long *dirty, unsigned nr)
>> +{
>> +    unsigned word = nr / (sizeof(*dirty) * 8);
>> +    unsigned bit = nr % (sizeof(*dirty) * 8);
>> +    int ret;
>> +
>> +    ret = (dirty[word] >> bit) & 1;
>> +    return ret;
>> +}
>> +
>> +/**
>> + * kvm_physical_sync_dirty_bitmap - Grab dirty bitmap from kernel space
>> + * If a phys_offset parameter is given, this function updates qemu's
>> dirty
>> + * bitmap using cpu_physical_memory_set_dirty(). This means all bits are
>> set
>> + * to dirty.
>> + *
>> + * @start_add: start of logged region. This is what we use to search the
>> memslot
>> + * @end_addr: end of logged region. Only matters if we are updating qemu
>> dirty bitmap.
>> + * @phys_offset: the phys_offset we want to use for qemu dirty bitmap
>> update. Passing
>> + *               NULL makes the update not happen. In this case, we only
>> grab the bitmap
>> + *               from kernel.
>> + */
>> +void kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
>> target_phys_addr_t end_addr,
>> +                                    ram_addr_t phys_offset)
>>
>
> This interface is weird and broken.  If we wanted to use this for live
> migration, we would end up passing phys_offset=0 but that has a special
> meaning here.
>
> But why are we passing phys_offset at all?  Why can't we do the lookup here?
Because, if you remember, the last time I sent a patch _without_ it,
you complained
that we can't really trust any translation based on userspace_addr.
Which is fine,
because it is fundamentally broken long term.

So, we need to be specific about what area are we writting the dirty bitmap too.
If the zero is the problem, we can have the interface to always write
to the given location,
and disregard 0 as a special meaning field.


-- 
Glauber  Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."