| Message ID | b1930038cf828dc13c080e2b46b3003a8c98ef24.1740238876.git.nicolinc@nvidia.com |
|---|---|
| State | New |
| Headers | show |
| Series | iommufd: Add vIOMMU infrastructure (Part-3: vEVENTQ) | expand |
On Sat, Feb 22, 2025 at 07:54:10AM -0800, Nicolin Chen wrote: > Aside from the IOPF framework, iommufd provides an additional pathway to > report hardware events, via the vEVENTQ of vIOMMU infrastructure. > > Define an iommu_vevent_arm_smmuv3 uAPI structure, and report stage-1 events > in the threaded IRQ handler. Also, add another four event record types that > can be forwarded to a VM. > > Reviewed-by: Kevin Tian <kevin.tian@intel.com> > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > --- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 7 +++ > include/uapi/linux/iommufd.h | 23 +++++++ > .../arm/arm-smmu-v3/arm-smmu-v3-iommufd.c | 17 ++++++ > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 60 +++++++++++-------- > 4 files changed, 82 insertions(+), 25 deletions(-) > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h > index 85352504343b..c8574969e700 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h > @@ -1067,6 +1067,7 @@ int arm_smmu_attach_prepare_vmaster(struct arm_smmu_attach_state *state, > struct iommu_domain *domain); > void arm_smmu_attach_commit_vmaster(struct arm_smmu_attach_state *state); > void arm_smmu_master_clear_vmaster(struct arm_smmu_master *master); > +int arm_vmaster_report_event(struct arm_smmu_vmaster *vmaster, u64 *evt); > #else > #define arm_smmu_hw_info NULL > #define arm_vsmmu_alloc NULL > @@ -1085,6 +1086,12 @@ arm_smmu_attach_commit_vmaster(struct arm_smmu_attach_state *state) > static inline void arm_smmu_master_clear_vmaster(struct arm_smmu_master *master) > { > } > + > +static inline int arm_vmaster_report_event(struct arm_smmu_vmaster *vmaster, > + u64 *evt) > +{ > + return -EOPNOTSUPP; > +} > #endif /* CONFIG_ARM_SMMU_V3_IOMMUFD */ > > #endif /* _ARM_SMMU_V3_H */ > diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h > index 2ade4839880d..5fc7e27804b7 100644 > --- a/include/uapi/linux/iommufd.h > +++ b/include/uapi/linux/iommufd.h > @@ -1054,9 +1054,32 @@ struct iommufd_vevent_header { > /** > * enum iommu_veventq_type - Virtual Event Queue Type > * @IOMMU_VEVENTQ_TYPE_DEFAULT: Reserved for future use > + * @IOMMU_VEVENTQ_TYPE_ARM_SMMUV3: ARM SMMUv3 Virtual Event Queue > */ > enum iommu_veventq_type { > IOMMU_VEVENTQ_TYPE_DEFAULT = 0, > + IOMMU_VEVENTQ_TYPE_ARM_SMMUV3 = 1, > +}; > + > +/** > + * struct iommu_vevent_arm_smmuv3 - ARM SMMUv3 Virtual Event > + * (IOMMU_VEVENTQ_TYPE_ARM_SMMUV3) > + * @evt: 256-bit ARM SMMUv3 Event record, little-endian. > + * Reported event records: (Refer to "7.3 Event records" in SMMUv3 HW Spec) > + * - 0x04 C_BAD_STE > + * - 0x06 F_STREAM_DISABLED > + * - 0x08 C_BAD_SUBSTREAMID > + * - 0x0a C_BAD_CD > + * - 0x10 F_TRANSLATION > + * - 0x11 F_ADDR_SIZE > + * - 0x12 F_ACCESS > + * - 0x13 F_PERMISSION > + * > + * StreamID field reports a virtual device ID. To receive a virtual event for a > + * device, a vDEVICE must be allocated via IOMMU_VDEVICE_ALLOC. > + */ > +struct iommu_vevent_arm_smmuv3 { > + __aligned_le64 evt[4]; > }; > > /** > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c > index 364d8469a480..42c7daf4c8c7 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c > @@ -445,4 +445,21 @@ struct iommufd_viommu *arm_vsmmu_alloc(struct device *dev, > return &vsmmu->core; > } > > +int arm_vmaster_report_event(struct arm_smmu_vmaster *vmaster, u64 *evt) > +{ > + struct iommu_vevent_arm_smmuv3 vevt; > + int i; > + > + lockdep_assert_held(&vmaster->vsmmu->smmu->streams_mutex); > + > + vevt.evt[0] = cpu_to_le64((evt[0] & ~EVTQ_0_SID) | > + FIELD_PREP(EVTQ_0_SID, vmaster->vsid)); > + for (i = 1; i < EVTQ_ENT_DWORDS; i++) > + vevt.evt[i] = cpu_to_le64(evt[i]); Just thinking out loud here: I understand the goal here is to "emulate" an IOMMU. But I'm just wondering if we could report struct events instead of the raw event? For example, can't we have something like arm_smmu_event here with the sid changed to vsid? Are we taking the raw event since we want to keep the `u64 event_data[]` field within `struct iommufd_vevent` generic to all architectures? > + > + return iommufd_viommu_report_event(&vmaster->vsmmu->core, > + IOMMU_VEVENTQ_TYPE_ARM_SMMUV3, &vevt, > + sizeof(vevt)); > +} > + > MODULE_IMPORT_NS("IOMMUFD"); > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > index 9e50bcee69d1..fdf8bba14303 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > @@ -1813,8 +1813,8 @@ static void arm_smmu_decode_event(struct arm_smmu_device *smmu, u64 *raw, > mutex_unlock(&smmu->streams_mutex); > } > > -static int arm_smmu_handle_event(struct arm_smmu_device *smmu, > - struct arm_smmu_event *event) > +static int arm_smmu_handle_event(struct arm_smmu_device *smmu, u64 *evt, > + struct arm_smmu_event *event) > { > int ret = 0; > u32 perm = 0; > @@ -1823,6 +1823,10 @@ static int arm_smmu_handle_event(struct arm_smmu_device *smmu, > struct iommu_fault *flt = &fault_evt.fault; > > switch (event->id) { > + case EVT_ID_BAD_STE_CONFIG: > + case EVT_ID_STREAM_DISABLED_FAULT: > + case EVT_ID_BAD_SUBSTREAMID_CONFIG: > + case EVT_ID_BAD_CD_CONFIG: > case EVT_ID_TRANSLATION_FAULT: > case EVT_ID_ADDR_SIZE_FAULT: > case EVT_ID_ACCESS_FAULT: > @@ -1832,31 +1836,30 @@ static int arm_smmu_handle_event(struct arm_smmu_device *smmu, > return -EOPNOTSUPP; > } > > - if (!event->stall) > - return -EOPNOTSUPP; > - > - if (event->read) > - perm |= IOMMU_FAULT_PERM_READ; > - else > - perm |= IOMMU_FAULT_PERM_WRITE; > + if (event->stall) { > + if (event->read) > + perm |= IOMMU_FAULT_PERM_READ; > + else > + perm |= IOMMU_FAULT_PERM_WRITE; > > - if (event->instruction) > - perm |= IOMMU_FAULT_PERM_EXEC; > + if (event->instruction) > + perm |= IOMMU_FAULT_PERM_EXEC; > > - if (event->privileged) > - perm |= IOMMU_FAULT_PERM_PRIV; > + if (event->privileged) > + perm |= IOMMU_FAULT_PERM_PRIV; > > - flt->type = IOMMU_FAULT_PAGE_REQ; > - flt->prm = (struct iommu_fault_page_request) { > - .flags = IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE, > - .grpid = event->stag, > - .perm = perm, > - .addr = event->iova, > - }; > + flt->type = IOMMU_FAULT_PAGE_REQ; > + flt->prm = (struct iommu_fault_page_request){ > + .flags = IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE, > + .grpid = event->stag, > + .perm = perm, > + .addr = event->iova, > + }; > > - if (event->ssv) { > - flt->prm.flags |= IOMMU_FAULT_PAGE_REQUEST_PASID_VALID; > - flt->prm.pasid = event->ssid; > + if (event->ssv) { > + flt->prm.flags |= IOMMU_FAULT_PAGE_REQUEST_PASID_VALID; > + flt->prm.pasid = event->ssid; > + } > } > > mutex_lock(&smmu->streams_mutex); > @@ -1866,7 +1869,14 @@ static int arm_smmu_handle_event(struct arm_smmu_device *smmu, > goto out_unlock; > } > > - ret = iommu_report_device_fault(master->dev, &fault_evt); > + if (event->stall) { > + ret = iommu_report_device_fault(master->dev, &fault_evt); > + } else { > + if (master->vmaster && !event->s2) > + ret = arm_vmaster_report_event(master->vmaster, evt); > + else > + ret = -EFAULT; /* Unhandled events should be pinned */ > + } Nit: I don't see the `arm_smmu_handle_event` being called elsewhere, is there a reason to return -EFAULT instead of -EOPNOTSUPP here? I think the current behavior here is to return -EOPNOTSUPP if (!event->stall). Whereas, what we're doing here is: if (event->stall) { ... /* do legacy stuff */ ... } else { if (master->vmaster && !event->s2) arm_vmaster_report_event(vmaster, evt); else ret = -EFAULT } mutex_unlock(&smmu->streams_mutex); return ret; Thus, we end up returning -EFAULT instead of -EOPNOTSUPP in case event->stall == false. I agree that we aren't really checking the return value in the evtq_thread handler, but I'm wondering if we should ensure that we end up retaining the same behaviour as we have right now? > out_unlock: > mutex_unlock(&smmu->streams_mutex); > return ret; > @@ -1944,7 +1954,7 @@ static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev) > do { > while (!queue_remove_raw(q, evt)) { > arm_smmu_decode_event(smmu, evt, &event); > - if (arm_smmu_handle_event(smmu, &event)) > + if (arm_smmu_handle_event(smmu, evt, &event)) > arm_smmu_dump_event(smmu, evt, &event, &rs); > > put_device(event.dev); > -- > 2.43.0 > Thanks, Praan
On Mon, Feb 24, 2025 at 09:35:14PM +0000, Pranjal Shrivastava wrote: > On Sat, Feb 22, 2025 at 07:54:10AM -0800, Nicolin Chen wrote: > > +int arm_vmaster_report_event(struct arm_smmu_vmaster *vmaster, u64 *evt) > > +{ > > + struct iommu_vevent_arm_smmuv3 vevt; > > + int i; > > + > > + lockdep_assert_held(&vmaster->vsmmu->smmu->streams_mutex); > > + > > + vevt.evt[0] = cpu_to_le64((evt[0] & ~EVTQ_0_SID) | > > + FIELD_PREP(EVTQ_0_SID, vmaster->vsid)); > > + for (i = 1; i < EVTQ_ENT_DWORDS; i++) > > + vevt.evt[i] = cpu_to_le64(evt[i]); > > Just thinking out loud here: > I understand the goal here is to "emulate" an IOMMU. But I'm just > wondering if we could report struct events instead of the raw event? > > For example, can't we have something like arm_smmu_event here with the > sid changed to vsid? > > Are we taking the raw event since we want to keep the `u64 event_data[]` > field within `struct iommufd_vevent` generic to all architectures? The ABIs for vSMMU are defined in the HW languange, e.g. cmd, ste. Thus, here evt in raw too. > > - ret = iommu_report_device_fault(master->dev, &fault_evt); > > + if (event->stall) { > > + ret = iommu_report_device_fault(master->dev, &fault_evt); > > + } else { > > + if (master->vmaster && !event->s2) > > + ret = arm_vmaster_report_event(master->vmaster, evt); > > + else > > + ret = -EFAULT; /* Unhandled events should be pinned */ > > + } > > Nit: > I don't see the `arm_smmu_handle_event` being called elsewhere, is there > a reason to return -EFAULT instead of -EOPNOTSUPP here? > > I think the current behavior here is to return -EOPNOTSUPP if (!event->stall). > Whereas, what we're doing here is: > if (event->stall) { > ... > /* do legacy stuff */ > ... > } > > else { > if (master->vmaster && !event->s2) > arm_vmaster_report_event(vmaster, evt); > else > ret = -EFAULT > } > > mutex_unlock(&smmu->streams_mutex); > return ret; > > Thus, we end up returning -EFAULT instead of -EOPNOTSUPP in case > event->stall == false. I agree that we aren't really checking the return > value in the evtq_thread handler, but I'm wondering if we should ensure > that we end up retaining the same behaviour as we have right now? Oh, it looks like -EOPNOTSUPP should be returned here. Will fix. Thanks Nicolin
On Mon, Feb 24, 2025 at 01:56:46PM -0800, Nicolin Chen wrote: > > Just thinking out loud here: > > I understand the goal here is to "emulate" an IOMMU. But I'm just > > wondering if we could report struct events instead of the raw event? > > > > For example, can't we have something like arm_smmu_event here with the > > sid changed to vsid? > > > > Are we taking the raw event since we want to keep the `u64 event_data[]` > > field within `struct iommufd_vevent` generic to all architectures? > > The ABIs for vSMMU are defined in the HW languange, e.g. cmd, ste. > Thus, here evt in raw too. Right, the point is that it gives as a safe uABI that is effectively being managed by ARM. If we make our own thing then we have to take the responsiblity to make it safe and extensible. I don't see a justification to do that.. It is the same discussion we had around the vSTE as input, the raw invalidation command and the IDRs. Since we've already done 'follow the SMMU spec' so many times already now we should keep doing it. Jason
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h index 85352504343b..c8574969e700 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h @@ -1067,6 +1067,7 @@ int arm_smmu_attach_prepare_vmaster(struct arm_smmu_attach_state *state, struct iommu_domain *domain); void arm_smmu_attach_commit_vmaster(struct arm_smmu_attach_state *state); void arm_smmu_master_clear_vmaster(struct arm_smmu_master *master); +int arm_vmaster_report_event(struct arm_smmu_vmaster *vmaster, u64 *evt); #else #define arm_smmu_hw_info NULL #define arm_vsmmu_alloc NULL @@ -1085,6 +1086,12 @@ arm_smmu_attach_commit_vmaster(struct arm_smmu_attach_state *state) static inline void arm_smmu_master_clear_vmaster(struct arm_smmu_master *master) { } + +static inline int arm_vmaster_report_event(struct arm_smmu_vmaster *vmaster, + u64 *evt) +{ + return -EOPNOTSUPP; +} #endif /* CONFIG_ARM_SMMU_V3_IOMMUFD */ #endif /* _ARM_SMMU_V3_H */ diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h index 2ade4839880d..5fc7e27804b7 100644 --- a/include/uapi/linux/iommufd.h +++ b/include/uapi/linux/iommufd.h @@ -1054,9 +1054,32 @@ struct iommufd_vevent_header { /** * enum iommu_veventq_type - Virtual Event Queue Type * @IOMMU_VEVENTQ_TYPE_DEFAULT: Reserved for future use + * @IOMMU_VEVENTQ_TYPE_ARM_SMMUV3: ARM SMMUv3 Virtual Event Queue */ enum iommu_veventq_type { IOMMU_VEVENTQ_TYPE_DEFAULT = 0, + IOMMU_VEVENTQ_TYPE_ARM_SMMUV3 = 1, +}; + +/** + * struct iommu_vevent_arm_smmuv3 - ARM SMMUv3 Virtual Event + * (IOMMU_VEVENTQ_TYPE_ARM_SMMUV3) + * @evt: 256-bit ARM SMMUv3 Event record, little-endian. + * Reported event records: (Refer to "7.3 Event records" in SMMUv3 HW Spec) + * - 0x04 C_BAD_STE + * - 0x06 F_STREAM_DISABLED + * - 0x08 C_BAD_SUBSTREAMID + * - 0x0a C_BAD_CD + * - 0x10 F_TRANSLATION + * - 0x11 F_ADDR_SIZE + * - 0x12 F_ACCESS + * - 0x13 F_PERMISSION + * + * StreamID field reports a virtual device ID. To receive a virtual event for a + * device, a vDEVICE must be allocated via IOMMU_VDEVICE_ALLOC. + */ +struct iommu_vevent_arm_smmuv3 { + __aligned_le64 evt[4]; }; /** diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c index 364d8469a480..42c7daf4c8c7 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c @@ -445,4 +445,21 @@ struct iommufd_viommu *arm_vsmmu_alloc(struct device *dev, return &vsmmu->core; } +int arm_vmaster_report_event(struct arm_smmu_vmaster *vmaster, u64 *evt) +{ + struct iommu_vevent_arm_smmuv3 vevt; + int i; + + lockdep_assert_held(&vmaster->vsmmu->smmu->streams_mutex); + + vevt.evt[0] = cpu_to_le64((evt[0] & ~EVTQ_0_SID) | + FIELD_PREP(EVTQ_0_SID, vmaster->vsid)); + for (i = 1; i < EVTQ_ENT_DWORDS; i++) + vevt.evt[i] = cpu_to_le64(evt[i]); + + return iommufd_viommu_report_event(&vmaster->vsmmu->core, + IOMMU_VEVENTQ_TYPE_ARM_SMMUV3, &vevt, + sizeof(vevt)); +} + MODULE_IMPORT_NS("IOMMUFD"); diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c index 9e50bcee69d1..fdf8bba14303 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -1813,8 +1813,8 @@ static void arm_smmu_decode_event(struct arm_smmu_device *smmu, u64 *raw, mutex_unlock(&smmu->streams_mutex); } -static int arm_smmu_handle_event(struct arm_smmu_device *smmu, - struct arm_smmu_event *event) +static int arm_smmu_handle_event(struct arm_smmu_device *smmu, u64 *evt, + struct arm_smmu_event *event) { int ret = 0; u32 perm = 0; @@ -1823,6 +1823,10 @@ static int arm_smmu_handle_event(struct arm_smmu_device *smmu, struct iommu_fault *flt = &fault_evt.fault; switch (event->id) { + case EVT_ID_BAD_STE_CONFIG: + case EVT_ID_STREAM_DISABLED_FAULT: + case EVT_ID_BAD_SUBSTREAMID_CONFIG: + case EVT_ID_BAD_CD_CONFIG: case EVT_ID_TRANSLATION_FAULT: case EVT_ID_ADDR_SIZE_FAULT: case EVT_ID_ACCESS_FAULT: @@ -1832,31 +1836,30 @@ static int arm_smmu_handle_event(struct arm_smmu_device *smmu, return -EOPNOTSUPP; } - if (!event->stall) - return -EOPNOTSUPP; - - if (event->read) - perm |= IOMMU_FAULT_PERM_READ; - else - perm |= IOMMU_FAULT_PERM_WRITE; + if (event->stall) { + if (event->read) + perm |= IOMMU_FAULT_PERM_READ; + else + perm |= IOMMU_FAULT_PERM_WRITE; - if (event->instruction) - perm |= IOMMU_FAULT_PERM_EXEC; + if (event->instruction) + perm |= IOMMU_FAULT_PERM_EXEC; - if (event->privileged) - perm |= IOMMU_FAULT_PERM_PRIV; + if (event->privileged) + perm |= IOMMU_FAULT_PERM_PRIV; - flt->type = IOMMU_FAULT_PAGE_REQ; - flt->prm = (struct iommu_fault_page_request) { - .flags = IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE, - .grpid = event->stag, - .perm = perm, - .addr = event->iova, - }; + flt->type = IOMMU_FAULT_PAGE_REQ; + flt->prm = (struct iommu_fault_page_request){ + .flags = IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE, + .grpid = event->stag, + .perm = perm, + .addr = event->iova, + }; - if (event->ssv) { - flt->prm.flags |= IOMMU_FAULT_PAGE_REQUEST_PASID_VALID; - flt->prm.pasid = event->ssid; + if (event->ssv) { + flt->prm.flags |= IOMMU_FAULT_PAGE_REQUEST_PASID_VALID; + flt->prm.pasid = event->ssid; + } } mutex_lock(&smmu->streams_mutex); @@ -1866,7 +1869,14 @@ static int arm_smmu_handle_event(struct arm_smmu_device *smmu, goto out_unlock; } - ret = iommu_report_device_fault(master->dev, &fault_evt); + if (event->stall) { + ret = iommu_report_device_fault(master->dev, &fault_evt); + } else { + if (master->vmaster && !event->s2) + ret = arm_vmaster_report_event(master->vmaster, evt); + else + ret = -EFAULT; /* Unhandled events should be pinned */ + } out_unlock: mutex_unlock(&smmu->streams_mutex); return ret; @@ -1944,7 +1954,7 @@ static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev) do { while (!queue_remove_raw(q, evt)) { arm_smmu_decode_event(smmu, evt, &event); - if (arm_smmu_handle_event(smmu, &event)) + if (arm_smmu_handle_event(smmu, evt, &event)) arm_smmu_dump_event(smmu, evt, &event, &rs); put_device(event.dev);