diff mbox series

[v2,03/12] iommu: Remove unrecoverable fault data

Message ID 20230727054837.147050-4-baolu.lu@linux.intel.com
State New
Headers
Series iommu: Prepare to deliver page faults to user space |

Commit Message

Baolu Lu July 27, 2023, 5:48 a.m. UTC 
  The unrecoverable fault data is not used anywhere. Remove it to avoid
dead code.

Suggested-by: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 include/linux/iommu.h | 70 +------------------------------------------
 1 file changed, 1 insertion(+), 69 deletions(-)

Comments

Tian, Kevin Aug. 3, 2023, 7:54 a.m. UTC | #1 
> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Thursday, July 27, 2023 1:48 PM
>
>  struct iommu_fault {
>  	__u32	type;
> -	__u32	padding;

this padding should be kept.

> -	union {
> -		struct iommu_fault_unrecoverable event;
> -		struct iommu_fault_page_request prm;
> -		__u8 padding2[56];
> -	};
> +	struct iommu_fault_page_request prm;
>  };
Baolu Lu Aug. 4, 2023, 2:58 a.m. UTC | #2 
On 2023/8/3 15:54, Tian, Kevin wrote:
>> From: Lu Baolu<baolu.lu@linux.intel.com>
>> Sent: Thursday, July 27, 2023 1:48 PM
>>
>>   struct iommu_fault {
>>   	__u32	type;
>> -	__u32	padding;
> this padding should be kept.
> 

To keep above 64-bit aligned, right?

I can't think of any other reason to keep it.

Best regards,
baolu
Tian, Kevin Aug. 4, 2023, 3:51 a.m. UTC | #3 
> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Friday, August 4, 2023 10:59 AM
> 
> On 2023/8/3 15:54, Tian, Kevin wrote:
> >> From: Lu Baolu<baolu.lu@linux.intel.com>
> >> Sent: Thursday, July 27, 2023 1:48 PM
> >>
> >>   struct iommu_fault {
> >>   	__u32	type;
> >> -	__u32	padding;
> > this padding should be kept.
> >
> 
> To keep above 64-bit aligned, right?
> 

yes
Baolu Lu Aug. 4, 2023, 5:34 a.m. UTC | #4 
On 8/4/23 11:51 AM, Tian, Kevin wrote:
>> From: Baolu Lu <baolu.lu@linux.intel.com>
>> Sent: Friday, August 4, 2023 10:59 AM
>>
>> On 2023/8/3 15:54, Tian, Kevin wrote:
>>>> From: Lu Baolu<baolu.lu@linux.intel.com>
>>>> Sent: Thursday, July 27, 2023 1:48 PM
>>>>
>>>>    struct iommu_fault {
>>>>    	__u32	type;
>>>> -	__u32	padding;
>>> this padding should be kept.
>>>
>>
>> To keep above 64-bit aligned, right?
>>
> 
> yes

Okay, thanks!

Best regards,
baolu
Jason Gunthorpe Aug. 8, 2023, 6:39 p.m. UTC | #5 
On Fri, Aug 04, 2023 at 03:51:30AM +0000, Tian, Kevin wrote:
> > From: Baolu Lu <baolu.lu@linux.intel.com>
> > Sent: Friday, August 4, 2023 10:59 AM
> > 
> > On 2023/8/3 15:54, Tian, Kevin wrote:
> > >> From: Lu Baolu<baolu.lu@linux.intel.com>
> > >> Sent: Thursday, July 27, 2023 1:48 PM
> > >>
> > >>   struct iommu_fault {
> > >>   	__u32	type;
> > >> -	__u32	padding;
> > > this padding should be kept.
> > >
> > 
> > To keep above 64-bit aligned, right?
> > 
> 
> yes

If it is not uapi we should not explicitly document padding (and __u32
should be u32). The compiler will add it if it is necessary.

If the compiler isn't right for some reason then something else has
gone wrong.

Jason
Tian, Kevin Aug. 9, 2023, 12:01 a.m. UTC | #6 
> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Wednesday, August 9, 2023 2:40 AM
> 
> On Fri, Aug 04, 2023 at 03:51:30AM +0000, Tian, Kevin wrote:
> > > From: Baolu Lu <baolu.lu@linux.intel.com>
> > > Sent: Friday, August 4, 2023 10:59 AM
> > >
> > > On 2023/8/3 15:54, Tian, Kevin wrote:
> > > >> From: Lu Baolu<baolu.lu@linux.intel.com>
> > > >> Sent: Thursday, July 27, 2023 1:48 PM
> > > >>
> > > >>   struct iommu_fault {
> > > >>   	__u32	type;
> > > >> -	__u32	padding;
> > > > this padding should be kept.
> > > >
> > >
> > > To keep above 64-bit aligned, right?
> > >
> >
> > yes
> 
> If it is not uapi we should not explicitly document padding (and __u32
> should be u32). The compiler will add it if it is necessary.
> 
> If the compiler isn't right for some reason then something else has
> gone wrong.
> 

I thought this will be used as uAPI later. I'm fine to leave it be and
add the padding when the uAPI is introduced.
Jason Gunthorpe Aug. 9, 2023, 2:05 p.m. UTC | #7 
On Wed, Aug 09, 2023 at 12:01:52AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@ziepe.ca>
> > Sent: Wednesday, August 9, 2023 2:40 AM
> > 
> > On Fri, Aug 04, 2023 at 03:51:30AM +0000, Tian, Kevin wrote:
> > > > From: Baolu Lu <baolu.lu@linux.intel.com>
> > > > Sent: Friday, August 4, 2023 10:59 AM
> > > >
> > > > On 2023/8/3 15:54, Tian, Kevin wrote:
> > > > >> From: Lu Baolu<baolu.lu@linux.intel.com>
> > > > >> Sent: Thursday, July 27, 2023 1:48 PM
> > > > >>
> > > > >>   struct iommu_fault {
> > > > >>   	__u32	type;
> > > > >> -	__u32	padding;
> > > > > this padding should be kept.
> > > > >
> > > >
> > > > To keep above 64-bit aligned, right?
> > > >
> > >
> > > yes
> > 
> > If it is not uapi we should not explicitly document padding (and __u32
> > should be u32). The compiler will add it if it is necessary.
> > 
> > If the compiler isn't right for some reason then something else has
> > gone wrong.
> > 
> 
> I thought this will be used as uAPI later. I'm fine to leave it be and
> add the padding when the uAPI is introduced.

Yes

Jason
Jason Gunthorpe Aug. 9, 2023, 4:59 p.m. UTC | #8 
On Thu, Jul 27, 2023 at 01:48:28PM +0800, Lu Baolu wrote:
> The unrecoverable fault data is not used anywhere. Remove it to avoid
> dead code.
> 
> Suggested-by: Kevin Tian <kevin.tian@intel.com>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  include/linux/iommu.h | 70 +------------------------------------------
>  1 file changed, 1 insertion(+), 69 deletions(-)

Do we plan to bring this back in some form? A driver specific fault
report via iommufd?

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason
Baolu Lu Aug. 10, 2023, 2:27 a.m. UTC | #9 
On 2023/8/10 0:59, Jason Gunthorpe wrote:
> On Thu, Jul 27, 2023 at 01:48:28PM +0800, Lu Baolu wrote:
>> The unrecoverable fault data is not used anywhere. Remove it to avoid
>> dead code.
>>
>> Suggested-by: Kevin Tian<kevin.tian@intel.com>
>> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
>> ---
>>   include/linux/iommu.h | 70 +------------------------------------------
>>   1 file changed, 1 insertion(+), 69 deletions(-)
> Do we plan to bring this back in some form? A driver specific fault
> report via iommufd?

I can hardly see the possibility.

The only necessary dma fault messages are the offending address and the
permissions. With these, the user space device model software knows that
"a DMA fault was generated when the IOMMU hardware tried to translate
the offending address with the given permissions".

And then, the device model software will walk the page table and figure
out what is missed before injecting the vendor-specific fault messages
to the VM guest.

Best regards,
baolu
Jason Gunthorpe Aug. 10, 2023, 4:46 p.m. UTC | #10 
On Thu, Aug 10, 2023 at 10:27:21AM +0800, Baolu Lu wrote:
> On 2023/8/10 0:59, Jason Gunthorpe wrote:
> > On Thu, Jul 27, 2023 at 01:48:28PM +0800, Lu Baolu wrote:
> > > The unrecoverable fault data is not used anywhere. Remove it to avoid
> > > dead code.
> > > 
> > > Suggested-by: Kevin Tian<kevin.tian@intel.com>
> > > Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
> > > ---
> > >   include/linux/iommu.h | 70 +------------------------------------------
> > >   1 file changed, 1 insertion(+), 69 deletions(-)
> > Do we plan to bring this back in some form? A driver specific fault
> > report via iommufd?
> 
> I can hardly see the possibility.
> 
> The only necessary dma fault messages are the offending address and the
> permissions. With these, the user space device model software knows that
> "a DMA fault was generated when the IOMMU hardware tried to translate
> the offending address with the given permissions".
> 
> And then, the device model software will walk the page table and figure
> out what is missed before injecting the vendor-specific fault messages
> to the VM guest.

Avoiding walking the page table sounds like a pretty big win if we
could manage it by forwarding more event data..

Jason
Baolu Lu Aug. 11, 2023, 1:15 a.m. UTC | #11 
On 2023/8/11 0:46, Jason Gunthorpe wrote:
> On Thu, Aug 10, 2023 at 10:27:21AM +0800, Baolu Lu wrote:
>> On 2023/8/10 0:59, Jason Gunthorpe wrote:
>>> On Thu, Jul 27, 2023 at 01:48:28PM +0800, Lu Baolu wrote:
>>>> The unrecoverable fault data is not used anywhere. Remove it to avoid
>>>> dead code.
>>>>
>>>> Suggested-by: Kevin Tian<kevin.tian@intel.com>
>>>> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
>>>> ---
>>>>    include/linux/iommu.h | 70 +------------------------------------------
>>>>    1 file changed, 1 insertion(+), 69 deletions(-)
>>> Do we plan to bring this back in some form? A driver specific fault
>>> report via iommufd?
>> I can hardly see the possibility.
>>
>> The only necessary dma fault messages are the offending address and the
>> permissions. With these, the user space device model software knows that
>> "a DMA fault was generated when the IOMMU hardware tried to translate
>> the offending address with the given permissions".
>>
>> And then, the device model software will walk the page table and figure
>> out what is missed before injecting the vendor-specific fault messages
>> to the VM guest.
> Avoiding walking the page table sounds like a pretty big win if we
> could manage it by forwarding more event data..

Fair enough. We can discuss what kind of extra event data could be
included later when we have real code for dma fault forwarding support
in iommufd.

Best regards,
baolu
Tian, Kevin Aug. 11, 2023, 4:17 a.m. UTC | #12 
> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Friday, August 11, 2023 9:16 AM
> 
> On 2023/8/11 0:46, Jason Gunthorpe wrote:
> > On Thu, Aug 10, 2023 at 10:27:21AM +0800, Baolu Lu wrote:
> >> On 2023/8/10 0:59, Jason Gunthorpe wrote:
> >>> On Thu, Jul 27, 2023 at 01:48:28PM +0800, Lu Baolu wrote:
> >>>> The unrecoverable fault data is not used anywhere. Remove it to avoid
> >>>> dead code.
> >>>>
> >>>> Suggested-by: Kevin Tian<kevin.tian@intel.com>
> >>>> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
> >>>> ---
> >>>>    include/linux/iommu.h | 70 +------------------------------------------
> >>>>    1 file changed, 1 insertion(+), 69 deletions(-)
> >>> Do we plan to bring this back in some form? A driver specific fault
> >>> report via iommufd?
> >> I can hardly see the possibility.
> >>
> >> The only necessary dma fault messages are the offending address and the
> >> permissions. With these, the user space device model software knows
> that
> >> "a DMA fault was generated when the IOMMU hardware tried to translate
> >> the offending address with the given permissions".
> >>
> >> And then, the device model software will walk the page table and figure
> >> out what is missed before injecting the vendor-specific fault messages
> >> to the VM guest.
> > Avoiding walking the page table sounds like a pretty big win if we
> > could manage it by forwarding more event data..
> 
> Fair enough. We can discuss what kind of extra event data could be
> included later when we have real code for dma fault forwarding support
> in iommufd.
> 

I'm afraid there might be cases where VMM cannot rely on the hw event
data. e.g. if there is a misconfiguation in virtual context entry or ste then
vIOMMU may decide to unmap and leave IOAS empty to trigger
unrecoverable fault when any DMA comes. Then upon notification vIOMMU
needs to check virtual context entry/ste to decide the right error code when
injecting unrecoverable fault into the guest. Here the hw event data will be
about missing stage-2 mapping hence incorrect.

and this is not a performance critical path. 😊
diff mbox series

Patch

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index b3537736f9cb..a1e4390d05a8 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -48,69 +48,9 @@  struct iommu_dma_cookie;
 
 /* Generic fault types, can be expanded IRQ remapping fault */
 enum iommu_fault_type {
-	IOMMU_FAULT_DMA_UNRECOV = 1,	/* unrecoverable fault */
 	IOMMU_FAULT_PAGE_REQ,		/* page request fault */
 };
 
-enum iommu_fault_reason {
-	IOMMU_FAULT_REASON_UNKNOWN = 0,
-
-	/* Could not access the PASID table (fetch caused external abort) */
-	IOMMU_FAULT_REASON_PASID_FETCH,
-
-	/* PASID entry is invalid or has configuration errors */
-	IOMMU_FAULT_REASON_BAD_PASID_ENTRY,
-
-	/*
-	 * PASID is out of range (e.g. exceeds the maximum PASID
-	 * supported by the IOMMU) or disabled.
-	 */
-	IOMMU_FAULT_REASON_PASID_INVALID,
-
-	/*
-	 * An external abort occurred fetching (or updating) a translation
-	 * table descriptor
-	 */
-	IOMMU_FAULT_REASON_WALK_EABT,
-
-	/*
-	 * Could not access the page table entry (Bad address),
-	 * actual translation fault
-	 */
-	IOMMU_FAULT_REASON_PTE_FETCH,
-
-	/* Protection flag check failed */
-	IOMMU_FAULT_REASON_PERMISSION,
-
-	/* access flag check failed */
-	IOMMU_FAULT_REASON_ACCESS,
-
-	/* Output address of a translation stage caused Address Size fault */
-	IOMMU_FAULT_REASON_OOR_ADDRESS,
-};
-
-/**
- * struct iommu_fault_unrecoverable - Unrecoverable fault data
- * @reason: reason of the fault, from &enum iommu_fault_reason
- * @flags: parameters of this fault (IOMMU_FAULT_UNRECOV_* values)
- * @pasid: Process Address Space ID
- * @perm: requested permission access using by the incoming transaction
- *        (IOMMU_FAULT_PERM_* values)
- * @addr: offending page address
- * @fetch_addr: address that caused a fetch abort, if any
- */
-struct iommu_fault_unrecoverable {
-	__u32	reason;
-#define IOMMU_FAULT_UNRECOV_PASID_VALID		(1 << 0)
-#define IOMMU_FAULT_UNRECOV_ADDR_VALID		(1 << 1)
-#define IOMMU_FAULT_UNRECOV_FETCH_ADDR_VALID	(1 << 2)
-	__u32	flags;
-	__u32	pasid;
-	__u32	perm;
-	__u64	addr;
-	__u64	fetch_addr;
-};
-
 /**
  * struct iommu_fault_page_request - Page Request data
  * @flags: encodes whether the corresponding fields are valid and whether this
@@ -140,19 +80,11 @@  struct iommu_fault_page_request {
 /**
  * struct iommu_fault - Generic fault data
  * @type: fault type from &enum iommu_fault_type
- * @padding: reserved for future use (should be zero)
- * @event: fault event, when @type is %IOMMU_FAULT_DMA_UNRECOV
  * @prm: Page Request message, when @type is %IOMMU_FAULT_PAGE_REQ
- * @padding2: sets the fault size to allow for future extensions
  */
 struct iommu_fault {
 	__u32	type;
-	__u32	padding;
-	union {
-		struct iommu_fault_unrecoverable event;
-		struct iommu_fault_page_request prm;
-		__u8 padding2[56];
-	};
+	struct iommu_fault_page_request prm;
 };
 
 /**