Re: [PATCH v2 08/16] iommu: introduce device fault data

[Jean-Philippe Brucker]

On 10/11/17 22:18, Jacob Pan wrote:
> On Fri, 10 Nov 2017 13:54:59 +0000
> Jean-Philippe Brucker <jean-philippe.brucker@xxxxxxx> wrote:
> 
>> On 09/11/17 19:36, Jacob Pan wrote:
>>> On Tue, 7 Nov 2017 11:38:50 +0000
>>> Jean-Philippe Brucker <jean-philippe.brucker@xxxxxxx> wrote:
>>>   
>>>> I think the IOMMU should pass the struct device associated to the
>>>> BDF to the fault handler. The fault handler can then deduce the
>>>> BDF from struct device if it needs to. This also allows to support
>>>> faults from non-PCI devices, where the BDF or deviceID is specific
>>>> to the IOMMU and doesn't mean anything to the device driver.
>>>>  
>>> Passing struct device is only useful if we use shared fault
>>> notification method, as I did in V1 patch with group level or
>>> current domain level.
>>>
>>> But the patch proposed here is a per device callback, there is no
>>> need for passing struct device since it is implied.  
>>
>> Sorry I had lost sight of the original patch in this thread. I think
>> the callback is fine as it is, in your patch:
>>
>> typedef int (*iommu_dev_fault_handler_t)(struct device *, struct
>> iommu_fault_event *);
>>
> I should have removed struct device here also. thanks for pointing it
> out.

Why remove it? The device driver will use a single C function as fault
handler for multiple devices, so it needs struct device argument to
understand the context.

[...]
>>>  * @pasid: contains process address space ID, used in shared
>>> virtual memory(SVM)
>>>  * @rid: requestor ID
>>>  * @page_req_group_id: page request group index
>>>  * @last_req: last request in a page request group
>>>  * @pasid_valid: indicates if the PRQ has a valid PASID
>>>  * @prot: page access protection flag, e.g. IOMMU_READ,
>>> IOMMU_WRITE  
>>
>> Should we also extend the prot flags then? PRI needs IOMMU_EXEC,
>> IOMMU_PRIVILEGED. The problem with IOMMU_READ and IOMMU_WRITE is that
>> it's not a bitfield, you can't OR values together. In order to extend
>> it we need to change the value of IOMMU_READ to be 1 and IOMMU_WRITE
>> to be 2. In PRI there is a case where R=W=0 (the PASID Stop marker),
>> and we can't represent it with the existing IOMMU_READ value.
>>
> don't we already have these in bit field? IOMMU_PRIV included. see
> include/linux/iommu.h
> #define IOMMU_READ	(1 << 0)
> #define IOMMU_WRITE	(1 << 1)
> #define IOMMU_CACHE	(1 << 2) /* DMA cache coherency */
> #define IOMMU_NOEXEC	(1 << 3)
> #define IOMMU_MMIO	(1 << 4) /* e.g. things like MSI doorbells */
> #define IOMMU_PRIV	(1 << 5)
> #define IOMMU_EXEC	(1 << 6)

Ah right, I was talking about IOMMU_FAULT_READ and IOMMU_FAULT_WRITE,
sorry for the confusion. These flags are for creating mappings, they
aren't really appropriate for fault reporting. What would the new
IOMMU_EXEC flag mean in the context of iommu_map, which is already using
IOMMU_NOEXEC (1 << 3)? What would IOMMU_CACHE and IOMMU_MMIO mean for
fault reporting? It's probably easier to use a distinct set of flags for
faults, by rewriting the IOMMU_FAULT_* flags.

>>>  * @private_data: uniquely identify device-specific private data
>>> for an
>>>  *                individual page request  
>>
>> We should specify how private_data is used by IOMMU driver and device
>> driver, for people not familiar with VT-d. Since it's device-specific,
>> is the device driver allowed to parse this field, is it allowed to
>> modify it before sending it back via iommu_page_response?
>>
> shall we say the private_data is iommu supplied device specific
> data, this data is only to be interpreted by the device driver or
> hardware.

That would work

>> For SMMU I've been abusing the private_data field to store
>> SMMU-specific flags that can be used by the page_response handler to
>> know how to send send the response:
>>
>> * Whether the fault was PRI or stall (the response command is
>> different)
>> * Whether the PRG response needs a PASID prefix or not. That's just a
>>   lazy shortcut and the property could be obtained differently.
>>
> can you use pasid_valid bit for it?

What I'm referring to is the "PRG Response PASID Required" bit in the PCI
PRI capability, which is needed for the PRI response. I could dig it back
from the struct device passed to the page response handler, but caching it
in the private flags was more convenient. However I think I can get rid of
the other flag PRI/stall by simply looking if struct device is a PCI dev.
So we don't need iommu_private for the moment.

Thanks,
Jean