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

[Liu, Yi L]

> From: Peter Xu
> Sent: Saturday, November 2, 2019 1:26 AM
> To: David Gibson <address@hidden>
> Subject: Re: [RFC v2 09/22] vfio/pci: add iommu_context notifier for pasid 
> alloc/free
> 
> 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.

Got it. Would fix it in next version.

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

I'm in with this proposal. This makes the notifier chain smaller.

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

yes, will fix it.

> --
> Peter Xu