Re: [PATCH v3 08/16] iommu: introduce device fault data

[Jacob Pan <jacob.jun.pan@linux.intel.com>]

On Wed, 10 Jan 2018 11:41:58 +0000
Jean-Philippe Brucker <jean-philippe.brucker@arm.com> wrote:

> Hi Jacob,
> 
> On 17/11/17 18:55, Jacob Pan wrote:
> [...]
> > +/**
> > + * 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
> > +
> > + * @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
> > + * @pasid: contains process address space ID, used in shared
> > virtual memory(SVM)
> > + * @rid: requestor ID
> > + * @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, e.g. IOMMU_FAULT_READ,
> > IOMMU_FAULT_WRITE
> > + * @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 {
> > +	enum iommu_fault_type type;
> > +	enum iommu_fault_reason reason;
> > +	u64 addr;
> > +	u32 pasid;
> > +	u32 page_req_group_id : 9;  
> 
> As I've been rebasing my work onto your series, I have a few more
> comments about this structure. Is there any advantage in limiting the
> PRGI as a bitfield? PCI uses 9 bits, but others might need more. For
> instance ARM Stall uses 16-bit IDs to identify a fault event.
> 
> Could you please make it a u32 (as well as in page_response_msg), and
> could page_req_group_id be renamed to simply "id"?
> 
sure, I will make it u32 in v4 version of the patchset. I was using PCI
standard as a base with no specific advantage.
I am running into little bit problem with testing, so perhaps next week.
> > +	u32 last_req : 1;
> > +	u32 pasid_valid : 1;  
> I noticed that page_response_msg in patch 15/16 calls this bit
> "pasid_present". Could you rename it to "pasid_valid" for consistency?
> 
make sense.
> Thanks,
> Jean

[Jacob Pan]