Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1740655 > unrolled thread

Re: [RFC] iommu: arm-smmu: stall support

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC] iommu: arm-smmu: stall support Joerg Roedel <joro@8bytes.org> - 2017-09-27 14:20 +0200
    Re: [RFC] iommu: arm-smmu: stall support Jean-Philippe Brucker <jean-philippe.brucker@arm.com> - 2017-09-27 15:50 +0200
      Re: [RFC] iommu: arm-smmu: stall support Joerg Roedel <joro@8bytes.org> - 2017-09-27 16:40 +0200
      Re: [RFC] iommu: arm-smmu: stall support Rob Clark <robdclark@gmail.com> - 2017-09-27 18:20 +0200

#1740655 — Re: [RFC] iommu: arm-smmu: stall support

Hi Rob, Jean,

On Fri, Sep 22, 2017 at 02:42:44PM -0400, Rob Clark wrote:
> I'm in favour if splitting the reporting *somehow*.. the two
> approaches that seemed sane are:
> 
> 1) call fault handler from irq and having separate domain->resume()
> called by the driver, potentially from a wq
> 2) or having two fault callbacks, first called before wq and then
> based on returned value, optionally 2nd callback called from wq
> 
> The first seemed less intrusive to me, but I'm flexible.

How about adding a flag to the fault-handler call-back that tells us
whether it wants to sleep or not. If it wants, we call it from a wq, if
not we call call it directly like we do today in the
report_iommu_fault() function.

In any case we call iommu_ops->resume() when set on completion of the
fault-handler either from the workqueue or report_iommu_fault itself.


Regards,

	Joerg

[toc] | [next] | [standalone]

#1740736

Hi Joerg,

On 27/09/17 13:15, Joerg Roedel wrote:
> Hi Rob, Jean,
> 
> On Fri, Sep 22, 2017 at 02:42:44PM -0400, Rob Clark wrote:
>> I'm in favour if splitting the reporting *somehow*.. the two
>> approaches that seemed sane are:
>> 
>> 1) call fault handler from irq and having separate domain->resume()
>> called by the driver, potentially from a wq
>> 2) or having two fault callbacks, first called before wq and then
>> based on returned value, optionally 2nd callback called from wq
>> 
>> The first seemed less intrusive to me, but I'm flexible.
> 
> How about adding a flag to the fault-handler call-back that tells us
> whether it wants to sleep or not. If it wants, we call it from a wq, if
> not we call call it directly like we do today in the
> report_iommu_fault() function.
> 
> In any case we call iommu_ops->resume() when set on completion of the
> fault-handler either from the workqueue or report_iommu_fault itself.

I like this approach. When the device driver registers a fault handler,
it also tells when it would like to be called (either in atomic context,
blocking context, or both).

Then the handler itself receives a flag that says which context it's
being called from. It returns a value telling the IOMMU how to proceed.
Depending on this value we either resume/abort immediately, or add the
fault to the workqueue if necessary.

How about using the following return values:

/**
 * enum iommu_fault_status - Return status of fault handlers, telling the IOMMU
 *      driver how to proceed with the fault.
 *
 * @IOMMU_FAULT_STATUS_NONE: Fault was not handled. Call the next handler, or
 *      terminate.
 * @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.
 * @IOMMU_FAULT_STATUS_HANDLED: Fault has been handled and the page tables
 *      populated, retry the access.
 * @IOMMU_FAULT_STATUS_IGNORE: Stop processing the fault, and do not send a
 *      reply to the device.
 *
 * For unrecoverable faults, the only valid status is IOMMU_FAULT_STATUS_NONE
 * For a recoverable fault, if no one handled the fault, treat as
 * IOMMU_FAULT_STATUS_INVALID.
 */
enum iommu_fault_status {
        IOMMU_FAULT_STATUS_NONE = 0,
        IOMMU_FAULT_STATUS_FAILURE,
        IOMMU_FAULT_STATUS_INVALID,
        IOMMU_FAULT_STATUS_HANDLED,
        IOMMU_FAULT_STATUS_IGNORE,
};

This would probably cover the two use-cases of reporting faults to
device drivers, and injecting them into the guest with VFIO, as well as
handling PPRs internally. I'm also working on providing more details
(pasid for instance) in the fault callback.

We could also use the fault handler for invalid PRI Page Requests
(currently specialized by amd_iommu_set_invalid_ppr_cb). It's just a
matter of adding a registration flag to iommu_set_fault_handler.

Thanks,
Jean

[toc] | [prev] | [next] | [standalone]

#1740791

Hi Jean,

On Wed, Sep 27, 2017 at 02:49:00PM +0100, Jean-Philippe Brucker wrote:
> I like this approach. When the device driver registers a fault handler,
> it also tells when it would like to be called (either in atomic context,
> blocking context, or both).

Is there a use-case for calling the same handler from both contexts?

> enum iommu_fault_status {
>         IOMMU_FAULT_STATUS_NONE = 0,
>         IOMMU_FAULT_STATUS_FAILURE,
>         IOMMU_FAULT_STATUS_INVALID,
>         IOMMU_FAULT_STATUS_HANDLED,
>         IOMMU_FAULT_STATUS_IGNORE,
> };


This all certainly makes sense for the PRI/PASID case, but I don't think
that it makes sense yet to extend the existing report_iommu_fault()
interface to also handle PASID/PPR faults.

The later needs a lot more parameters to successfully handle a fault. In
the AMD driver these are all in 'struct fault', the relevant members
are:

        u64 address;
        u16 devid;
        u16 pasid;
        u16 tag;
        u16 finish;
        u16 flags;

And passing all this through the existing interface which also handles
non-pasid faults is cumbersome. So I'd like to keep the PASID/PPR
interface separate from the old one for now.

Regards,

	Joerg

[toc] | [prev] | [next] | [standalone]

#1740860

On Wed, Sep 27, 2017 at 9:49 AM, Jean-Philippe Brucker
<jean-philippe.brucker@arm.com> wrote:
> Hi Joerg,
>
> On 27/09/17 13:15, Joerg Roedel wrote:
>> Hi Rob, Jean,
>>
>> On Fri, Sep 22, 2017 at 02:42:44PM -0400, Rob Clark wrote:
>>> I'm in favour if splitting the reporting *somehow*.. the two
>>> approaches that seemed sane are:
>>>
>>> 1) call fault handler from irq and having separate domain->resume()
>>> called by the driver, potentially from a wq
>>> 2) or having two fault callbacks, first called before wq and then
>>> based on returned value, optionally 2nd callback called from wq
>>>
>>> The first seemed less intrusive to me, but I'm flexible.
>>
>> How about adding a flag to the fault-handler call-back that tells us
>> whether it wants to sleep or not. If it wants, we call it from a wq, if
>> not we call call it directly like we do today in the
>> report_iommu_fault() function.
>>
>> In any case we call iommu_ops->resume() when set on completion of the
>> fault-handler either from the workqueue or report_iommu_fault itself.
>
> I like this approach. When the device driver registers a fault handler,
> it also tells when it would like to be called (either in atomic context,
> blocking context, or both).

What I have in mind is still a case-by-case decision.  Ie. I'd decide
if it is the first fault from a particular submit (job), in which case
I'd want to schedule the wq, or if it is one of the 999 following
faults from the same submit (in which case, skip the wq).

So a static decision when registering the fault handler doesn't work.

BR,
-R


> Then the handler itself receives a flag that says which context it's
> being called from. It returns a value telling the IOMMU how to proceed.
> Depending on this value we either resume/abort immediately, or add the
> fault to the workqueue if necessary.
>
> How about using the following return values:
>
> /**
>  * enum iommu_fault_status - Return status of fault handlers, telling the IOMMU
>  *      driver how to proceed with the fault.
>  *
>  * @IOMMU_FAULT_STATUS_NONE: Fault was not handled. Call the next handler, or
>  *      terminate.
>  * @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.
>  * @IOMMU_FAULT_STATUS_HANDLED: Fault has been handled and the page tables
>  *      populated, retry the access.
>  * @IOMMU_FAULT_STATUS_IGNORE: Stop processing the fault, and do not send a
>  *      reply to the device.
>  *
>  * For unrecoverable faults, the only valid status is IOMMU_FAULT_STATUS_NONE
>  * For a recoverable fault, if no one handled the fault, treat as
>  * IOMMU_FAULT_STATUS_INVALID.
>  */
> enum iommu_fault_status {
>         IOMMU_FAULT_STATUS_NONE = 0,
>         IOMMU_FAULT_STATUS_FAILURE,
>         IOMMU_FAULT_STATUS_INVALID,
>         IOMMU_FAULT_STATUS_HANDLED,
>         IOMMU_FAULT_STATUS_IGNORE,
> };
>
> This would probably cover the two use-cases of reporting faults to
> device drivers, and injecting them into the guest with VFIO, as well as
> handling PPRs internally. I'm also working on providing more details
> (pasid for instance) in the fault callback.
>
> We could also use the fault handler for invalid PRI Page Requests
> (currently specialized by amd_iommu_set_invalid_ppr_cb). It's just a
> matter of adding a registration flag to iommu_set_fault_handler.
>
> Thanks,
> Jean

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web