> From: Peter Xu
> Sent: Saturday, November 2, 2019 1:26 AM
> To: David Gibson <david@xxxxxxxxxxxxxxxxxxxxx>
> 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
