Re: [PATCH v4 02/22] iommu: introduce device fault data

[Jean-Philippe Brucker]

On 06/03/2019 14:30, Auger Eric wrote:
>>> +#define IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE	(1 << 1)
>>> +#define IOMMU_FAULT_PAGE_REQUEST_PRIV_DATA	(1 << 2)
>>> +	__u32   flags;
>>> +	__u32	pasid;
>>> +	__u32	grpid;
>>> +	__u32	perm;
>>> +	__u64	addr;
>>
>> Given that we'll be reporting stall faults using this struct, it would
>> be good to have the fetch_addr field and flag here as well.
> As the stall model looks really ARM specific shouldn't we introduce a
> dedicated struct and iommu_fault_type enum value?

There is no reason for the generic page fault handler to differentiate
between stall and PRI, they are page requests. For a stall we write STAG
into grpid and set LAST_PAGE=1. Then the SMMU driver writes the page
response either as a PRI_RESP or a RESUME depending on the device type.

> Also for stall faults don't we need to expose the stall tag (STAG) that,
> as far as I understand is going to be used by the guest we it wants to
> retry or terminate the faulted transaction. In practice doesn't the
> stall fault have the same fields of the unrecoverable fault + STAG? I am
> afraid adding the fetch_addr in the page request struct may "pollute"
> the PRI struct that can be understood by both aarch64 and x86 parties atm.

Let's leave out the fetch_addr field then, I was suggesting it for
completeness but I don't need it immediately, at least not for host SVA.
For dual-stage SVA (where both stage-1 and stage-2 are shared with the
CPU) we'll need the IPA field, but that's still a long way away.

> Also couldn't we envision to put this STALL struct in a new revision of
> the fault ABI.
As said above, generic code doesn't have to know the difference until we
start implementing nested SVA. Also, we need stall support in the fault
handler soon, since there is hardware supporting it.

Thanks,
Jean