RE: [PATCH v5 06/14] iommufd: Add IOMMUFD_OBJ_VEVENTQ and IOMMUFD_CMD_VEVENTQ_ALLOC

Tian, Kevin
2025-01-13T08:17:37Z

> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Monday, January 13, 2025 12:51 PM
>
> On Mon, Jan 13, 2025 at 02:52:32AM +0000, Tian, Kevin wrote:
> > > From: Nicolin Chen <nicolinc@nvidia.com>
> > > Sent: Saturday, January 11, 2025 5:29 AM
> > >
> > > On Fri, Jan 10, 2025 at 07:06:49AM +0000, Tian, Kevin wrote:
> > > > > From: Nicolin Chen <nicolinc@nvidia.com>
> > > > > Sent: Wednesday, January 8, 2025 1:10 AM
> > > > > +
> > > > > +int iommufd_veventq_alloc(struct iommufd_ucmd *ucmd)
> > > > > +{
> > > > > + struct iommu_veventq_alloc *cmd = ucmd->cmd;
> > > > > + struct iommufd_veventq *veventq;
> > > > > + struct iommufd_viommu *viommu;
> > > > > + int fdno;
> > > > > + int rc;
> > > > > +
> > > > > + if (cmd->flags || cmd->type ==
> IOMMU_VEVENTQ_TYPE_DEFAULT)
> > > > > + return -EOPNOTSUPP;
> > > > > +
> > > > > + 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;
> > > > > +
> > > >
> > > > I'm not sure about the necessity of above check. The event queue
> > > > is just a software struct with a user-specified format for the iommu
> > > > driver to report viommu event. The struct itself is not constrained
> > > > by the hardware capability, though I'm not sure a real usage in
> > > > which a smmu driver wants to report a vtd event. But legitimately
> > > > an user can create any type of event queues which might just be
> > > > never used.
> > >
> > > Allowing a random type that a driver will never use for reporting
> > > doesn't sound to make a lot of sense to me...
> > >
> > > That being said, yea..I guess we could drop the limit here, since
> > > it isn't going to break anything?
> > >
> > > > It sounds clearer to do the check when IOPF cap is actually enabled
> > > > on a device contained in the viommu. At that point check whether
> > > > a required type eventqueue has been created. If not then fail the
> > > > iopf enabling.
> > >
> > > Hmm, isn't IOPF a different channel?
> >
> > We have a fault queue for delivering IOPF on hwpt, when vIOMMU is
> > not involved
> >
> > Now with vIOMMU my understanding was that all events including
> > IOPF are delivered via the event queue in the vIOMMU. Just echoed
> > by the documentation patch:
> >
> > +- IOMMUFD_OBJ_VEVENTQ, representing a software queue for a
> vIOMMU to report its
> > + events such as translation faults occurred to a nested stage-1 and HW-
> specific
> > + events.
>
> Oh, looks like that line misguided you.. It should be non-PRI type
> of fault, e.g. a stage-1 DMA translation error should be forwarded
> to the guest. I can make it clearer.
>
> > >
> > > And a vEVENTQ is per vIOMMU, not necessarily per vDEVICE/device..
> >
> > Yes. My point was to verify whether the vEVENTQ type is compatible when
> > a nested faultable hwpt is created with vIOMMU as the parent. then when
> > attaching a device to the nested hwpt we dynamically turn on PRI on the
> > device just like how it's handled in the fault queue path.
>
> We will still have the fault queue:
> if (error is handled by PRI)
> report via fault queue; // need response
> else (error is handled by vEVENTQ)
> report via vEVENTQ; // no need of response
> else
> dump unhandled faults;
>
> > > > Then it reveals probably another todo in this series. Seems you still
> > > > let the smmu driver statically enable iopf when probing the device.
> > > > Sounds like iommufd_viommu_alloc_hwpt_nested() may accept
> > > > IOMMU_HWPT_FAULT_ID_VALID to refer to a event queue and
> > > > later dynamically enable/disable iopf when attaching a device to the
> > > > hwpt and check the event queue type there. Just like how the fault
> > > > object is handled.
> > >
> > > You've lost me here :-/
> > >
> >
> > Hope above explanation makes my point clearer. Then for a nested
> > hwpt created within a vIOMMU there is an open whether we want
> > a per-hwpt option to mark whether it allows fault, or assume that
> > every nested hwpt (and the devices attached to it) must be faultable
> > once any vEVENTQ is created in the vIOMMU.
>
> A vIOMMU-based nested HWPT should still enable IOPF via the flag
> IOMMU_HWPT_FAULT_ID_VALID.
>

Thanks for clarification. Clearly I got it messed. 😊