On Tue, Oct 29, 2019 at 01:15:44PM +0100, David Gibson wrote:
> > +union IOMMUCTXPASIDReqDesc {
> > + struct {
> > + uint32_t min_pasid;
> > + uint32_t max_pasid;
> > + int32_t alloc_result; /* pasid allocated for the alloc request */
> > + };
> > + struct {
> > + uint32_t pasid; /* pasid to be free */
> > + int free_result;
> > + };
> > +};
>
> Apart from theproblem with writable fields, using a big union for
> event data is pretty ugly. If you need this different information for
> the different events, it might make more sense to have a separate
> notifier chain with a separate call interface for each event type,
> rather than trying to multiplex them together.
I have no issue on the union definiion, however I do agree that it's a
bit awkward to register one notifier for each event.
Instead of introducing even more notifier chains, I'm thinking whether
we can simply provide a single notifier hook for all the four events.
After all I don't see in what case we'll only register some of the
events, like we can't register alloc_pasid() without registering to
free_pasid() because otherwise it does not make sense.. And also you
have the wrapper struct ("IOMMUCTXEventData") which contains the event
type, so the notify() hook will know which message is this.
A side note is that I think you don't need the
IOMMUCTXEventData.length. If you see the code, vtd_bind_guest_pasid()
does not even initialize length right now, and I think it could still
work only because none of the vfio notify() hook
(e.g. vfio_iommu_pasid_bind_notify) checks that length...
--
Peter Xu
