Re: [RFC v3 14/21] iommu: introduce device fault data

Jacob Pan
2019-01-10T18:45:44Z

On Tue, 8 Jan 2019 11:26:26 +0100
Eric Auger <eric.auger-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:

> From: Jacob Pan <jacob.jun.pan-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>
> Device faults detected by IOMMU can be reported outside IOMMU
> subsystem for further processing. This patch intends to provide
> a generic device fault data such that device drivers can be
> communicated with IOMMU faults without model specific knowledge.
>
> The proposed format is the result of discussion at:
> https://lkml.org/lkml/2017/11/10/291
> Part of the code is based on Jean-Philippe Brucker's patchset
> (https://patchwork.kernel.org/patch/9989315/).
>
> The assumption is that model specific IOMMU driver can filter and
> handle most of the internal faults if the cause is within IOMMU driver
> control. Therefore, the fault reasons can be reported are grouped
> and generalized based common specifications such as PCI ATS.
>
> Signed-off-by: Jacob Pan <jacob.jun.pan-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker-5wv7dgnIgG8@public.gmane.org>
> Signed-off-by: Liu, Yi L <yi.l.liu-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> Signed-off-by: Ashok Raj <ashok.raj-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Eric Auger <eric.auger-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> [moved part of the iommu_fault_event struct in the uapi, enriched
> the fault reasons to be able to map unrecoverable SMMUv3 errors]
> ---
> include/linux/iommu.h | 55 ++++++++++++++++++++++++-
> include/uapi/linux/iommu.h | 83
> ++++++++++++++++++++++++++++++++++++++ 2 files changed, 136
> insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 244c1a3d5989..1dedc2d247c2 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -49,13 +49,17 @@ struct bus_type;
> struct device;
> struct iommu_domain;
> struct notifier_block;
> +struct iommu_fault_event;
>
> /* iommu fault flags */
> -#define IOMMU_FAULT_READ 0x0
> -#define IOMMU_FAULT_WRITE 0x1
> +#define IOMMU_FAULT_READ (1 << 0)
> +#define IOMMU_FAULT_WRITE (1 << 1)
> +#define IOMMU_FAULT_EXEC (1 << 2)
> +#define IOMMU_FAULT_PRIV (1 << 3)
>
> typedef int (*iommu_fault_handler_t)(struct iommu_domain *,
> struct device *, unsigned long, int, void *);
> +typedef int (*iommu_dev_fault_handler_t)(struct iommu_fault_event *,
> void *);
> struct iommu_domain_geometry {
> dma_addr_t aperture_start; /* First address that can be
> mapped */ @@ -255,6 +259,52 @@ struct iommu_device {
> struct device *dev;
> };
>
> +/**
> + * struct iommu_fault_event - Generic per device fault data
> + *
> + * - PCI and non-PCI devices
> + * - Recoverable faults (e.g. page request), information based on
> PCI ATS
> + * and PASID spec.
> + * - Un-recoverable faults of device interest
> + * - DMA remapping and IRQ remapping faults
> + *
> + * @fault: fault descriptor
> + * @device_private: if present, uniquely identify device-specific
> + * private data for an individual page request.
> + * @iommu_private: used by the IOMMU driver for storing
> fault-specific
> + * data. Users should not modify this field before
> + * sending the fault response.
> + */
> +struct iommu_fault_event {
> + struct iommu_fault fault;
> + u64 device_private;
I think we want to move device_private to uapi since it gets injected
into the guest, then returned by guest in case of page response. For
VT-d we also need 128 bits of private data. VT-d spec. 7.7.1

For exception tracking (e.g. unanswered page request), I can add timer
and list info later when I include PRQ. sounds ok?
> + u64 iommu_private;

> +};
> +
> +/**
> + * struct iommu_fault_param - per-device IOMMU fault data
> + * @dev_fault_handler: Callback function to handle IOMMU faults at
> device level
> + * @data: handler private data
> + *
> + */
> +struct iommu_fault_param {
> + iommu_dev_fault_handler_t handler;
> + void *data;
> +};
> +
> +/**
> + * struct iommu_param - collection of per-device IOMMU data
> + *
> + * @fault_param: IOMMU detected device fault reporting data
> + *
> + * TODO: migrate other per device data pointers under
> iommu_dev_data, e.g.
> + * struct iommu_group *iommu_group;
> + * struct iommu_fwspec *iommu_fwspec;
> + */
> +struct iommu_param {
> + struct iommu_fault_param *fault_param;
> +};
> +
> int iommu_device_register(struct iommu_device *iommu);
> void iommu_device_unregister(struct iommu_device *iommu);
> int iommu_device_sysfs_add(struct iommu_device *iommu,
> @@ -438,6 +488,7 @@ struct iommu_ops {};
> struct iommu_group {};
> struct iommu_fwspec {};
> struct iommu_device {};
> +struct iommu_fault_param {};
>
> static inline bool iommu_present(struct bus_type *bus)
> {
> diff --git a/include/uapi/linux/iommu.h b/include/uapi/linux/iommu.h
> index f28cd9a1aa96..e9b5330a13c8 100644
> --- a/include/uapi/linux/iommu.h
> +++ b/include/uapi/linux/iommu.h
> @@ -148,4 +148,87 @@ struct iommu_guest_msi_binding {
> __u64 gpa;
> __u32 granule;
> };
> +
> +/* Generic fault types, can be expanded IRQ remapping fault */
> +enum iommu_fault_type {
> + IOMMU_FAULT_DMA_UNRECOV = 1, /* unrecoverable fault */
> + IOMMU_FAULT_PAGE_REQ, /* page request fault */
> +};
> +
> +enum iommu_fault_reason {
> + IOMMU_FAULT_REASON_UNKNOWN = 0,
> +
> + /* IOMMU internal error, no specific reason to report out */
> + IOMMU_FAULT_REASON_INTERNAL,
> +
> + /* Could not access the PASID table (fetch caused external
> abort) */
> + IOMMU_FAULT_REASON_PASID_FETCH,
> +
> + /* could not access the device context (fetch caused
> external abort) */
> + IOMMU_FAULT_REASON_DEVICE_CONTEXT_FETCH,
> +
> + /* pasid entry is invalid or has configuration errors */
> + IOMMU_FAULT_REASON_BAD_PASID_ENTRY,
> +
> + /* device context entry is invalid or has configuration
> errors */
> + IOMMU_FAULT_REASON_BAD_DEVICE_CONTEXT_ENTRY,
> + /*
> + * PASID is out of range (e.g. exceeds the maximum PASID
> + * supported by the IOMMU) or disabled.
> + */
> + IOMMU_FAULT_REASON_PASID_INVALID,
> +
> + /* source id is out of range */
> + IOMMU_FAULT_REASON_SOURCEID_INVALID,
> +
> + /*
> + * An external abort occurred fetching (or updating) a
> translation
> + * table descriptor
> + */
> + IOMMU_FAULT_REASON_WALK_EABT,
> +
> + /*
> + * Could not access the page table entry (Bad address),
> + * actual translation fault
> + */
> + IOMMU_FAULT_REASON_PTE_FETCH,
> +
> + /* Protection flag check failed */
> + IOMMU_FAULT_REASON_PERMISSION,
> +
> + /* access flag check failed */
> + IOMMU_FAULT_REASON_ACCESS,
> +
> + /* Output address of a translation stage caused Address Size
> fault */
> + IOMMU_FAULT_REASON_OOR_ADDRESS
> +};
> +
> +/**
> + * struct iommu_fault - Generic fault data
> + *
> + * @type contains fault type
> + * @reason fault reasons if relevant outside IOMMU driver.
> + * IOMMU driver internal faults are not reported.
> + * @addr: tells the offending page address
> + * @fetch_addr: tells the address that caused an abort, if any
> + * @pasid: contains process address space ID, used in shared virtual
> memory
> + * @page_req_group_id: page request group index
> + * @last_req: last request in a page request group
> + * @pasid_valid: indicates if the PRQ has a valid PASID
> + * @prot: page access protection flag:
> + * IOMMU_FAULT_READ, IOMMU_FAULT_WRITE
> + */
> +
> +struct iommu_fault {
> + __u32 type; /* enum iommu_fault_type */
> + __u32 reason; /* enum iommu_fault_reason */
> + __u64 addr;
> + __u64 fetch_addr;
> + __u32 pasid;
> + __u32 page_req_group_id;
> + __u32 last_req;
> + __u32 pasid_valid;
> + __u32 prot;
> + __u32 access;
relocated to uapi, Yi can you confirm?
__u64 device_private[2];

> +};
> #endif /* _UAPI_IOMMU_H */