RE: [PATCH v5 3/9] iommu: Add attachment handle to struct iopf_group

2024-05-19 Thread Tian, Kevin 
> From: Baolu Lu 
> Sent: Sunday, May 19, 2024 10:04 PM
> 
> On 2024/5/15 15:31, Tian, Kevin wrote:
> >> From: Lu Baolu 
> >> Sent: Tuesday, April 30, 2024 10:57 PM
> >>
> >> +  handle = iommu_attach_handle_get(dev->iommu_group, pasid, 0);
> >> +  if (IS_ERR(handle))
> >> +  return PTR_ERR(handle);
> >>
> >> -  if (!domain || !domain->iopf_handler) {
> >> -  dev_warn_ratelimited(dev,
> >> -  "iopf (pasid %d) without domain attached or handler
> >> installed\n",
> >> -   fault->prm.pasid);
> >> +  group->attach_handle = handle;
> >> +  group->domain = handle->domain;
> >
> > this change also removes the warning message. Is it desired?
> 
> Not desired.
> 
> Perhaps we can add a message when the iopf handling is aborted.
> Something like below:
> 
> err_abort:
> +dev_warn_ratelimited(dev, "iopf with pasid %d aborted\n",
> + fault->prm.pasid);
>  iopf_group_response(group, IOMMU_PAGE_RESP_FAILURE);
> 
> Though I am not sure which is better dev_warn() or dev_info().
> 

yes this works. dev_warn() is fine as long as we don't expect it to
happen in sane cases.

Re: [PATCH v5 3/9] iommu: Add attachment handle to struct iopf_group

2024-05-19 Thread Baolu Lu 

On 2024/5/15 15:31, Tian, Kevin wrote:

From: Lu Baolu 
Sent: Tuesday, April 30, 2024 10:57 PM

Previously, the domain that a page fault targets is stored in an
iopf_group, which represents a minimal set of page faults. With the
introduction of attachment handle, replace the domain with the handle


It's better to use 'attach handle' as the code does.


Done.




+   handle = iommu_attach_handle_get(dev->iommu_group, pasid, 0);
+   if (IS_ERR(handle))
+   return PTR_ERR(handle);

-   if (!domain || !domain->iopf_handler) {
-   dev_warn_ratelimited(dev,
-   "iopf (pasid %d) without domain attached or handler
installed\n",
-fault->prm.pasid);
+   group->attach_handle = handle;
+   group->domain = handle->domain;


this change also removes the warning message. Is it desired?


Not desired.

Perhaps we can add a message when the iopf handling is aborted.
Something like below:

err_abort:
+dev_warn_ratelimited(dev, "iopf with pasid %d aborted\n",
+ fault->prm.pasid);
iopf_group_response(group, IOMMU_PAGE_RESP_FAILURE);

Though I am not sure which is better dev_warn() or dev_info().

Best regards,
baolu

RE: [PATCH v5 3/9] iommu: Add attachment handle to struct iopf_group

2024-05-15 Thread Tian, Kevin 
> From: Lu Baolu 
> Sent: Tuesday, April 30, 2024 10:57 PM
> 
> Previously, the domain that a page fault targets is stored in an
> iopf_group, which represents a minimal set of page faults. With the
> introduction of attachment handle, replace the domain with the handle

It's better to use 'attach handle' as the code does.

> + handle = iommu_attach_handle_get(dev->iommu_group, pasid, 0);
> + if (IS_ERR(handle))
> + return PTR_ERR(handle);
> 
> - if (!domain || !domain->iopf_handler) {
> - dev_warn_ratelimited(dev,
> - "iopf (pasid %d) without domain attached or handler
> installed\n",
> -  fault->prm.pasid);
> + group->attach_handle = handle;
> + group->domain = handle->domain;

this change also removes the warning message. Is it desired?

RE: [PATCH v5 3/9] iommu: Add attachment handle to struct iopf_group

2024-05-15 Thread Tian, Kevin 
> From: Jason Gunthorpe 
> Sent: Saturday, May 11, 2024 12:29 AM
> 
> On Fri, May 10, 2024 at 10:30:10PM +0800, Baolu Lu wrote:
> 
> > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > index 35ae9a6f73d3..09b4e671dcee 100644
> > --- a/include/linux/iommu.h
> > +++ b/include/linux/iommu.h
> > @@ -173,6 +173,8 @@ struct iommu_domain_geometry {
> >
> >  #define __IOMMU_DOMAIN_NESTED  (1U << 6)  /* User-managed address
> space
> > nested
> >   on a stage-2 translation
> > */
> > +#define __IOMMU_DOMAIN_PASID   (1U << 7)  /* User-managed domain
> for a
> > + specific PASID*/
> >
> >  #define IOMMU_DOMAIN_ALLOC_FLAGS ~__IOMMU_DOMAIN_DMA_FQ
> >  /*
> > @@ -204,6 +206,9 @@ struct iommu_domain_geometry {
> >  #define IOMMU_DOMAIN_SVA   (__IOMMU_DOMAIN_SVA)
> >  #define IOMMU_DOMAIN_PLATFORM  (__IOMMU_DOMAIN_PLATFORM)
> >  #define IOMMU_DOMAIN_NESTED(__IOMMU_DOMAIN_NESTED)
> > +#define IOMMU_DOMAIN_NESTED_PASID  \
> > +   (__IOMMU_DOMAIN_NESTED |\
> > +__IOMMU_DOMAIN_PASID)
> 
> Yeah, something like that, I don't know about naming though..
> 
> How about
> 
> DOMAIN_NESTED = Attachment to RID or PASID
> DOMAIN_NESTED_PASID_TABLE = RID and the entire PASID space
> 

this sounds clearer

Re: [PATCH v5 3/9] iommu: Add attachment handle to struct iopf_group

2024-05-10 Thread Jason Gunthorpe 
On Fri, May 10, 2024 at 10:30:10PM +0800, Baolu Lu wrote:

> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 35ae9a6f73d3..09b4e671dcee 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -173,6 +173,8 @@ struct iommu_domain_geometry {
> 
>  #define __IOMMU_DOMAIN_NESTED  (1U << 6)  /* User-managed address space
> nested
>   on a stage-2 translation
> */
> +#define __IOMMU_DOMAIN_PASID   (1U << 7)  /* User-managed domain for a
> + specific PASID*/
> 
>  #define IOMMU_DOMAIN_ALLOC_FLAGS ~__IOMMU_DOMAIN_DMA_FQ
>  /*
> @@ -204,6 +206,9 @@ struct iommu_domain_geometry {
>  #define IOMMU_DOMAIN_SVA   (__IOMMU_DOMAIN_SVA)
>  #define IOMMU_DOMAIN_PLATFORM  (__IOMMU_DOMAIN_PLATFORM)
>  #define IOMMU_DOMAIN_NESTED(__IOMMU_DOMAIN_NESTED)
> +#define IOMMU_DOMAIN_NESTED_PASID  \
> +   (__IOMMU_DOMAIN_NESTED |\
> +__IOMMU_DOMAIN_PASID)

Yeah, something like that, I don't know about naming though..

How about

DOMAIN_NESTED = Attachment to RID or PASID
DOMAIN_NESTED_PASID_TABLE = RID and the entire PASID space

?

Jason

Re: [PATCH v5 3/9] iommu: Add attachment handle to struct iopf_group

2024-05-10 Thread Baolu Lu 

On 2024/5/10 21:38, Jason Gunthorpe wrote:

On Fri, May 10, 2024 at 11:14:20AM +0800, Baolu Lu wrote:

On 5/8/24 8:04 AM, Jason Gunthorpe wrote:

On Tue, Apr 30, 2024 at 10:57:04PM +0800, Lu Baolu wrote:

@@ -206,8 +197,11 @@ void iommu_report_device_fault(struct device *dev, struct 
iopf_fault *evt)
if (group == _group)
goto err_abort;
-   group->domain = get_domain_for_iopf(dev, fault);
-   if (!group->domain)
+   if (!(fault->prm.flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID) ||
+   get_attach_handle_for_iopf(dev, fault->prm.pasid, group))
+   get_attach_handle_for_iopf(dev, IOMMU_NO_PASID, group);

That seems a bit weird looking?

Agreed.


get_attach_handle_for_iopf(dev,
 (fault->prm.flags &
 IOMMU_FAULT_PAGE_REQUEST_PASID_VALID) ? fault->prm.pasid : IOMMU_NO_PASID,
 group);

The logic here is that it tries the PASID domain and if it doesn't
exist, then tries the RID domain as well. I explained this in the commit
message:

"
... if the pasid table of a device is wholly managed by user space,
there is no domain attached to the PASID of the device ...
"

Okay, it needs a comment in the code, and the RID fallback should be
based aroudn checking for a NESTING domain type which includes the
PASID table. (ie ARM and AMD not Intel)

It appears that Intel is just special. ARM, AMD, and RISC-V all support
a NESTING domain which includes the PASID table. So, how about defining
a special NESTING domain type for Intel? Like this,

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 35ae9a6f73d3..09b4e671dcee 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -173,6 +173,8 @@ struct iommu_domain_geometry {

 #define __IOMMU_DOMAIN_NESTED  (1U << 6)  /* User-managed address 
space nested
  on a stage-2 translation 
  */

+#define __IOMMU_DOMAIN_PASID   (1U << 7)  /* User-managed domain for a
+ specific PASID*/

 #define IOMMU_DOMAIN_ALLOC_FLAGS ~__IOMMU_DOMAIN_DMA_FQ
 /*
@@ -204,6 +206,9 @@ struct iommu_domain_geometry {
 #define IOMMU_DOMAIN_SVA   (__IOMMU_DOMAIN_SVA)
 #define IOMMU_DOMAIN_PLATFORM  (__IOMMU_DOMAIN_PLATFORM)
 #define IOMMU_DOMAIN_NESTED(__IOMMU_DOMAIN_NESTED)
+#define IOMMU_DOMAIN_NESTED_PASID  \
+   (__IOMMU_DOMAIN_NESTED |\
+__IOMMU_DOMAIN_PASID)

And current IOMMU_DOMAIN_NESTED domain type is just for a nesting domain
with PASID table.

The Intel IOMMU driver will convert to use IOMMU_DOMAIN_NESTED_PASID in
the PASID interface series.


We shouldn't just elevate a random PASID to the RID if it isn't
approprite..


If above propose looks good to you, then I just need to add a domain
type check before RID fallback.

Best regards,
baolu

Re: [PATCH v5 3/9] iommu: Add attachment handle to struct iopf_group

2024-05-10 Thread Jason Gunthorpe 
On Fri, May 10, 2024 at 11:14:20AM +0800, Baolu Lu wrote:
> On 5/8/24 8:04 AM, Jason Gunthorpe wrote:
> > On Tue, Apr 30, 2024 at 10:57:04PM +0800, Lu Baolu wrote:
> > > @@ -206,8 +197,11 @@ void iommu_report_device_fault(struct device *dev, 
> > > struct iopf_fault *evt)
> > >   if (group == _group)
> > >   goto err_abort;
> > > - group->domain = get_domain_for_iopf(dev, fault);
> > > - if (!group->domain)
> > > + if (!(fault->prm.flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID) ||
> > > + get_attach_handle_for_iopf(dev, fault->prm.pasid, group))
> > > + get_attach_handle_for_iopf(dev, IOMMU_NO_PASID, group);
> > That seems a bit weird looking?
> 
> Agreed.
> 
> > get_attach_handle_for_iopf(dev,
> > (fault->prm.flags &
> > IOMMU_FAULT_PAGE_REQUEST_PASID_VALID) ? fault->prm.pasid : 
> > IOMMU_NO_PASID,
> > group);
> 
> The logic here is that it tries the PASID domain and if it doesn't
> exist, then tries the RID domain as well. I explained this in the commit
> message:
> 
> "
> ... if the pasid table of a device is wholly managed by user space,
> there is no domain attached to the PASID of the device ...
> "

Okay, it needs a comment in the code, and the RID fallback should be
based aroudn checking for a NESTING domain type which includes the
PASID table. (ie ARM and AMD not Intel)

We shouldn't just elevate a random PASID to the RID if it isn't
approprite..

Jason

Re: [PATCH v5 3/9] iommu: Add attachment handle to struct iopf_group

2024-05-09 Thread Baolu Lu 

On 5/8/24 8:04 AM, Jason Gunthorpe wrote:

On Tue, Apr 30, 2024 at 10:57:04PM +0800, Lu Baolu wrote:

@@ -206,8 +197,11 @@ void iommu_report_device_fault(struct device *dev, struct 
iopf_fault *evt)
if (group == _group)
goto err_abort;
  
-	group->domain = get_domain_for_iopf(dev, fault);

-   if (!group->domain)
+   if (!(fault->prm.flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID) ||
+   get_attach_handle_for_iopf(dev, fault->prm.pasid, group))
+   get_attach_handle_for_iopf(dev, IOMMU_NO_PASID, group);

That seems a bit weird looking?


Agreed.


get_attach_handle_for_iopf(dev,
(fault->prm.flags &
IOMMU_FAULT_PAGE_REQUEST_PASID_VALID) ? fault->prm.pasid : IOMMU_NO_PASID,
group);


The logic here is that it tries the PASID domain and if it doesn't
exist, then tries the RID domain as well. I explained this in the commit
message:

"
... if the pasid table of a device is wholly managed by user space,
there is no domain attached to the PASID of the device ...
"

Perhaps I can improve it like this,

int rc = -EINVAL;
...
if (fault->prm.flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID)
rc = get_attach_handle_for_iopf(dev, fault->prm.pasid, group);
if (rc)
rc = get_attach_handle_for_iopf(dev, IOMMU_NO_PASID, group);

if (rc || !group->attach_handle->domain->iopf_handler)
goto err_abort;

Best regards,
baolu

Re: [PATCH v5 3/9] iommu: Add attachment handle to struct iopf_group

2024-05-07 Thread Jason Gunthorpe 
On Tue, Apr 30, 2024 at 10:57:04PM +0800, Lu Baolu wrote:
> @@ -206,8 +197,11 @@ void iommu_report_device_fault(struct device *dev, 
> struct iopf_fault *evt)
>   if (group == _group)
>   goto err_abort;
>  
> - group->domain = get_domain_for_iopf(dev, fault);
> - if (!group->domain)
> + if (!(fault->prm.flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID) ||
> + get_attach_handle_for_iopf(dev, fault->prm.pasid, group))
> + get_attach_handle_for_iopf(dev, IOMMU_NO_PASID, group);

That seems a bit weird looking?

get_attach_handle_for_iopf(dev, 
   (fault->prm.flags &
   IOMMU_FAULT_PAGE_REQUEST_PASID_VALID) ? fault->prm.pasid : IOMMU_NO_PASID,
   group);

Jason