On Thu, Dec 12, 2019 at 11:07:42AM +0800, Alex Williamson wrote: > On Wed, 11 Dec 2019 21:02:40 -0500 > Yan Zhao <yan.y.zhao@xxxxxxxxx> wrote: > > > On Thu, Dec 12, 2019 at 02:56:55AM +0800, Alex Williamson wrote: > > > On Wed, 11 Dec 2019 01:25:55 -0500 > > > Yan Zhao <yan.y.zhao@xxxxxxxxx> wrote: > > > > > > > On Wed, Dec 11, 2019 at 12:38:05AM +0800, Alex Williamson wrote: > > > > > On Tue, 10 Dec 2019 02:44:44 -0500 > > > > > Yan Zhao <yan.y.zhao@xxxxxxxxx> wrote: > > > > > > > > > > > On Tue, Dec 10, 2019 at 05:16:08AM +0800, Alex Williamson wrote: > > > > > > > On Mon, 9 Dec 2019 01:22:12 -0500 > > > > > > > Yan Zhao <yan.y.zhao@xxxxxxxxx> wrote: > > > > > > > > > > > > > > > On Fri, Dec 06, 2019 at 11:20:38PM +0800, Alex Williamson wrote: > > > > > > > > > On Fri, 6 Dec 2019 01:04:07 -0500 > > > > > > > > > Yan Zhao <yan.y.zhao@xxxxxxxxx> wrote: > > > > > > > > > > > > > > > > > > > On Fri, Dec 06, 2019 at 07:55:30AM +0800, Alex Williamson wrote: > > > > > > > > > > > On Wed, 4 Dec 2019 22:26:50 -0500 > > > > > > > > > > > Yan Zhao <yan.y.zhao@xxxxxxxxx> wrote: > > > > > > > > > > > > > > > > > > > > > > > Dynamic trap bar info region is a channel for QEMU and vendor driver to > > > > > > > > > > > > communicate dynamic trap info. It is of type > > > > > > > > > > > > VFIO_REGION_TYPE_DYNAMIC_TRAP_BAR_INFO and subtype > > > > > > > > > > > > VFIO_REGION_SUBTYPE_DYNAMIC_TRAP_BAR_INFO. > > > > > > > > > > > > > > > > > > > > > > > > This region has two fields: dt_fd and trap. > > > > > > > > > > > > When QEMU detects a device regions of this type, it will create an > > > > > > > > > > > > eventfd and write its eventfd id to dt_fd field. > > > > > > > > > > > > When vendor drivre signals this eventfd, QEMU reads trap field of this > > > > > > > > > > > > info region. > > > > > > > > > > > > - If trap is true, QEMU would search the device's PCI BAR > > > > > > > > > > > > regions and disable all the sparse mmaped subregions (if the sparse > > > > > > > > > > > > mmaped subregion is disablable). > > > > > > > > > > > > - If trap is false, QEMU would re-enable those subregions. > > > > > > > > > > > > > > > > > > > > > > > > A typical usage is > > > > > > > > > > > > 1. vendor driver first cuts its bar 0 into several sections, all in a > > > > > > > > > > > > sparse mmap array. So initally, all its bar 0 are passthroughed. > > > > > > > > > > > > 2. vendor driver specifys part of bar 0 sections to be disablable. > > > > > > > > > > > > 3. on migration starts, vendor driver signals dt_fd and set trap to true > > > > > > > > > > > > to notify QEMU disabling the bar 0 sections of disablable flags on. > > > > > > > > > > > > 4. QEMU disables those bar 0 section and hence let vendor driver be able > > > > > > > > > > > > to trap access of bar 0 registers and make dirty page tracking possible. > > > > > > > > > > > > 5. on migration failure, vendor driver signals dt_fd to QEMU again. > > > > > > > > > > > > QEMU reads trap field of this info region which is false and QEMU > > > > > > > > > > > > re-passthrough the whole bar 0 region. > > > > > > > > > > > > > > > > > > > > > > > > Vendor driver specifies whether it supports dynamic-trap-bar-info region > > > > > > > > > > > > through cap VFIO_PCI_DEVICE_CAP_DYNAMIC_TRAP_BAR in > > > > > > > > > > > > vfio_pci_mediate_ops->open(). > > > > > > > > > > > > > > > > > > > > > > > > If vfio-pci detects this cap, it will create a default > > > > > > > > > > > > dynamic_trap_bar_info region on behalf of vendor driver with region len=0 > > > > > > > > > > > > and region->ops=null. > > > > > > > > > > > > Vvendor driver should override this region's len, flags, rw, mmap in its > > > > > > > > > > > > vfio_pci_mediate_ops. > > > > > > > > > > > > > > > > > > > > > > TBH, I don't like this interface at all. Userspace doesn't pass data > > > > > > > > > > > to the kernel via INFO ioctls. We have a SET_IRQS ioctl for > > > > > > > > > > > configuring user signaling with eventfds. I think we only need to > > > > > > > > > > > define an IRQ type that tells the user to re-evaluate the sparse mmap > > > > > > > > > > > information for a region. The user would enumerate the device IRQs via > > > > > > > > > > > GET_IRQ_INFO, find one of this type where the IRQ info would also > > > > > > > > > > > indicate which region(s) should be re-evaluated on signaling. The user > > > > > > > > > > > would enable that signaling via SET_IRQS and simply re-evaluate the > > > > > > > > > > ok. I'll try to switch to this way. Thanks for this suggestion. > > > > > > > > > > > > > > > > > > > > > sparse mmap capability for the associated regions when signaled. > > > > > > > > > > > > > > > > > > > > Do you like the "disablable" flag of sparse mmap ? > > > > > > > > > > I think it's a lightweight way for user to switch mmap state of a whole region, > > > > > > > > > > otherwise going through a complete flow of GET_REGION_INFO and re-setup > > > > > > > > > > region might be too heavy. > > > > > > > > > > > > > > > > > > No, I don't like the disable-able flag. At what frequency do we expect > > > > > > > > > regions to change? It seems like we'd only change when switching into > > > > > > > > > and out of the _SAVING state, which is rare. It seems easy for > > > > > > > > > userspace, at least QEMU, to drop the entire mmap configuration and > > > > > > > > ok. I'll try this way. > > > > > > > > > > > > > > > > > re-read it. Another concern here is how do we synchronize the event? > > > > > > > > > Are we assuming that this event would occur when a user switch to > > > > > > > > > _SAVING mode on the device? That operation is synchronous, the device > > > > > > > > > must be in saving mode after the write to device state completes, but > > > > > > > > > it seems like this might be trying to add an asynchronous dependency. > > > > > > > > > Will the write to device_state only complete once the user handles the > > > > > > > > > eventfd? How would the kernel know when the mmap re-evaluation is > > > > > > > > > complete. It seems like there are gaps here that the vendor driver > > > > > > > > > could miss traps required for migration because the user hasn't > > > > > > > > > completed the mmap transition yet. Thanks, > > > > > > > > > > > > > > > > > > Alex > > > > > > > > > > > > > > > > yes, this asynchronous event notification will cause vendor driver miss > > > > > > > > traps. But it's supposed to be of very short period time. That's also a > > > > > > > > reason for us to wish the re-evaluation to be lightweight. E.g. if it's > > > > > > > > able to be finished before the first iterate, it's still safe. > > > > > > > > > > > > > > Making the re-evaluation lightweight cannot solve the race, it only > > > > > > > masks it. > > > > > > > > > > > > > > > But I agree, the timing is not guaranteed, and so it's best for kernel > > > > > > > > to wait for mmap re-evaluation to complete. > > > > > > > > > > > > > > > > migration_thread > > > > > > > > |->qemu_savevm_state_setup > > > > > > > > | |->ram_save_setup > > > > > > > > | | |->migration_bitmap_sync > > > > > > > > | | |->kvm_log_sync > > > > > > > > | | |->vfio_log_sync > > > > > > > > | | > > > > > > > > | |->vfio_save_setup > > > > > > > > | |->set_device_state(_SAVING) > > > > > > > > | > > > > > > > > |->qemu_savevm_state_pending > > > > > > > > | |->ram_save_pending > > > > > > > > | | |->migration_bitmap_sync > > > > > > > > | | |->kvm_log_sync > > > > > > > > | | |->vfio_log_sync > > > > > > > > | |->vfio_save_pending > > > > > > > > | > > > > > > > > |->qemu_savevm_state_iterate > > > > > > > > | |->ram_save_iterate //send pages > > > > > > > > | |->vfio_save_iterate > > > > > > > > ... > > > > > > > > > > > > > > > > > > > > > > > > Actually, we previously let qemu trigger the re-evaluation when migration starts. > > > > > > > > And now the reason for we to wish kernel to trigger the mmap re-evaluation is that > > > > > > > > there're other two possible use cases: > > > > > > > > (1) keep passing through devices when migration starts and track dirty pages > > > > > > > > using hardware IOMMU. Then when migration is about to complete, stop the > > > > > > > > device and start trap PCI BARs for software emulation. (we made some > > > > > > > > changes to let device stop ahead of vcpu ) > > > > > > > > > > > > > > How is that possible? I/O devices need to continue to work until the > > > > > > > vCPU stops otherwise the vCPU can get blocked on the device. Maybe QEMU > > > > > > hi Alex > > > > > > For devices like DSA [1], it can support SVM mode. In this mode, when a > > > > > > page fault happens, the Intel DSA device blocks until the page fault is > > > > > > resolved, if PRS is enabled; otherwise it is reported as an error. > > > > > > > > > > > > Therefore, to pass through DSA into guest and do live migration with it, > > > > > > it is desired to stop DSA before stopping vCPU, as there may be an > > > > > > outstanding page fault to be resolved. > > > > > > > > > > > > During the period when DSA is stopped and vCPUs are still running, all the > > > > > > pass-through resources are trapped and emulated by host mediation driver until > > > > > > vCPUs stop. > > > > > > > > > > If the DSA is stopped and resources are trapped and emulated, then is > > > > > the device really stopped from a QEMU perspective or has it simply > > > > > switched modes underneath QEMU? If the device is truly stopped, then > > > > > I'd like to understand how a vCPU doing a PIO read from the device > > > > > wouldn't wedge the VM. > > > > > > > > > It doesn't matter if the device is truly stopped or not (although from > > > > my point of view, just draining commands and keeping device running is > > > > better as it handles live migration failure better). > > > > PIOs also need to be trapped and emulated if a vCPU accesses them. > > > > > > We seem to be talking around each other here. If PIOs are trapped and > > > emulated then the device is not "stopped" as far as QEMU is concerned, > > > right? "Stopping" a device suggests to me that a running vCPU doing a > > > PIO read from the device would block and cause problems in the still > > > running VM. So I think you're suggesting some sort of mode switch in > > > the device where direct access is disabled an emulation takes over > > > until the vCPUs are stopped. > > > > sorry for this confusion. > > yes, it's a kind of mode switch from a QEMU perspective. > > Currently, its implementation in our local branch is like that: > > 1. before migration thread stopping vCPUs, a migration state > > (COMPLETING) notification is sent to vfio migration state notifier, and > > this notifier would put device state to !RUNNING, and put all BARs to trap > > state. > > 2. in the kernel, when device state is set to !RUNNING, draining all > > pending device requests, and starts emulation. > > > > This implementation has two issues: > > 1. it requires hardcode in QEMU to put all BARs trapped and the time > > spending on revoking mmaps is not necessary for devices that do not need it. > > 2. !RUNNING state here is not accurate and it will confuse vendor > > drivers who stop devices after vCPUs stop. > > > > For the 2nd issue, I think we can propose a new device state like > > PRE-STOPPING. > > Yes, this is absolutely abusing the !RUNNING state, if the device is > still processing accesses by the vCPU, it's still running. > > > But for the 1st issue, not sure how to fix it right now. > > Maybe we can still add an asynchronous kernel notification and wait until > > QEMU have switched the region mmap state? > > It seems like you're preemptively trying to optimize the SAVING state > before we even have migration working. Shouldn't SAVING be the point > at which you switch to trapping the device in order to track it? > Thanks, But for some devices, start trapping on entering SAVING state is too early. They don't really need the trapping until PRE_STOPPING stage. E.g. for DSA, it can get dirty pages without trapping. The intention for it to enter trap is not for SAVING, but for emulation. Thanks Yan
