| Message ID | 20200518025245.14425-1-yan.y.zhao@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce vendor ops in vfio-pci | expand |
On Sun, 17 May 2020 22:52:45 -0400 Yan Zhao <yan.y.zhao@intel.com> wrote: > This is a virtual irq type. > vendor driver triggers this irq when it wants to notify userspace to > remap PCI BARs. > > 1. vendor driver triggers this irq and packs the target bar number in > the ctx count. i.e. "1 << bar_number". > if a bit is set, the corresponding bar is to be remapped. > > 2. userspace requery the specified PCI BAR from kernel and if flags of > the bar regions are changed, it removes the old subregions and attaches > subregions according to the new flags. > > 3. userspace notifies back to kernel by writing one to the eventfd of > this irq. > > Please check the corresponding qemu implementation from the reply of this > patch, and a sample usage in vendor driver in patch [10/10]. > > Cc: Kevin Tian <kevin.tian@intel.com> > Signed-off-by: Yan Zhao <yan.y.zhao@intel.com> > --- > include/uapi/linux/vfio.h | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h > index 2d0d85c7c4d4..55895f75d720 100644 > --- a/include/uapi/linux/vfio.h > +++ b/include/uapi/linux/vfio.h > @@ -704,6 +704,17 @@ struct vfio_irq_info_cap_type { > __u32 subtype; /* type specific */ > }; > > +/* Bar Region Query IRQ TYPE */ > +#define VFIO_IRQ_TYPE_REMAP_BAR_REGION (1) > + > +/* sub-types for VFIO_IRQ_TYPE_REMAP_BAR_REGION */ > +/* > + * This irq notifies userspace to re-query BAR region and remaps the > + * subregions. > + */ > +#define VFIO_IRQ_SUBTYPE_REMAP_BAR_REGION (0) Hi Yan, How do we do this in a way that's backwards compatible? Or maybe, how do we perform a handshake between the vendor driver and userspace to indicate this support? Would the vendor driver refuse to change device_state in the migration region if the user has not enabled this IRQ? Everything you've described in the commit log needs to be in this header, we can't have the usage protocol buried in a commit log. It also seems like this is unnecessarily PCI specific. Can't the count bitmap simply indicate the region index to re-evaluate? Maybe you were worried about running out of bits in the ctx count? An IRQ per region could resolve that, but maybe we could also just add another IRQ for the next bitmap of regions. I assume that the bitmap can indicate multiple regions to re-evaluate, but that should be documented. Also, what sort of service requirements does this imply? Would the vendor driver send this IRQ when the user tries to set the device_state to _SAVING and therefore we'd require the user to accept, implement the mapping change, and acknowledge the IRQ all while waiting for the write to device_state to return? That implies quite a lot of asynchronous support in the userspace driver. Thanks, Alex > + > + > /** > * VFIO_DEVICE_SET_IRQS - _IOW(VFIO_TYPE, VFIO_BASE + 10, struct vfio_irq_set) > *
On Fri, May 29, 2020 at 03:45:47PM -0600, Alex Williamson wrote: > On Sun, 17 May 2020 22:52:45 -0400 > Yan Zhao <yan.y.zhao@intel.com> wrote: > > > This is a virtual irq type. > > vendor driver triggers this irq when it wants to notify userspace to > > remap PCI BARs. > > > > 1. vendor driver triggers this irq and packs the target bar number in > > the ctx count. i.e. "1 << bar_number". > > if a bit is set, the corresponding bar is to be remapped. > > > > 2. userspace requery the specified PCI BAR from kernel and if flags of > > the bar regions are changed, it removes the old subregions and attaches > > subregions according to the new flags. > > > > 3. userspace notifies back to kernel by writing one to the eventfd of > > this irq. > > > > Please check the corresponding qemu implementation from the reply of this > > patch, and a sample usage in vendor driver in patch [10/10]. > > > > Cc: Kevin Tian <kevin.tian@intel.com> > > Signed-off-by: Yan Zhao <yan.y.zhao@intel.com> > > --- > > include/uapi/linux/vfio.h | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h > > index 2d0d85c7c4d4..55895f75d720 100644 > > --- a/include/uapi/linux/vfio.h > > +++ b/include/uapi/linux/vfio.h > > @@ -704,6 +704,17 @@ struct vfio_irq_info_cap_type { > > __u32 subtype; /* type specific */ > > }; > > > > +/* Bar Region Query IRQ TYPE */ > > +#define VFIO_IRQ_TYPE_REMAP_BAR_REGION (1) > > + > > +/* sub-types for VFIO_IRQ_TYPE_REMAP_BAR_REGION */ > > +/* > > + * This irq notifies userspace to re-query BAR region and remaps the > > + * subregions. > > + */ > > +#define VFIO_IRQ_SUBTYPE_REMAP_BAR_REGION (0) > > Hi Yan, > > How do we do this in a way that's backwards compatible? Or maybe, how > do we perform a handshake between the vendor driver and userspace to > indicate this support? hi Alex thank you for your thoughtful review! do you think below sequence can provide enough backwards compatibility? - on vendor driver opening, it registers an irq of type VFIO_IRQ_TYPE_REMAP_BAR_REGION, and reports to driver vfio-pci there's 1 vendor irq. - after userspace detects the irq of type VFIO_IRQ_TYPE_REMAP_BAR_REGION it enables it by signaling ACTION_TRIGGER. - on receiving this ACTION_TRIGGER, vendor driver will try to setup a virqfd to monitor file write to the fd of this irq, enable this irq and return its enabling status to userspace. > Would the vendor driver refuse to change > device_state in the migration region if the user has not enabled this > IRQ? yes, vendor driver can refuse to change device_state if the irq VFIO_IRQ_TYPE_REMAP_BAR_REGION is not enabled. in my sample i40e_vf driver (patch 10/10), it implemented this logic like below: i40e_vf_set_device_state |-> case VFIO_DEVICE_STATE_SAVING | VFIO_DEVICE_STATE_RUNNING: | ret = i40e_vf_prepare_dirty_track(i40e_vf_dev); |->ret = i40e_vf_remap_bars(i40e_vf_dev, true); |->if (!i40e_vf_dev->remap_irq_ctx.init) return -ENODEV; (i40e_vf_dev->remap_irq_ctx.init is set in below path) i40e_vf_ioctl(cmd==VFIO_DEVICE_SET_IRQS) |->i40e_vf_set_irq_remap_bars |->i40e_vf_enable_remap_bars_irq |-> vf_dev->remap_irq_ctx.init = true; > > Everything you've described in the commit log needs to be in this > header, we can't have the usage protocol buried in a commit log. It got it! I'll move all descriptions in commit logs to this header so that readers can understand the whole picture here. > also seems like this is unnecessarily PCI specific. Can't the count > bitmap simply indicate the region index to re-evaluate? Maybe you were yes, it is possible. but what prevented me from doing it is that it's not easy to write an irq handler in qemu to remap other regions dynamically. for BAR regions, there're 3 layers as below. 1. bar->mr -->bottom layer 2. bar->region.mem --> slow path 3. bar->region->mmaps[i].mem --> fast path so, bar remap irq handler can simply re-revaluate the region and remove/re-generate the layer 3 (fast path) without losing track of any guest accesses to the bar regions. actually so far, the bar remap irq handler in qemu only supports remap mmap'd subregions (layout of mmap'd subregions are re-queried) and not supports updating the whole bar region size. (do you think updating bar region size is a must?) however, there are no such fast path and slow path in other regions, so remap handlers for them are region specific. > worried about running out of bits in the ctx count? An IRQ per region yes. that's also possible :) but current ctx count is 64bit, so it can support regions of index up to 63. if we don't need to remap dev regions, seems it's enough? > could resolve that, but maybe we could also just add another IRQ for > the next bitmap of regions. I assume that the bitmap can indicate > multiple regions to re-evaluate, but that should be documented. hmm. would you mind elaborating more about it? > > Also, what sort of service requirements does this imply? Would the > vendor driver send this IRQ when the user tries to set the device_state > to _SAVING and therefore we'd require the user to accept, implement the > mapping change, and acknowledge the IRQ all while waiting for the write > to device_state to return? That implies quite a lot of asynchronous > support in the userspace driver. Thanks, yes. (1) when user sets device_state to _SAVING, the vendor driver notifies this IRQ, waits until user IRQ ack is received. (2) in IRQ handler, user decodes and sends IRQ ack to vendor driver. if a wait is required in (1) returns, it demands the qemu_mutex_iothread is not locked in migration thread when device_state is set in (1), as before entering (2), acquiring of this mutex is required. Currently, this lock is not hold in vfio_migration_set_state() at save_setup stage but is hold in stop and copy stage. so we wait in kernel in save_setup stage and not wait in stop stage. it can be fixed by calling qemu_mutex_unlock_iothread() on entering vfio_migration_set_state() and qemu_mutex_lock_iothread() on leaving vfio_migration_set_state() in qemu. do you think it's acceptable? Thanks Yan > > > > + > > + > > /** > > * VFIO_DEVICE_SET_IRQS - _IOW(VFIO_TYPE, VFIO_BASE + 10, struct vfio_irq_set) > > * >
On Mon, 1 Jun 2020 02:57:26 -0400 Yan Zhao <yan.y.zhao@intel.com> wrote: > On Fri, May 29, 2020 at 03:45:47PM -0600, Alex Williamson wrote: > > On Sun, 17 May 2020 22:52:45 -0400 > > Yan Zhao <yan.y.zhao@intel.com> wrote: > > > > > This is a virtual irq type. > > > vendor driver triggers this irq when it wants to notify userspace to > > > remap PCI BARs. > > > > > > 1. vendor driver triggers this irq and packs the target bar number in > > > the ctx count. i.e. "1 << bar_number". > > > if a bit is set, the corresponding bar is to be remapped. > > > > > > 2. userspace requery the specified PCI BAR from kernel and if flags of > > > the bar regions are changed, it removes the old subregions and attaches > > > subregions according to the new flags. > > > > > > 3. userspace notifies back to kernel by writing one to the eventfd of > > > this irq. > > > > > > Please check the corresponding qemu implementation from the reply of this > > > patch, and a sample usage in vendor driver in patch [10/10]. > > > > > > Cc: Kevin Tian <kevin.tian@intel.com> > > > Signed-off-by: Yan Zhao <yan.y.zhao@intel.com> > > > --- > > > include/uapi/linux/vfio.h | 11 +++++++++++ > > > 1 file changed, 11 insertions(+) > > > > > > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h > > > index 2d0d85c7c4d4..55895f75d720 100644 > > > --- a/include/uapi/linux/vfio.h > > > +++ b/include/uapi/linux/vfio.h > > > @@ -704,6 +704,17 @@ struct vfio_irq_info_cap_type { > > > __u32 subtype; /* type specific */ > > > }; > > > > > > +/* Bar Region Query IRQ TYPE */ > > > +#define VFIO_IRQ_TYPE_REMAP_BAR_REGION (1) > > > + > > > +/* sub-types for VFIO_IRQ_TYPE_REMAP_BAR_REGION */ > > > +/* > > > + * This irq notifies userspace to re-query BAR region and remaps the > > > + * subregions. > > > + */ > > > +#define VFIO_IRQ_SUBTYPE_REMAP_BAR_REGION (0) > > > > Hi Yan, > > > > How do we do this in a way that's backwards compatible? Or maybe, how > > do we perform a handshake between the vendor driver and userspace to > > indicate this support? > hi Alex > thank you for your thoughtful review! > > do you think below sequence can provide enough backwards compatibility? > > - on vendor driver opening, it registers an irq of type > VFIO_IRQ_TYPE_REMAP_BAR_REGION, and reports to driver vfio-pci there's > 1 vendor irq. > > - after userspace detects the irq of type VFIO_IRQ_TYPE_REMAP_BAR_REGION > it enables it by signaling ACTION_TRIGGER. > > - on receiving this ACTION_TRIGGER, vendor driver will try to setup a > virqfd to monitor file write to the fd of this irq, enable this irq > and return its enabling status to userspace. I'm not sure I follow here, what's the purpose of the irqfd? When and what does the user signal by writing to the irqfd? Is this an ACK mechanism? Is this a different fd from the signaling eventfd? > > Would the vendor driver refuse to change > > device_state in the migration region if the user has not enabled this > > IRQ? > yes, vendor driver can refuse to change device_state if the irq > VFIO_IRQ_TYPE_REMAP_BAR_REGION is not enabled. > in my sample i40e_vf driver (patch 10/10), it implemented this logic > like below: > > i40e_vf_set_device_state > |-> case VFIO_DEVICE_STATE_SAVING | VFIO_DEVICE_STATE_RUNNING: > | ret = i40e_vf_prepare_dirty_track(i40e_vf_dev); > |->ret = i40e_vf_remap_bars(i40e_vf_dev, true); > |->if (!i40e_vf_dev->remap_irq_ctx.init) > return -ENODEV; > > > (i40e_vf_dev->remap_irq_ctx.init is set in below path) > i40e_vf_ioctl(cmd==VFIO_DEVICE_SET_IRQS) > |->i40e_vf_set_irq_remap_bars > |->i40e_vf_enable_remap_bars_irq > |-> vf_dev->remap_irq_ctx.init = true; This should be a documented aspect of the uapi, not left to vendor discretion to implement. > > > > Everything you've described in the commit log needs to be in this > > header, we can't have the usage protocol buried in a commit log. It > got it! I'll move all descriptions in commit logs to this header so that > readers can understand the whole picture here. > > > also seems like this is unnecessarily PCI specific. Can't the count > > bitmap simply indicate the region index to re-evaluate? Maybe you were > yes, it is possible. but what prevented me from doing it is that it's not > easy to write an irq handler in qemu to remap other regions dynamically. > > for BAR regions, there're 3 layers as below. > 1. bar->mr -->bottom layer > 2. bar->region.mem --> slow path > 3. bar->region->mmaps[i].mem --> fast path > so, bar remap irq handler can simply re-revaluate the region and > remove/re-generate the layer 3 (fast path) without losing track of any > guest accesses to the bar regions. > > actually so far, the bar remap irq handler in qemu only supports remap > mmap'd subregions (layout of mmap'd subregions are re-queried) and > not supports updating the whole bar region size. > (do you think updating bar region size is a must?) It depends on whether our interrupt is defined that the user should re-evaluate the entire region_info or just the spare mmap capability. A device spontaneously changing region size seems like a much more abstract problem. We do need to figure out how to support resizeable BARs, but it seems that would be at the direction of the user, for example emulating the resizeable BAR capability and requiring userspace to re-evaluate the region_info after interacting with that emulation. So long as we specify that this IRQ is limited to re-evaluating the sparse mmap capability for the indicated regions, I don't think we need to handle the remainder of region_info spontaneously changing. > however, there are no such fast path and slow path in other regions, so > remap handlers for them are region specific. QEMU support for re-evaluating arbitrary regions for sparse mmap changes should not limit our kernel implementation. Maybe it does suggest though that userspace should be informed of the region indexes subject to re-evaluation such that it can choose to ignore this interrupt (and lose the features enabled by the IRQ), if it doesn't support re-evaluating all of the indicated regions. For example the capability could include a bitmap indicating regions that might be signaled and the QEMU driver might skip registering an eventfd via SET_IRQS if support for non-BAR region indexes is indicated as a requirement. I'd really prefer if we can design this to not be limited to PCI BARs. > > worried about running out of bits in the ctx count? An IRQ per region > yes. that's also possible :) > but current ctx count is 64bit, so it can support regions of index up to 63. > if we don't need to remap dev regions, seems it's enough? This is the kind of decision we might look back on 10yrs later and wonder how we were so short sighted, but yes it does seem like enough and we can define additional IRQs for each of the next 64 region indexes if we need too. > > could resolve that, but maybe we could also just add another IRQ for > > the next bitmap of regions. I assume that the bitmap can indicate > > multiple regions to re-evaluate, but that should be documented. > hmm. would you mind elaborating more about it? I'm just confirming that the usage expectation would allow the user to be signaled with multiple bits in the bitmap set and the user is expected to re-evaluate each region index sparse bitmap. > > Also, what sort of service requirements does this imply? Would the > > vendor driver send this IRQ when the user tries to set the device_state > > to _SAVING and therefore we'd require the user to accept, implement the > > mapping change, and acknowledge the IRQ all while waiting for the write > > to device_state to return? That implies quite a lot of asynchronous > > support in the userspace driver. Thanks, > yes. > (1) when user sets device_state to _SAVING, the vendor driver notifies this > IRQ, waits until user IRQ ack is received. > (2) in IRQ handler, user decodes and sends IRQ ack to vendor driver. > > if a wait is required in (1) returns, it demands the qemu_mutex_iothread is > not locked in migration thread when device_state is set in (1), as before > entering (2), acquiring of this mutex is required. > > Currently, this lock is not hold in vfio_migration_set_state() at > save_setup stage but is hold in stop and copy stage. so we wait in > kernel in save_setup stage and not wait in stop stage. > it can be fixed by calling qemu_mutex_unlock_iothread() on entering > vfio_migration_set_state() and qemu_mutex_lock_iothread() on leaving > vfio_migration_set_state() in qemu. > > do you think it's acceptable? I'm not thrilled by it, it seems a bit tricky for both userspace and the vendor driver to get right. Userspace needs to handle this eventfd while blocked on write(2) into a region, which for QEMU means additional ioctls to retrieve new REGION_INFO, closing some mmaps, maybe opening other mmaps, which implies new layering of MemoryRegion sub-regions and all of the calls through KVM to implement those address space changes. The vendor driver must also be able to support concurrency of handling the REGION_INFO ioctl, new calls to mmap regions, and maybe vm_ops.close and vm_ops.fault. These regions might also be IOMMU mapped, so re-evaluating the sparse mmap could result in DMA maps and unmaps, which the vendor driver might see via the notify forcing it to unpin pages. How would the vendor driver know when to unblock the write to device_state, would it look for vm_ops.close on infringing vmas or are you thinking of an ACK via irqfd? I wouldn't want to debug lockups as a result of this design :-\ What happens if the mmap re-evaluation occurs asynchronous to the device_state write? The vendor driver can track outstanding mmap vmas to areas it's trying to revoke, so the vendor driver can know when userspace has reached an acceptable state (assuming we require userspace to munmap areas that are no longer valid). We should also consider what we can accomplish by invalidating user mmaps, ex. can we fault them back in on a per-page basis and continue to mark them dirty in the migration state, re-invalidating on each iteration until they've finally been closed. It seems the vendor driver needs to handle incrementally closing each mmap anyway, there's no requirement to the user to stop the device (ie. block all access), make these changes, then restart the device. So perhaps the vendor driver can "limp" along until userspace completes the changes. I think we can assume we are in a cooperative environment here, userspace wants to perform a migration, disabling direct access to some regions is for mediating those accesses during migration, not for preventing the user from accessing something they shouldn't have access to, userspace is only delaying the migration or affecting the state of their device by not promptly participating in the protocol. Another problem I see though is what about p2p DMA? If the vendor driver invalidates an mmap we're removing it from both direct CPU as well as DMA access via the IOMMU. We can't signal to the guest OS that a DMA channel they've been using is suddenly no longer valid. Is QEMU going to need to avoid ever IOMMU mapping device_ram for regions subject to mmap invalidation? That would introduce an undesirable need to choose whether we want to support p2p or migration unless we had an IOMMU that could provide dirty tracking via p2p, right? Thanks, Alex
On Mon, Jun 01, 2020 at 10:43:07AM -0600, Alex Williamson wrote: > On Mon, 1 Jun 2020 02:57:26 -0400 > Yan Zhao <yan.y.zhao@intel.com> wrote: > > > On Fri, May 29, 2020 at 03:45:47PM -0600, Alex Williamson wrote: > > > On Sun, 17 May 2020 22:52:45 -0400 > > > Yan Zhao <yan.y.zhao@intel.com> wrote: > > > > > > > This is a virtual irq type. > > > > vendor driver triggers this irq when it wants to notify userspace to > > > > remap PCI BARs. > > > > > > > > 1. vendor driver triggers this irq and packs the target bar number in > > > > the ctx count. i.e. "1 << bar_number". > > > > if a bit is set, the corresponding bar is to be remapped. > > > > > > > > 2. userspace requery the specified PCI BAR from kernel and if flags of > > > > the bar regions are changed, it removes the old subregions and attaches > > > > subregions according to the new flags. > > > > > > > > 3. userspace notifies back to kernel by writing one to the eventfd of > > > > this irq. > > > > > > > > Please check the corresponding qemu implementation from the reply of this > > > > patch, and a sample usage in vendor driver in patch [10/10]. > > > > > > > > Cc: Kevin Tian <kevin.tian@intel.com> > > > > Signed-off-by: Yan Zhao <yan.y.zhao@intel.com> > > > > --- > > > > include/uapi/linux/vfio.h | 11 +++++++++++ > > > > 1 file changed, 11 insertions(+) > > > > > > > > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h > > > > index 2d0d85c7c4d4..55895f75d720 100644 > > > > --- a/include/uapi/linux/vfio.h > > > > +++ b/include/uapi/linux/vfio.h > > > > @@ -704,6 +704,17 @@ struct vfio_irq_info_cap_type { > > > > __u32 subtype; /* type specific */ > > > > }; > > > > > > > > +/* Bar Region Query IRQ TYPE */ > > > > +#define VFIO_IRQ_TYPE_REMAP_BAR_REGION (1) > > > > + > > > > +/* sub-types for VFIO_IRQ_TYPE_REMAP_BAR_REGION */ > > > > +/* > > > > + * This irq notifies userspace to re-query BAR region and remaps the > > > > + * subregions. > > > > + */ > > > > +#define VFIO_IRQ_SUBTYPE_REMAP_BAR_REGION (0) > > > > > > Hi Yan, > > > > > > How do we do this in a way that's backwards compatible? Or maybe, how > > > do we perform a handshake between the vendor driver and userspace to > > > indicate this support? > > hi Alex > > thank you for your thoughtful review! > > > > do you think below sequence can provide enough backwards compatibility? > > > > - on vendor driver opening, it registers an irq of type > > VFIO_IRQ_TYPE_REMAP_BAR_REGION, and reports to driver vfio-pci there's > > 1 vendor irq. > > > > - after userspace detects the irq of type VFIO_IRQ_TYPE_REMAP_BAR_REGION > > it enables it by signaling ACTION_TRIGGER. > > > > - on receiving this ACTION_TRIGGER, vendor driver will try to setup a > > virqfd to monitor file write to the fd of this irq, enable this irq > > and return its enabling status to userspace. > > I'm not sure I follow here, what's the purpose of the irqfd? When and > what does the user signal by writing to the irqfd? Is this an ACK > mechanism? Is this a different fd from the signaling eventfd? it's not the kvm irqfd. in the vendor driver side, once ACTION_TRIGGER is received for the remap irq, interface vfio_virqfd_enable() is called to monitor writes to the eventfd of this irq. when vendor driver signals the eventfd, remap handler in QEMU is called and it writes to the eventfd after remapping is done. Then the virqfd->handler registered in vendor driver is called to receive the QEMU ack. > > > > Would the vendor driver refuse to change > > > device_state in the migration region if the user has not enabled this > > > IRQ? > > yes, vendor driver can refuse to change device_state if the irq > > VFIO_IRQ_TYPE_REMAP_BAR_REGION is not enabled. > > in my sample i40e_vf driver (patch 10/10), it implemented this logic > > like below: > > > > i40e_vf_set_device_state > > |-> case VFIO_DEVICE_STATE_SAVING | VFIO_DEVICE_STATE_RUNNING: > > | ret = i40e_vf_prepare_dirty_track(i40e_vf_dev); > > |->ret = i40e_vf_remap_bars(i40e_vf_dev, true); > > |->if (!i40e_vf_dev->remap_irq_ctx.init) > > return -ENODEV; > > > > > > (i40e_vf_dev->remap_irq_ctx.init is set in below path) > > i40e_vf_ioctl(cmd==VFIO_DEVICE_SET_IRQS) > > |->i40e_vf_set_irq_remap_bars > > |->i40e_vf_enable_remap_bars_irq > > |-> vf_dev->remap_irq_ctx.init = true; > > This should be a documented aspect of the uapi, not left to vendor > discretion to implement. > ok. got it. > > > > > > Everything you've described in the commit log needs to be in this > > > header, we can't have the usage protocol buried in a commit log. It > > got it! I'll move all descriptions in commit logs to this header so that > > readers can understand the whole picture here. > > > > > also seems like this is unnecessarily PCI specific. Can't the count > > > bitmap simply indicate the region index to re-evaluate? Maybe you were > > yes, it is possible. but what prevented me from doing it is that it's not > > easy to write an irq handler in qemu to remap other regions dynamically. > > > > for BAR regions, there're 3 layers as below. > > 1. bar->mr -->bottom layer > > 2. bar->region.mem --> slow path > > 3. bar->region->mmaps[i].mem --> fast path > > so, bar remap irq handler can simply re-revaluate the region and > > remove/re-generate the layer 3 (fast path) without losing track of any > > guest accesses to the bar regions. > > > > actually so far, the bar remap irq handler in qemu only supports remap > > mmap'd subregions (layout of mmap'd subregions are re-queried) and > > not supports updating the whole bar region size. > > (do you think updating bar region size is a must?) > > It depends on whether our interrupt is defined that the user should > re-evaluate the entire region_info or just the spare mmap capability. > A device spontaneously changing region size seems like a much more > abstract problem. We do need to figure out how to support resizeable > BARs, but it seems that would be at the direction of the user, for > example emulating the resizeable BAR capability and requiring userspace > to re-evaluate the region_info after interacting with that emulation. > So long as we specify that this IRQ is limited to re-evaluating the > sparse mmap capability for the indicated regions, I don't think we need > to handle the remainder of region_info spontaneously changing. got it. > > however, there are no such fast path and slow path in other regions, so > > remap handlers for them are region specific. > > QEMU support for re-evaluating arbitrary regions for sparse mmap > changes should not limit our kernel implementation. Maybe it does > suggest though that userspace should be informed of the region indexes > subject to re-evaluation such that it can choose to ignore this > interrupt (and lose the features enabled by the IRQ), if it doesn't > support re-evaluating all of the indicated regions. For example the > capability could include a bitmap indicating regions that might be > signaled and the QEMU driver might skip registering an eventfd via > SET_IRQS if support for non-BAR region indexes is indicated as a > requirement. I'd really prefer if we can design this to not be limited > to PCI BARs. > what about use the irq_set->start and irq_set->count in ioctl SET_IRQS to notify vendor driver of the supported invalidation range of region indexes? e.g. currently it's irq_set->start = 0, and irq_set->count=6. this SET_IRQS ioctl can be called multiple of times to notify vendor driver all supported ranges. if vendor driver signals indexes outside of this range, QEMU just ignores the request. > > > worried about running out of bits in the ctx count? An IRQ per region > > yes. that's also possible :) > > but current ctx count is 64bit, so it can support regions of index up to 63. > > if we don't need to remap dev regions, seems it's enough? > > This is the kind of decision we might look back on 10yrs later and > wonder how we were so short sighted, but yes it does seem like enough > and we can define additional IRQs for each of the next 64 region > indexes if we need too. ok. > > > > could resolve that, but maybe we could also just add another IRQ for > > > the next bitmap of regions. I assume that the bitmap can indicate > > > multiple regions to re-evaluate, but that should be documented. > > hmm. would you mind elaborating more about it? > > I'm just confirming that the usage expectation would allow the user to > be signaled with multiple bits in the bitmap set and the user is > expected to re-evaluate each region index sparse bitmap. yes, currently, vendor driver is able to specify multiple bits in the bitmap set. > > > > Also, what sort of service requirements does this imply? Would the > > > vendor driver send this IRQ when the user tries to set the device_state > > > to _SAVING and therefore we'd require the user to accept, implement the > > > mapping change, and acknowledge the IRQ all while waiting for the write > > > to device_state to return? That implies quite a lot of asynchronous > > > support in the userspace driver. Thanks, > > yes. > > (1) when user sets device_state to _SAVING, the vendor driver notifies this > > IRQ, waits until user IRQ ack is received. > > (2) in IRQ handler, user decodes and sends IRQ ack to vendor driver. > > > > if a wait is required in (1) returns, it demands the qemu_mutex_iothread is > > not locked in migration thread when device_state is set in (1), as before > > entering (2), acquiring of this mutex is required. > > > > Currently, this lock is not hold in vfio_migration_set_state() at > > save_setup stage but is hold in stop and copy stage. so we wait in > > kernel in save_setup stage and not wait in stop stage. > > it can be fixed by calling qemu_mutex_unlock_iothread() on entering > > vfio_migration_set_state() and qemu_mutex_lock_iothread() on leaving > > vfio_migration_set_state() in qemu. > > > > do you think it's acceptable? > > I'm not thrilled by it, it seems a bit tricky for both userspace and > the vendor driver to get right. Userspace needs to handle this eventfd > while blocked on write(2) into a region, which for QEMU means > additional ioctls to retrieve new REGION_INFO, closing some mmaps, > maybe opening other mmaps, which implies new layering of MemoryRegion > sub-regions and all of the calls through KVM to implement those address > space changes. The vendor driver must also be able to support > concurrency of handling the REGION_INFO ioctl, new calls to mmap > regions, and maybe vm_ops.close and vm_ops.fault. These regions might > also be IOMMU mapped, so re-evaluating the sparse mmap could result in > DMA maps and unmaps, which the vendor driver might see via the notify > forcing it to unpin pages. How would the vendor driver know when to > unblock the write to device_state, would it look for vm_ops.close on > infringing vmas or are you thinking of an ACK via irqfd? I wouldn't > want to debug lockups as a result of this design :-\ hmm, do you think below sequence is acceptable? 1. QEMU sets device_state to PRE_SAVING. vendor driver signals the remap irq and returns the device_state write. 2. QEMU remap irq handler is invoked to do the region remapping. after that user ack is sent to vendor driver by the handler writing to eventfd. 3. QEMU sets device state to SAVING. vendor driver returns success if user ack is received or failure after a timeout wait. > > What happens if the mmap re-evaluation occurs asynchronous to the > device_state write? The vendor driver can track outstanding mmap vmas > to areas it's trying to revoke, so the vendor driver can know when > userspace has reached an acceptable state (assuming we require > userspace to munmap areas that are no longer valid). We should also > consider what we can accomplish by invalidating user mmaps, ex. can we > fault them back in on a per-page basis and continue to mark them dirty > in the migration state, re-invalidating on each iteration until they've > finally been closed. It seems the vendor driver needs to handle > incrementally closing each mmap anyway, there's no requirement to the > user to stop the device (ie. block all access), make these changes, > then restart the device. So perhaps the vendor driver can "limp" along > until userspace completes the changes. I think we can assume we are in > a cooperative environment here, userspace wants to perform a migration, > disabling direct access to some regions is for mediating those accesses > during migration, not for preventing the user from accessing something > they shouldn't have access to, userspace is only delaying the migration > or affecting the state of their device by not promptly participating in > the protocol. > the problem is that the mmap re-evaluation has to be done before device_state is successfully set to SAVING. otherwise, the QEMU may have left save_setup stage and it's too late to start dirty tracking. And the reason for us to trap the BAR regions is not because there're dirty data in this region, it is because we want to know when the device registers mapped in the BARs are written, so we can do dirty page track of system memory in software way. > Another problem I see though is what about p2p DMA? If the vendor > driver invalidates an mmap we're removing it from both direct CPU as > well as DMA access via the IOMMU. We can't signal to the guest OS that > a DMA channel they've been using is suddenly no longer valid. Is QEMU > going to need to avoid ever IOMMU mapping device_ram for regions > subject to mmap invalidation? That would introduce an undesirable need > to choose whether we want to support p2p or migration unless we had an > IOMMU that could provide dirty tracking via p2p, right? Thanks, yes, if there are device memory mapped in the BARs to be remapped, p2p DMA would be affected. Perhaps it is what vendor driver should be aware of and know what it is doing before sending out the remap irq ? in i40e vf's case, the BAR 0 to be remapped is only for device registers, so is it still good? Thanks Yan
On Tue, 2 Jun 2020 04:28:58 -0400 Yan Zhao <yan.y.zhao@intel.com> wrote: > On Mon, Jun 01, 2020 at 10:43:07AM -0600, Alex Williamson wrote: > > On Mon, 1 Jun 2020 02:57:26 -0400 > > Yan Zhao <yan.y.zhao@intel.com> wrote: > > > > > On Fri, May 29, 2020 at 03:45:47PM -0600, Alex Williamson wrote: > > > > On Sun, 17 May 2020 22:52:45 -0400 > > > > Yan Zhao <yan.y.zhao@intel.com> wrote: > > > > > > > > > This is a virtual irq type. > > > > > vendor driver triggers this irq when it wants to notify userspace to > > > > > remap PCI BARs. > > > > > > > > > > 1. vendor driver triggers this irq and packs the target bar number in > > > > > the ctx count. i.e. "1 << bar_number". > > > > > if a bit is set, the corresponding bar is to be remapped. > > > > > > > > > > 2. userspace requery the specified PCI BAR from kernel and if flags of > > > > > the bar regions are changed, it removes the old subregions and attaches > > > > > subregions according to the new flags. > > > > > > > > > > 3. userspace notifies back to kernel by writing one to the eventfd of > > > > > this irq. > > > > > > > > > > Please check the corresponding qemu implementation from the reply of this > > > > > patch, and a sample usage in vendor driver in patch [10/10]. > > > > > > > > > > Cc: Kevin Tian <kevin.tian@intel.com> > > > > > Signed-off-by: Yan Zhao <yan.y.zhao@intel.com> > > > > > --- > > > > > include/uapi/linux/vfio.h | 11 +++++++++++ > > > > > 1 file changed, 11 insertions(+) > > > > > > > > > > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h > > > > > index 2d0d85c7c4d4..55895f75d720 100644 > > > > > --- a/include/uapi/linux/vfio.h > > > > > +++ b/include/uapi/linux/vfio.h > > > > > @@ -704,6 +704,17 @@ struct vfio_irq_info_cap_type { > > > > > __u32 subtype; /* type specific */ > > > > > }; > > > > > > > > > > +/* Bar Region Query IRQ TYPE */ > > > > > +#define VFIO_IRQ_TYPE_REMAP_BAR_REGION (1) > > > > > + > > > > > +/* sub-types for VFIO_IRQ_TYPE_REMAP_BAR_REGION */ > > > > > +/* > > > > > + * This irq notifies userspace to re-query BAR region and remaps the > > > > > + * subregions. > > > > > + */ > > > > > +#define VFIO_IRQ_SUBTYPE_REMAP_BAR_REGION (0) > > > > > > > > Hi Yan, > > > > > > > > How do we do this in a way that's backwards compatible? Or maybe, how > > > > do we perform a handshake between the vendor driver and userspace to > > > > indicate this support? > > > hi Alex > > > thank you for your thoughtful review! > > > > > > do you think below sequence can provide enough backwards compatibility? > > > > > > - on vendor driver opening, it registers an irq of type > > > VFIO_IRQ_TYPE_REMAP_BAR_REGION, and reports to driver vfio-pci there's > > > 1 vendor irq. > > > > > > - after userspace detects the irq of type VFIO_IRQ_TYPE_REMAP_BAR_REGION > > > it enables it by signaling ACTION_TRIGGER. > > > > > > - on receiving this ACTION_TRIGGER, vendor driver will try to setup a > > > virqfd to monitor file write to the fd of this irq, enable this irq > > > and return its enabling status to userspace. > > > > I'm not sure I follow here, what's the purpose of the irqfd? When and > > what does the user signal by writing to the irqfd? Is this an ACK > > mechanism? Is this a different fd from the signaling eventfd? > it's not the kvm irqfd. > in the vendor driver side, once ACTION_TRIGGER is received for the remap irq, > interface vfio_virqfd_enable() is called to monitor writes to the eventfd of > this irq. > > when vendor driver signals the eventfd, remap handler in QEMU is > called and it writes to the eventfd after remapping is done. > Then the virqfd->handler registered in vendor driver is called to receive > the QEMU ack. This seems racy to use the same fd as both an eventfd and irqfd, does the host need to wait for the user to service the previous IRQ before sending a new one? Don't we have gaps where the user is either reading or writing where we can lose an interrupt? Does the user also write a bitmap? How do we avoid getting out of sync? Why do we even need this, can't the vendor driver look for vm_ops.close callbacks for the offending vma mmaps? > > > > Would the vendor driver refuse to change > > > > device_state in the migration region if the user has not enabled this > > > > IRQ? > > > yes, vendor driver can refuse to change device_state if the irq > > > VFIO_IRQ_TYPE_REMAP_BAR_REGION is not enabled. > > > in my sample i40e_vf driver (patch 10/10), it implemented this logic > > > like below: > > > > > > i40e_vf_set_device_state > > > |-> case VFIO_DEVICE_STATE_SAVING | VFIO_DEVICE_STATE_RUNNING: > > > | ret = i40e_vf_prepare_dirty_track(i40e_vf_dev); > > > |->ret = i40e_vf_remap_bars(i40e_vf_dev, true); > > > |->if (!i40e_vf_dev->remap_irq_ctx.init) > > > return -ENODEV; > > > > > > > > > (i40e_vf_dev->remap_irq_ctx.init is set in below path) > > > i40e_vf_ioctl(cmd==VFIO_DEVICE_SET_IRQS) > > > |->i40e_vf_set_irq_remap_bars > > > |->i40e_vf_enable_remap_bars_irq > > > |-> vf_dev->remap_irq_ctx.init = true; > > > > This should be a documented aspect of the uapi, not left to vendor > > discretion to implement. > > > ok. got it. > > > > > > > > > Everything you've described in the commit log needs to be in this > > > > header, we can't have the usage protocol buried in a commit log. It > > > got it! I'll move all descriptions in commit logs to this header so that > > > readers can understand the whole picture here. > > > > > > > also seems like this is unnecessarily PCI specific. Can't the count > > > > bitmap simply indicate the region index to re-evaluate? Maybe you were > > > yes, it is possible. but what prevented me from doing it is that it's not > > > easy to write an irq handler in qemu to remap other regions dynamically. > > > > > > for BAR regions, there're 3 layers as below. > > > 1. bar->mr -->bottom layer > > > 2. bar->region.mem --> slow path > > > 3. bar->region->mmaps[i].mem --> fast path > > > so, bar remap irq handler can simply re-revaluate the region and > > > remove/re-generate the layer 3 (fast path) without losing track of any > > > guest accesses to the bar regions. > > > > > > actually so far, the bar remap irq handler in qemu only supports remap > > > mmap'd subregions (layout of mmap'd subregions are re-queried) and > > > not supports updating the whole bar region size. > > > (do you think updating bar region size is a must?) > > > > It depends on whether our interrupt is defined that the user should > > re-evaluate the entire region_info or just the spare mmap capability. > > A device spontaneously changing region size seems like a much more > > abstract problem. We do need to figure out how to support resizeable > > BARs, but it seems that would be at the direction of the user, for > > example emulating the resizeable BAR capability and requiring userspace > > to re-evaluate the region_info after interacting with that emulation. > > So long as we specify that this IRQ is limited to re-evaluating the > > sparse mmap capability for the indicated regions, I don't think we need > > to handle the remainder of region_info spontaneously changing. > got it. > > > > however, there are no such fast path and slow path in other regions, so > > > remap handlers for them are region specific. > > > > QEMU support for re-evaluating arbitrary regions for sparse mmap > > changes should not limit our kernel implementation. Maybe it does > > suggest though that userspace should be informed of the region indexes > > subject to re-evaluation such that it can choose to ignore this > > interrupt (and lose the features enabled by the IRQ), if it doesn't > > support re-evaluating all of the indicated regions. For example the > > capability could include a bitmap indicating regions that might be > > signaled and the QEMU driver might skip registering an eventfd via > > SET_IRQS if support for non-BAR region indexes is indicated as a > > requirement. I'd really prefer if we can design this to not be limited > > to PCI BARs. > > > what about use the irq_set->start and irq_set->count in ioctl SET_IRQS > to notify vendor driver of the supported invalidation range of region > indexes? e.g. currently it's irq_set->start = 0, and irq_set->count=6. > this SET_IRQS ioctl can be called multiple of times to notify vendor > driver all supported ranges. > if vendor driver signals indexes outside of this range, QEMU just > ignores the request. You want an IRQ per region? I don't think I understand this proposal. Overloading sub-indexes within a SET_IRQS index is not acceptable. Also, "if vendor driver signals indexes outside of this range, QEMU just ignores the request", if that means that QEMU doesn't handle a request to re-evaluate the sparse mmap capability I think that nullifies the entire proposal. > > > > worried about running out of bits in the ctx count? An IRQ per region > > > yes. that's also possible :) > > > but current ctx count is 64bit, so it can support regions of index up to 63. > > > if we don't need to remap dev regions, seems it's enough? > > > > This is the kind of decision we might look back on 10yrs later and > > wonder how we were so short sighted, but yes it does seem like enough > > and we can define additional IRQs for each of the next 64 region > > indexes if we need too. > ok. > > > > > > > could resolve that, but maybe we could also just add another IRQ for > > > > the next bitmap of regions. I assume that the bitmap can indicate > > > > multiple regions to re-evaluate, but that should be documented. > > > hmm. would you mind elaborating more about it? > > > > I'm just confirming that the usage expectation would allow the user to > > be signaled with multiple bits in the bitmap set and the user is > > expected to re-evaluate each region index sparse bitmap. > yes, currently, vendor driver is able to specify multiple bits in the > bitmap set. > > > > > > > Also, what sort of service requirements does this imply? Would the > > > > vendor driver send this IRQ when the user tries to set the device_state > > > > to _SAVING and therefore we'd require the user to accept, implement the > > > > mapping change, and acknowledge the IRQ all while waiting for the write > > > > to device_state to return? That implies quite a lot of asynchronous > > > > support in the userspace driver. Thanks, > > > yes. > > > (1) when user sets device_state to _SAVING, the vendor driver notifies this > > > IRQ, waits until user IRQ ack is received. > > > (2) in IRQ handler, user decodes and sends IRQ ack to vendor driver. > > > > > > if a wait is required in (1) returns, it demands the qemu_mutex_iothread is > > > not locked in migration thread when device_state is set in (1), as before > > > entering (2), acquiring of this mutex is required. > > > > > > Currently, this lock is not hold in vfio_migration_set_state() at > > > save_setup stage but is hold in stop and copy stage. so we wait in > > > kernel in save_setup stage and not wait in stop stage. > > > it can be fixed by calling qemu_mutex_unlock_iothread() on entering > > > vfio_migration_set_state() and qemu_mutex_lock_iothread() on leaving > > > vfio_migration_set_state() in qemu. > > > > > > do you think it's acceptable? > > > > I'm not thrilled by it, it seems a bit tricky for both userspace and > > the vendor driver to get right. Userspace needs to handle this eventfd > > while blocked on write(2) into a region, which for QEMU means > > additional ioctls to retrieve new REGION_INFO, closing some mmaps, > > maybe opening other mmaps, which implies new layering of MemoryRegion > > sub-regions and all of the calls through KVM to implement those address > > space changes. The vendor driver must also be able to support > > concurrency of handling the REGION_INFO ioctl, new calls to mmap > > regions, and maybe vm_ops.close and vm_ops.fault. These regions might > > also be IOMMU mapped, so re-evaluating the sparse mmap could result in > > DMA maps and unmaps, which the vendor driver might see via the notify > > forcing it to unpin pages. How would the vendor driver know when to > > unblock the write to device_state, would it look for vm_ops.close on > > infringing vmas or are you thinking of an ACK via irqfd? I wouldn't > > want to debug lockups as a result of this design :-\ > hmm, do you think below sequence is acceptable? > 1. QEMU sets device_state to PRE_SAVING. > vendor driver signals the remap irq and returns the device_state > write. We have no PRE_SAVING device state. We haven't even gotten an implementation of the agreed migration protocol into mainline and we're already abandoning it? > 2. QEMU remap irq handler is invoked to do the region remapping. after > that user ack is sent to vendor driver by the handler writing to eventfd. Why would we even need an remap IRQ, if we're going to throw away the migration protocol we could just define that the user should re-evaluate the sparse mmaps when switching to _SAVING. > 3. QEMU sets device state to SAVING. > vendor driver returns success if user ack is received or failure > after a timeout wait. I'm not at all happy with this. Why do we need to hide the migration sparse mmap from the user until migration time? What if instead we introduced a new VFIO_REGION_INFO_CAP_SPARSE_MMAP_SAVING capability where the existing capability is the normal runtime sparse setup and the user is required to use this new one prior to enabled device_state with _SAVING. The vendor driver could then simply track mmap vmas to the region and refuse to change device_state if there are outstanding mmaps conflicting with the _SAVING sparse mmap layout. No new IRQs required, no new irqfds, an incremental change to the protocol, backwards compatible to the extent that a vendor driver requiring this will automatically fail migration. > > What happens if the mmap re-evaluation occurs asynchronous to the > > device_state write? The vendor driver can track outstanding mmap vmas > > to areas it's trying to revoke, so the vendor driver can know when > > userspace has reached an acceptable state (assuming we require > > userspace to munmap areas that are no longer valid). We should also > > consider what we can accomplish by invalidating user mmaps, ex. can we > > fault them back in on a per-page basis and continue to mark them dirty > > in the migration state, re-invalidating on each iteration until they've > > finally been closed. It seems the vendor driver needs to handle > > incrementally closing each mmap anyway, there's no requirement to the > > user to stop the device (ie. block all access), make these changes, > > then restart the device. So perhaps the vendor driver can "limp" along > > until userspace completes the changes. I think we can assume we are in > > a cooperative environment here, userspace wants to perform a migration, > > disabling direct access to some regions is for mediating those accesses > > during migration, not for preventing the user from accessing something > > they shouldn't have access to, userspace is only delaying the migration > > or affecting the state of their device by not promptly participating in > > the protocol. > > > the problem is that the mmap re-evaluation has to be done before > device_state is successfully set to SAVING. otherwise, the QEMU may > have left save_setup stage and it's too late to start dirty tracking. > And the reason for us to trap the BAR regions is not because there're > dirty data in this region, it is because we want to know when the device > registers mapped in the BARs are written, so we can do dirty page track > of system memory in software way. I think my proposal above resolves this. > > Another problem I see though is what about p2p DMA? If the vendor > > driver invalidates an mmap we're removing it from both direct CPU as > > well as DMA access via the IOMMU. We can't signal to the guest OS that > > a DMA channel they've been using is suddenly no longer valid. Is QEMU > > going to need to avoid ever IOMMU mapping device_ram for regions > > subject to mmap invalidation? That would introduce an undesirable need > > to choose whether we want to support p2p or migration unless we had an > > IOMMU that could provide dirty tracking via p2p, right? Thanks, > > yes, if there are device memory mapped in the BARs to be remapped, p2p > DMA would be affected. Perhaps it is what vendor driver should be aware > of and know what it is doing before sending out the remap irq ? > in i40e vf's case, the BAR 0 to be remapped is only for device registers, > so is it still good? No, we can't design the interface based on one vendor driver's implementation of the interface or the requirements of a single device. If we took the approach above where the user is provided both the normal sparse mmap and the _SAVING sparse mmap, perhaps QEMU could avoid DMA mapping portions that don't exist in the _SAVING version, at least then the p2p DMA mappings would be consistent across the transition. QEMU might be able to combine the sparse mmap maps such that it can easily drop ranges not present during _SAVING. QEMU would need to munmap() the dropped ranges rather than simply mark the MemoryRegion disabled though for the vendor driver to have visibility of the vm_ops.close callback. Thanks, Alex
On Tue, Jun 02, 2020 at 01:34:35PM -0600, Alex Williamson wrote: > I'm not at all happy with this. Why do we need to hide the migration > sparse mmap from the user until migration time? What if instead we > introduced a new VFIO_REGION_INFO_CAP_SPARSE_MMAP_SAVING capability > where the existing capability is the normal runtime sparse setup and > the user is required to use this new one prior to enabled device_state > with _SAVING. The vendor driver could then simply track mmap vmas to > the region and refuse to change device_state if there are outstanding > mmaps conflicting with the _SAVING sparse mmap layout. No new IRQs > required, no new irqfds, an incremental change to the protocol, > backwards compatible to the extent that a vendor driver requiring this > will automatically fail migration. > right. looks we need to use this approach to solve the problem. thanks for your guide. so I'll abandon the current remap irq way for dirty tracking during live migration. but anyway, it demos how to customize irq_types in vendor drivers. then, what do you think about patches 1-5? > > > What happens if the mmap re-evaluation occurs asynchronous to the > > > device_state write? The vendor driver can track outstanding mmap vmas > > > to areas it's trying to revoke, so the vendor driver can know when > > > userspace has reached an acceptable state (assuming we require > > > userspace to munmap areas that are no longer valid). We should also > > > consider what we can accomplish by invalidating user mmaps, ex. can we > > > fault them back in on a per-page basis and continue to mark them dirty > > > in the migration state, re-invalidating on each iteration until they've > > > finally been closed. It seems the vendor driver needs to handle > > > incrementally closing each mmap anyway, there's no requirement to the > > > user to stop the device (ie. block all access), make these changes, > > > then restart the device. So perhaps the vendor driver can "limp" along > > > until userspace completes the changes. I think we can assume we are in > > > a cooperative environment here, userspace wants to perform a migration, > > > disabling direct access to some regions is for mediating those accesses > > > during migration, not for preventing the user from accessing something > > > they shouldn't have access to, userspace is only delaying the migration > > > or affecting the state of their device by not promptly participating in > > > the protocol. > > > > > the problem is that the mmap re-evaluation has to be done before > > device_state is successfully set to SAVING. otherwise, the QEMU may > > have left save_setup stage and it's too late to start dirty tracking. > > And the reason for us to trap the BAR regions is not because there're > > dirty data in this region, it is because we want to know when the device > > registers mapped in the BARs are written, so we can do dirty page track > > of system memory in software way. > > I think my proposal above resolves this. > yes. > > > Another problem I see though is what about p2p DMA? If the vendor > > > driver invalidates an mmap we're removing it from both direct CPU as > > > well as DMA access via the IOMMU. We can't signal to the guest OS that > > > a DMA channel they've been using is suddenly no longer valid. Is QEMU > > > going to need to avoid ever IOMMU mapping device_ram for regions > > > subject to mmap invalidation? That would introduce an undesirable need > > > to choose whether we want to support p2p or migration unless we had an > > > IOMMU that could provide dirty tracking via p2p, right? Thanks, > > > > yes, if there are device memory mapped in the BARs to be remapped, p2p > > DMA would be affected. Perhaps it is what vendor driver should be aware > > of and know what it is doing before sending out the remap irq ? > > in i40e vf's case, the BAR 0 to be remapped is only for device registers, > > so is it still good? > > No, we can't design the interface based on one vendor driver's > implementation of the interface or the requirements of a single device. > If we took the approach above where the user is provided both the > normal sparse mmap and the _SAVING sparse mmap, perhaps QEMU could > avoid DMA mapping portions that don't exist in the _SAVING version, at > least then the p2p DMA mappings would be consistent across the > transition. QEMU might be able to combine the sparse mmap maps such > that it can easily drop ranges not present during _SAVING. QEMU would > need to munmap() the dropped ranges rather than simply mark the > MemoryRegion disabled though for the vendor driver to have visibility > of the vm_ops.close callback. Thanks, > ok. got it! thanks you! Yan
On Tue, 2 Jun 2020 21:40:58 -0400 Yan Zhao <yan.y.zhao@intel.com> wrote: > On Tue, Jun 02, 2020 at 01:34:35PM -0600, Alex Williamson wrote: > > I'm not at all happy with this. Why do we need to hide the migration > > sparse mmap from the user until migration time? What if instead we > > introduced a new VFIO_REGION_INFO_CAP_SPARSE_MMAP_SAVING capability > > where the existing capability is the normal runtime sparse setup and > > the user is required to use this new one prior to enabled device_state > > with _SAVING. The vendor driver could then simply track mmap vmas to > > the region and refuse to change device_state if there are outstanding > > mmaps conflicting with the _SAVING sparse mmap layout. No new IRQs > > required, no new irqfds, an incremental change to the protocol, > > backwards compatible to the extent that a vendor driver requiring this > > will automatically fail migration. > > > right. looks we need to use this approach to solve the problem. > thanks for your guide. > so I'll abandon the current remap irq way for dirty tracking during live > migration. > but anyway, it demos how to customize irq_types in vendor drivers. > then, what do you think about patches 1-5? In broad strokes, I don't think we've found the right solution yet. I really question whether it's supportable to parcel out vfio-pci like this and I don't know how I'd support unraveling whether we have a bug in vfio-pci, the vendor driver, or how the vendor driver is making use of vfio-pci. Let me also ask, why does any of this need to be in the kernel? We spend 5 patches slicing up vfio-pci so that we can register a vendor driver and have that vendor driver call into vfio-pci as it sees fit. We have two patches creating device specific interrupts and a BAR remapping scheme that we've decided we don't need. That brings us to the actual i40e vendor driver, where the first patch is simply making the vendor driver work like vfio-pci already does, the second patch is handling the migration region, and the third patch is implementing the BAR remapping IRQ that we decided we don't need. It's difficult to actually find the small bit of code that's required to support migration outside of just dealing with the protocol we've defined to expose this from the kernel. So why are we trying to do this in the kernel? We have quirk support in QEMU, we can easily flip MemoryRegions on and off, etc. What access to the device outside of what vfio-pci provides to the user, and therefore QEMU, is necessary to implement this migration support for i40e VFs? Is this just an exercise in making use of the migration interface? Thanks, Alex
On Wed, Jun 03, 2020 at 05:04:52PM -0600, Alex Williamson wrote: > On Tue, 2 Jun 2020 21:40:58 -0400 > Yan Zhao <yan.y.zhao@intel.com> wrote: > > > On Tue, Jun 02, 2020 at 01:34:35PM -0600, Alex Williamson wrote: > > > I'm not at all happy with this. Why do we need to hide the migration > > > sparse mmap from the user until migration time? What if instead we > > > introduced a new VFIO_REGION_INFO_CAP_SPARSE_MMAP_SAVING capability > > > where the existing capability is the normal runtime sparse setup and > > > the user is required to use this new one prior to enabled device_state > > > with _SAVING. The vendor driver could then simply track mmap vmas to > > > the region and refuse to change device_state if there are outstanding > > > mmaps conflicting with the _SAVING sparse mmap layout. No new IRQs > > > required, no new irqfds, an incremental change to the protocol, > > > backwards compatible to the extent that a vendor driver requiring this > > > will automatically fail migration. > > > > > right. looks we need to use this approach to solve the problem. > > thanks for your guide. > > so I'll abandon the current remap irq way for dirty tracking during live > > migration. > > but anyway, it demos how to customize irq_types in vendor drivers. > > then, what do you think about patches 1-5? > > In broad strokes, I don't think we've found the right solution yet. I > really question whether it's supportable to parcel out vfio-pci like > this and I don't know how I'd support unraveling whether we have a bug > in vfio-pci, the vendor driver, or how the vendor driver is making use > of vfio-pci. > > Let me also ask, why does any of this need to be in the kernel? We > spend 5 patches slicing up vfio-pci so that we can register a vendor > driver and have that vendor driver call into vfio-pci as it sees fit. > We have two patches creating device specific interrupts and a BAR > remapping scheme that we've decided we don't need. That brings us to > the actual i40e vendor driver, where the first patch is simply making > the vendor driver work like vfio-pci already does, the second patch is > handling the migration region, and the third patch is implementing the > BAR remapping IRQ that we decided we don't need. It's difficult to > actually find the small bit of code that's required to support > migration outside of just dealing with the protocol we've defined to > expose this from the kernel. So why are we trying to do this in the > kernel? We have quirk support in QEMU, we can easily flip > MemoryRegions on and off, etc. What access to the device outside of > what vfio-pci provides to the user, and therefore QEMU, is necessary to > implement this migration support for i40e VFs? Is this just an > exercise in making use of the migration interface? Thanks, > hi Alex There was a description of intention of this series in RFC v1 (https://www.spinics.net/lists/kernel/msg3337337.html). sorry, I didn't include it in starting from RFC v2. " The reason why we don't choose the way of writing mdev parent driver is that (1) VFs are almost all the time directly passthroughed. Directly binding to vfio-pci can make most of the code shared/reused. If we write a vendor specific mdev parent driver, most of the code (like passthrough style of rw/mmap) still needs to be copied from vfio-pci driver, which is actually a duplicated and tedious work. (2) For features like dynamically trap/untrap pci bars, if they are in vfio-pci, they can be available to most people without repeated code copying and re-testing. (3) with a 1:1 mdev driver which passes through VFs most of the time, people have to decide whether to bind VFs to vfio-pci or mdev parent driver before it runs into a real migration need. However, if vfio-pci is bound initially, they have no chance to do live migration when there's a need later. " particularly, there're some devices (like NVMe) they purely reply on vfio-pci to do device pass-through and they have no standalone parent driver to do mdev way. I think live migration is a general requirement for most devices and to interact with the migration interface requires vendor drivers to do device specific tasks like geting/seting device state, starting/stopping devices, tracking dirty data, report migration capabilities... all those works need be in kernel. do you think it's better to create numerous vendor quirks in vfio-pci? as to this series, though patch 9/10 currently only demos reporting a migration region, it actually shows the capability iof vendor driver to customize device regions. e.g. in patch 10/10, it customizes the BAR0 to be read/write. and though we abandoned the REMAP BAR irq_type in patch 10/10 for migration purpose, I have to say this irq_type has its usage in other use cases, where synchronization is not a hard requirement and all it needs is a notification channel from kernel to use. this series just provides a possibility for vendors to customize device regions and irqs. for interfaces exported in patch 3/10-5/10, they anyway need to be exported for writing mdev parent drivers that pass through devices at normal time to avoid duplication. and yes, your worry about identification of bug sources is reasonable. but if a device is binding to vfio-pci with a vendor module loaded, and there's a bug, they can do at least two ways to identify if it's a bug in vfio-pci itself. (1) prevent vendor modules from loading and see if the problem exists with pure vfio-pci. (2) do what's demoed in patch 8/10, i.e. do nothing but simply pass all operations to vfio-pci. so, do you think this series has its merit and we can continue improving it? Thanks Yan
On Wed, 3 Jun 2020 22:42:28 -0400 Yan Zhao <yan.y.zhao@intel.com> wrote: > On Wed, Jun 03, 2020 at 05:04:52PM -0600, Alex Williamson wrote: > > On Tue, 2 Jun 2020 21:40:58 -0400 > > Yan Zhao <yan.y.zhao@intel.com> wrote: > > > > > On Tue, Jun 02, 2020 at 01:34:35PM -0600, Alex Williamson wrote: > > > > I'm not at all happy with this. Why do we need to hide the migration > > > > sparse mmap from the user until migration time? What if instead we > > > > introduced a new VFIO_REGION_INFO_CAP_SPARSE_MMAP_SAVING capability > > > > where the existing capability is the normal runtime sparse setup and > > > > the user is required to use this new one prior to enabled device_state > > > > with _SAVING. The vendor driver could then simply track mmap vmas to > > > > the region and refuse to change device_state if there are outstanding > > > > mmaps conflicting with the _SAVING sparse mmap layout. No new IRQs > > > > required, no new irqfds, an incremental change to the protocol, > > > > backwards compatible to the extent that a vendor driver requiring this > > > > will automatically fail migration. > > > > > > > right. looks we need to use this approach to solve the problem. > > > thanks for your guide. > > > so I'll abandon the current remap irq way for dirty tracking during live > > > migration. > > > but anyway, it demos how to customize irq_types in vendor drivers. > > > then, what do you think about patches 1-5? > > > > In broad strokes, I don't think we've found the right solution yet. I > > really question whether it's supportable to parcel out vfio-pci like > > this and I don't know how I'd support unraveling whether we have a bug > > in vfio-pci, the vendor driver, or how the vendor driver is making use > > of vfio-pci. > > > > Let me also ask, why does any of this need to be in the kernel? We > > spend 5 patches slicing up vfio-pci so that we can register a vendor > > driver and have that vendor driver call into vfio-pci as it sees fit. > > We have two patches creating device specific interrupts and a BAR > > remapping scheme that we've decided we don't need. That brings us to > > the actual i40e vendor driver, where the first patch is simply making > > the vendor driver work like vfio-pci already does, the second patch is > > handling the migration region, and the third patch is implementing the > > BAR remapping IRQ that we decided we don't need. It's difficult to > > actually find the small bit of code that's required to support > > migration outside of just dealing with the protocol we've defined to > > expose this from the kernel. So why are we trying to do this in the > > kernel? We have quirk support in QEMU, we can easily flip > > MemoryRegions on and off, etc. What access to the device outside of > > what vfio-pci provides to the user, and therefore QEMU, is necessary to > > implement this migration support for i40e VFs? Is this just an > > exercise in making use of the migration interface? Thanks, > > > hi Alex > > There was a description of intention of this series in RFC v1 > (https://www.spinics.net/lists/kernel/msg3337337.html). > sorry, I didn't include it in starting from RFC v2. > > " > The reason why we don't choose the way of writing mdev parent driver is > that I didn't mention an mdev approach, I'm asking what are we accomplishing by doing this in the kernel at all versus exposing the device as normal through vfio-pci and providing the migration support in QEMU. Are you actually leveraging having some sort of access to the PF in supporting migration of the VF? Is vfio-pci masking the device in a way that prevents migrating the state from QEMU? > (1) VFs are almost all the time directly passthroughed. Directly binding > to vfio-pci can make most of the code shared/reused. If we write a > vendor specific mdev parent driver, most of the code (like passthrough > style of rw/mmap) still needs to be copied from vfio-pci driver, which is > actually a duplicated and tedious work. > (2) For features like dynamically trap/untrap pci bars, if they are in > vfio-pci, they can be available to most people without repeated code > copying and re-testing. > (3) with a 1:1 mdev driver which passes through VFs most of the time, people > have to decide whether to bind VFs to vfio-pci or mdev parent driver before > it runs into a real migration need. However, if vfio-pci is bound > initially, they have no chance to do live migration when there's a need > later. > " > particularly, there're some devices (like NVMe) they purely reply on > vfio-pci to do device pass-through and they have no standalone parent driver > to do mdev way. > > I think live migration is a general requirement for most devices and to > interact with the migration interface requires vendor drivers to do > device specific tasks like geting/seting device state, starting/stopping > devices, tracking dirty data, report migration capabilities... all those > works need be in kernel. I think Alex Graf proved they don't necessarily need to be done in kernel back in 2015: https://www.youtube.com/watch?v=4RFsSgzuFso He was able to achieve i40e VF live migration by only hacking QEMU. In this series you're allowing a vendor driver to interpose itself between the user (QEMU) and vfio-pci such that we switch to the vendor code during migration. Why can't that interpose layer be in QEMU rather than the kernel? It seems that it only must be in the kernel if we need to provide migration state via backdoor, perhaps like going through the PF. So what access to the i40e VF device is not provided to the user through vfio-pci that is necessary to implement migration of this device? The tasks listed above are mostly standard device driver activities and clearly vfio-pci allows userspace device drivers. > do you think it's better to create numerous vendor quirks in vfio-pci? In QEMU, perhaps. Alternatively, let's look at exactly what access is not provided through vfio-pci that's necessary for this and decide if we want to enable that access or if cracking vfio-pci wide open for vendor drivers to pick and choose when and how to use it is really the right answer. > as to this series, though patch 9/10 currently only demos reporting a > migration region, it actually shows the capability iof vendor driver to > customize device regions. e.g. in patch 10/10, it customizes the BAR0 to > be read/write. and though we abandoned the REMAP BAR irq_type in patch > 10/10 for migration purpose, I have to say this irq_type has its usage > in other use cases, where synchronization is not a hard requirement and > all it needs is a notification channel from kernel to use. this series > just provides a possibility for vendors to customize device regions and > irqs. I don't disagree that a device specific interrupt might be useful, but I would object to implementing this one only as an artificial use case. We can wait for a legitimate use case to implement that. > for interfaces exported in patch 3/10-5/10, they anyway need to be > exported for writing mdev parent drivers that pass through devices at > normal time to avoid duplication. and yes, your worry about Where are those parent drivers? What are their actual requirements? > identification of bug sources is reasonable. but if a device is binding > to vfio-pci with a vendor module loaded, and there's a bug, they can do at > least two ways to identify if it's a bug in vfio-pci itself. > (1) prevent vendor modules from loading and see if the problem exists > with pure vfio-pci. > (2) do what's demoed in patch 8/10, i.e. do nothing but simply pass all > operations to vfio-pci. The code split is still extremely ad-hoc, there's no API. An mdev driver isn't even a sub-driver of vfio-pci like you're trying to accomplish here, there would need to be a much more defined API when the base device isn't even a vfio_pci_device. I don't see how this series would directly enable an mdev use case. > so, do you think this series has its merit and we can continue improving > it? I think this series is trying to push an artificial use case that is perhaps better done in userspace. What is the actual interaction with the VF device that can only be done in the host kernel for this example? Thanks, Alex
> From: Alex Williamson <alex.williamson@redhat.com> > Sent: Thursday, June 4, 2020 12:11 PM > > On Wed, 3 Jun 2020 22:42:28 -0400 > Yan Zhao <yan.y.zhao@intel.com> wrote: > > > On Wed, Jun 03, 2020 at 05:04:52PM -0600, Alex Williamson wrote: > > > On Tue, 2 Jun 2020 21:40:58 -0400 > > > Yan Zhao <yan.y.zhao@intel.com> wrote: > > > > > > > On Tue, Jun 02, 2020 at 01:34:35PM -0600, Alex Williamson wrote: > > > > > I'm not at all happy with this. Why do we need to hide the > > > > > migration sparse mmap from the user until migration time? What > > > > > if instead we introduced a new > > > > > VFIO_REGION_INFO_CAP_SPARSE_MMAP_SAVING capability where > the > > > > > existing capability is the normal runtime sparse setup and the > > > > > user is required to use this new one prior to enabled > > > > > device_state with _SAVING. The vendor driver could then simply > > > > > track mmap vmas to the region and refuse to change device_state > > > > > if there are outstanding mmaps conflicting with the _SAVING > > > > > sparse mmap layout. No new IRQs required, no new irqfds, an > > > > > incremental change to the protocol, backwards compatible to the > extent that a vendor driver requiring this will automatically fail migration. > > > > > > > > > right. looks we need to use this approach to solve the problem. > > > > thanks for your guide. > > > > so I'll abandon the current remap irq way for dirty tracking > > > > during live migration. > > > > but anyway, it demos how to customize irq_types in vendor drivers. > > > > then, what do you think about patches 1-5? > > > > > > In broad strokes, I don't think we've found the right solution yet. > > > I really question whether it's supportable to parcel out vfio-pci > > > like this and I don't know how I'd support unraveling whether we > > > have a bug in vfio-pci, the vendor driver, or how the vendor driver > > > is making use of vfio-pci. > > > > > > Let me also ask, why does any of this need to be in the kernel? We > > > spend 5 patches slicing up vfio-pci so that we can register a vendor > > > driver and have that vendor driver call into vfio-pci as it sees fit. > > > We have two patches creating device specific interrupts and a BAR > > > remapping scheme that we've decided we don't need. That brings us > > > to the actual i40e vendor driver, where the first patch is simply > > > making the vendor driver work like vfio-pci already does, the second > > > patch is handling the migration region, and the third patch is > > > implementing the BAR remapping IRQ that we decided we don't need. > > > It's difficult to actually find the small bit of code that's > > > required to support migration outside of just dealing with the > > > protocol we've defined to expose this from the kernel. So why are > > > we trying to do this in the kernel? We have quirk support in QEMU, > > > we can easily flip MemoryRegions on and off, etc. What access to > > > the device outside of what vfio-pci provides to the user, and > > > therefore QEMU, is necessary to implement this migration support for > > > i40e VFs? Is this just an exercise in making use of the migration > > > interface? Thanks, > > > > > hi Alex > > > > There was a description of intention of this series in RFC v1 > > (https://www.spinics.net/lists/kernel/msg3337337.html). > > sorry, I didn't include it in starting from RFC v2. > > > > " > > The reason why we don't choose the way of writing mdev parent driver > > is that > > I didn't mention an mdev approach, I'm asking what are we accomplishing by > doing this in the kernel at all versus exposing the device as normal through > vfio-pci and providing the migration support in QEMU. Are you actually > leveraging having some sort of access to the PF in supporting migration of the > VF? Is vfio-pci masking the device in a way that prevents migrating the state > from QEMU? > > > (1) VFs are almost all the time directly passthroughed. Directly > > binding to vfio-pci can make most of the code shared/reused. If we > > write a vendor specific mdev parent driver, most of the code (like > > passthrough style of rw/mmap) still needs to be copied from vfio-pci > > driver, which is actually a duplicated and tedious work. > > (2) For features like dynamically trap/untrap pci bars, if they are in > > vfio-pci, they can be available to most people without repeated code > > copying and re-testing. > > (3) with a 1:1 mdev driver which passes through VFs most of the time, > > people have to decide whether to bind VFs to vfio-pci or mdev parent > > driver before it runs into a real migration need. However, if vfio-pci > > is bound initially, they have no chance to do live migration when > > there's a need later. > > " > > particularly, there're some devices (like NVMe) they purely reply on > > vfio-pci to do device pass-through and they have no standalone parent > > driver to do mdev way. > > > > I think live migration is a general requirement for most devices and > > to interact with the migration interface requires vendor drivers to do > > device specific tasks like geting/seting device state, > > starting/stopping devices, tracking dirty data, report migration > > capabilities... all those works need be in kernel. > > I think Alex Graf proved they don't necessarily need to be done in kernel back > in 2015: https://www.youtube.com/watch?v=4RFsSgzuFso > He was able to achieve i40e VF live migration by only hacking QEMU. In this > series you're allowing a vendor driver to interpose itself between the user > (QEMU) and vfio-pci such that we switch to the vendor code during migration. > Why can't that interpose layer be in QEMU rather than the kernel? It seems > that it only must be in the kernel if we need to provide migration state via > backdoor, perhaps like going through the PF. So what access to the i40e VF > device is not provided to the user through vfio-pci that is necessary to > implement migration of this device? The tasks listed above are mostly > standard device driver activities and clearly vfio-pci allows userspace device > drivers. > > > do you think it's better to create numerous vendor quirks in vfio-pci? > > In QEMU, perhaps. Alternatively, let's look at exactly what access is not > provided through vfio-pci that's necessary for this and decide if we want to > enable that access or if cracking vfio-pci wide open for vendor drivers to pick > and choose when and how to use it is really the right answer. > > > as to this series, though patch 9/10 currently only demos reporting a > > migration region, it actually shows the capability iof vendor driver > > to customize device regions. e.g. in patch 10/10, it customizes the > > BAR0 to be read/write. and though we abandoned the REMAP BAR irq_type > > in patch > > 10/10 for migration purpose, I have to say this irq_type has its usage > > in other use cases, where synchronization is not a hard requirement > > and all it needs is a notification channel from kernel to use. this > > series just provides a possibility for vendors to customize device > > regions and irqs. > > I don't disagree that a device specific interrupt might be useful, but I would > object to implementing this one only as an artificial use case. > We can wait for a legitimate use case to implement that. > > > for interfaces exported in patch 3/10-5/10, they anyway need to be > > exported for writing mdev parent drivers that pass through devices at > > normal time to avoid duplication. and yes, your worry about > > Where are those parent drivers? What are their actual requirements? > > > identification of bug sources is reasonable. but if a device is > > binding to vfio-pci with a vendor module loaded, and there's a bug, > > they can do at least two ways to identify if it's a bug in vfio-pci itself. > > (1) prevent vendor modules from loading and see if the problem exists > > with pure vfio-pci. > > (2) do what's demoed in patch 8/10, i.e. do nothing but simply pass > > all operations to vfio-pci. > > The code split is still extremely ad-hoc, there's no API. An mdev driver isn't > even a sub-driver of vfio-pci like you're trying to accomplish here, there > would need to be a much more defined API when the base device isn't even a > vfio_pci_device. I don't see how this series would directly enable an mdev > use case. > > > so, do you think this series has its merit and we can continue > > improving it? > > I think this series is trying to push an artificial use case that is perhaps better > done in userspace. What is the actual interaction with the VF device that can > only be done in the host kernel for this example? Thanks, Hi Alex, As shared in KVM Forum last November(https://www.youtube.com/watch?v=aiCCUFXxVEA), we already have one PoC working internally. This series is part of that, if going well, we plan to support it in our future network, storage, security etc. device drivers. This series has two enhancements to support passthrough device live migration: general support for SR-IOV live migration and Software assisted dirty page tracking. We tried PoC for other solutions too, but this series seems to work the best balancing on feasibility, code duplication, performance etc. We are more focusing on enabling our latest E810 NIC product now, but we will check again how we could make it public earlier, as low quality i40e PoC or formal E810 driver, so you may see "the actual interaction" more clearly. Thanks, --Shaopeng > > Alex
On Wed, Jun 03, 2020 at 10:10:58PM -0600, Alex Williamson wrote: > On Wed, 3 Jun 2020 22:42:28 -0400 > Yan Zhao <yan.y.zhao@intel.com> wrote: > > > On Wed, Jun 03, 2020 at 05:04:52PM -0600, Alex Williamson wrote: > > > On Tue, 2 Jun 2020 21:40:58 -0400 > > > Yan Zhao <yan.y.zhao@intel.com> wrote: > > > > > > > On Tue, Jun 02, 2020 at 01:34:35PM -0600, Alex Williamson wrote: > > > > > I'm not at all happy with this. Why do we need to hide the migration > > > > > sparse mmap from the user until migration time? What if instead we > > > > > introduced a new VFIO_REGION_INFO_CAP_SPARSE_MMAP_SAVING capability > > > > > where the existing capability is the normal runtime sparse setup and > > > > > the user is required to use this new one prior to enabled device_state > > > > > with _SAVING. The vendor driver could then simply track mmap vmas to > > > > > the region and refuse to change device_state if there are outstanding > > > > > mmaps conflicting with the _SAVING sparse mmap layout. No new IRQs > > > > > required, no new irqfds, an incremental change to the protocol, > > > > > backwards compatible to the extent that a vendor driver requiring this > > > > > will automatically fail migration. > > > > > > > > > right. looks we need to use this approach to solve the problem. > > > > thanks for your guide. > > > > so I'll abandon the current remap irq way for dirty tracking during live > > > > migration. > > > > but anyway, it demos how to customize irq_types in vendor drivers. > > > > then, what do you think about patches 1-5? > > > > > > In broad strokes, I don't think we've found the right solution yet. I > > > really question whether it's supportable to parcel out vfio-pci like > > > this and I don't know how I'd support unraveling whether we have a bug > > > in vfio-pci, the vendor driver, or how the vendor driver is making use > > > of vfio-pci. > > > > > > Let me also ask, why does any of this need to be in the kernel? We > > > spend 5 patches slicing up vfio-pci so that we can register a vendor > > > driver and have that vendor driver call into vfio-pci as it sees fit. > > > We have two patches creating device specific interrupts and a BAR > > > remapping scheme that we've decided we don't need. That brings us to > > > the actual i40e vendor driver, where the first patch is simply making > > > the vendor driver work like vfio-pci already does, the second patch is > > > handling the migration region, and the third patch is implementing the > > > BAR remapping IRQ that we decided we don't need. It's difficult to > > > actually find the small bit of code that's required to support > > > migration outside of just dealing with the protocol we've defined to > > > expose this from the kernel. So why are we trying to do this in the > > > kernel? We have quirk support in QEMU, we can easily flip > > > MemoryRegions on and off, etc. What access to the device outside of > > > what vfio-pci provides to the user, and therefore QEMU, is necessary to > > > implement this migration support for i40e VFs? Is this just an > > > exercise in making use of the migration interface? Thanks, > > > > > hi Alex > > > > There was a description of intention of this series in RFC v1 > > (https://www.spinics.net/lists/kernel/msg3337337.html). > > sorry, I didn't include it in starting from RFC v2. > > > > " > > The reason why we don't choose the way of writing mdev parent driver is > > that > > I didn't mention an mdev approach, I'm asking what are we accomplishing > by doing this in the kernel at all versus exposing the device as normal > through vfio-pci and providing the migration support in QEMU. Are you > actually leveraging having some sort of access to the PF in supporting > migration of the VF? Is vfio-pci masking the device in a way that > prevents migrating the state from QEMU? > yes, communication to PF is required. VF state is managed by PF and is queried from PF when VF is stopped. migration support in QEMU seems only suitable to devices with dirty pages and device state available by reading/writing device MMIOs, which is not the case for most devices. > > (1) VFs are almost all the time directly passthroughed. Directly binding > > to vfio-pci can make most of the code shared/reused. If we write a > > vendor specific mdev parent driver, most of the code (like passthrough > > style of rw/mmap) still needs to be copied from vfio-pci driver, which is > > actually a duplicated and tedious work. > > (2) For features like dynamically trap/untrap pci bars, if they are in > > vfio-pci, they can be available to most people without repeated code > > copying and re-testing. > > (3) with a 1:1 mdev driver which passes through VFs most of the time, people > > have to decide whether to bind VFs to vfio-pci or mdev parent driver before > > it runs into a real migration need. However, if vfio-pci is bound > > initially, they have no chance to do live migration when there's a need > > later. > > " > > particularly, there're some devices (like NVMe) they purely reply on > > vfio-pci to do device pass-through and they have no standalone parent driver > > to do mdev way. > > > > I think live migration is a general requirement for most devices and to > > interact with the migration interface requires vendor drivers to do > > device specific tasks like geting/seting device state, starting/stopping > > devices, tracking dirty data, report migration capabilities... all those > > works need be in kernel. > > I think Alex Graf proved they don't necessarily need to be done in > kernel back in 2015: https://www.youtube.com/watch?v=4RFsSgzuFso > He was able to achieve i40e VF live migration by only hacking QEMU. In I checked the qemu code. https://github.com/agraf/qemu/tree/vfio-i40vf. a new vfio-i40e device type is registered as a child type of vfio-pci, as well as its exclusive savevm handlers, which are not compatible to Kirti's general VFIO live migration framework. > this series you're allowing a vendor driver to interpose itself between > the user (QEMU) and vfio-pci such that we switch to the vendor code > during migration. Why can't that interpose layer be in QEMU rather > than the kernel? It seems that it only must be in the kernel if we > need to provide migration state via backdoor, perhaps like going > through the PF. So what access to the i40e VF device is not provided to > the user through vfio-pci that is necessary to implement migration of > this device? The tasks listed above are mostly standard device driver > activities and clearly vfio-pci allows userspace device drivers. > tasks like interacting with PF driver, preparing resources and tracking dirty pages in device internal memory, detecting of whether dirty page is able to be tracked by hardware and reporting migration capabilities, exposing hardware dirty bitmap buffer... all those are hard to be done in QEMU. maintaining migration code in kernel can also allow vendors to re-use common code for devices across generations. e.g. for i40e, in some generations, software dirty page track is used, some generations hardware dirty track is enabled and some other generations leveraging IOMMU A/D bit is feasible. is QEMU quirks allowing such flexibility as in kernel? besides, migration version string as a sysfs attribute requires a vendor driver to generate. > > do you think it's better to create numerous vendor quirks in vfio-pci? > > In QEMU, perhaps. Alternatively, let's look at exactly what access is > not provided through vfio-pci that's necessary for this and decide if > we want to enable that access or if cracking vfio-pci wide open for > vendor drivers to pick and choose when and how to use it is really the > right answer. > I think the position of vendor modules is just like vfio_pci_igd.c under vfio-pci. the difference is that the vendor modules are able to be dynamically loaded outside of vfio-pci. > > as to this series, though patch 9/10 currently only demos reporting a > > migration region, it actually shows the capability iof vendor driver to > > customize device regions. e.g. in patch 10/10, it customizes the BAR0 to > > be read/write. and though we abandoned the REMAP BAR irq_type in patch > > 10/10 for migration purpose, I have to say this irq_type has its usage > > in other use cases, where synchronization is not a hard requirement and > > all it needs is a notification channel from kernel to use. this series > > just provides a possibility for vendors to customize device regions and > > irqs. > > I don't disagree that a device specific interrupt might be useful, but > I would object to implementing this one only as an artificial use case. > We can wait for a legitimate use case to implement that. > ok. sure. > > for interfaces exported in patch 3/10-5/10, they anyway need to be > > exported for writing mdev parent drivers that pass through devices at > > normal time to avoid duplication. and yes, your worry about > > Where are those parent drivers? What are their actual requirements? > if this way of registering vendor ops to vfio-pci is not permitted, vendors have to resort to writing its mdev parent drivers for VFs. Those parent drivers need to pass through VFs at normal time, doing exactly what vfio-pci does and only doing what vendor ops does during migration. if vfio-pci could export common code to those parent drivers, lots of duplicated code can be avoided. > > identification of bug sources is reasonable. but if a device is binding > > to vfio-pci with a vendor module loaded, and there's a bug, they can do at > > least two ways to identify if it's a bug in vfio-pci itself. > > (1) prevent vendor modules from loading and see if the problem exists > > with pure vfio-pci. > > (2) do what's demoed in patch 8/10, i.e. do nothing but simply pass all > > operations to vfio-pci. > > The code split is still extremely ad-hoc, there's no API. An mdev > driver isn't even a sub-driver of vfio-pci like you're trying to > accomplish here, there would need to be a much more defined API when > the base device isn't even a vfio_pci_device. I don't see how this > series would directly enable an mdev use case. > similar to Yi's series https://patchwork.kernel.org/patch/11320841/. we can parcel the vdev creation code in vfio_pci_probe() to allow calling from mdev parent probe routine. (of course, also need to parcel code to free vdev) e.g. void *vfio_pci_alloc_vdev(struct pci_dev *pdev, const struct pci_device_id *id) { struct vfio_pci_device *vdev; vdev = kzalloc(sizeof(*vdev), GFP_KERNEL); if (!vdev) { ret = -ENOMEM; goto out_group_put; } vdev->pdev = pdev; vdev->irq_type = VFIO_PCI_NUM_IRQS; mutex_init(&vdev->igate); spin_lock_init(&vdev->irqlock); mutex_init(&vdev->ioeventfds_lock); INIT_LIST_HEAD(&vdev->ioeventfds_list); ... vfio_pci_probe_power_state(vdev); if (!disable_idle_d3) { vfio_pci_set_power_state(vdev, PCI_D0); vfio_pci_set_power_state(vdev, PCI_D3hot); } return vdev; } static int vfio_mdev_pci_driver_probe(struct pci_dev *pdev, const struct pci_device_id *id)) { void *vdev = vfio_pci_alloc_vdev(pdev, id); //save the vdev pointer } then all the exported interfaces from this series can also benefit the mdev use case. > > so, do you think this series has its merit and we can continue improving > > it? > > I think this series is trying to push an artificial use case that is > perhaps better done in userspace. What is the actual interaction with > the VF device that can only be done in the host kernel for this > example? Thanks, > yes, as with shaopeng's reply, looking forward to their early release of code. Besides, there're other real use cases as well, like NVMe, QAT. we can show the interactions for those devices as well. Thanks Yan
On Thu, 4 Jun 2020 22:02:31 -0400 Yan Zhao <yan.y.zhao@intel.com> wrote: > On Wed, Jun 03, 2020 at 10:10:58PM -0600, Alex Williamson wrote: > > On Wed, 3 Jun 2020 22:42:28 -0400 > > Yan Zhao <yan.y.zhao@intel.com> wrote: > > > > > On Wed, Jun 03, 2020 at 05:04:52PM -0600, Alex Williamson wrote: > > > > On Tue, 2 Jun 2020 21:40:58 -0400 > > > > Yan Zhao <yan.y.zhao@intel.com> wrote: > > > > > > > > > On Tue, Jun 02, 2020 at 01:34:35PM -0600, Alex Williamson wrote: > > > > > > I'm not at all happy with this. Why do we need to hide the migration > > > > > > sparse mmap from the user until migration time? What if instead we > > > > > > introduced a new VFIO_REGION_INFO_CAP_SPARSE_MMAP_SAVING capability > > > > > > where the existing capability is the normal runtime sparse setup and > > > > > > the user is required to use this new one prior to enabled device_state > > > > > > with _SAVING. The vendor driver could then simply track mmap vmas to > > > > > > the region and refuse to change device_state if there are outstanding > > > > > > mmaps conflicting with the _SAVING sparse mmap layout. No new IRQs > > > > > > required, no new irqfds, an incremental change to the protocol, > > > > > > backwards compatible to the extent that a vendor driver requiring this > > > > > > will automatically fail migration. > > > > > > > > > > > right. looks we need to use this approach to solve the problem. > > > > > thanks for your guide. > > > > > so I'll abandon the current remap irq way for dirty tracking during live > > > > > migration. > > > > > but anyway, it demos how to customize irq_types in vendor drivers. > > > > > then, what do you think about patches 1-5? > > > > > > > > In broad strokes, I don't think we've found the right solution yet. I > > > > really question whether it's supportable to parcel out vfio-pci like > > > > this and I don't know how I'd support unraveling whether we have a bug > > > > in vfio-pci, the vendor driver, or how the vendor driver is making use > > > > of vfio-pci. > > > > > > > > Let me also ask, why does any of this need to be in the kernel? We > > > > spend 5 patches slicing up vfio-pci so that we can register a vendor > > > > driver and have that vendor driver call into vfio-pci as it sees fit. > > > > We have two patches creating device specific interrupts and a BAR > > > > remapping scheme that we've decided we don't need. That brings us to > > > > the actual i40e vendor driver, where the first patch is simply making > > > > the vendor driver work like vfio-pci already does, the second patch is > > > > handling the migration region, and the third patch is implementing the > > > > BAR remapping IRQ that we decided we don't need. It's difficult to > > > > actually find the small bit of code that's required to support > > > > migration outside of just dealing with the protocol we've defined to > > > > expose this from the kernel. So why are we trying to do this in the > > > > kernel? We have quirk support in QEMU, we can easily flip > > > > MemoryRegions on and off, etc. What access to the device outside of > > > > what vfio-pci provides to the user, and therefore QEMU, is necessary to > > > > implement this migration support for i40e VFs? Is this just an > > > > exercise in making use of the migration interface? Thanks, > > > > > > > hi Alex > > > > > > There was a description of intention of this series in RFC v1 > > > (https://www.spinics.net/lists/kernel/msg3337337.html). > > > sorry, I didn't include it in starting from RFC v2. > > > > > > " > > > The reason why we don't choose the way of writing mdev parent driver is > > > that > > > > I didn't mention an mdev approach, I'm asking what are we accomplishing > > by doing this in the kernel at all versus exposing the device as normal > > through vfio-pci and providing the migration support in QEMU. Are you > > actually leveraging having some sort of access to the PF in supporting > > migration of the VF? Is vfio-pci masking the device in a way that > > prevents migrating the state from QEMU? > > > yes, communication to PF is required. VF state is managed by PF and is > queried from PF when VF is stopped. > > migration support in QEMU seems only suitable to devices with dirty > pages and device state available by reading/writing device MMIOs, which > is not the case for most devices. Post code for such a device. > > > (1) VFs are almost all the time directly passthroughed. Directly binding > > > to vfio-pci can make most of the code shared/reused. If we write a > > > vendor specific mdev parent driver, most of the code (like passthrough > > > style of rw/mmap) still needs to be copied from vfio-pci driver, which is > > > actually a duplicated and tedious work. > > > (2) For features like dynamically trap/untrap pci bars, if they are in > > > vfio-pci, they can be available to most people without repeated code > > > copying and re-testing. > > > (3) with a 1:1 mdev driver which passes through VFs most of the time, people > > > have to decide whether to bind VFs to vfio-pci or mdev parent driver before > > > it runs into a real migration need. However, if vfio-pci is bound > > > initially, they have no chance to do live migration when there's a need > > > later. > > > " > > > particularly, there're some devices (like NVMe) they purely reply on > > > vfio-pci to do device pass-through and they have no standalone parent driver > > > to do mdev way. > > > > > > I think live migration is a general requirement for most devices and to > > > interact with the migration interface requires vendor drivers to do > > > device specific tasks like geting/seting device state, starting/stopping > > > devices, tracking dirty data, report migration capabilities... all those > > > works need be in kernel. > > > > I think Alex Graf proved they don't necessarily need to be done in > > kernel back in 2015: https://www.youtube.com/watch?v=4RFsSgzuFso > > He was able to achieve i40e VF live migration by only hacking QEMU. In > I checked the qemu code. https://github.com/agraf/qemu/tree/vfio-i40vf. > a new vfio-i40e device type is registered as a child type of vfio-pci, as well > as its exclusive savevm handlers, which are not compatible to Kirti's > general VFIO live migration framework. Obviously, saved state is managed within QEMU. We've already seen pushback to using mdev as a means to implement emulation in the kernel. A vfio migration interface is not an excuse to move everything to in-kernel drivers just to make use of it. IF migration support can be achieved for a device within QEMU, then that's the correct place to put it. > > this series you're allowing a vendor driver to interpose itself between > > the user (QEMU) and vfio-pci such that we switch to the vendor code > > during migration. Why can't that interpose layer be in QEMU rather > > than the kernel? It seems that it only must be in the kernel if we > > need to provide migration state via backdoor, perhaps like going > > through the PF. So what access to the i40e VF device is not provided to > > the user through vfio-pci that is necessary to implement migration of > > this device? The tasks listed above are mostly standard device driver > > activities and clearly vfio-pci allows userspace device drivers. > > > tasks like interacting with PF driver, preparing resources and tracking dirty > pages in device internal memory, detecting of whether dirty page is able > to be tracked by hardware and reporting migration capabilities, exposing > hardware dirty bitmap buffer... all those are hard to be done in QEMU. Something being easier to do in the kernel does not automatically make the kernel the right place to do it. The kernel manages resources, so if access through a PF, where the PF is a shared resources, is necessary then those aspects might justify a kernel interface. We should also consider that the kernel presents a much richer attack vector. QEMU is already confined and a single set of ioctls through vfio-pci is much easier to audit for security than allowing every vendor driver to re-implement their own version. Attempting to re-use vfio-pci code is an effort to contain that risk, but I think it ends up turning into a Frankenstein's monster of intermingled dependencies without a defined API. > maintaining migration code in kernel can also allow vendors to re-use common > code for devices across generations. e.g. for i40e, in some generations, > software dirty page track is used, some generations hardware dirty > track is enabled and some other generations leveraging IOMMU A/D bit is > feasible. is QEMU quirks allowing such flexibility as in kernel? These arguments all sound like excuses, ie. hiding migration code in the kernel for convenience. Obviously we can re-use code between devices in QEMU. What I think I see happening here is using the vfio migration interface as an excuse to push more code into the kernel, and the vfio-pci vendor extensions are a mechanism to masquerade behind a known driver and avoid defining interfaces for specific features. > besides, migration version string as a sysfs attribute requires a vendor > driver to generate. We don't have that yet anyway, and it's also a false dependency. The external version string is required _because_ the migration backend is not provided _within_ QEMU. If QEMU manages generating the migration data for a device, we don't need an external version string. > > > do you think it's better to create numerous vendor quirks in vfio-pci? > > > > In QEMU, perhaps. Alternatively, let's look at exactly what access is > > not provided through vfio-pci that's necessary for this and decide if > > we want to enable that access or if cracking vfio-pci wide open for > > vendor drivers to pick and choose when and how to use it is really the > > right answer. > > > I think the position of vendor modules is just like vfio_pci_igd.c under > vfio-pci. the difference is that the vendor modules are able to be > dynamically loaded outside of vfio-pci. No, this is entirely false. vfio_pci_igd provides two supplemental, read-only regions necessary to satisfy some of the dependencies of the guest driver. It does not attempt to take over the device. > > > as to this series, though patch 9/10 currently only demos reporting a > > > migration region, it actually shows the capability iof vendor driver to > > > customize device regions. e.g. in patch 10/10, it customizes the BAR0 to > > > be read/write. and though we abandoned the REMAP BAR irq_type in patch > > > 10/10 for migration purpose, I have to say this irq_type has its usage > > > in other use cases, where synchronization is not a hard requirement and > > > all it needs is a notification channel from kernel to use. this series > > > just provides a possibility for vendors to customize device regions and > > > irqs. > > > > I don't disagree that a device specific interrupt might be useful, but > > I would object to implementing this one only as an artificial use case. > > We can wait for a legitimate use case to implement that. > > > ok. sure. > > > > for interfaces exported in patch 3/10-5/10, they anyway need to be > > > exported for writing mdev parent drivers that pass through devices at > > > normal time to avoid duplication. and yes, your worry about > > > > Where are those parent drivers? What are their actual requirements? > > > if this way of registering vendor ops to vfio-pci is not permitted, > vendors have to resort to writing its mdev parent drivers for VFs. Those > parent drivers need to pass through VFs at normal time, doing exactly what > vfio-pci does and only doing what vendor ops does during migration. > > if vfio-pci could export common code to those parent drivers, lots of > duplicated code can be avoided. There are two sides to this argument though. We could also argue that mdev has already made it too easy to implement device emulation in the kernel, the barrier is that such emulation is more transparent because it does require a fair bit of code duplication from vfio-pci. If we make it easier to simply re-use vfio-pci for much of this, and even take it a step further by allowing vendor drivers to masquerade behind vfio-pci, then we're creating an environment where vendors don't need to work with QEMU to get their device emulation accepted. They can write their own vendor drivers, which are now simplified and sanctioned by exported functions in vfio-pci. They can do this easily and open up massive attack vectors, hiding behind vfio-pci. I know that I was advocating avoiding user driver confusion, ie. does the user bind a device to vfio-pci, i40e_vf_vfio, etc, but maybe that's the barrier we need such that a user can make an informed decision about what they're actually using. If a vendor then wants to implement a feature in vfio-pci, we'll need to architect an interface for it rather than letting them pick and choose which pieces of vfio-pci to override. > > > identification of bug sources is reasonable. but if a device is binding > > > to vfio-pci with a vendor module loaded, and there's a bug, they can do at > > > least two ways to identify if it's a bug in vfio-pci itself. > > > (1) prevent vendor modules from loading and see if the problem exists > > > with pure vfio-pci. > > > (2) do what's demoed in patch 8/10, i.e. do nothing but simply pass all > > > operations to vfio-pci. > > > > The code split is still extremely ad-hoc, there's no API. An mdev > > driver isn't even a sub-driver of vfio-pci like you're trying to > > accomplish here, there would need to be a much more defined API when > > the base device isn't even a vfio_pci_device. I don't see how this > > series would directly enable an mdev use case. > > > similar to Yi's series https://patchwork.kernel.org/patch/11320841/. > we can parcel the vdev creation code in vfio_pci_probe() to allow calling from > mdev parent probe routine. (of course, also need to parcel code to free vdev) > e.g. > > void *vfio_pci_alloc_vdev(struct pci_dev *pdev, const struct pci_device_id *id) > { > struct vfio_pci_device *vdev; > vdev = kzalloc(sizeof(*vdev), GFP_KERNEL); > if (!vdev) { > ret = -ENOMEM; > goto out_group_put; > } > > vdev->pdev = pdev; > vdev->irq_type = VFIO_PCI_NUM_IRQS; > mutex_init(&vdev->igate); > spin_lock_init(&vdev->irqlock); > mutex_init(&vdev->ioeventfds_lock); > INIT_LIST_HEAD(&vdev->ioeventfds_list); > ... > vfio_pci_probe_power_state(vdev); > > if (!disable_idle_d3) { > vfio_pci_set_power_state(vdev, PCI_D0); > vfio_pci_set_power_state(vdev, PCI_D3hot); > } > return vdev; > } > > static int vfio_mdev_pci_driver_probe(struct pci_dev *pdev, const struct pci_device_id *id)) > { > > void *vdev = vfio_pci_alloc_vdev(pdev, id); > > //save the vdev pointer > > } > then all the exported interfaces from this series can also benefit the > mdev use case. You need to convince me that we're not just doing this for the sake of re-using a migration interface. We do need vendor specific drivers to support migration, but implementing those vendor specific drivers in the kernel just because we have that interface is the wrong answer. If we can implement that device specific migration support in QEMU and limit the attack surface from the hypervisor or guest into the host kernel, that's a better answer. As I've noted above, I'm afraid all of these attempts to parcel out vfio-pci are only going to serve to proliferate vendor modules that have limited community review, expand the attack surface, and potentially harm the vfio ecosystem overall through bad actors and reduced autonomy. Thanks, Alex
On Fri, 5 Jun 2020 00:26:10 +0000 "He, Shaopeng" <shaopeng.he@intel.com> wrote: > > From: Alex Williamson <alex.williamson@redhat.com> > > Sent: Thursday, June 4, 2020 12:11 PM > > > > On Wed, 3 Jun 2020 22:42:28 -0400 > > Yan Zhao <yan.y.zhao@intel.com> wrote: > > > > > On Wed, Jun 03, 2020 at 05:04:52PM -0600, Alex Williamson wrote: > > > > On Tue, 2 Jun 2020 21:40:58 -0400 > > > > Yan Zhao <yan.y.zhao@intel.com> wrote: > > > > > > > > > On Tue, Jun 02, 2020 at 01:34:35PM -0600, Alex Williamson wrote: > > > > > > I'm not at all happy with this. Why do we need to hide the > > > > > > migration sparse mmap from the user until migration time? What > > > > > > if instead we introduced a new > > > > > > VFIO_REGION_INFO_CAP_SPARSE_MMAP_SAVING capability where > > the > > > > > > existing capability is the normal runtime sparse setup and the > > > > > > user is required to use this new one prior to enabled > > > > > > device_state with _SAVING. The vendor driver could then simply > > > > > > track mmap vmas to the region and refuse to change device_state > > > > > > if there are outstanding mmaps conflicting with the _SAVING > > > > > > sparse mmap layout. No new IRQs required, no new irqfds, an > > > > > > incremental change to the protocol, backwards compatible to the > > extent that a vendor driver requiring this will automatically fail migration. > > > > > > > > > > > right. looks we need to use this approach to solve the problem. > > > > > thanks for your guide. > > > > > so I'll abandon the current remap irq way for dirty tracking > > > > > during live migration. > > > > > but anyway, it demos how to customize irq_types in vendor drivers. > > > > > then, what do you think about patches 1-5? > > > > > > > > In broad strokes, I don't think we've found the right solution yet. > > > > I really question whether it's supportable to parcel out vfio-pci > > > > like this and I don't know how I'd support unraveling whether we > > > > have a bug in vfio-pci, the vendor driver, or how the vendor driver > > > > is making use of vfio-pci. > > > > > > > > Let me also ask, why does any of this need to be in the kernel? We > > > > spend 5 patches slicing up vfio-pci so that we can register a vendor > > > > driver and have that vendor driver call into vfio-pci as it sees fit. > > > > We have two patches creating device specific interrupts and a BAR > > > > remapping scheme that we've decided we don't need. That brings us > > > > to the actual i40e vendor driver, where the first patch is simply > > > > making the vendor driver work like vfio-pci already does, the second > > > > patch is handling the migration region, and the third patch is > > > > implementing the BAR remapping IRQ that we decided we don't need. > > > > It's difficult to actually find the small bit of code that's > > > > required to support migration outside of just dealing with the > > > > protocol we've defined to expose this from the kernel. So why are > > > > we trying to do this in the kernel? We have quirk support in QEMU, > > > > we can easily flip MemoryRegions on and off, etc. What access to > > > > the device outside of what vfio-pci provides to the user, and > > > > therefore QEMU, is necessary to implement this migration support for > > > > i40e VFs? Is this just an exercise in making use of the migration > > > > interface? Thanks, > > > > > > > hi Alex > > > > > > There was a description of intention of this series in RFC v1 > > > (https://www.spinics.net/lists/kernel/msg3337337.html). > > > sorry, I didn't include it in starting from RFC v2. > > > > > > " > > > The reason why we don't choose the way of writing mdev parent driver > > > is that > > > > I didn't mention an mdev approach, I'm asking what are we accomplishing by > > doing this in the kernel at all versus exposing the device as normal through > > vfio-pci and providing the migration support in QEMU. Are you actually > > leveraging having some sort of access to the PF in supporting migration of the > > VF? Is vfio-pci masking the device in a way that prevents migrating the state > > from QEMU? > > > > > (1) VFs are almost all the time directly passthroughed. Directly > > > binding to vfio-pci can make most of the code shared/reused. If we > > > write a vendor specific mdev parent driver, most of the code (like > > > passthrough style of rw/mmap) still needs to be copied from vfio-pci > > > driver, which is actually a duplicated and tedious work. > > > (2) For features like dynamically trap/untrap pci bars, if they are in > > > vfio-pci, they can be available to most people without repeated code > > > copying and re-testing. > > > (3) with a 1:1 mdev driver which passes through VFs most of the time, > > > people have to decide whether to bind VFs to vfio-pci or mdev parent > > > driver before it runs into a real migration need. However, if vfio-pci > > > is bound initially, they have no chance to do live migration when > > > there's a need later. > > > " > > > particularly, there're some devices (like NVMe) they purely reply on > > > vfio-pci to do device pass-through and they have no standalone parent > > > driver to do mdev way. > > > > > > I think live migration is a general requirement for most devices and > > > to interact with the migration interface requires vendor drivers to do > > > device specific tasks like geting/seting device state, > > > starting/stopping devices, tracking dirty data, report migration > > > capabilities... all those works need be in kernel. > > > > I think Alex Graf proved they don't necessarily need to be done in kernel back > > in 2015: https://www.youtube.com/watch?v=4RFsSgzuFso > > He was able to achieve i40e VF live migration by only hacking QEMU. In this > > series you're allowing a vendor driver to interpose itself between the user > > (QEMU) and vfio-pci such that we switch to the vendor code during migration. > > Why can't that interpose layer be in QEMU rather than the kernel? It seems > > that it only must be in the kernel if we need to provide migration state via > > backdoor, perhaps like going through the PF. So what access to the i40e VF > > device is not provided to the user through vfio-pci that is necessary to > > implement migration of this device? The tasks listed above are mostly > > standard device driver activities and clearly vfio-pci allows userspace device > > drivers. > > > > > do you think it's better to create numerous vendor quirks in vfio-pci? > > > > In QEMU, perhaps. Alternatively, let's look at exactly what access is not > > provided through vfio-pci that's necessary for this and decide if we want to > > enable that access or if cracking vfio-pci wide open for vendor drivers to pick > > and choose when and how to use it is really the right answer. > > > > > as to this series, though patch 9/10 currently only demos reporting a > > > migration region, it actually shows the capability iof vendor driver > > > to customize device regions. e.g. in patch 10/10, it customizes the > > > BAR0 to be read/write. and though we abandoned the REMAP BAR irq_type > > > in patch > > > 10/10 for migration purpose, I have to say this irq_type has its usage > > > in other use cases, where synchronization is not a hard requirement > > > and all it needs is a notification channel from kernel to use. this > > > series just provides a possibility for vendors to customize device > > > regions and irqs. > > > > I don't disagree that a device specific interrupt might be useful, but I would > > object to implementing this one only as an artificial use case. > > We can wait for a legitimate use case to implement that. > > > > > for interfaces exported in patch 3/10-5/10, they anyway need to be > > > exported for writing mdev parent drivers that pass through devices at > > > normal time to avoid duplication. and yes, your worry about > > > > Where are those parent drivers? What are their actual requirements? > > > > > identification of bug sources is reasonable. but if a device is > > > binding to vfio-pci with a vendor module loaded, and there's a bug, > > > they can do at least two ways to identify if it's a bug in vfio-pci itself. > > > (1) prevent vendor modules from loading and see if the problem exists > > > with pure vfio-pci. > > > (2) do what's demoed in patch 8/10, i.e. do nothing but simply pass > > > all operations to vfio-pci. > > > > The code split is still extremely ad-hoc, there's no API. An mdev driver isn't > > even a sub-driver of vfio-pci like you're trying to accomplish here, there > > would need to be a much more defined API when the base device isn't even a > > vfio_pci_device. I don't see how this series would directly enable an mdev > > use case. > > > > > so, do you think this series has its merit and we can continue > > > improving it? > > > > I think this series is trying to push an artificial use case that is perhaps better > > done in userspace. What is the actual interaction with the VF device that can > > only be done in the host kernel for this example? Thanks, > > Hi Alex, > > As shared in KVM Forum last November(https://www.youtube.com/watch?v=aiCCUFXxVEA), > we already have one PoC working internally. This series is part of that, if going well, > we plan to support it in our future network, storage, security etc. device drivers. > > This series has two enhancements to support passthrough device live migration: > general support for SR-IOV live migration and Software assisted dirty page tracking. > We tried PoC for other solutions too, but this series seems to work the best > balancing on feasibility, code duplication, performance etc. > > We are more focusing on enabling our latest E810 NIC product now, but we > will check again how we could make it public earlier, as low quality i40e PoC > or formal E810 driver, so you may see "the actual interaction" more clearly. "General support for SR-IOV live migration" is not a thing, there's always device specific code. "Software assisted dirty page tracking" implies to me trapping and emulating device accesses in order to learn about DMA targets, which is something that we can also do in QEMU. In your list of "balancing on feasibility, code duplication, performance, etc", an explicit mention of security is strikingly lacking. The first two are rather obvious, it's more feasible to implement features like migration for any device once the code necessary for such a feature is buried in a vendor driver surrounded by a small development community. The actual protocol of the device state is hidden behind an interface that's opaque to userspace and requires trust that the vendor has implemented a robust scheme and ultimately relying on the vendor for ongoing support. Reducing code duplication by exporting the guts of vfio-pci makes that even easier. Vendors drivers get to ad-hoc pick and choose how and when they interact with vfio-pci, leaving vfio-pci with objects and device state it can't really trust. The performance claim is harder to justify as the path to trapping a region in the kernel vendor driver necessarily passes through trapping that same region in QEMU. Maintaining a dirty bitmap in the vfio IOMMU backend and later retrieving it via the dirty page tracking actually sounds like more overhead than setting dirty bits within QEMU. But then there's the entire security aspect where the same thing that I think makes this more feasible for the vendor driver opens the door for a much larger attack surface from the user (ie. we're intentionally choosing to open the door to vendor drivers running privileged inside the host kernel, reviewed and supported by smaller communities). Additionally, by masquerading these vendor drivers behind vfio-pci, we simplify usage for the user, but also prevent them from making an informed decision of the security risk of binding a device to "vfio-pci". Is it really vfio-pci, or is it i40e_vf_migration, which may or may not share common code with vfio-pci? VFIO already supports a plugin bus driver architecture, vendors can already supply their own, but users/admin must choose to use it. I originally thought that's something we should abstract for the user, but I'm beginning to see that might actually be the one remaining leverage point we have to architect secure interfaces rather than turning vfio into a playground of insecure and abandoned vendor drivers. VFIO migration support is trying to provide a solution for device specific state and dirty page tracking, but I don't think the desire to make use of this migration interface in and of itself as justification for an in-kernel vendor driver. We assume with mdevs that we're exposing a portion of a device to the user and the mediation of that portion of the device contains state and awareness of page dirtying that we can extract for migration. Implementing that state awareness and dirty page tracking in the host kernel for the sole purpose of being able to extract it via the migration interface seems like a non-goal and unwise security choice. Thanks, Alex
On Fri, Jun 05, 2020 at 10:13:01AM -0600, Alex Williamson wrote: > On Thu, 4 Jun 2020 22:02:31 -0400 > Yan Zhao <yan.y.zhao@intel.com> wrote: > > > On Wed, Jun 03, 2020 at 10:10:58PM -0600, Alex Williamson wrote: > > > On Wed, 3 Jun 2020 22:42:28 -0400 > > > Yan Zhao <yan.y.zhao@intel.com> wrote: > > > > > > > On Wed, Jun 03, 2020 at 05:04:52PM -0600, Alex Williamson wrote: > > > > > On Tue, 2 Jun 2020 21:40:58 -0400 > > > > > Yan Zhao <yan.y.zhao@intel.com> wrote: > > > > > > > > > > > On Tue, Jun 02, 2020 at 01:34:35PM -0600, Alex Williamson wrote: > > > > > > > I'm not at all happy with this. Why do we need to hide the migration > > > > > > > sparse mmap from the user until migration time? What if instead we > > > > > > > introduced a new VFIO_REGION_INFO_CAP_SPARSE_MMAP_SAVING capability > > > > > > > where the existing capability is the normal runtime sparse setup and > > > > > > > the user is required to use this new one prior to enabled device_state > > > > > > > with _SAVING. The vendor driver could then simply track mmap vmas to > > > > > > > the region and refuse to change device_state if there are outstanding > > > > > > > mmaps conflicting with the _SAVING sparse mmap layout. No new IRQs > > > > > > > required, no new irqfds, an incremental change to the protocol, > > > > > > > backwards compatible to the extent that a vendor driver requiring this > > > > > > > will automatically fail migration. > > > > > > > > > > > > > right. looks we need to use this approach to solve the problem. > > > > > > thanks for your guide. > > > > > > so I'll abandon the current remap irq way for dirty tracking during live > > > > > > migration. > > > > > > but anyway, it demos how to customize irq_types in vendor drivers. > > > > > > then, what do you think about patches 1-5? > > > > > > > > > > In broad strokes, I don't think we've found the right solution yet. I > > > > > really question whether it's supportable to parcel out vfio-pci like > > > > > this and I don't know how I'd support unraveling whether we have a bug > > > > > in vfio-pci, the vendor driver, or how the vendor driver is making use > > > > > of vfio-pci. > > > > > > > > > > Let me also ask, why does any of this need to be in the kernel? We > > > > > spend 5 patches slicing up vfio-pci so that we can register a vendor > > > > > driver and have that vendor driver call into vfio-pci as it sees fit. > > > > > We have two patches creating device specific interrupts and a BAR > > > > > remapping scheme that we've decided we don't need. That brings us to > > > > > the actual i40e vendor driver, where the first patch is simply making > > > > > the vendor driver work like vfio-pci already does, the second patch is > > > > > handling the migration region, and the third patch is implementing the > > > > > BAR remapping IRQ that we decided we don't need. It's difficult to > > > > > actually find the small bit of code that's required to support > > > > > migration outside of just dealing with the protocol we've defined to > > > > > expose this from the kernel. So why are we trying to do this in the > > > > > kernel? We have quirk support in QEMU, we can easily flip > > > > > MemoryRegions on and off, etc. What access to the device outside of > > > > > what vfio-pci provides to the user, and therefore QEMU, is necessary to > > > > > implement this migration support for i40e VFs? Is this just an > > > > > exercise in making use of the migration interface? Thanks, > > > > > > > > > hi Alex > > > > > > > > There was a description of intention of this series in RFC v1 > > > > (https://www.spinics.net/lists/kernel/msg3337337.html). > > > > sorry, I didn't include it in starting from RFC v2. > > > > > > > > " > > > > The reason why we don't choose the way of writing mdev parent driver is > > > > that > > > > > > I didn't mention an mdev approach, I'm asking what are we accomplishing > > > by doing this in the kernel at all versus exposing the device as normal > > > through vfio-pci and providing the migration support in QEMU. Are you > > > actually leveraging having some sort of access to the PF in supporting > > > migration of the VF? Is vfio-pci masking the device in a way that > > > prevents migrating the state from QEMU? > > > > > yes, communication to PF is required. VF state is managed by PF and is > > queried from PF when VF is stopped. > > > > migration support in QEMU seems only suitable to devices with dirty > > pages and device state available by reading/writing device MMIOs, which > > is not the case for most devices. > > Post code for such a device. > hi Alex, There's an example in i40e vf. virtual channel related resources are in guest memory. dirty page tracking requires the info stored in those guest memory. there're two ways to get the resources addresses: (1) always trap VF registers related. as in Alex Graf's qemu code. starting from beginning, it tracks rw of Admin Queue Configuration registers. Then in the write handler vfio_i40evf_aq_mmio_mem_region_write(), guest commands are processed to record the guest dma addresses of the virtual channel related resources. e.g. vdev->vsi_config is read from the guest dma addr contained in command I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES. vfio_i40evf_initfn() { ... memory_region_init_io(&vdev->aq_mmio_mem, OBJECT(dev), &vfio_i40evf_aq_mmio_mem_region_ops, vdev, "i40evf AQ config", I40E_VFGEN_RSTAT - I40E_VF_ARQBAH1); ... } vfio_i40evf_aq_mmio_mem_region_write() { ... switch (addr) { case I40E_VF_ARQBAH1: case I40E_VF_ARQBAL1: case I40E_VF_ARQH1: case I40E_VF_ARQLEN1: case I40E_VF_ARQT1: case I40E_VF_ATQBAH1: case I40E_VF_ATQBAL1: case I40E_VF_ATQH1: case I40E_VF_ATQT1: case I40E_VF_ATQLEN1: vfio_i40evf_vw32(vdev, addr, data); vfio_i40e_aq_update(vdev); ==> update & process atq commands break; default: vfio_i40evf_w32(vdev, addr, data); break; } } vfio_i40e_aq_update(vdev) |->vfio_i40e_atq_process_one(vdev, vfio_i40evf_vr32(vdev, I40E_VF_ATQH1) |-> hwaddr addr = vfio_i40e_get_atqba(vdev) + (index * sizeof(desc)); | pci_dma_read(pdev, addr, &desc, sizeof(desc));//read guest's command | vfio_i40e_record_atq_cmd(vdev, pdev, &desc) vfio_i40e_record_atq_cmd(...I40eAdminQueueDescriptor *desc) { data_addr = desc->params.external.addr_high; ... switch (desc->cookie_high) { ... case I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES: pci_dma_read(pdev, data_addr, &vdev->vsi_config, MIN(desc->datalen, sizeof(vdev->vsi_config))); ... } ... } (2) pass through all guest MMIO accesses and only do MMIO trap when migration is about to start. This is the way we're using in the host vfio-pci vendor driver (or mdev parent driver) of i40e vf device (sorry for no public code available still). when migration is about to start, it's already too late to get the guest dma address for those virtual channel related resources merely by MMIO trapping, so we have to ask for them from PF. <...> > > > > for interfaces exported in patch 3/10-5/10, they anyway need to be > > > > exported for writing mdev parent drivers that pass through devices at > > > > normal time to avoid duplication. and yes, your worry about > > > > > > Where are those parent drivers? What are their actual requirements? > > > > > if this way of registering vendor ops to vfio-pci is not permitted, > > vendors have to resort to writing its mdev parent drivers for VFs. Those > > parent drivers need to pass through VFs at normal time, doing exactly what > > vfio-pci does and only doing what vendor ops does during migration. > > > > if vfio-pci could export common code to those parent drivers, lots of > > duplicated code can be avoided. > > There are two sides to this argument though. We could also argue that > mdev has already made it too easy to implement device emulation in the > kernel, the barrier is that such emulation is more transparent because > it does require a fair bit of code duplication from vfio-pci. If we > make it easier to simply re-use vfio-pci for much of this, and even > take it a step further by allowing vendor drivers to masquerade behind > vfio-pci, then we're creating an environment where vendors don't need > to work with QEMU to get their device emulation accepted. They can > write their own vendor drivers, which are now simplified and sanctioned > by exported functions in vfio-pci. They can do this easily and open up > massive attack vectors, hiding behind vfio-pci. > your concern is reasonable. > I know that I was advocating avoiding user driver confusion, ie. does > the user bind a device to vfio-pci, i40e_vf_vfio, etc, but maybe that's > the barrier we need such that a user can make an informed decision > about what they're actually using. If a vendor then wants to implement > a feature in vfio-pci, we'll need to architect an interface for it > rather than letting them pick and choose which pieces of vfio-pci to > override. > > > > > identification of bug sources is reasonable. but if a device is binding > > > > to vfio-pci with a vendor module loaded, and there's a bug, they can do at > > > > least two ways to identify if it's a bug in vfio-pci itself. > > > > (1) prevent vendor modules from loading and see if the problem exists > > > > with pure vfio-pci. > > > > (2) do what's demoed in patch 8/10, i.e. do nothing but simply pass all > > > > operations to vfio-pci. > > > > > > The code split is still extremely ad-hoc, there's no API. An mdev > > > driver isn't even a sub-driver of vfio-pci like you're trying to > > > accomplish here, there would need to be a much more defined API when > > > the base device isn't even a vfio_pci_device. I don't see how this > > > series would directly enable an mdev use case. > > > > > similar to Yi's series https://patchwork.kernel.org/patch/11320841/. > > we can parcel the vdev creation code in vfio_pci_probe() to allow calling from > > mdev parent probe routine. (of course, also need to parcel code to free vdev) > > e.g. > > > > void *vfio_pci_alloc_vdev(struct pci_dev *pdev, const struct pci_device_id *id) > > { > > struct vfio_pci_device *vdev; > > vdev = kzalloc(sizeof(*vdev), GFP_KERNEL); > > if (!vdev) { > > ret = -ENOMEM; > > goto out_group_put; > > } > > > > vdev->pdev = pdev; > > vdev->irq_type = VFIO_PCI_NUM_IRQS; > > mutex_init(&vdev->igate); > > spin_lock_init(&vdev->irqlock); > > mutex_init(&vdev->ioeventfds_lock); > > INIT_LIST_HEAD(&vdev->ioeventfds_list); > > ... > > vfio_pci_probe_power_state(vdev); > > > > if (!disable_idle_d3) { > > vfio_pci_set_power_state(vdev, PCI_D0); > > vfio_pci_set_power_state(vdev, PCI_D3hot); > > } > > return vdev; > > } > > > > static int vfio_mdev_pci_driver_probe(struct pci_dev *pdev, const struct pci_device_id *id)) > > { > > > > void *vdev = vfio_pci_alloc_vdev(pdev, id); > > > > //save the vdev pointer > > > > } > > then all the exported interfaces from this series can also benefit the > > mdev use case. > > You need to convince me that we're not just doing this for the sake of > re-using a migration interface. We do need vendor specific drivers to > support migration, but implementing those vendor specific drivers in > the kernel just because we have that interface is the wrong answer. If > we can implement that device specific migration support in QEMU and > limit the attack surface from the hypervisor or guest into the host > kernel, that's a better answer. As I've noted above, I'm afraid all of > these attempts to parcel out vfio-pci are only going to serve to > proliferate vendor modules that have limited community review, expand > the attack surface, and potentially harm the vfio ecosystem overall > through bad actors and reduced autonomy. Thanks, > The requirement to access PF as mentioned above is one of the reason for us to implement the emulation in kernel. Another reason is that we don't want to duplicate a lot of kernel logic in QEMU as what'd done in Alex Graf's "vfio-i40e". then QEMU has to be updated along kernel driver changing. The effort for maintenance and version matching is a big burden to vendors. But you are right, there're less review in virtualization side to code under vendor specific directory. That's also the pulse for us to propose common helper APIs for them to call, not only for convenience and duplication-less, but also for code with full review. would you mind giving us some suggestions for where to go? Thanks Yan
On Wed, 10 Jun 2020 01:23:14 -0400 Yan Zhao <yan.y.zhao@intel.com> wrote: > On Fri, Jun 05, 2020 at 10:13:01AM -0600, Alex Williamson wrote: > > On Thu, 4 Jun 2020 22:02:31 -0400 > > Yan Zhao <yan.y.zhao@intel.com> wrote: > > > > > On Wed, Jun 03, 2020 at 10:10:58PM -0600, Alex Williamson wrote: > > > > On Wed, 3 Jun 2020 22:42:28 -0400 > > > > Yan Zhao <yan.y.zhao@intel.com> wrote: > > > > > > > > > On Wed, Jun 03, 2020 at 05:04:52PM -0600, Alex Williamson wrote: > > > > > > On Tue, 2 Jun 2020 21:40:58 -0400 > > > > > > Yan Zhao <yan.y.zhao@intel.com> wrote: > > > > > > > > > > > > > On Tue, Jun 02, 2020 at 01:34:35PM -0600, Alex Williamson wrote: > > > > > > > > I'm not at all happy with this. Why do we need to hide the migration > > > > > > > > sparse mmap from the user until migration time? What if instead we > > > > > > > > introduced a new VFIO_REGION_INFO_CAP_SPARSE_MMAP_SAVING capability > > > > > > > > where the existing capability is the normal runtime sparse setup and > > > > > > > > the user is required to use this new one prior to enabled device_state > > > > > > > > with _SAVING. The vendor driver could then simply track mmap vmas to > > > > > > > > the region and refuse to change device_state if there are outstanding > > > > > > > > mmaps conflicting with the _SAVING sparse mmap layout. No new IRQs > > > > > > > > required, no new irqfds, an incremental change to the protocol, > > > > > > > > backwards compatible to the extent that a vendor driver requiring this > > > > > > > > will automatically fail migration. > > > > > > > > > > > > > > > right. looks we need to use this approach to solve the problem. > > > > > > > thanks for your guide. > > > > > > > so I'll abandon the current remap irq way for dirty tracking during live > > > > > > > migration. > > > > > > > but anyway, it demos how to customize irq_types in vendor drivers. > > > > > > > then, what do you think about patches 1-5? > > > > > > > > > > > > In broad strokes, I don't think we've found the right solution yet. I > > > > > > really question whether it's supportable to parcel out vfio-pci like > > > > > > this and I don't know how I'd support unraveling whether we have a bug > > > > > > in vfio-pci, the vendor driver, or how the vendor driver is making use > > > > > > of vfio-pci. > > > > > > > > > > > > Let me also ask, why does any of this need to be in the kernel? We > > > > > > spend 5 patches slicing up vfio-pci so that we can register a vendor > > > > > > driver and have that vendor driver call into vfio-pci as it sees fit. > > > > > > We have two patches creating device specific interrupts and a BAR > > > > > > remapping scheme that we've decided we don't need. That brings us to > > > > > > the actual i40e vendor driver, where the first patch is simply making > > > > > > the vendor driver work like vfio-pci already does, the second patch is > > > > > > handling the migration region, and the third patch is implementing the > > > > > > BAR remapping IRQ that we decided we don't need. It's difficult to > > > > > > actually find the small bit of code that's required to support > > > > > > migration outside of just dealing with the protocol we've defined to > > > > > > expose this from the kernel. So why are we trying to do this in the > > > > > > kernel? We have quirk support in QEMU, we can easily flip > > > > > > MemoryRegions on and off, etc. What access to the device outside of > > > > > > what vfio-pci provides to the user, and therefore QEMU, is necessary to > > > > > > implement this migration support for i40e VFs? Is this just an > > > > > > exercise in making use of the migration interface? Thanks, > > > > > > > > > > > hi Alex > > > > > > > > > > There was a description of intention of this series in RFC v1 > > > > > (https://www.spinics.net/lists/kernel/msg3337337.html). > > > > > sorry, I didn't include it in starting from RFC v2. > > > > > > > > > > " > > > > > The reason why we don't choose the way of writing mdev parent driver is > > > > > that > > > > > > > > I didn't mention an mdev approach, I'm asking what are we accomplishing > > > > by doing this in the kernel at all versus exposing the device as normal > > > > through vfio-pci and providing the migration support in QEMU. Are you > > > > actually leveraging having some sort of access to the PF in supporting > > > > migration of the VF? Is vfio-pci masking the device in a way that > > > > prevents migrating the state from QEMU? > > > > > > > yes, communication to PF is required. VF state is managed by PF and is > > > queried from PF when VF is stopped. > > > > > > migration support in QEMU seems only suitable to devices with dirty > > > pages and device state available by reading/writing device MMIOs, which > > > is not the case for most devices. > > > > Post code for such a device. > > > hi Alex, > There's an example in i40e vf. virtual channel related resources are in > guest memory. dirty page tracking requires the info stored in those > guest memory. > > there're two ways to get the resources addresses: > (1) always trap VF registers related. as in Alex Graf's qemu code. > > starting from beginning, it tracks rw of Admin Queue Configuration registers. > Then in the write handler vfio_i40evf_aq_mmio_mem_region_write(), guest > commands are processed to record the guest dma addresses of the virtual > channel related resources. > e.g. vdev->vsi_config is read from the guest dma addr contained in > command I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES. > > > vfio_i40evf_initfn() > { > ... > memory_region_init_io(&vdev->aq_mmio_mem, OBJECT(dev), > &vfio_i40evf_aq_mmio_mem_region_ops, > vdev, "i40evf AQ config", > I40E_VFGEN_RSTAT - I40E_VF_ARQBAH1); > ... > } > > vfio_i40evf_aq_mmio_mem_region_write() > { > ... > switch (addr) { > case I40E_VF_ARQBAH1: > case I40E_VF_ARQBAL1: > case I40E_VF_ARQH1: > case I40E_VF_ARQLEN1: > case I40E_VF_ARQT1: > case I40E_VF_ATQBAH1: > case I40E_VF_ATQBAL1: > case I40E_VF_ATQH1: > case I40E_VF_ATQT1: > case I40E_VF_ATQLEN1: > vfio_i40evf_vw32(vdev, addr, data); > vfio_i40e_aq_update(vdev); ==> update & process atq commands > break; > default: > vfio_i40evf_w32(vdev, addr, data); > break; > } > } > vfio_i40e_aq_update(vdev) > |->vfio_i40e_atq_process_one(vdev, vfio_i40evf_vr32(vdev, I40E_VF_ATQH1) > |-> hwaddr addr = vfio_i40e_get_atqba(vdev) + (index * sizeof(desc)); > | pci_dma_read(pdev, addr, &desc, sizeof(desc));//read guest's command > | vfio_i40e_record_atq_cmd(vdev, pdev, &desc) > > > > vfio_i40e_record_atq_cmd(...I40eAdminQueueDescriptor *desc) { > data_addr = desc->params.external.addr_high; > ... > > switch (desc->cookie_high) { > ... > case I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES: > pci_dma_read(pdev, data_addr, &vdev->vsi_config, > MIN(desc->datalen, sizeof(vdev->vsi_config))); > ... > } > ... > } > > > (2) pass through all guest MMIO accesses and only do MMIO trap when migration > is about to start. > This is the way we're using in the host vfio-pci vendor driver (or mdev parent driver) > of i40e vf device (sorry for no public code available still). > > when migration is about to start, it's already too late to get the guest dma > address for those virtual channel related resources merely by MMIO > trapping, so we have to ask for them from PF. > > > > <...> > > > > > > for interfaces exported in patch 3/10-5/10, they anyway need to be > > > > > exported for writing mdev parent drivers that pass through devices at > > > > > normal time to avoid duplication. and yes, your worry about > > > > > > > > Where are those parent drivers? What are their actual requirements? > > > > > > > if this way of registering vendor ops to vfio-pci is not permitted, > > > vendors have to resort to writing its mdev parent drivers for VFs. Those > > > parent drivers need to pass through VFs at normal time, doing exactly what > > > vfio-pci does and only doing what vendor ops does during migration. > > > > > > if vfio-pci could export common code to those parent drivers, lots of > > > duplicated code can be avoided. > > > > There are two sides to this argument though. We could also argue that > > mdev has already made it too easy to implement device emulation in the > > kernel, the barrier is that such emulation is more transparent because > > it does require a fair bit of code duplication from vfio-pci. If we > > make it easier to simply re-use vfio-pci for much of this, and even > > take it a step further by allowing vendor drivers to masquerade behind > > vfio-pci, then we're creating an environment where vendors don't need > > to work with QEMU to get their device emulation accepted. They can > > write their own vendor drivers, which are now simplified and sanctioned > > by exported functions in vfio-pci. They can do this easily and open up > > massive attack vectors, hiding behind vfio-pci. > > > your concern is reasonable. > > > I know that I was advocating avoiding user driver confusion, ie. does > > the user bind a device to vfio-pci, i40e_vf_vfio, etc, but maybe that's > > the barrier we need such that a user can make an informed decision > > about what they're actually using. If a vendor then wants to implement > > a feature in vfio-pci, we'll need to architect an interface for it > > rather than letting them pick and choose which pieces of vfio-pci to > > override. > > > > > > > identification of bug sources is reasonable. but if a device is binding > > > > > to vfio-pci with a vendor module loaded, and there's a bug, they can do at > > > > > least two ways to identify if it's a bug in vfio-pci itself. > > > > > (1) prevent vendor modules from loading and see if the problem exists > > > > > with pure vfio-pci. > > > > > (2) do what's demoed in patch 8/10, i.e. do nothing but simply pass all > > > > > operations to vfio-pci. > > > > > > > > The code split is still extremely ad-hoc, there's no API. An mdev > > > > driver isn't even a sub-driver of vfio-pci like you're trying to > > > > accomplish here, there would need to be a much more defined API when > > > > the base device isn't even a vfio_pci_device. I don't see how this > > > > series would directly enable an mdev use case. > > > > > > > similar to Yi's series https://patchwork.kernel.org/patch/11320841/. > > > we can parcel the vdev creation code in vfio_pci_probe() to allow calling from > > > mdev parent probe routine. (of course, also need to parcel code to free vdev) > > > e.g. > > > > > > void *vfio_pci_alloc_vdev(struct pci_dev *pdev, const struct pci_device_id *id) > > > { > > > struct vfio_pci_device *vdev; > > > vdev = kzalloc(sizeof(*vdev), GFP_KERNEL); > > > if (!vdev) { > > > ret = -ENOMEM; > > > goto out_group_put; > > > } > > > > > > vdev->pdev = pdev; > > > vdev->irq_type = VFIO_PCI_NUM_IRQS; > > > mutex_init(&vdev->igate); > > > spin_lock_init(&vdev->irqlock); > > > mutex_init(&vdev->ioeventfds_lock); > > > INIT_LIST_HEAD(&vdev->ioeventfds_list); > > > ... > > > vfio_pci_probe_power_state(vdev); > > > > > > if (!disable_idle_d3) { > > > vfio_pci_set_power_state(vdev, PCI_D0); > > > vfio_pci_set_power_state(vdev, PCI_D3hot); > > > } > > > return vdev; > > > } > > > > > > static int vfio_mdev_pci_driver_probe(struct pci_dev *pdev, const struct pci_device_id *id)) > > > { > > > > > > void *vdev = vfio_pci_alloc_vdev(pdev, id); > > > > > > //save the vdev pointer > > > > > > } > > > then all the exported interfaces from this series can also benefit the > > > mdev use case. > > > > You need to convince me that we're not just doing this for the sake of > > re-using a migration interface. We do need vendor specific drivers to > > support migration, but implementing those vendor specific drivers in > > the kernel just because we have that interface is the wrong answer. If > > we can implement that device specific migration support in QEMU and > > limit the attack surface from the hypervisor or guest into the host > > kernel, that's a better answer. As I've noted above, I'm afraid all of > > these attempts to parcel out vfio-pci are only going to serve to > > proliferate vendor modules that have limited community review, expand > > the attack surface, and potentially harm the vfio ecosystem overall > > through bad actors and reduced autonomy. Thanks, > > > The requirement to access PF as mentioned above is one of the reason for > us to implement the emulation in kernel. > Another reason is that we don't want to duplicate a lot of kernel logic in > QEMU as what'd done in Alex Graf's "vfio-i40e". then QEMU has to be > updated along kernel driver changing. The effort for maintenance and > version matching is a big burden to vendors. > But you are right, there're less review in virtualization side to code under > vendor specific directory. That's also the pulse for us to propose > common helper APIs for them to call, not only for convenience and > duplication-less, but also for code with full review. > > would you mind giving us some suggestions for where to go? Not duplicating kernel code into userspace isn't a great excuse. What we need to do to emulate a device is not an exact mapping to what a driver for that device needs to do. If we need to keep the device driver and the emulation in sync then we haven't done a good job with the emulation. What would it look like if we only had an additional device specific region on the vfio device fd we could use to get the descriptor information we need from the PF? This would be more inline with the quirks we provide for IGD assignment. Thanks, Alex
On Fri, Jun 19, 2020 at 04:55:34PM -0600, Alex Williamson wrote: > On Wed, 10 Jun 2020 01:23:14 -0400 > Yan Zhao <yan.y.zhao@intel.com> wrote: > > > On Fri, Jun 05, 2020 at 10:13:01AM -0600, Alex Williamson wrote: > > > On Thu, 4 Jun 2020 22:02:31 -0400 > > > Yan Zhao <yan.y.zhao@intel.com> wrote: > > > > > > > On Wed, Jun 03, 2020 at 10:10:58PM -0600, Alex Williamson wrote: > > > > > On Wed, 3 Jun 2020 22:42:28 -0400 > > > > > Yan Zhao <yan.y.zhao@intel.com> wrote: > > > > > > > > > > > On Wed, Jun 03, 2020 at 05:04:52PM -0600, Alex Williamson wrote: > > > > > > > On Tue, 2 Jun 2020 21:40:58 -0400 > > > > > > > Yan Zhao <yan.y.zhao@intel.com> wrote: > > > > > > > > > > > > > > > On Tue, Jun 02, 2020 at 01:34:35PM -0600, Alex Williamson wrote: > > > > > > > > > I'm not at all happy with this. Why do we need to hide the migration > > > > > > > > > sparse mmap from the user until migration time? What if instead we > > > > > > > > > introduced a new VFIO_REGION_INFO_CAP_SPARSE_MMAP_SAVING capability > > > > > > > > > where the existing capability is the normal runtime sparse setup and > > > > > > > > > the user is required to use this new one prior to enabled device_state > > > > > > > > > with _SAVING. The vendor driver could then simply track mmap vmas to > > > > > > > > > the region and refuse to change device_state if there are outstanding > > > > > > > > > mmaps conflicting with the _SAVING sparse mmap layout. No new IRQs > > > > > > > > > required, no new irqfds, an incremental change to the protocol, > > > > > > > > > backwards compatible to the extent that a vendor driver requiring this > > > > > > > > > will automatically fail migration. > > > > > > > > > > > > > > > > > right. looks we need to use this approach to solve the problem. > > > > > > > > thanks for your guide. > > > > > > > > so I'll abandon the current remap irq way for dirty tracking during live > > > > > > > > migration. > > > > > > > > but anyway, it demos how to customize irq_types in vendor drivers. > > > > > > > > then, what do you think about patches 1-5? > > > > > > > > > > > > > > In broad strokes, I don't think we've found the right solution yet. I > > > > > > > really question whether it's supportable to parcel out vfio-pci like > > > > > > > this and I don't know how I'd support unraveling whether we have a bug > > > > > > > in vfio-pci, the vendor driver, or how the vendor driver is making use > > > > > > > of vfio-pci. > > > > > > > > > > > > > > Let me also ask, why does any of this need to be in the kernel? We > > > > > > > spend 5 patches slicing up vfio-pci so that we can register a vendor > > > > > > > driver and have that vendor driver call into vfio-pci as it sees fit. > > > > > > > We have two patches creating device specific interrupts and a BAR > > > > > > > remapping scheme that we've decided we don't need. That brings us to > > > > > > > the actual i40e vendor driver, where the first patch is simply making > > > > > > > the vendor driver work like vfio-pci already does, the second patch is > > > > > > > handling the migration region, and the third patch is implementing the > > > > > > > BAR remapping IRQ that we decided we don't need. It's difficult to > > > > > > > actually find the small bit of code that's required to support > > > > > > > migration outside of just dealing with the protocol we've defined to > > > > > > > expose this from the kernel. So why are we trying to do this in the > > > > > > > kernel? We have quirk support in QEMU, we can easily flip > > > > > > > MemoryRegions on and off, etc. What access to the device outside of > > > > > > > what vfio-pci provides to the user, and therefore QEMU, is necessary to > > > > > > > implement this migration support for i40e VFs? Is this just an > > > > > > > exercise in making use of the migration interface? Thanks, > > > > > > > > > > > > > hi Alex > > > > > > > > > > > > There was a description of intention of this series in RFC v1 > > > > > > (https://www.spinics.net/lists/kernel/msg3337337.html). > > > > > > sorry, I didn't include it in starting from RFC v2. > > > > > > > > > > > > " > > > > > > The reason why we don't choose the way of writing mdev parent driver is > > > > > > that > > > > > > > > > > I didn't mention an mdev approach, I'm asking what are we accomplishing > > > > > by doing this in the kernel at all versus exposing the device as normal > > > > > through vfio-pci and providing the migration support in QEMU. Are you > > > > > actually leveraging having some sort of access to the PF in supporting > > > > > migration of the VF? Is vfio-pci masking the device in a way that > > > > > prevents migrating the state from QEMU? > > > > > > > > > yes, communication to PF is required. VF state is managed by PF and is > > > > queried from PF when VF is stopped. > > > > > > > > migration support in QEMU seems only suitable to devices with dirty > > > > pages and device state available by reading/writing device MMIOs, which > > > > is not the case for most devices. > > > > > > Post code for such a device. > > > > > hi Alex, > > There's an example in i40e vf. virtual channel related resources are in > > guest memory. dirty page tracking requires the info stored in those > > guest memory. > > > > there're two ways to get the resources addresses: > > (1) always trap VF registers related. as in Alex Graf's qemu code. > > > > starting from beginning, it tracks rw of Admin Queue Configuration registers. > > Then in the write handler vfio_i40evf_aq_mmio_mem_region_write(), guest > > commands are processed to record the guest dma addresses of the virtual > > channel related resources. > > e.g. vdev->vsi_config is read from the guest dma addr contained in > > command I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES. > > > > > > vfio_i40evf_initfn() > > { > > ... > > memory_region_init_io(&vdev->aq_mmio_mem, OBJECT(dev), > > &vfio_i40evf_aq_mmio_mem_region_ops, > > vdev, "i40evf AQ config", > > I40E_VFGEN_RSTAT - I40E_VF_ARQBAH1); > > ... > > } > > > > vfio_i40evf_aq_mmio_mem_region_write() > > { > > ... > > switch (addr) { > > case I40E_VF_ARQBAH1: > > case I40E_VF_ARQBAL1: > > case I40E_VF_ARQH1: > > case I40E_VF_ARQLEN1: > > case I40E_VF_ARQT1: > > case I40E_VF_ATQBAH1: > > case I40E_VF_ATQBAL1: > > case I40E_VF_ATQH1: > > case I40E_VF_ATQT1: > > case I40E_VF_ATQLEN1: > > vfio_i40evf_vw32(vdev, addr, data); > > vfio_i40e_aq_update(vdev); ==> update & process atq commands > > break; > > default: > > vfio_i40evf_w32(vdev, addr, data); > > break; > > } > > } > > vfio_i40e_aq_update(vdev) > > |->vfio_i40e_atq_process_one(vdev, vfio_i40evf_vr32(vdev, I40E_VF_ATQH1) > > |-> hwaddr addr = vfio_i40e_get_atqba(vdev) + (index * sizeof(desc)); > > | pci_dma_read(pdev, addr, &desc, sizeof(desc));//read guest's command > > | vfio_i40e_record_atq_cmd(vdev, pdev, &desc) > > > > > > > > vfio_i40e_record_atq_cmd(...I40eAdminQueueDescriptor *desc) { > > data_addr = desc->params.external.addr_high; > > ... > > > > switch (desc->cookie_high) { > > ... > > case I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES: > > pci_dma_read(pdev, data_addr, &vdev->vsi_config, > > MIN(desc->datalen, sizeof(vdev->vsi_config))); > > ... > > } > > ... > > } > > > > > > (2) pass through all guest MMIO accesses and only do MMIO trap when migration > > is about to start. > > This is the way we're using in the host vfio-pci vendor driver (or mdev parent driver) > > of i40e vf device (sorry for no public code available still). > > > > when migration is about to start, it's already too late to get the guest dma > > address for those virtual channel related resources merely by MMIO > > trapping, so we have to ask for them from PF. > > > > > > > > <...> > > > > > > > > for interfaces exported in patch 3/10-5/10, they anyway need to be > > > > > > exported for writing mdev parent drivers that pass through devices at > > > > > > normal time to avoid duplication. and yes, your worry about > > > > > > > > > > Where are those parent drivers? What are their actual requirements? > > > > > > > > > if this way of registering vendor ops to vfio-pci is not permitted, > > > > vendors have to resort to writing its mdev parent drivers for VFs. Those > > > > parent drivers need to pass through VFs at normal time, doing exactly what > > > > vfio-pci does and only doing what vendor ops does during migration. > > > > > > > > if vfio-pci could export common code to those parent drivers, lots of > > > > duplicated code can be avoided. > > > > > > There are two sides to this argument though. We could also argue that > > > mdev has already made it too easy to implement device emulation in the > > > kernel, the barrier is that such emulation is more transparent because > > > it does require a fair bit of code duplication from vfio-pci. If we > > > make it easier to simply re-use vfio-pci for much of this, and even > > > take it a step further by allowing vendor drivers to masquerade behind > > > vfio-pci, then we're creating an environment where vendors don't need > > > to work with QEMU to get their device emulation accepted. They can > > > write their own vendor drivers, which are now simplified and sanctioned > > > by exported functions in vfio-pci. They can do this easily and open up > > > massive attack vectors, hiding behind vfio-pci. > > > > > your concern is reasonable. > > > > > I know that I was advocating avoiding user driver confusion, ie. does > > > the user bind a device to vfio-pci, i40e_vf_vfio, etc, but maybe that's > > > the barrier we need such that a user can make an informed decision > > > about what they're actually using. If a vendor then wants to implement > > > a feature in vfio-pci, we'll need to architect an interface for it > > > rather than letting them pick and choose which pieces of vfio-pci to > > > override. > > > > > > > > > identification of bug sources is reasonable. but if a device is binding > > > > > > to vfio-pci with a vendor module loaded, and there's a bug, they can do at > > > > > > least two ways to identify if it's a bug in vfio-pci itself. > > > > > > (1) prevent vendor modules from loading and see if the problem exists > > > > > > with pure vfio-pci. > > > > > > (2) do what's demoed in patch 8/10, i.e. do nothing but simply pass all > > > > > > operations to vfio-pci. > > > > > > > > > > The code split is still extremely ad-hoc, there's no API. An mdev > > > > > driver isn't even a sub-driver of vfio-pci like you're trying to > > > > > accomplish here, there would need to be a much more defined API when > > > > > the base device isn't even a vfio_pci_device. I don't see how this > > > > > series would directly enable an mdev use case. > > > > > > > > > similar to Yi's series https://patchwork.kernel.org/patch/11320841/. > > > > we can parcel the vdev creation code in vfio_pci_probe() to allow calling from > > > > mdev parent probe routine. (of course, also need to parcel code to free vdev) > > > > e.g. > > > > > > > > void *vfio_pci_alloc_vdev(struct pci_dev *pdev, const struct pci_device_id *id) > > > > { > > > > struct vfio_pci_device *vdev; > > > > vdev = kzalloc(sizeof(*vdev), GFP_KERNEL); > > > > if (!vdev) { > > > > ret = -ENOMEM; > > > > goto out_group_put; > > > > } > > > > > > > > vdev->pdev = pdev; > > > > vdev->irq_type = VFIO_PCI_NUM_IRQS; > > > > mutex_init(&vdev->igate); > > > > spin_lock_init(&vdev->irqlock); > > > > mutex_init(&vdev->ioeventfds_lock); > > > > INIT_LIST_HEAD(&vdev->ioeventfds_list); > > > > ... > > > > vfio_pci_probe_power_state(vdev); > > > > > > > > if (!disable_idle_d3) { > > > > vfio_pci_set_power_state(vdev, PCI_D0); > > > > vfio_pci_set_power_state(vdev, PCI_D3hot); > > > > } > > > > return vdev; > > > > } > > > > > > > > static int vfio_mdev_pci_driver_probe(struct pci_dev *pdev, const struct pci_device_id *id)) > > > > { > > > > > > > > void *vdev = vfio_pci_alloc_vdev(pdev, id); > > > > > > > > //save the vdev pointer > > > > > > > > } > > > > then all the exported interfaces from this series can also benefit the > > > > mdev use case. > > > > > > You need to convince me that we're not just doing this for the sake of > > > re-using a migration interface. We do need vendor specific drivers to > > > support migration, but implementing those vendor specific drivers in > > > the kernel just because we have that interface is the wrong answer. If > > > we can implement that device specific migration support in QEMU and > > > limit the attack surface from the hypervisor or guest into the host > > > kernel, that's a better answer. As I've noted above, I'm afraid all of > > > these attempts to parcel out vfio-pci are only going to serve to > > > proliferate vendor modules that have limited community review, expand > > > the attack surface, and potentially harm the vfio ecosystem overall > > > through bad actors and reduced autonomy. Thanks, > > > > > The requirement to access PF as mentioned above is one of the reason for > > us to implement the emulation in kernel. > > Another reason is that we don't want to duplicate a lot of kernel logic in > > QEMU as what'd done in Alex Graf's "vfio-i40e". then QEMU has to be > > updated along kernel driver changing. The effort for maintenance and > > version matching is a big burden to vendors. > > But you are right, there're less review in virtualization side to code under > > vendor specific directory. That's also the pulse for us to propose > > common helper APIs for them to call, not only for convenience and > > duplication-less, but also for code with full review. > > > > would you mind giving us some suggestions for where to go? > > Not duplicating kernel code into userspace isn't a great excuse. What > we need to do to emulate a device is not an exact mapping to what a > driver for that device needs to do. If we need to keep the device > driver and the emulation in sync then we haven't done a good job with > the emulation. What would it look like if we only had an additional > device specific region on the vfio device fd we could use to get the > descriptor information we need from the PF? This would be more inline > with the quirks we provide for IGD assignment. Thanks, > hi Alex Thanks for this suggestion. As migration region is a generic vendor region, do you think below way to specify device specific region is acceptable? (1) provide/export an interface to let vendor driver register its device specific region or substitute get_region_info/rw/mmap of existing regions. (2) export vfio_pci_default_rw(), vfio_pci_default_mmap() to called from both vendor driver handlers and vfio-pci. Or do you still prefer to adding quirks per device so you can have a better review of all code? we can add a disable flag to disable regions registered/modified by vendor drivers in bullet (1) for debug purpose. Thanks Yan
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h index 2d0d85c7c4d4..55895f75d720 100644 --- a/include/uapi/linux/vfio.h +++ b/include/uapi/linux/vfio.h @@ -704,6 +704,17 @@ struct vfio_irq_info_cap_type { __u32 subtype; /* type specific */ }; +/* Bar Region Query IRQ TYPE */ +#define VFIO_IRQ_TYPE_REMAP_BAR_REGION (1) + +/* sub-types for VFIO_IRQ_TYPE_REMAP_BAR_REGION */ +/* + * This irq notifies userspace to re-query BAR region and remaps the + * subregions. + */ +#define VFIO_IRQ_SUBTYPE_REMAP_BAR_REGION (0) + + /** * VFIO_DEVICE_SET_IRQS - _IOW(VFIO_TYPE, VFIO_BASE + 10, struct vfio_irq_set) *
This is a virtual irq type. vendor driver triggers this irq when it wants to notify userspace to remap PCI BARs. 1. vendor driver triggers this irq and packs the target bar number in the ctx count. i.e. "1 << bar_number". if a bit is set, the corresponding bar is to be remapped. 2. userspace requery the specified PCI BAR from kernel and if flags of the bar regions are changed, it removes the old subregions and attaches subregions according to the new flags. 3. userspace notifies back to kernel by writing one to the eventfd of this irq. Please check the corresponding qemu implementation from the reply of this patch, and a sample usage in vendor driver in patch [10/10]. Cc: Kevin Tian <kevin.tian@intel.com> Signed-off-by: Yan Zhao <yan.y.zhao@intel.com> --- include/uapi/linux/vfio.h | 11 +++++++++++ 1 file changed, 11 insertions(+)