On 07.06.2018 13:13, Gonglei (Arei) wrote: > >> -----Original Message----- >> From: David Hildenbrand [mailto:david@xxxxxxxxxx] >> 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
