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.
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.
>
> 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. :)
>
> 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. :)
IMHO we can keep the IOMMUAccessFlags scemantics, since it's still correct for
a general translated IOMMUTLBEntry object.
Thanks,
--
Peter Xu
