Re: [RFC v2 09/22] vfio/pci: add iommu_context notifier for pasid alloc/free

[Peter Xu]

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