Re: [PATCH RFC PREVIEW 1/6] iommu/vt-d: Separate page request queue from SVM

[Joel Granados]

On Sun, Sep 01, 2024 at 01:16:43PM +0800, Baolu Lu wrote:
> On 2024/8/26 19:40, Klaus Jensen wrote:
> > From: Joel Granados<j.granados@xxxxxxxxxxx>
> > 
> > IO page faults are no longer dependent on CONFIG_INTEL_IOMMU_SVM. Move
> > all Page Request Queue (PRQ) functions that handle prq events to a new
> > file in drivers/iommu/intel/prq.c. The page_req_des struct is made
> > available in drivers/iommu/intel/iommu.h.
> > 
> > No functional changes are intended. This is a preparation patch to
> > enable the use of IO page faults outside the SVM and nested use cases.
> > 
> > Signed-off-by: Joel Granados<j.granados@xxxxxxxxxxx>
> > ---
> >   drivers/iommu/intel/Makefile |   2 +-
> >   drivers/iommu/intel/iommu.c  |  18 +--
> >   drivers/iommu/intel/iommu.h  |  40 +++++-
> >   drivers/iommu/intel/prq.c    | 290 ++++++++++++++++++++++++++++++++++++++++
> >   drivers/iommu/intel/svm.c    | 308 -------------------------------------------
> >   5 files changed, 331 insertions(+), 327 deletions(-)
...
> > diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
> > index b67c14da1240..b3d98e706ed8 100644
> > --- a/drivers/iommu/intel/iommu.h
> > +++ b/drivers/iommu/intel/iommu.h
> > @@ -694,6 +694,35 @@ struct iommu_pmu {
> >   #define IOMMU_IRQ_ID_OFFSET_PRQ		(DMAR_UNITS_SUPPORTED)
> >   #define IOMMU_IRQ_ID_OFFSET_PERF	(2 * DMAR_UNITS_SUPPORTED)
> >   
> > +/* Page request queue descriptor */
> > +struct page_req_dsc {
> > +	union {
> > +		struct {
> > +			u64 type:8;
> > +			u64 pasid_present:1;
> > +			u64 rsvd:7;
> > +			u64 rid:16;
> > +			u64 pasid:20;
> > +			u64 exe_req:1;
> > +			u64 pm_req:1;
> > +			u64 rsvd2:10;
> > +		};
> > +		u64 qw_0;
> > +	};
> > +	union {
> > +		struct {
> > +			u64 rd_req:1;
> > +			u64 wr_req:1;
> > +			u64 lpig:1;
> > +			u64 prg_index:9;
> > +			u64 addr:52;
> > +		};
> > +		u64 qw_1;
> > +	};
> > +	u64 qw_2;
> > +	u64 qw_3;
> > +};
> 
> Why not move this structure to prq.c? It is specific to that file. Or
> not?
I had left it here because it was included in `struct intel_iommu` but
since it is just a pointer, I think it can be put there if it makes more
sense.

> 
> > +
> >   struct intel_iommu {
> >   	void __iomem	*reg; /* Pointer to hardware regs, virtual addr */
> >   	u64 		reg_phys; /* physical address of hw register set */
> > @@ -719,12 +748,10 @@ struct intel_iommu {
> >   
> >   	struct iommu_flush flush;
> >   #endif
> > -#ifdef CONFIG_INTEL_IOMMU_SVM
> >   	struct page_req_dsc *prq;
> >   	unsigned char prq_name[16];    /* Name for PRQ interrupt */
> >   	unsigned long prq_seq_number;
> >   	struct completion prq_complete;
> > -#endif
> >   	struct iopf_queue *iopf_queue;
> >   	unsigned char iopfq_name[16];
> >   	/* Synchronization between fault report and iommu device release. */
> > @@ -1156,12 +1183,13 @@ void intel_context_flush_present(struct device_domain_info *info,
> >   				 struct context_entry *context,
> >   				 bool affect_domains);
> >   
> > +int intel_enable_prq(struct intel_iommu *iommu);
> > +int intel_finish_prq(struct intel_iommu *iommu);
> > +void intel_page_response(struct device *dev, struct iopf_fault *evt,
> > +			struct iommu_page_response *msg);
> > +
> >   #ifdef CONFIG_INTEL_IOMMU_SVM
> >   void intel_svm_check(struct intel_iommu *iommu);
> > -int intel_svm_enable_prq(struct intel_iommu *iommu);
> > -int intel_svm_finish_prq(struct intel_iommu *iommu);
> > -void intel_svm_page_response(struct device *dev, struct iopf_fault *evt,
> > -			     struct iommu_page_response *msg);
> >   struct iommu_domain *intel_svm_domain_alloc(struct device *dev,
> >   					    struct mm_struct *mm);
> >   void intel_drain_pasid_prq(struct device *dev, u32 pasid);
> > diff --git a/drivers/iommu/intel/prq.c b/drivers/iommu/intel/prq.c
> > new file mode 100644
> > index 000000000000..2814373e95d8
> > --- /dev/null
> > +++ b/drivers/iommu/intel/prq.c
> > @@ -0,0 +1,290 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright © 2015 Intel Corporation.
> > + *
> > + * Authors: David Woodhouse<dwmw2@xxxxxxxxxxxxx>
> 
> Many contributors have worked on the code moved in this change. The
> original authorship is no longer relevant.
> 
> Consider adding a comment like 'Split from svm.c' to document the
> origin.
Good point. Will do that.

> > + */
> > +
> > +#include <linux/pci.h>
> > +
> > +#include "iommu.h"
> > +#include "../iommu-pages.h"
> > +#include "trace.h"
> > +
> > +static bool is_canonical_address(u64 addr)
> > +{
> > +	int shift = 64 - (__VIRTUAL_MASK_SHIFT + 1);
> > +	long saddr = (long) addr;
> > +
> > +	return (((saddr << shift) >> shift) == saddr);
> > +}
...
> > +			QI_PGRP_RESP_CODE(msg->code) |
> > +			QI_PGRP_RESP_TYPE;
> > +	desc.qw1 = QI_PGRP_IDX(prm->grpid) | QI_PGRP_LPIG(last_page);
> > +	desc.qw2 = 0;
> > +	desc.qw3 = 0;
> > +
> > +	qi_submit_sync(iommu, &desc, 1, 0);
> > +}
> 
> The intel_drain_pasid_prq() helper should be moved to prq.c. It's no
> longer specific to SVM.
Oops. missed that one. Will do.

Will address these and the rest of the comments in my V1

Thx for the review

Best

-- 

Joel Granados