| Message ID | 69a46c72e43ed086840be462eef731167d90a9d8.1735933254.git.nicolinc@nvidia.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | iommufd: Add vIOMMU infrastructure (Part-3: vEVENTQ) | expand |
On 1/4/25 03:43, Nicolin Chen wrote: > diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h > index 0a08aa82e7cc..55e3d5a14cca 100644 > --- a/include/uapi/linux/iommufd.h > +++ b/include/uapi/linux/iommufd.h > @@ -1016,9 +1016,24 @@ struct iommu_ioas_change_process { > /** > * 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. > + * (Refer to "7.3 Event records" in SMMUv3 HW Spec) > + * > + * 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]; > }; Nit: I think it would be more readable to add a check in the vevent reporting helper. diff --git a/drivers/iommu/iommufd/driver.c b/drivers/iommu/iommufd/driver.c index 77c34f8791ef..ccada0ada5ff 100644 --- a/drivers/iommu/iommufd/driver.c +++ b/drivers/iommu/iommufd/driver.c @@ -86,6 +86,9 @@ int iommufd_viommu_report_event(struct iommufd_viommu *viommu, if (WARN_ON_ONCE(!data_len || !event_data)) return -EINVAL; + if (WARN_ON_ONCE(type != IOMMU_VEVENTQ_TYPE_ARM_SMMUV3)) + return -EINVAL; + down_read(&viommu->veventqs_rwsem); veventq = iommufd_viommu_find_veventq(viommu, type); Or perhaps the compiler could automatically make a warning if the @type is not one of those values in enum iommu_veventq_type? Others look good to me. Thanks, baolu
On Mon, Jan 06, 2025 at 11:01:32AM +0800, Baolu Lu wrote: > On 1/4/25 03:43, Nicolin Chen wrote: > > diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h > > index 0a08aa82e7cc..55e3d5a14cca 100644 > > --- a/include/uapi/linux/iommufd.h > > +++ b/include/uapi/linux/iommufd.h > > @@ -1016,9 +1016,24 @@ struct iommu_ioas_change_process { > > /** > > * 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. > > + * (Refer to "7.3 Event records" in SMMUv3 HW Spec) > > + * > > + * 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]; > > }; > > Nit: I think it would be more readable to add a check in the vevent > reporting helper. > > diff --git a/drivers/iommu/iommufd/driver.c b/drivers/iommu/iommufd/driver.c > index 77c34f8791ef..ccada0ada5ff 100644 > --- a/drivers/iommu/iommufd/driver.c > +++ b/drivers/iommu/iommufd/driver.c > @@ -86,6 +86,9 @@ int iommufd_viommu_report_event(struct iommufd_viommu > *viommu, > if (WARN_ON_ONCE(!data_len || !event_data)) > return -EINVAL; > > + if (WARN_ON_ONCE(type != IOMMU_VEVENTQ_TYPE_ARM_SMMUV3)) > + return -EINVAL; > + Hmm, that's a good point I think. > down_read(&viommu->veventqs_rwsem); > > veventq = iommufd_viommu_find_veventq(viommu, type); ^ | We actually have been missing a type validation entirely, so the type could have been rejected by this function. Perhaps we should add a static list of supported types to struct iommufd_viommu_ops for drivers to report so that then the core could reject from the first place during a vEVENTQ allocation. > Or perhaps the compiler could automatically make a warning if the @type > is not one of those values in enum iommu_veventq_type? Just gave that a try. Mine doesn't give any warning. Not sure if needs to be some "-W" augment though.. > Others look good to me. Thanks for the review! Nicolin
On Mon, Jan 06, 2025 at 10:46:21AM -0800, Nicolin Chen wrote: > On Mon, Jan 06, 2025 at 11:01:32AM +0800, Baolu Lu wrote: > > Nit: I think it would be more readable to add a check in the vevent > > reporting helper. > > > > diff --git a/drivers/iommu/iommufd/driver.c b/drivers/iommu/iommufd/driver.c > > index 77c34f8791ef..ccada0ada5ff 100644 > > --- a/drivers/iommu/iommufd/driver.c > > +++ b/drivers/iommu/iommufd/driver.c > > @@ -86,6 +86,9 @@ int iommufd_viommu_report_event(struct iommufd_viommu > > *viommu, > > if (WARN_ON_ONCE(!data_len || !event_data)) > > return -EINVAL; > > > > + if (WARN_ON_ONCE(type != IOMMU_VEVENTQ_TYPE_ARM_SMMUV3)) > > + return -EINVAL; > > + > > Hmm, that's a good point I think. > > > down_read(&viommu->veventqs_rwsem); > > > > veventq = iommufd_viommu_find_veventq(viommu, type); > ^ > | > We actually have been missing a type validation entirely, so the > type could have been rejected by this function. Perhaps we should > add a static list of supported types to struct iommufd_viommu_ops > for drivers to report so that then the core could reject from the > first place during a vEVENTQ allocation. I added something like this. Will send a v5. 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 0c7a5894ba07..348179f3cf2a 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 @@ -399,9 +399,15 @@ static int arm_vsmmu_cache_invalidate(struct iommufd_viommu *viommu, return ret; } +static bool arm_vsmmu_supports_veventq(unsigned int type) +{ + return type == IOMMU_VIOMMU_TYPE_ARM_SMMUV3; +} + static const struct iommufd_viommu_ops arm_vsmmu_ops = { .alloc_domain_nested = arm_vsmmu_alloc_domain_nested, .cache_invalidate = arm_vsmmu_cache_invalidate, + .supports_veventq = arm_vsmmu_supports_veventq, }; struct iommufd_viommu *arm_vsmmu_alloc(struct device *dev, diff --git a/drivers/iommu/iommufd/eventq.c b/drivers/iommu/iommufd/eventq.c index bf0c04f571a7..b5be629f38ed 100644 --- a/drivers/iommu/iommufd/eventq.c +++ b/drivers/iommu/iommufd/eventq.c @@ -555,6 +555,11 @@ int iommufd_veventq_alloc(struct iommufd_ucmd *ucmd) viommu = iommufd_get_viommu(ucmd, cmd->viommu_id); if (IS_ERR(viommu)) return PTR_ERR(viommu); + + if (!viommu->ops || !viommu->ops->supports_veventq || + !viommu->ops->supports_veventq(cmd->type)) + return -EOPNOTSUPP; + down_write(&viommu->veventqs_rwsem); if (iommufd_viommu_find_veventq(viommu, cmd->type)) { diff --git a/include/linux/iommufd.h b/include/linux/iommufd.h index 6b3b46e382f8..55e71dca3664 100644 --- a/include/linux/iommufd.h +++ b/include/linux/iommufd.h @@ -117,6 +117,7 @@ struct iommufd_viommu { * array->entry_num to report the number of handled requests. * The data structure of the array entry must be defined in * include/uapi/linux/iommufd.h + * @supports_veventq: Whether the vIOMMU supports a given vEVENTQ type */ struct iommufd_viommu_ops { void (*destroy)(struct iommufd_viommu *viommu); @@ -125,6 +126,7 @@ struct iommufd_viommu_ops { const struct iommu_user_data *user_data); int (*cache_invalidate)(struct iommufd_viommu *viommu, struct iommu_user_data_array *array); + bool (*supports_veventq)(unsigned int type); }; #if IS_ENABLED(CONFIG_IOMMUFD)
On Mon, Jan 06, 2025 at 08:37:04PM -0800, Nicolin Chen wrote: > I added something like this. Will send a v5. > > 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 0c7a5894ba07..348179f3cf2a 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 > @@ -399,9 +399,15 @@ static int arm_vsmmu_cache_invalidate(struct iommufd_viommu *viommu, > return ret; > } > > +static bool arm_vsmmu_supports_veventq(unsigned int type) > +{ > + return type == IOMMU_VIOMMU_TYPE_ARM_SMMUV3; Oops. Corrected this: IOMMU_VEVENTQ_TYPE_ARM_SMMUV3 And added your bits too: @@ -83,6 +83,9 @@ int iommufd_viommu_report_event(struct iommufd_viommu *viommu, if (!viommu) return -ENODEV; + if (WARN_ON_ONCE(!viommu->ops || !viommu->ops->supports_veventq || + !viommu->ops->supports_veventq(type))) + return -EOPNOTSUPP; if (WARN_ON_ONCE(!data_len || !event_data)) return -EINVAL; Thanks! Nicolin
On 1/7/25 12:36, Nicolin Chen wrote: > On Mon, Jan 06, 2025 at 10:46:21AM -0800, Nicolin Chen wrote: >> On Mon, Jan 06, 2025 at 11:01:32AM +0800, Baolu Lu wrote: >>> Nit: I think it would be more readable to add a check in the vevent >>> reporting helper. >>> >>> diff --git a/drivers/iommu/iommufd/driver.c b/drivers/iommu/iommufd/driver.c >>> index 77c34f8791ef..ccada0ada5ff 100644 >>> --- a/drivers/iommu/iommufd/driver.c >>> +++ b/drivers/iommu/iommufd/driver.c >>> @@ -86,6 +86,9 @@ int iommufd_viommu_report_event(struct iommufd_viommu >>> *viommu, >>> if (WARN_ON_ONCE(!data_len || !event_data)) >>> return -EINVAL; >>> >>> + if (WARN_ON_ONCE(type != IOMMU_VEVENTQ_TYPE_ARM_SMMUV3)) >>> + return -EINVAL; >>> + >> Hmm, that's a good point I think. >> >>> down_read(&viommu->veventqs_rwsem); >>> >>> veventq = iommufd_viommu_find_veventq(viommu, type); >> ^ >> | >> We actually have been missing a type validation entirely, so the >> type could have been rejected by this function. Perhaps we should >> add a static list of supported types to struct iommufd_viommu_ops >> for drivers to report so that then the core could reject from the >> first place during a vEVENTQ allocation. > I added something like this. Will send a v5. > > 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 0c7a5894ba07..348179f3cf2a 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 > @@ -399,9 +399,15 @@ static int arm_vsmmu_cache_invalidate(struct iommufd_viommu *viommu, > return ret; > } > > +static bool arm_vsmmu_supports_veventq(unsigned int type) > +{ > + return type == IOMMU_VIOMMU_TYPE_ARM_SMMUV3; Do you need to check the hardware capabilities before reporting this? I am not familiar with the ARM architecture, but typically it's better to make it like this, static bool arm_vsmmu_supports_veventq(struct iommufd_viommu *viommu, enum iommu_veventq_type type) { if (type != IOMMU_VEVENTQ_TYPE_ARM_SMMUV3) return false; if (hardware_not_capable(viommu)) return false; return true; } > +} > + > static const struct iommufd_viommu_ops arm_vsmmu_ops = { > .alloc_domain_nested = arm_vsmmu_alloc_domain_nested, > .cache_invalidate = arm_vsmmu_cache_invalidate, > + .supports_veventq = arm_vsmmu_supports_veventq, > }; Others look good to me. Thanks, baolu
On Tue, Jan 07, 2025 at 01:54:00PM +0800, Baolu Lu wrote: > On 1/7/25 12:36, Nicolin Chen wrote: > > +static bool arm_vsmmu_supports_veventq(unsigned int type) > > +{ > > + return type == IOMMU_VIOMMU_TYPE_ARM_SMMUV3; > > Do you need to check the hardware capabilities before reporting this? I > am not familiar with the ARM architecture, but typically it's better to > make it like this, > > static bool arm_vsmmu_supports_veventq(struct iommufd_viommu *viommu, > enum iommu_veventq_type type) > { > if (type != IOMMU_VEVENTQ_TYPE_ARM_SMMUV3) > return false; > > if (hardware_not_capable(viommu)) > return false; I think the ARM version of viommu_alloc op has already implemented enough capability checks and rejected any of hardware_not_capable. So, viommu shouldn't be a thing that the driver could actually use to call this helper :) Thanks Nicolin
On 1/7/25 14:00, Nicolin Chen wrote: > On Tue, Jan 07, 2025 at 01:54:00PM +0800, Baolu Lu wrote: >> On 1/7/25 12:36, Nicolin Chen wrote: >>> +static bool arm_vsmmu_supports_veventq(unsigned int type) >>> +{ >>> + return type == IOMMU_VIOMMU_TYPE_ARM_SMMUV3; >> Do you need to check the hardware capabilities before reporting this? I >> am not familiar with the ARM architecture, but typically it's better to >> make it like this, >> >> static bool arm_vsmmu_supports_veventq(struct iommufd_viommu *viommu, >> enum iommu_veventq_type type) >> { >> if (type != IOMMU_VEVENTQ_TYPE_ARM_SMMUV3) >> return false; >> >> if (hardware_not_capable(viommu)) >> return false; > I think the ARM version of viommu_alloc op has already implemented > enough capability checks and rejected any of hardware_not_capable. > So, viommu shouldn't be a thing that the driver could actually use > to call this helper
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 4435ad7db776..d24c3d8ee397 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h @@ -1066,6 +1066,7 @@ struct iommufd_viommu *arm_vsmmu_alloc(struct device *dev, 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); +int arm_vmaster_report_event(struct arm_smmu_vmaster *vmaster, u64 *evt); #else #define arm_smmu_hw_info NULL #define arm_vsmmu_alloc NULL @@ -1081,6 +1082,12 @@ static inline void arm_smmu_attach_commit_vmaster(struct arm_smmu_attach_state *state) { } + +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 0a08aa82e7cc..55e3d5a14cca 100644 --- a/include/uapi/linux/iommufd.h +++ b/include/uapi/linux/iommufd.h @@ -1016,9 +1016,24 @@ struct iommu_ioas_change_process { /** * 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. + * (Refer to "7.3 Event records" in SMMUv3 HW Spec) + * + * 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 2b6253ef0e8f..f88fd2f5cae8 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 @@ -447,4 +447,20 @@ 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 = + *(struct iommu_vevent_arm_smmuv3 *)evt; + + vevt.evt[0] &= ~EVTQ_0_SID; + vevt.evt[0] |= FIELD_PREP(EVTQ_0_SID, vmaster->vsid); + + vevt.evt[0] = cpu_to_le64(vevt.evt[0]); + vevt.evt[1] = cpu_to_le64(vevt.evt[1]); + + 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 686c171dd273..59fbc342a095 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -1812,8 +1812,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; @@ -1831,31 +1831,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); @@ -1865,7 +1864,16 @@ 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 { + down_read(&master->vmaster_rwsem); + if (master->vmaster && !event->s2) + ret = arm_vmaster_report_event(master->vmaster, evt); + else + ret = -EFAULT; /* Unhandled events should be pinned */ + up_read(&master->vmaster_rwsem); + } out_unlock: mutex_unlock(&smmu->streams_mutex); return ret; @@ -1943,7 +1951,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);
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. Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> --- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 7 +++ include/uapi/linux/iommufd.h | 15 +++++ .../arm/arm-smmu-v3/arm-smmu-v3-iommufd.c | 16 +++++ drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 58 +++++++++++-------- 4 files changed, 71 insertions(+), 25 deletions(-)