[virtio-dev] Re: [PATCH v4 2/4] migration: API to clear bits of guest free pages from the dirty bitmap

[Wei Wang <wei.w.wang@intel.com>]

On 03/15/2018 09:50 PM, Michael S. Tsirkin wrote:
> On Thu, Mar 15, 2018 at 06:52:41PM +0800, Wei Wang wrote:
>> On 03/15/2018 02:11 AM, Dr. David Alan Gilbert wrote:
>>> * Wei Wang (wei.w.wang@intel.com) wrote:
>>>> This patch adds an API to clear bits corresponding to guest free pages
>>>> from the dirty bitmap. Spilt the free page block if it crosses the QEMU
>>>> RAMBlock boundary.
>>>>
>>>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
>>>> CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>>> CC: Juan Quintela <quintela@redhat.com>
>>>> CC: Michael S. Tsirkin <mst@redhat.com>
>>>> ---
>>>>    include/migration/misc.h |  2 ++
>>>>    migration/ram.c          | 21 +++++++++++++++++++++
>>>>    2 files changed, 23 insertions(+)
>>>>
>>>> diff --git a/include/migration/misc.h b/include/migration/misc.h
>>>> index 77fd4f5..fae1acf 100644
>>>> --- a/include/migration/misc.h
>>>> +++ b/include/migration/misc.h
>>>> @@ -14,11 +14,13 @@
>>>>    #ifndef MIGRATION_MISC_H
>>>>    #define MIGRATION_MISC_H
>>>> +#include "exec/cpu-common.h"
>>>>    #include "qemu/notify.h"
>>>>    /* migration/ram.c */
>>>>    void ram_mig_init(void);
>>>> +void qemu_guest_free_page_hint(void *addr, size_t len);
>>>>    /* migration/block.c */
>>>> diff --git a/migration/ram.c b/migration/ram.c
>>>> index 5e33e5c..e172798 100644
>>>> --- a/migration/ram.c
>>>> +++ b/migration/ram.c
>>>> @@ -2189,6 +2189,27 @@ static int ram_init_all(RAMState **rsp)
>>>>        return 0;
>>>>    }
>>> This could do with some comments
>> OK, I'll add some.
>>
>>>> +void qemu_guest_free_page_hint(void *addr, size_t len)
>>>> +{
>>>> +    RAMBlock *block;
>>>> +    ram_addr_t offset;
>>>> +    size_t used_len, start, npages;
>>>   From your use I think the addr and len are coming raw from the guest;
>>> so we need to take some care.
>>>
>> Actually the "addr" here has been the host address that corresponds to the
>> guest free page. It's from elem->in_sg[0].iov_base.
>>
>>>> +        if (unlikely(offset + len > block->used_length)) {
>>> I think to make that overflow safe, that should be:
>>>     if (len > (block->used_length - offset)) {
>>>
>>> But we'll need another test before it, because qemu_ram_block_from_host
>>> seems to check max_length not used_length, so we need to check
>>> for offset > block->used_length first
>> OK, how about adding an assert above, like this:
>>
>> block = qemu_ram_block_from_host(addr, false, &offset);
>> assert (offset  < block->used_length );
>> if (!block)
>>      ...
>>
>> The address corresponds to a guest free page, which means it should be
>> within used_length. If not, something weird happens, I think we'd better to
>> assert it in that case.
>>
>> Best,
>> Wei
> What if memory has been removed by hotunplug after guest sent the
> free page notification?
>
> This seems to actually be likely to happen as memory being unplugged
> would typically be mostly free.


OK, thanks for the reminder. Instead of using an assert, I think we can 
let the function just return if (offset > block->used_length).

Best,
Wei

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org