Re: [Qemu-devel] An emulation failure occurs, if I hotplug vcpus immediately after the VM start

[David Hildenbrand]

On 07.06.2018 13:13, Gonglei (Arei) wrote:
> 
>> -----Original Message-----
>> From: David Hildenbrand [mailto:address@hidden
>> Sent: Thursday, June 07, 2018 6:40 PM
>> Subject: Re: An emulation failure occurs,if I hotplug vcpus immediately 
>> after the
>> VM start
>>
>> On 06.06.2018 15:57, Paolo Bonzini wrote:
>>> On 06/06/2018 15:28, Gonglei (Arei) wrote:
>>>> gonglei********: mem.slot: 3, mem.guest_phys_addr=0xc0000,
>>>> mem.userspace_addr=0x7fc343ec0000, mem.flags=0, memory_size=0x0
>>>> gonglei********: mem.slot: 3, mem.guest_phys_addr=0xc0000,
>>>> mem.userspace_addr=0x7fc343ec0000, mem.flags=0,
>> memory_size=0x9000
>>>>
>>>> When the memory region is cleared, the KVM will tell the slot to be
>>>> invalid (which it is set to KVM_MEMSLOT_INVALID).
>>>>
>>>> If SeaBIOS accesses this memory and cause page fault, it will find an
>>>> invalid value according to gfn (by __gfn_to_pfn_memslot), and finally
>>>> it will return an invalid value, and finally it will return a
>>>> failure.
>>>>
>>>> So, My questions are:
>>>>
>>>> 1) Why don't we hold kvm->slots_lock during page fault processing?
>>>
>>> Because it's protected by SRCU.  We don't need kvm->slots_lock on the
>>> read side.
>>>
>>>> 2) How do we assure that vcpus will not access the corresponding
>>>> region when deleting an memory slot?
>>>
>>> We don't.  It's generally a guest bug if they do, but the problem here
>>> is that QEMU is splitting a memory region in two parts and that is not
>>> atomic.
>>
>> BTW, one ugly (but QEMU-only) fix would be to temporarily pause all
>> VCPUs, do the change and then unpause all VCPUs.
>>
> 
> The updating process of memory region is triggered by vcpu thread, not
> the main process though.

Yes, I also already ran into this problem. Because it involves calling
pause_all_vcpus() from a VCPU thread. I sent a patch for that already,
but we were able to solve the s390x problem differently.

https://patchwork.kernel.org/patch/10331305/

The major problem of pause_all_vcpus() is that it will temporarily drop
the iothread mutex, which can result in "funny" side effects :) Handling
parallel call to pause_all_vcpus() is the smaller issue.

So right now, it can only be used from the main thread.

> 
> Thanks,
> -Gonglei
> 
>>>
>>> One fix could be to add a KVM_SET_USER_MEMORY_REGIONS ioctl that
>>> replaces the entire memory map atomically.
>>>
>>> Paolo
>>>
>>
>>
>> --
>>
>> Thanks,
>>
>> David / dhildenb


-- 

Thanks,

David / dhildenb