From: Nicolin Chen <redacted>
Sent: Monday, January 13, 2025 12:51 PM

On Mon, Jan 13, 2025 at 02:52:32AM +0000, Tian, Kevin wrote:

quoted

quoted

From: Nicolin Chen <redacted>
Sent: Saturday, January 11, 2025 5:29 AM

On Fri, Jan 10, 2025 at 07:06:49AM +0000, Tian, Kevin wrote:

quoted

quoted

From: Nicolin Chen <redacted>
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)

quoted

quoted

quoted

quoted

+		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?

quoted

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

quoted

+  events such as translation faults occurred to a nested stage-1 and HW-

specific

quoted

+  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.

quoted

quoted

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;

quoted

quoted

quoted

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. 😊