Re: [PATCH v6 2/6] iommufd: Add IOMMU_HWPT_INVALIDATE

[Yi Liu]

On 2023/12/1 15:10, Tian, Kevin wrote:
> > From: Liu, Yi L <yi.l.liu@xxxxxxxxx>
> > Sent: Friday, December 1, 2023 3:05 PM
> >
> > On 2023/12/1 13:19, Tian, Kevin wrote:
> > > > From: Nicolin Chen <nicolinc@xxxxxxxxxx>
> > > > Sent: Friday, December 1, 2023 12:50 PM
> > > >
> > > > On Fri, Dec 01, 2023 at 11:51:26AM +0800, Yi Liu wrote:
> > > > > On 2023/11/29 08:57, Jason Gunthorpe wrote:
> > > > > > On Tue, Nov 28, 2023 at 04:51:21PM -0800, Nicolin Chen wrote:
> > > > > > > > > I also thought about making this out_driver_error_code per HW.
> > > > > > > > > Yet, an error can be either per array or per entry/quest. The
> > > > > > > > > array-related error should be reported in the array structure
> > > > > > > > > that is a core uAPI, v.s. the per-HW entry structure. Though
> > > > > > > > > we could still report an array error in the entry structure
> > > > > > > > > at the first entry (or indexed by "array->entry_num")?
> > > > > > > >
> > > > > > > > why would there be an array error? array is just a software
> > > > > > > > entity containing actual HW invalidation cmds. If there is
> > > > > > > > any error with the array itself it should be reported via
> > > > > > > > ioctl errno.
> > > > > > >
> > > > > > > User array reading is a software operation, but kernel array
> > > > > > > reading is a hardware operation that can raise an error when
> > > > > > > the memory location to the array is incorrect or so.
> > > > > >
> > > > > > Well, we shouldn't get into a situation like that.. By the time the HW
> > > > > > got the address it should be valid.
> > > > > >
> > > > > > > With that being said, I think errno (-EIO) could do the job,
> > > > > > > as you suggested too.
> > > > > >
> > > > > > Do we have any idea what HW failures can be generated by the
> > > > commands
> > > > > > this will execture? IIRC I don't remember seeing any smmu specific
> > > > > > codes related to invalid invalidation? Everything is a valid input?
> > > > > >
> > > > > > Can vt-d fail single commands? What about AMD?
> > > > >
> > > > > Intel VT-d side, after each invalidation request, there is a wait
> > > > > descriptor which either provide an interrupt or an address for the
> > > > > hw to notify software the request before the wait descriptor has been
> > > > > completed. While, if there is error happened on the invalidation request,
> > > > > a flag (IQE, ICE, ITE) would be set in the Fault Status Register, and some
> > > > > detailed information would be recorded in the Invalidation Queue Error
> > > > > Record Register. So an invalidation request may be failed with some
> > error
> > > > > reported. If no error, will return completion via the wait descriptor. Is
> > > > > this what you mean by "fail a single command"?
> > > >
> > > > I see the current VT-d series marking those as "REVISIT". How
> > > > will it report an error to the user space from those register?
> > > >
> > > > Are they global status registers so that it might be difficult
> > > > to direct the error to the nested domain for an event fd?
> > >
> > > They are global registers but invalidation queue is also the global
> > > resource. intel-iommu driver polls the status register after queueing
> > > new invalidation descriptors. The submission is serialized.
> > >
> > > If the error is related to a descriptor itself (e.g. format issue) then
> > > the head register points to the problematic descriptor so software
> > > can direct it to the related domain.
> > >
> > > If the error is related to device tlb invalidation (e.g. timeout) there
> > > is no way to associate the error with a specific descriptor by current
> > > spec. But intel-iommu driver batches descriptors per domain so
> > > we can still direct the error to the nested domain.
> > >
> > > But I don't see the need of doing it via eventfd.
> > >
> > > The poll semantics in intel-iommu driver is essentially a sync model.
> > > vt-d spec does allow software to optionally enable notification upon
> > > those errors but it's not used so far.
> > >
> > > With that I still prefer to having driver-specific error code defined
> > > in the entry. If ARM is an event-driven model then we can define
> > > that field at least in vtd specific data structure.
> > >
> > > btw given vtd doesn't use native format in uAPI it doesn't make
> > > sense to forward descriptor formatting errors back to userspace.
> > > Those, if happen, are driver's own problem. intel-iommu driver
> > > should verify the uAPI structure and return -EINVAL or proper
> > > errno to userspace purely in software.
> > >
> > > With that Yi please just define error codes for device tlb related
> > > errors for vtd.
> >
> > hmmm, this sounds like customized error code. is it? So far, VT-d
>
> yes. there is no need to replicate hardware registers/bits if most
> of them are irrelevant to userspace.
>
> > spec has two errors (ICE and ITE). ITE is valuable to let userspace
> > know. For ICE, looks like no much value. Intel iommu driver should
> > be responsible to submit a valid device-tlb invalidation to device.
>
> it's an invalid completion message from the device which could be
> caused by various reasons (not exactly due to the invalidation
> request by iommu driver). so it still makes sense to forward.

ok. so we may need to define a field to forward the detailed info to
user as well. This data is error-code specific. @Nic, are we aligned
that the error_code field and error data reporting should be moved
to the driver-specific part since it is different between vendors?

--
Regards,
Yi Liu