Messages in this thread | | | Date | Thu, 22 Jun 2017 16:53:17 -0600 | From | Alex Williamson <> | Subject | Re: [RFC 5/9] iommu: Introduce fault notifier API |
| |
On Wed, 14 Jun 2017 15:22:59 -0700 Jacob Pan <jacob.jun.pan@linux.intel.com> wrote:
> Traditionally, device specific faults are detected and handled within > their own device drivers. When IOMMU is enabled, faults such as DMA > related transactions are detected by IOMMU. There is no generic > reporting mechanism to report faults back to the in-kernel device > driver or the guest OS in case of assigned devices. > > Faults detected by IOMMU is based on the transaction's source ID which > can be reported at per device basis, regardless of the device type is a > PCI device or not. > > The fault types includes recoverable (e.g. page request) and > unrecoverable faults(e.g. invalid context). In most cases, faults can be > handled by IOMMU drivers. However, there are use cases that require > fault processing outside IOMMU driver, e.g. > > 1. page request fault originated from an SVM capable device that is > assigned to guest via vIOMMU. In this case, the first level page tables > are owned by the guest. Page request must be propagated to the guest to > let guest OS fault in the pages then send page response. In this > mechanism, the direct receiver of IOMMU fault notification is VFIO, > which can relay notification events to QEMU or other user space > software. > > 2. faults need more subtle handling by device drivers. Other than > simply invoke reset function, there are needs to let device driver > handle the fault with a smaller impact. > > This patchset is intended to create a generic fault notification API such > that it can scale as follows: > - all IOMMU types > - PCI and non-PCI devices > - recoverable and unrecoverable faults > - VFIO and other other in kernel users > - DMA & IRQ remapping (TBD) > > The event data contains both generic and raw architectural data > such that performance is not compromised as the data propagation may > involve many layers. > > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com> > Signed-off-by: Ashok Raj <ashok.raj@intel.com> > --- > drivers/iommu/iommu.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++ > include/linux/iommu.h | 54 +++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 117 insertions(+) > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index c786370..04c73f3 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -48,6 +48,7 @@ struct iommu_group { > struct list_head devices; > struct mutex mutex; > struct blocking_notifier_head notifier; > + struct blocking_notifier_head fault_notifier; > void *iommu_data; > void (*iommu_data_release)(void *iommu_data); > char *name; > @@ -345,6 +346,7 @@ struct iommu_group *iommu_group_alloc(void) > mutex_init(&group->mutex); > INIT_LIST_HEAD(&group->devices); > BLOCKING_INIT_NOTIFIER_HEAD(&group->notifier); > + BLOCKING_INIT_NOTIFIER_HEAD(&group->fault_notifier); > > ret = ida_simple_get(&iommu_group_ida, 0, 0, GFP_KERNEL); > if (ret < 0) { > @@ -790,6 +792,67 @@ int iommu_group_unregister_notifier(struct iommu_group *group, > EXPORT_SYMBOL_GPL(iommu_group_unregister_notifier); > > /** > + * iommu_register_fault_notifier - Register a notifier for fault reporting > + * @dev: device to notify fault events > + * @nb: notifier block to signal > + * > + */ > +int iommu_register_fault_notifier(struct device *dev, > + struct notifier_block *nb) > +{ > + int ret; > + struct iommu_group *group = iommu_group_get(dev); > + > + if (!group) > + return -EINVAL; > + > + ret = blocking_notifier_chain_register(&group->fault_notifier, nb); > + iommu_group_put(group); > + > + return ret; > + > +} > +EXPORT_SYMBOL_GPL(iommu_register_fault_notifier); > + > +/** > + * iommu_unregister_fault_notifier - Unregister a notifier for fault reporting > + * @domain: the domain to watch > + * @nb: notifier block to signal > + * > + */ > +int iommu_unregister_fault_notifier(struct device *dev, > + struct notifier_block *nb) > +{ > + int ret; > + struct iommu_group *group = iommu_group_get(dev); > + > + if (!group) > + return -EINVAL; > + > + ret = blocking_notifier_chain_unregister(&group->fault_notifier, nb); > + iommu_group_put(group); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(iommu_unregister_fault_notifier);
If the call chains are on the group, why do we register with a device? If I registered with a group, I'd know that I only need to register once per group, that's not clear here and may lead to callers of this getting multiple notifications, one for each device in a group.
> + > +int iommu_fault_notifier_call_chain(struct iommu_fault_event *event) > +{ > + int ret; > + struct iommu_group *group = iommu_group_get(event->dev); > + > + if (!group) > + return -EINVAL; > + /* caller provide generic data related to the event, TBD */ > + ret = (blocking_notifier_call_chain(&group->fault_notifier, 0, (void *)event) > + == NOTIFY_BAD) ? -EINVAL : 0; > + iommu_group_put(group); > + > + return ret; > +} > +EXPORT_SYMBOL(iommu_fault_notifier_call_chain); > + > +/** > * iommu_group_id - Return ID for a group > * @group: the group to ID > * > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > index 2cdbaa3..fe89e88 100644 > --- a/include/linux/iommu.h > +++ b/include/linux/iommu.h > @@ -42,6 +42,7 @@ > * if the IOMMU page table format is equivalent. > */ > #define IOMMU_PRIV (1 << 5) > +#define IOMMU_EXEC (1 << 6)
Irrelevant change?
> > struct iommu_ops; > struct iommu_group; > @@ -97,6 +98,36 @@ struct iommu_domain { > void *iova_cookie; > }; > > +/* > + * Generic fault event notification data, used by all IOMMU architectures. > + * > + * - PCI and non-PCI devices > + * - Recoverable faults (e.g. page request) & un-recoverable faults > + * - DMA remapping and IRQ remapping faults > + * > + * @dev The device which faults are reported by IOMMU > + * @addr tells the offending address > + * @pasid contains process address space ID, used in shared virtual memory (SVM) > + * @prot page access protection flag, e.g. IOMMU_READ, IOMMU_WRITE > + * @flags contains fault type, etc. > + * @length tells the size of the buf > + * @buf contains any raw or arch specific data > + * > + */ > +struct iommu_fault_event { > + struct device *dev; > + __u64 addr; > + __u32 pasid; > + __u32 prot; > + __u32 flags; > +#define IOMMU_FAULT_PAGE_REQ BIT(0) > +#define IOMMU_FAULT_UNRECOV BIT(1) > +#define IOMMU_FAULT_IRQ_REMAP BIT(2) > +#define IOMMU_FAULT_INVAL BIT(3)
Details of each of these are defined where?
> + __u32 length; > + __u8 buf[]; > +};
This is not UAPI, so I'll be curious how vfio is supposed to expose this to userspace.
> + > enum iommu_cap { > IOMMU_CAP_CACHE_COHERENCY, /* IOMMU can enforce cache coherent DMA > transactions */ > @@ -341,6 +372,12 @@ extern int iommu_group_register_notifier(struct iommu_group *group, > struct notifier_block *nb); > extern int iommu_group_unregister_notifier(struct iommu_group *group, > struct notifier_block *nb); > +extern int iommu_register_fault_notifier(struct device *dev, > + struct notifier_block *nb); > +extern int iommu_unregister_fault_notifier(struct device *dev, > + struct notifier_block *nb); > +extern int iommu_fault_notifier_call_chain(struct iommu_fault_event *event); > + > extern int iommu_group_id(struct iommu_group *group); > extern struct iommu_group *iommu_group_get_for_dev(struct device *dev); > extern struct iommu_domain *iommu_group_default_domain(struct iommu_group *); > @@ -573,6 +610,23 @@ static inline int iommu_group_unregister_notifier(struct iommu_group *group, > return 0; > } > > +static inline int iommu_register_fault_notifier(struct device *dev, > + struct notifier_block *nb) > +{ > + return 0; > +} > + > +static inline int iommu_unregister_fault_notifier(struct device *dev, > + struct notifier_block *nb) > +{ > + return 0; > +} > + > +static inline int iommu_fault_notifier_call_chain(struct iommu_fault_event *event) > +{ > + return 0; > +} > + > static inline int iommu_group_id(struct iommu_group *group) > { > return -ENODEV;
| |