Re: [RFC v6 00/13] memory: Delete assertion in memory_region_unregister_iommu_notifier

[Eugenio Perez Martin]

On Wed, Aug 26, 2020 at 5:54 PM Peter Xu <peterx@redhat.com> wrote:
>
> On Wed, Aug 26, 2020 at 05:00:30PM +0200, Eugenio Perez Martin wrote:
> > Hi!
> >
> > Sending v6 to see if that is on the same page as what you meant.
> > Making each setting of "type" explicitly IOMMU_IOTLB_NONE if not used
> > in notifications. This is done in different commits in case this helps
> > review of different architectures.
>
> I've also proposed IOMMUTLBEvent in the other reply, that might help too.
>

I think IOMMUTLBEvent would be way clearer, yes :).

> Since at it, there's also another trick to use - we don't need to touch those
> "type" as long as the default type is "zero", so as long as we make sure the
> default type (IOMMU_NOTIFIER_NONE) is zero, then we don't need to set it
> everywhere either.
>

Right, I was just making it explicit.

> >
> > I think that this way we have too much freedom between entry flags
> > (currently only access type, RW) and notification type. Since not all
> > of them are valid nor used in the same context, I think this adds
> > complexity. I'm wondering if:
> >
> > Option a) We could make it private to memory.c, and make it a flag of
> > memory_region_notify_iommu (like "bool deviotlb_type)". IOW, instead
> > of making it a member of IOMMUTLBEntry, wrap the "entry" parameter of
> > memory_region_notify_iommu in a new private structure defined in
> > memory.c that adds that flag.
>
> No strong preference from me.  But since you posted the series before you
> provide the options... Maybe continue with what we have can be easier. :)
>

I just wanted to be sure I understood your proposal before comparing :)

> >
> > Option b) We could keep the IOMMUTLBNotificationType enum (open to
> > suggestions for a better name :)), but not embed it in the struct,
> > like:
> >
> > diff --git a/include/exec/memory.h b/include/exec/memory.h
> > index 477c3af24c..d9150e7b7e 100644
> > --- a/include/exec/memory.h
> > +++ b/include/exec/memory.h
> > @@ -72,7 +72,8 @@ typedef enum {
> >      IOMMU_RO   = 1,
> >      IOMMU_WO   = 2,
> >      IOMMU_RW   = 3,
> > -} IOMMUAccessFlags;
> > +    IOMMU_DEVIOTLB = 4,
> > +} IOMMUEntryFlags;
>
> Just in case you didn't notice - IOMMUAccessFlags is actaully a bitmap. :)
>

I know I know, that is why I compared with IOMMU_RW in proposed
iommu_tlb_entry_type.

Thanks!

> IMHO we can keep the IOMMUAccessFlags scemantics, since it's still correct for
> a general translated IOMMUTLBEntry object.
>
> Thanks,
>
> --
> Peter Xu
>