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

[Nicolin Chen <nicolinc@nvidia.com>]

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
Nicolin