The iommu fault data is currently defined in uapi/linux/iommu.h, but is
only used inside the iommu subsystem. Move it to linux/iommu.h, where it
will be more accessible to kernel drivers.
With this done, uapi/linux/iommu.h becomes empty and can be removed from
the tree.
Signed-off-by: Lu Baolu <[email protected]>
---
include/linux/iommu.h | 152 +++++++++++++++++++++++++++++++++-
include/uapi/linux/iommu.h | 161 -------------------------------------
MAINTAINERS | 1 -
3 files changed, 151 insertions(+), 163 deletions(-)
delete mode 100644 include/uapi/linux/iommu.h
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index d31642596675..0eb0fb852020 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -13,7 +13,6 @@
#include <linux/errno.h>
#include <linux/err.h>
#include <linux/of.h>
-#include <uapi/linux/iommu.h>
#define IOMMU_READ (1 << 0)
#define IOMMU_WRITE (1 << 1)
@@ -42,6 +41,157 @@ struct iommu_sva;
struct iommu_fault_event;
struct iommu_dma_cookie;
+#define IOMMU_FAULT_PERM_READ (1 << 0) /* read */
+#define IOMMU_FAULT_PERM_WRITE (1 << 1) /* write */
+#define IOMMU_FAULT_PERM_EXEC (1 << 2) /* exec */
+#define IOMMU_FAULT_PERM_PRIV (1 << 3) /* privileged */
+
+/* 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
+ * is the last page in group (IOMMU_FAULT_PAGE_REQUEST_* values).
+ * When IOMMU_FAULT_PAGE_RESPONSE_NEEDS_PASID is set, the page response
+ * must have the same PASID value as the page request. When it is clear,
+ * the page response should not have a PASID.
+ * @pasid: Process Address Space ID
+ * @grpid: Page Request Group Index
+ * @perm: requested page permissions (IOMMU_FAULT_PERM_* values)
+ * @addr: page address
+ * @private_data: device-specific private information
+ */
+struct iommu_fault_page_request {
+#define IOMMU_FAULT_PAGE_REQUEST_PASID_VALID (1 << 0)
+#define IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE (1 << 1)
+#define IOMMU_FAULT_PAGE_REQUEST_PRIV_DATA (1 << 2)
+#define IOMMU_FAULT_PAGE_RESPONSE_NEEDS_PASID (1 << 3)
+ __u32 flags;
+ __u32 pasid;
+ __u32 grpid;
+ __u32 perm;
+ __u64 addr;
+ __u64 private_data[2];
+};
+
+/**
+ * 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];
+ };
+};
+
+/**
+ * enum iommu_page_response_code - Return status of fault handlers
+ * @IOMMU_PAGE_RESP_SUCCESS: Fault has been handled and the page tables
+ * populated, retry the access. This is "Success" in PCI PRI.
+ * @IOMMU_PAGE_RESP_FAILURE: General error. Drop all subsequent faults from
+ * this device if possible. This is "Response Failure" in PCI PRI.
+ * @IOMMU_PAGE_RESP_INVALID: Could not handle this fault, don't retry the
+ * access. This is "Invalid Request" in PCI PRI.
+ */
+enum iommu_page_response_code {
+ IOMMU_PAGE_RESP_SUCCESS = 0,
+ IOMMU_PAGE_RESP_INVALID,
+ IOMMU_PAGE_RESP_FAILURE,
+};
+
+/**
+ * struct iommu_page_response - Generic page response information
+ * @argsz: User filled size of this data
+ * @version: API version of this structure
+ * @flags: encodes whether the corresponding fields are valid
+ * (IOMMU_FAULT_PAGE_RESPONSE_* values)
+ * @pasid: Process Address Space ID
+ * @grpid: Page Request Group Index
+ * @code: response code from &enum iommu_page_response_code
+ */
+struct iommu_page_response {
+ __u32 argsz;
+#define IOMMU_PAGE_RESP_VERSION_1 1
+ __u32 version;
+#define IOMMU_PAGE_RESP_PASID_VALID (1 << 0)
+ __u32 flags;
+ __u32 pasid;
+ __u32 grpid;
+ __u32 code;
+};
+
+
/* iommu fault flags */
#define IOMMU_FAULT_READ 0x0
#define IOMMU_FAULT_WRITE 0x1
diff --git a/include/uapi/linux/iommu.h b/include/uapi/linux/iommu.h
deleted file mode 100644
index 65d8b0234f69..000000000000
--- a/include/uapi/linux/iommu.h
+++ /dev/null
@@ -1,161 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
-/*
- * IOMMU user API definitions
- */
-
-#ifndef _UAPI_IOMMU_H
-#define _UAPI_IOMMU_H
-
-#include <linux/types.h>
-
-#define IOMMU_FAULT_PERM_READ (1 << 0) /* read */
-#define IOMMU_FAULT_PERM_WRITE (1 << 1) /* write */
-#define IOMMU_FAULT_PERM_EXEC (1 << 2) /* exec */
-#define IOMMU_FAULT_PERM_PRIV (1 << 3) /* privileged */
-
-/* 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
- * is the last page in group (IOMMU_FAULT_PAGE_REQUEST_* values).
- * When IOMMU_FAULT_PAGE_RESPONSE_NEEDS_PASID is set, the page response
- * must have the same PASID value as the page request. When it is clear,
- * the page response should not have a PASID.
- * @pasid: Process Address Space ID
- * @grpid: Page Request Group Index
- * @perm: requested page permissions (IOMMU_FAULT_PERM_* values)
- * @addr: page address
- * @private_data: device-specific private information
- */
-struct iommu_fault_page_request {
-#define IOMMU_FAULT_PAGE_REQUEST_PASID_VALID (1 << 0)
-#define IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE (1 << 1)
-#define IOMMU_FAULT_PAGE_REQUEST_PRIV_DATA (1 << 2)
-#define IOMMU_FAULT_PAGE_RESPONSE_NEEDS_PASID (1 << 3)
- __u32 flags;
- __u32 pasid;
- __u32 grpid;
- __u32 perm;
- __u64 addr;
- __u64 private_data[2];
-};
-
-/**
- * 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];
- };
-};
-
-/**
- * enum iommu_page_response_code - Return status of fault handlers
- * @IOMMU_PAGE_RESP_SUCCESS: Fault has been handled and the page tables
- * populated, retry the access. This is "Success" in PCI PRI.
- * @IOMMU_PAGE_RESP_FAILURE: General error. Drop all subsequent faults from
- * this device if possible. This is "Response Failure" in PCI PRI.
- * @IOMMU_PAGE_RESP_INVALID: Could not handle this fault, don't retry the
- * access. This is "Invalid Request" in PCI PRI.
- */
-enum iommu_page_response_code {
- IOMMU_PAGE_RESP_SUCCESS = 0,
- IOMMU_PAGE_RESP_INVALID,
- IOMMU_PAGE_RESP_FAILURE,
-};
-
-/**
- * struct iommu_page_response - Generic page response information
- * @argsz: User filled size of this data
- * @version: API version of this structure
- * @flags: encodes whether the corresponding fields are valid
- * (IOMMU_FAULT_PAGE_RESPONSE_* values)
- * @pasid: Process Address Space ID
- * @grpid: Page Request Group Index
- * @code: response code from &enum iommu_page_response_code
- */
-struct iommu_page_response {
- __u32 argsz;
-#define IOMMU_PAGE_RESP_VERSION_1 1
- __u32 version;
-#define IOMMU_PAGE_RESP_PASID_VALID (1 << 0)
- __u32 flags;
- __u32 pasid;
- __u32 grpid;
- __u32 code;
-};
-
-#endif /* _UAPI_IOMMU_H */
diff --git a/MAINTAINERS b/MAINTAINERS
index 3be1bdfe8ecc..8fded0298961 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10877,7 +10877,6 @@ F: drivers/iommu/
F: include/linux/iommu.h
F: include/linux/iova.h
F: include/linux/of_iommu.h
-F: include/uapi/linux/iommu.h
IOMMUFD
M: Jason Gunthorpe <[email protected]>
--
2.34.1
> From: Lu Baolu <[email protected]>
> Sent: Tuesday, July 11, 2023 9:07 AM
> +
> +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;
> +};
Currently there is no handler for unrecoverable faults.
Both Intel/ARM register iommu_queue_iopf() as the device fault handler.
It returns -EOPNOTSUPP for unrecoverable faults.
In your series the common iommu_handle_io_pgfault() also only works
for PRQ.
It kinds of suggest above definitions are dead code, though arm-smmu-v3
does attempt to set them.
Probably it's right time to remove them.
In the future even if there might be a need of forwarding unrecoverable
faults to the user via iommufd, fault reasons reported by the physical
IOMMU doesn't make any sense to the guest. Presumably the vIOMMU
should walk guest configurations to set a fault reason which makes sense
from guest p.o.v.
On 2023/7/11 14:05, Tian, Kevin wrote:
>> From: Lu Baolu <[email protected]>
>> Sent: Tuesday, July 11, 2023 9:07 AM
>> +
>> +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;
>> +};
>
> Currently there is no handler for unrecoverable faults.
>
> Both Intel/ARM register iommu_queue_iopf() as the device fault handler.
> It returns -EOPNOTSUPP for unrecoverable faults.
>
> In your series the common iommu_handle_io_pgfault() also only works
> for PRQ.
>
> It kinds of suggest above definitions are dead code, though arm-smmu-v3
> does attempt to set them.
>
> Probably it's right time to remove them.
>
> In the future even if there might be a need of forwarding unrecoverable
> faults to the user via iommufd, fault reasons reported by the physical
> IOMMU doesn't make any sense to the guest. Presumably the vIOMMU
> should walk guest configurations to set a fault reason which makes sense
> from guest p.o.v.
I am fine to remove unrecoverable faults data. But it was added by Jean,
so I'd like to know his opinion on this.
Best regards,
baolu
On Wed, Jul 12, 2023 at 10:07:22AM +0800, Baolu Lu wrote:
> > > +/**
> > > + * 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;
> > > +};
> >
> > Currently there is no handler for unrecoverable faults.
Yes those were meant for guest injection. Another goal was to replace
report_iommu_fault(), which also passes unrecoverable faults to host
drivers. Three drivers use that API:
* usnic just prints the error, which could be done by the IOMMU driver,
* remoteproc attempts to recover from the crash,
* msm attempts to handle the fault, or at least recover from the crash.
So the first one can be removed, and the others could move over to IOPF
(which may need to indicate that the fault is not actually recoverable by
the IOMMU) and return IOMMU_PAGE_RESP_INVALID.
> >
> > Both Intel/ARM register iommu_queue_iopf() as the device fault handler.
> > It returns -EOPNOTSUPP for unrecoverable faults.
> >
> > In your series the common iommu_handle_io_pgfault() also only works
> > for PRQ.
> >
> > It kinds of suggest above definitions are dead code, though arm-smmu-v3
> > does attempt to set them.
> >
> > Probably it's right time to remove them.
> >
> > In the future even if there might be a need of forwarding unrecoverable
> > faults to the user via iommufd, fault reasons reported by the physical
> > IOMMU doesn't make any sense to the guest.
I guess it depends on the architecture? The SMMU driver can report only
stage-1 faults through iommu_report_device_fault(), which are faults due
to a guest misconfiguring the tables assigned to it. At the moment
arm_smmu_handle_evt() only passes down stage-1 page table errors, the rest
is printed by the host.
> > Presumably the vIOMMU
> > should walk guest configurations to set a fault reason which makes sense
> > from guest p.o.v.
>
> I am fine to remove unrecoverable faults data. But it was added by Jean,
> so I'd like to know his opinion on this.
Passing errors to the guest could be a useful diagnostics tool for
debugging, once the guest gets more controls over the IOMMU hardware, but
it doesn't have a purpose beyond that. It could be the only tool
available, though: to avoid a guest voluntarily flooding the host logs by
misconfiguring its tables, we may have to disable printing in the host
errors that come from guest misconfiguration, in which case there won't be
any diagnostics available for guest bugs.
For now I don't mind if they're removed, if there is an easy way to
reintroduce them later.
Thanks,
Jean
> From: Jean-Philippe Brucker <[email protected]>
> Sent: Wednesday, July 12, 2023 5:34 PM
>
> On Wed, Jul 12, 2023 at 10:07:22AM +0800, Baolu Lu wrote:
> > > > +/**
> > > > + * 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;
> > > > +};
> > >
> > > Currently there is no handler for unrecoverable faults.
>
> Yes those were meant for guest injection. Another goal was to replace
> report_iommu_fault(), which also passes unrecoverable faults to host
> drivers. Three drivers use that API:
> * usnic just prints the error, which could be done by the IOMMU driver,
> * remoteproc attempts to recover from the crash,
> * msm attempts to handle the fault, or at least recover from the crash.
I was not aware of them. Thanks for pointing out.
>
> So the first one can be removed, and the others could move over to IOPF
> (which may need to indicate that the fault is not actually recoverable by
> the IOMMU) and return IOMMU_PAGE_RESP_INVALID.
Yep, presumably we should have just one interface to handle fault.
>
> > >
> > > Both Intel/ARM register iommu_queue_iopf() as the device fault handler.
> > > It returns -EOPNOTSUPP for unrecoverable faults.
> > >
> > > In your series the common iommu_handle_io_pgfault() also only works
> > > for PRQ.
> > >
> > > It kinds of suggest above definitions are dead code, though arm-smmu-v3
> > > does attempt to set them.
> > >
> > > Probably it's right time to remove them.
> > >
> > > In the future even if there might be a need of forwarding unrecoverable
> > > faults to the user via iommufd, fault reasons reported by the physical
> > > IOMMU doesn't make any sense to the guest.
>
> I guess it depends on the architecture? The SMMU driver can report only
> stage-1 faults through iommu_report_device_fault(), which are faults due
> to a guest misconfiguring the tables assigned to it. At the moment
> arm_smmu_handle_evt() only passes down stage-1 page table errors, the
> rest
> is printed by the host.
In that case the kernel just needs to notify the vIOMMU an error happened
along with access permissions (r/w/e/p). vIOMMU can figure out the reason
itself by walking the stage-1 page table. Likely it will find the same reason
as host reports, but that sounds a clearer path in concept.
>
> > > Presumably the vIOMMU
> > > should walk guest configurations to set a fault reason which makes sense
> > > from guest p.o.v.
> >
> > I am fine to remove unrecoverable faults data. But it was added by Jean,
> > so I'd like to know his opinion on this.
>
> Passing errors to the guest could be a useful diagnostics tool for
> debugging, once the guest gets more controls over the IOMMU hardware,
> but
> it doesn't have a purpose beyond that. It could be the only tool
> available, though: to avoid a guest voluntarily flooding the host logs by
> misconfiguring its tables, we may have to disable printing in the host
> errors that come from guest misconfiguration, in which case there won't be
> any diagnostics available for guest bugs.
>
> For now I don't mind if they're removed, if there is an easy way to
> reintroduce them later.
>
We can keep whatever is required to satisfy the kernel drivers which
want to know the fault.
But for anything invented for old uAPI (e.g. fault_reason) let's remove
them and redefine later when introducing the support to the user.
On 2023/7/13 11:22, Tian, Kevin wrote:
>> From: Jean-Philippe Brucker <[email protected]>
>> Sent: Wednesday, July 12, 2023 5:34 PM
>>
>> On Wed, Jul 12, 2023 at 10:07:22AM +0800, Baolu Lu wrote:
>>>>> +/**
>>>>> + * 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;
>>>>> +};
>>>>
>>>> Currently there is no handler for unrecoverable faults.
>>
>> Yes those were meant for guest injection. Another goal was to replace
>> report_iommu_fault(), which also passes unrecoverable faults to host
>> drivers. Three drivers use that API:
>> * usnic just prints the error, which could be done by the IOMMU driver,
>> * remoteproc attempts to recover from the crash,
>> * msm attempts to handle the fault, or at least recover from the crash.
>
> I was not aware of them. Thanks for pointing out.
>
>>
>> So the first one can be removed, and the others could move over to IOPF
>> (which may need to indicate that the fault is not actually recoverable by
>> the IOMMU) and return IOMMU_PAGE_RESP_INVALID.
>
> Yep, presumably we should have just one interface to handle fault.
>
>>
>>>>
>>>> Both Intel/ARM register iommu_queue_iopf() as the device fault handler.
>>>> It returns -EOPNOTSUPP for unrecoverable faults.
>>>>
>>>> In your series the common iommu_handle_io_pgfault() also only works
>>>> for PRQ.
>>>>
>>>> It kinds of suggest above definitions are dead code, though arm-smmu-v3
>>>> does attempt to set them.
>>>>
>>>> Probably it's right time to remove them.
>>>>
>>>> In the future even if there might be a need of forwarding unrecoverable
>>>> faults to the user via iommufd, fault reasons reported by the physical
>>>> IOMMU doesn't make any sense to the guest.
>>
>> I guess it depends on the architecture? The SMMU driver can report only
>> stage-1 faults through iommu_report_device_fault(), which are faults due
>> to a guest misconfiguring the tables assigned to it. At the moment
>> arm_smmu_handle_evt() only passes down stage-1 page table errors, the
>> rest
>> is printed by the host.
>
> In that case the kernel just needs to notify the vIOMMU an error happened
> along with access permissions (r/w/e/p). vIOMMU can figure out the reason
> itself by walking the stage-1 page table. Likely it will find the same reason
> as host reports, but that sounds a clearer path in concept.
>
>>
>>>> Presumably the vIOMMU
>>>> should walk guest configurations to set a fault reason which makes sense
>>>> from guest p.o.v.
>>>
>>> I am fine to remove unrecoverable faults data. But it was added by Jean,
>>> so I'd like to know his opinion on this.
>>
>> Passing errors to the guest could be a useful diagnostics tool for
>> debugging, once the guest gets more controls over the IOMMU hardware,
>> but
>> it doesn't have a purpose beyond that. It could be the only tool
>> available, though: to avoid a guest voluntarily flooding the host logs by
>> misconfiguring its tables, we may have to disable printing in the host
>> errors that come from guest misconfiguration, in which case there won't be
>> any diagnostics available for guest bugs.
>>
>> For now I don't mind if they're removed, if there is an easy way to
>> reintroduce them later.
>>
>
> We can keep whatever is required to satisfy the kernel drivers which
> want to know the fault.
>
> But for anything invented for old uAPI (e.g. fault_reason) let's remove
> them and redefine later when introducing the support to the user.
Okay, I will do this in the next version.
Best regards,
baolu