On Thu, 17 Nov 2016 20:31:07 +0800
Jike Song <jike.song@xxxxxxxxx> wrote:
> On 11/17/2016 02:03 PM, Alex Williamson wrote:
> > On Thu, 17 Nov 2016 13:24:59 +0800
> > Jike Song <jike.song@xxxxxxxxx> wrote:
> >
> >> On 11/17/2016 03:45 AM, Alex Williamson wrote:
> >>> Perhaps calling it a filter is not correct, I was thinking that a
> >>> vendor driver would register the notifier with a set of required
> >>> events. The driver would need to handle/ignore additional events
> >>> outside of the required mask. There are certainly some complications
> >>> to this model though that I hadn't thought all the way through until
> >>> now. For instance what if we add event XYZ in the future and the
> >>> vendor driver adds this to their required mask. If we run that on an
> >>> older kernel where the vfio infrastructure doesn't know about that
> >>> event, the vendor driver needs to fail, or at least know that event is
> >>> not supported and retry with a set of supported events.
> >>>
> >>> There's another problem with my proposal too, we can't put a single
> >>> notifier_block on multiple notifier_block heads, that just doesn't
> >>> work. So we probably need to separate a group notifier from an iommu
> >>> notifier, the vendor driver will need to register for each.
> >>>
> >>> Maybe we end up with something like:
> >>>
> >>> int vfio_register_notifier(struct device *dev,
> >>> vfio_notify_type_t type,
> >>> unsigned long *required_events,
> >>> struct notifier_block *nb);
> >>>
> >>> typedef unsigned short vfio_notify_type_t;
> >>> enum vfio_notify_type {
> >>> VFIO_IOMMU_NOTIFY = (__force vfio_notify_type_t)0,
> >>> VFIO_GROUP_NOTIFY = (__force vfio_notify_type_t)1,
> >>> };
> >>>
> >>> (stealing this from pci_dev_flags_t, hope it works)
> >>>
> >>> A VFIO_GROUP_NOTIFY would add the notifier_block to the vfio_group, a
> >>> VFIO_IOMMU_NOTIFY would add it to vfio_iommu. Each would have their
> >>> own unique set of events and each would compare their supported events
> >>> to the required events by the caller. Supported events would be
> >>> cleared from the callers required_events arg. If required_events still
> >>> has bits set, the notifier_block is not registered, an error is
> >>> returned, and the caller can identify the unsupported events by the
> >>> remaining bits in the required_events arg. Can that work? Thanks,
> >>
> >> Let me summarize the discussion:
> >>
> >> - There should be 2 notifier heads, one in vfio_iommu another in vfio_group;
> >> - vfio_{un}register_notifier() has the type specified in parameter
> >> - In vfio_register_notifier, maybe:
> >>
> >> static vfio_iommu_register_notifier() {..}
> >> static vfio_group_register_notifier() {..}
> >> int vfio_register_notififer(type..
> >> {
> >> if (type == VFIO_IOMMU_NOTIFY)
> >> return vfio_iommu_register_notifier();
> >> if (type == VFIO_GROUP_NOTIFY)
> >> return vfio_group_register_notifier();
> >> }
> >>
> >>
> >>
> >> What's more, if we still want registration to be done in mdev framework,
> >> we should change parent_ops:
> >>
> >> - rename 'notifier' to 'iommu_notifier'
> >> - add "group_notifier"
> >> - Add a group_events and a iommu_events to indicate what events vendor is
> >> interested in, respectively
> >>
> >> or otherwise don't touch it from mdev framework at all?
> >
> > I think we should remove the notifier from the mdev framework and have
> > the vendor drivers call vfio_{un}register_notifier() directly. Note:
> >
> > - vfio_group_release() should be modified to remove any notifier
> > blocks remaining to prevent a stale call chain for the next user.
>
> vfio_group_release calls vfio_group_unlock_and_free, which in turn calls
> kfree(group), so I guess a WARN_ON(group->notifier.head) before kfree
> is enough?
Sorry, vfio_group_fops_release() is the code where I was thinking we
should unregister any notifiers. The group will still exist after
that. I was thinking we do not need to WARN_ON if the vendor driver
doesn't de-populate the notifier list on the group because the group is
tied to the device. On the other hand if the vendor driver registers
on device open, a device could be opened and closed multiple times
within the same open instance of the group, so we could end up with
duplicate call chain entries if we take that approach. What do you
think, should we require the vendor driver to unregister the group
notifier on device release and therefore WARN_ON if any remain in
vfio_group_fops_release()? This is at least consistent with what we
must require for the iommu notifier, so I tend to lean that way.
>
> > - vfio_sanity_check_pfn_list() should be modified to WARN_ON remaining
> > notifier blocks on the vfio_iommu (ie. vendor drivers will need to
> > actively remove iommu notifiers since the vfio_iommu can persist
> > beyond the attachment of the mdev group, the WARN_ON will promote a
> > proactive approach to surfacing such issues).
>
> I guess Kirti will prefer to pick up this? if not I also can do it :-)
>
> > I'd like to get Kirti's current series in linux-next ASAP, so please
> > submit a follow-on series to make these changes. I hope we can get
> > that finalized and added on top of Kirti's series before the v4.10
> > merge window opens. Thanks,
>
> Yes, I'll send out the follow-on series ASAP, since we also have KVMGT
> depending on it to get notified by vfio...
Thanks,
Alex
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
