Re: [PATCH 2/9] iommu: Add device parameter to iopf handler

[Jacob Pan]

Hi Baolu,

On Wed, 12 Jul 2023 10:16:11 +0800, Baolu Lu <baolu.lu@xxxxxxxxxxxxxxx>
wrote:

> On 2023/7/12 1:26, Jacob Pan wrote:
> > Hi BaoLu,  
> 
> Hi Jacob,
> 
> > 
> > On Tue, 11 Jul 2023 09:06:35 +0800, Lu Baolu<baolu.lu@xxxxxxxxxxxxxxx>
> > wrote:
> >   
> >> Add the device parameter to the iopf handler so that it can know which
> >> device this fault was generated.
> >>
> >> This is necessary for use cases such as delivering IO page faults to
> >> user space. The IOMMUFD layer needs to be able to lookup the device id
> >> of a fault and route it together with the fault message to the user
> >> space.
> >>
> >> Signed-off-by: Lu Baolu<baolu.lu@xxxxxxxxxxxxxxx>
> >> ---
> >>   include/linux/iommu.h      | 1 +
> >>   drivers/iommu/iommu-sva.h  | 4 ++--
> >>   drivers/iommu/io-pgfault.c | 2 +-
> >>   drivers/iommu/iommu-sva.c  | 2 +-
> >>   4 files changed, 5 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> >> index 0eb0fb852020..a00fb43b5e73 100644
> >> --- a/include/linux/iommu.h
> >> +++ b/include/linux/iommu.h
> >> @@ -249,6 +249,7 @@ struct iommu_domain {
> >>   	struct iommu_domain_geometry geometry;
> >>   	struct iommu_dma_cookie *iova_cookie;
> >>   	enum iommu_page_response_code (*iopf_handler)(struct
> >> iommu_fault *fault,
> >> +						      struct device
> >> *dev, void *data);
> >>   	void *fault_data;
> >>   	union {
> >> diff --git a/drivers/iommu/iommu-sva.h b/drivers/iommu/iommu-sva.h
> >> index 54946b5a7caf..c848661c4e20 100644
> >> --- a/drivers/iommu/iommu-sva.h
> >> +++ b/drivers/iommu/iommu-sva.h
> >> @@ -23,7 +23,7 @@ struct iopf_queue *iopf_queue_alloc(const char
> >> *name); void iopf_queue_free(struct iopf_queue *queue);
> >>   int iopf_queue_discard_partial(struct iopf_queue *queue);
> >>   enum iommu_page_response_code
> >> -iommu_sva_handle_iopf(struct iommu_fault *fault, void *data);
> >> +iommu_sva_handle_iopf(struct iommu_fault *fault, struct device *dev,
> >> void *data);
> >>   #else /* CONFIG_IOMMU_SVA */
> >>   static inline int iommu_queue_iopf(struct iommu_fault *fault, void
> >> *cookie) @@ -63,7 +63,7 @@ static inline int
> >> iopf_queue_discard_partial(struct iopf_queue *queue) }
> >>   
> >>   static inline enum iommu_page_response_code
> >> -iommu_sva_handle_iopf(struct iommu_fault *fault, void *data)
> >> +iommu_sva_handle_iopf(struct iommu_fault *fault, struct device *dev,
> >> void *data) {
> >>   	return IOMMU_PAGE_RESP_INVALID;
> >>   }
> >> diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
> >> index e5b8b9110c13..fa604e1b5c5c 100644
> >> --- a/drivers/iommu/io-pgfault.c
> >> +++ b/drivers/iommu/io-pgfault.c
> >> @@ -88,7 +88,7 @@ static void iopf_handler(struct work_struct *work)
> >>   		 * faults in the group if there is an error.
> >>   		 */
> >>   		if (status == IOMMU_PAGE_RESP_SUCCESS)
> >> -			status = domain->iopf_handler(&iopf->fault,
> >> +			status = domain->iopf_handler(&iopf->fault,
> >> group->dev, domain->fault_data);
> >>   
> >>   		if (!(iopf->fault.prm.flags &
> >> diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
> >> index 3ebd4b6586b3..14766a2b61af 100644
> >> --- a/drivers/iommu/iommu-sva.c
> >> +++ b/drivers/iommu/iommu-sva.c
> >> @@ -157,7 +157,7 @@ EXPORT_SYMBOL_GPL(iommu_sva_get_pasid);
> >>    * I/O page fault handler for SVA
> >>    */
> >>   enum iommu_page_response_code
> >> -iommu_sva_handle_iopf(struct iommu_fault *fault, void *data)
> >> +iommu_sva_handle_iopf(struct iommu_fault *fault, struct device *dev,  
> > dev has no use for sva handler, right? mark them __always_unused?  
> 
> My understanding is that __always_unused attribute in Linux kernel code
> marks a variable or function as unused. It implies that the compiler is
> free to optimize the variable or function away.
that is my understanding as well, I meant
iommu_sva_handle_iopf(struct iommu_fault *fault, struct device
__always_unused *dev,  

I tested compile w/ and w/o __always_unused, seems no difference but it
makes the code clear that dev is not used here.


Thanks,

Jacob