Re: [PATCH v4 10/22] iommu: introduce device fault data

[Jacob Pan]

On Sun, 20 May 2018 08:17:43 +
"Liu, Yi L"  wrote:

> Hi Jacob,
> 
> > From: Jacob Pan [mailto:jacob.jun@linux.intel.com]
> > Sent: Tuesday, April 17, 2018 5:49 AM
> >  include/linux/iommu.h | 102
> > +-
> >  1 file changed, 100 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > index e963dbd..8968933 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*/ @@ -264,6 +268,99 @@ struct iommu_device {
> > struct device *dev;
> >  };
> > 
> > +/*  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 */
> > +   IOMMU_FAULT_REASON_PASID_FETCH,
> > +
> > +   /*
> > +* PASID is out of range (e.g. exceeds the maximum PASID
> > +* supported by the IOMMU) or disabled.
> > +*/
> > +   IOMMU_FAULT_REASON_PASID_INVALID,
> > +
> > +   /* Could not access the page directory (Invalid PASID
> > entry) */
> > +   IOMMU_FAULT_REASON_PGD_FETCH,
> > +
> > +   /* Could not access the page table entry (Bad address) */
> > +   IOMMU_FAULT_REASON_PTE_FETCH,
> > +
> > +   /* Protection flag check failed */
> > +   IOMMU_FAULT_REASON_PERMISSION,
> > +};
> > +
> > +/**
> > + * 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;
> > +   u32 last_req : 1;
> > +   u32 pasid_valid : 1;
> > +   u32 prot;  
> 
> I think userspace also needs to know the fault type, reason, pasid,
> addr, goup_id, prot. So the definition should be included in
> uapi/Linux/iommu.h.
> 
> This comment also applies to "struct page_response_msg". Qemu also
> wants to pass the page response to host.
> 
sounds good. i assume vfio layer would reuse these data.
> > +   u64 device_private;
> > +   u64 iommu_private;  
> 
> These two seems to be in kernel driver specific data. May split the
> iommu_fault_event definition into two parts. One part for the data
> required by both driver and userspace. One part for in kernel driver
> specific data.
> 
even device and iommu private data can be potentially consumed by the
guest kernel for some special processing. But for now, we just copy
back to the response message. e.g. vt-d streaming page response
request is embedded in the iommu private data.

> Thanks,
> Yi Liu
> 

[Jacob Pan]
___
iommu mailing list
iommu@lists.linux-foundation.org

RE: [PATCH v4 10/22] iommu: introduce device fault data

[Liu, Yi L]

Hi Jacob,

> From: Jacob Pan [mailto:jacob.jun@linux.intel.com]
> Sent: Tuesday, April 17, 2018 5:49 AM
>  include/linux/iommu.h | 102
> +-
>  1 file changed, 100 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index e963dbd..8968933 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_WRITE0x1
> +#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*/
> @@ -264,6 +268,99 @@ struct iommu_device {
>   struct device *dev;
>  };
> 
> +/*  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 */
> + IOMMU_FAULT_REASON_PASID_FETCH,
> +
> + /*
> +  * PASID is out of range (e.g. exceeds the maximum PASID
> +  * supported by the IOMMU) or disabled.
> +  */
> + IOMMU_FAULT_REASON_PASID_INVALID,
> +
> + /* Could not access the page directory (Invalid PASID entry) */
> + IOMMU_FAULT_REASON_PGD_FETCH,
> +
> + /* Could not access the page table entry (Bad address) */
> + IOMMU_FAULT_REASON_PTE_FETCH,
> +
> + /* Protection flag check failed */
> + IOMMU_FAULT_REASON_PERMISSION,
> +};
> +
> +/**
> + * 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;
> + u32 last_req : 1;
> + u32 pasid_valid : 1;
> + u32 prot;

I think userspace also needs to know the fault type, reason, pasid, addr, 
goup_id,
prot. So the definition should be included in uapi/Linux/iommu.h.

This comment also applies to "struct page_response_msg". Qemu also wants to
pass the page response to host.

> + u64 device_private;
> + u64 iommu_private;

These two seems to be in kernel driver specific data. May split the 
iommu_fault_event
definition into two parts. One part for the data required by both driver and 
userspace.
One part for in kernel driver specific data.

Thanks,
Yi Liu

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 10/22] iommu: introduce device fault data

[Jacob Pan]

On Mon, 23 Apr 2018 11:11:41 +0100
Jean-Philippe Brucker  wrote:

> On Mon, Apr 16, 2018 at 10:48:59PM +0100, 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  
> 
> You can remove @rid from the comment
> 
thanks, will do.
> > + * @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.  
> 
> In my opinion you can remove @iommu_private entirely. I proposed this
> field so that the IOMMU driver can store fault metadata when reporting
> them, and read them back when completing the fault. I'm not using it
> in SMMUv3 anymore (instead re-fetching the metadata) and it can't be
> used anyway, because the value isn't copied into page_response_msg.
> 
In vt-d use, I use private data for preserving vt-d specific fault data
across request and response. e.g. vt-d has streaming response type in
addition to group response (standard). This way, generic code does not
need to know about it.

At device level, since we have to sanitize page response based on
pending page requests, I am doing the private data copy in iommu.c, in
the pending event list. 
> Thanks,
> Jean
> 
> > + */
> > +struct iommu_fault_event {
> > +   enum iommu_fault_type type;
> > +   enum iommu_fault_reason reason;
> > +   u64 addr;
> > +   u32 pasid;
> > +   u32 page_req_group_id;
> > +   u32 last_req : 1;
> > +   u32 pasid_valid : 1;
> > +   u32 prot;
> > +   u64 device_private;
> > +   u64 iommu_private;
> > +};  

[Jacob Pan]
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 10/22] iommu: introduce device fault data

[Jean-Philippe Brucker]

On Mon, Apr 16, 2018 at 10:48:59PM +0100, 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

You can remove @rid from the comment

> + * @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.

In my opinion you can remove @iommu_private entirely. I proposed this
field so that the IOMMU driver can store fault metadata when reporting
them, and read them back when completing the fault. I'm not using it in
SMMUv3 anymore (instead re-fetching the metadata) and it can't be used
anyway, because the value isn't copied into page_response_msg.

Thanks,
Jean

> + */
> +struct iommu_fault_event {
> + enum iommu_fault_type type;
> + enum iommu_fault_reason reason;
> + u64 addr;
> + u32 pasid;
> + u32 page_req_group_id;
> + u32 last_req : 1;
> + u32 pasid_valid : 1;
> + u32 prot;
> + u64 device_private;
> + u64 iommu_private;
> +};
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu