Hi Jacob,
On 12/15/18 1:30 AM, Jacob Pan wrote:
> On Wed, 12 Dec 2018 09:21:43 +0100
> Auger Eric <eric.au...@redhat.com> wrote:
>
>> Hi Jacob,
>>
>> On 9/21/18 12:06 AM, Jacob Pan wrote:
>>> On Tue, 18 Sep 2018 16:24:51 +0200
>>> Eric Auger <eric.au...@redhat.com> wrote:
>>>
>>>> From: Jacob Pan <jacob.jun....@linux.intel.com>
>>>>
>>>> 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....@linux.intel.com>
>>>> Signed-off-by: Jean-Philippe Brucker
>>>> <jean-philippe.bruc...@arm.com> Signed-off-by: Liu, Yi L
>>>> <yi.l....@linux.intel.com> Signed-off-by: Ashok Raj
>>>> <ashok....@intel.com> Signed-off-by: Eric Auger
>>>> <eric.au...@redhat.com> [moved part of the iommu_fault_event
>>>> struct in the uapi, enriched the fault reasons to be able to map
>>>> unrecoverable SMMUv3 errors]
>>> Sounds good to me.
>>> There are also other "enrichment" we need to do to support mdev or
>>> finer granularity fault reporting below physical device. e.g. PASID
>>> level.
>>>
>>> The current scheme works for PCIe physical device level, where each
>>> device registers a single handler only once. When device fault is
>>> detected by the IOMMU, it will find the matching handler and private
>>> data to report back. However, for devices partitioned by PASID and
>>> represented by mdev this may not work. Since IOMMU is not mdev aware
>>> and only works at physical device level.
>>> So I am thinking we should allow multiple registration of fault
>>> handler with different data and ID. i.e.
>>>
>>> int iommu_register_device_fault_handler(struct device *dev,
>>> iommu_dev_fault_handler_t
>>> handler, int id,
>>> void *data)
>>>
>>> where the new "id field" is
>>> * @id: Identification of the handler private data, will be used by
>>> fault
>>> * reporting code to match the handler data to be returned.
>>> For page
>>> * request, this can be the PASID. ID must be unique per
>>> device, i.e.
>>> * each ID can only be registered once per device.
>>> * - IOMMU_DEV_FAULT_ID_UNRECOVERY (~0U) is reserved for fault
>>> reporting
>>> * w/o ID. e.g. unrecoverable faults.
>>>
>>> I am still testing, but just wanted to have feedback on this idea.
>>
>> I am currently respinning this series. Do you have a respin for this
>> patch including iommu_register_device_fault_handler with the @id param
>> as you suggested above? Otherwise 2 solutions: I keep the code as is
>> or I do the modification myself implementing a list of fault_params?
>>
> you can keep the code as is if it fits your current needs. Yi and I
> have thought of some new cases for supporting mdev. We are thinking to
> support many to many handler vs PASID relationship. i.e. allow
> registration of many fault handlers per device, each associated with an
> ID and data. The use case is that a physical device may register a
> fault handler for its own PASID or non-PASID related faults. Such
> physical device can also be partitioned into sub-device, e.g. mdev, but
> fault handler registration is at physical device level in that IOMMU is
> not mdev aware.
> Anyway, still need some work to flush out the details.
>> Besides do you plans for "[PATCH v5 00/23] IOMMU and VT-d driver
>> support for Shared Virtual Address (SVA)" respin - hope I didn't miss
>> anything? - ?
Thank you for your reply. OK I will see how much time I can dedicate to
this API change. At the moment I am working on the vfio-pci side and
exposing a fault region as initiated by Yi.
>>
> You did not miss anything. Yes, we are still working on some internal
> integration issues. It should not affect the common interface much. Or
> I can send out a common API spin first once we have the functionality
> tested.
No worries. I can live without at the moment
Thanks
Eric
>
> Thanks for checking.
>
>> Thanks
>>
>> Eric
>>>
>>> Thanks,
>>>
>>> Jacob
>>>
>>>
>>>> ---
>>>> 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 9bd3e63d562b..7529c14ff506 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 */ @@ -262,6 +266,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;
>>>> + 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,
>>>> @@ -429,6 +479,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 21adb2a964e5..a0fe5c2fb236
>>>> 100644 --- a/include/uapi/linux/iommu.h
>>>> +++ b/include/uapi/linux/iommu.h
>>>> @@ -150,5 +150,88 @@ 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;
>>>> +};
>>>> #endif /* _UAPI_IOMMU_H */
>>>>
>>>
>>> [Jacob Pan]
>>>
>
> [Jacob Pan]
>
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
