LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Jean-Philippe Brucker <Jean-Philippe.Brucker@arm.com>
To: Jacob Pan <jacob.jun.pan@linux.intel.com>
Cc: "iommu@lists.linux-foundation.org"
	<iommu@lists.linux-foundation.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Joerg Roedel <joro@8bytes.org>,
	David Woodhouse <dwmw2@infradead.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Alex Williamson <alex.williamson@redhat.com>,
	Rafael Wysocki <rafael.j.wysocki@intel.com>,
	"Liu, Yi L" <yi.l.liu@intel.com>,
	"Tian, Kevin" <kevin.tian@intel.com>,
	Raj Ashok <ashok.raj@intel.com>,
	Jean Delvare <khali@linux-fr.org>,
	Christoph Hellwig <hch@infradead.org>,
	Lu Baolu <baolu.lu@linux.intel.com>
Subject: Re: [PATCH v4 13/22] iommu: introduce page response function
Date: Mon, 23 Apr 2018 12:47:10 +0100	[thread overview]
Message-ID: <AM4PR0802MB23698019B80CCE6B083D1448A3890@AM4PR0802MB2369.eurprd08.prod.outlook.com> (raw)
In-Reply-To: <1523915351-54415-14-git-send-email-jacob.jun.pan@linux.intel.com>

On Mon, Apr 16, 2018 at 10:49:02PM +0100, Jacob Pan wrote:
[...]
> +	/*
> +	 * Check if we have a matching page request pending to respond,
> +	 * otherwise return -EINVAL
> +	 */
> +	list_for_each_entry_safe(evt, iter, &param->fault_param->faults, list) {

I don't think you need the "_safe" iterator if you're exiting the loop
right after removing the event.

> +		if (evt->pasid == msg->pasid &&
> +		    msg->page_req_group_id == evt->page_req_group_id) {
> +			msg->private_data = evt->iommu_private;

Ah sorry, I missed this bit in my review of 10/22. I thought
private_data would be for evt->device_private. In this case I guess we
can drop device_private, or do you plan to use it?

> +			ret = domain->ops->page_response(dev, msg);
> +			list_del(&evt->list);
> +			kfree(evt);
> +			break;
> +		}
> +	}
> +
> +done_unlock:
> +	mutex_unlock(&param->fault_param->lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(iommu_page_response);
> +
>  static void __iommu_detach_device(struct iommu_domain *domain,
>  				  struct device *dev)
>  {
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 32435f9..058b552 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -163,6 +163,55 @@ struct iommu_resv_region {
>  #ifdef CONFIG_IOMMU_API
>  
>  /**
> + * enum page_response_code - Return status of fault handlers, telling the IOMMU
> + * driver how to proceed with the fault.
> + *
> + * @IOMMU_FAULT_STATUS_SUCCESS: Fault has been handled and the page tables
> + *	populated, retry the access. This is "Success" in PCI PRI.
> + * @IOMMU_FAULT_STATUS_FAILURE: General error. Drop all subsequent faults from
> + *	this device if possible. This is "Response Failure" in PCI PRI.
> + * @IOMMU_FAULT_STATUS_INVALID: Could not handle this fault, don't retry the
> + *	access. This is "Invalid Request" in PCI PRI.
> + */
> +enum page_response_code {
> +	IOMMU_PAGE_RESP_SUCCESS = 0,
> +	IOMMU_PAGE_RESP_INVALID,
> +	IOMMU_PAGE_RESP_FAILURE,
> +};

Field names aren't consistent with the comment. I'd go with
IOMMU_PAGE_RESP_* 

> +
> +/**
> + * enum page_request_handle_t - Return page request/response handler status
> + *
> + * @IOMMU_FAULT_STATUS_HANDLED: Stop processing the fault, and do not send a
> + *	reply to the device.
> + * @IOMMU_FAULT_STATUS_CONTINUE: Fault was not handled. Call the next handler,
> + *	or terminate.
> + */
> +enum page_request_handle_t {
> +	IOMMU_PAGE_RESP_HANDLED = 0,
> +	IOMMU_PAGE_RESP_CONTINUE,

Same here regarding the comment. Here I'd prefer "iommu_fault_status_t"
for the enum and IOMMU_FAULT_STATUS_* for the fields, because they can
be used for unrecoverable faults as well.

But since you're not using these values in your patches, I guess you can
drop this enum? At the moment the return value of fault handler is 0 (as
specified at iommu_register_device_fault_handler), meaning that the
handler always takes ownership of the fault.

It will be easy to extend once we introduce multiple fault handlers that
can either take the fault or pass it to the next one. Existing
implementations will still return 0 - HANDLED, and new ones will return
either HANDLED or CONTINUE.

> +/**
> + * Generic page response information based on PCI ATS and PASID spec.
> + * @addr: servicing page address
> + * @pasid: contains process address space ID
> + * @resp_code: response code
> + * @page_req_group_id: page request group index
> + * @type: group or stream/single page response

@type isn't in the structure

> + * @private_data: uniquely identify device-specific private data for an
> + *                individual page response

IOMMU-specific? If it is set by iommu.c, I think we should comment about
it, something like "This field is written by the IOMMU core". Maybe also
rename it to iommu_private to be consistent with iommu_fault_event

> + */
> +struct page_response_msg {
> +	u64 addr;
> +	u32 pasid;
> +	enum page_response_code resp_code;
> +	u32 pasid_present:1;
> +	u32 page_req_group_id;
> +	u64 private_data;
> +};
> +
> +/**
>   * struct iommu_ops - iommu ops and capabilities
>   * @capable: check capability
>   * @domain_alloc: allocate iommu domain
> @@ -195,6 +244,7 @@ struct iommu_resv_region {
>   * @bind_pasid_table: bind pasid table pointer for guest SVM
>   * @unbind_pasid_table: unbind pasid table pointer and restore defaults
>   * @sva_invalidate: invalidate translation caches of shared virtual address
> + * @page_response: handle page request response
>   */
>  struct iommu_ops {
>  	bool (*capable)(enum iommu_cap);
> @@ -250,6 +300,7 @@ struct iommu_ops {
>  				struct device *dev);
>  	int (*sva_invalidate)(struct iommu_domain *domain,
>  		struct device *dev, struct tlb_invalidate_info *inv_info);
> +	int (*page_response)(struct device *dev, struct page_response_msg *msg);
>  
>  	unsigned long pgsize_bitmap;
>  };
> @@ -471,6 +522,7 @@ extern int iommu_unregister_device_fault_handler(struct device *dev);
>  
>  extern int iommu_report_device_fault(struct device *dev, struct iommu_fault_event *evt);
>  
> +extern int iommu_page_response(struct device *dev, struct page_response_msg *msg);

Please also define a -ENODEV function for !CONFIG_IOMMU_API, otherwise
it doesn't build.

And I think struct page_response_msg and the enums should be declared
outside #ifdef CONFIG_IOMMU_API. Same for struct iommu_fault_event and
the enums in patch 10/22. Otherwise device drivers will have to add
#ifdefs everywhere their code accesses these structures.

Thanks,
Jean

>  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 *);
> -- 
> 2.7.4
> 

  reply	other threads:[~2018-04-23 11:47 UTC|newest]

Thread overview: 69+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-16 21:48 [PATCH v4 00/22] IOMMU and VT-d driver support for Shared Virtual Address (SVA) Jacob Pan
2018-04-16 21:48 ` [PATCH v4 01/22] iommu: introduce bind_pasid_table API function Jacob Pan
2018-04-16 21:48 ` [PATCH v4 02/22] iommu/vt-d: move device_domain_info to header Jacob Pan
2018-04-16 21:48 ` [PATCH v4 03/22] iommu/vt-d: add a flag for pasid table bound status Jacob Pan
2018-04-16 21:48 ` [PATCH v4 04/22] iommu/vt-d: add bind_pasid_table function Jacob Pan
2018-04-17 19:10   ` Alex Williamson
2018-04-20 18:25     ` Jean-Philippe Brucker
2018-04-20 23:42       ` Jacob Pan
2018-05-29 20:09         ` Alex Williamson
2018-05-30  1:41           ` Tian, Kevin
2018-05-30  3:17             ` Alex Williamson
2018-05-30  3:45               ` Tian, Kevin
2018-05-30 11:53                 ` Jean-Philippe Brucker
2018-05-30 19:52                   ` Jacob Pan
2018-05-31  9:09                     ` Jean-Philippe Brucker
2018-06-05 17:32                       ` Jacob Pan
2018-06-06 11:20                         ` Jean-Philippe Brucker
2018-06-06 21:22                           ` Jacob Pan
2018-06-07 13:21                             ` Jean-Philippe Brucker
2018-04-20 23:22     ` Jacob Pan
2018-04-16 21:48 ` [PATCH v4 05/22] iommu: introduce iommu invalidate API function Jacob Pan
2018-04-20 18:19   ` Jean-Philippe Brucker
2018-04-23 20:43     ` Jacob Pan
2018-04-27 18:07       ` Jean-Philippe Brucker
2018-04-28  2:41         ` Tian, Kevin
2018-05-01 22:58         ` Jacob Pan
2018-05-02  9:31           ` Jean-Philippe Brucker
2018-05-04  4:46             ` Jacob Pan
2018-05-04 18:07               ` Jacob Pan
2018-05-08 10:35                 ` Jean-Philippe Brucker
2018-05-09 12:55                   ` Jacob Pan
2018-05-05 22:19   ` Jerry Snitselaar
2018-05-07 15:41     ` Jacob Pan
2018-04-16 21:48 ` [PATCH v4 06/22] iommu/vt-d: add definitions for PFSID Jacob Pan
2018-04-16 21:48 ` [PATCH v4 07/22] iommu/vt-d: fix dev iotlb pfsid use Jacob Pan
2018-04-16 21:48 ` [PATCH v4 08/22] iommu/vt-d: support flushing more translation cache types Jacob Pan
2018-04-16 21:48 ` [PATCH v4 09/22] iommu/vt-d: add svm/sva invalidate function Jacob Pan
2018-04-17 19:10   ` Alex Williamson
2018-04-20 22:36     ` Jacob Pan
2018-04-16 21:48 ` [PATCH v4 10/22] iommu: introduce device fault data Jacob Pan
2018-04-23 10:11   ` Jean-Philippe Brucker
2018-04-23 11:54     ` Jacob Pan
2018-05-20  8:17   ` Liu, Yi L
2018-05-21 23:16     ` Jacob Pan
2018-04-16 21:49 ` [PATCH v4 11/22] driver core: add per device iommu param Jacob Pan
2018-04-23 10:26   ` Greg Kroah-Hartman
2018-04-16 21:49 ` [PATCH v4 12/22] iommu: introduce device fault report API Jacob Pan
2018-04-23 11:30   ` Jean-Philippe Brucker
2018-04-24 18:29     ` Jacob Pan
2018-04-30 16:53   ` Jean-Philippe Brucker
2018-04-30 18:54     ` Jacob Pan
2018-04-16 21:49 ` [PATCH v4 13/22] iommu: introduce page response function Jacob Pan
2018-04-23 11:47   ` Jean-Philippe Brucker [this message]
2018-04-23 12:16     ` Jacob Pan
2018-04-23 15:50       ` Jean-Philippe Brucker
2018-04-16 21:49 ` [PATCH v4 14/22] iommu: handle page response timeout Jacob Pan
2018-04-23 15:36   ` Jean-Philippe Brucker
2018-04-25 15:37     ` Jacob Pan
2018-04-30 10:58       ` Jean-Philippe Brucker
2018-04-30 17:54         ` Jacob Pan
2018-04-16 21:49 ` [PATCH v4 15/22] iommu/config: add build dependency for dmar Jacob Pan
2018-04-16 21:49 ` [PATCH v4 16/22] iommu/vt-d: report non-recoverable faults to device Jacob Pan
2018-04-16 21:49 ` [PATCH v4 17/22] iommu/intel-svm: report device page request Jacob Pan
2018-04-16 21:49 ` [PATCH v4 18/22] iommu/intel-svm: replace dev ops with fault report API Jacob Pan
2018-04-16 21:49 ` [PATCH v4 19/22] iommu/intel-svm: do not flush iotlb for viommu Jacob Pan
2018-04-16 21:49 ` [PATCH v4 20/22] iommu/vt-d: add intel iommu page response function Jacob Pan
2018-04-16 21:49 ` [PATCH v4 21/22] trace/iommu: add sva trace events Jacob Pan
2018-04-16 21:49 ` [PATCH v4 22/22] iommu: use sva invalidate and device fault trace event Jacob Pan
  -- strict thread matches above, loose matches on Subject: below --
2018-03-23  3:11 [PATCH v4 00/22] IOMMU and VT-d driver support for Shared Virtual Address (SVA) Jacob Pan
2018-03-23  3:12 ` [PATCH v4 13/22] iommu: introduce page response function Jacob Pan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=AM4PR0802MB23698019B80CCE6B083D1448A3890@AM4PR0802MB2369.eurprd08.prod.outlook.com \
    --to=jean-philippe.brucker@arm.com \
    --cc=alex.williamson@redhat.com \
    --cc=ashok.raj@intel.com \
    --cc=baolu.lu@linux.intel.com \
    --cc=dwmw2@infradead.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=hch@infradead.org \
    --cc=iommu@lists.linux-foundation.org \
    --cc=jacob.jun.pan@linux.intel.com \
    --cc=joro@8bytes.org \
    --cc=kevin.tian@intel.com \
    --cc=khali@linux-fr.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=rafael.j.wysocki@intel.com \
    --cc=yi.l.liu@intel.com \
    --subject='Re: [PATCH v4 13/22] iommu: introduce page response function' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).