Re: [Qemu-arm] [RFC v4 09/27] memory: Prepare for different kinds of IOMMU MR notifiers

[Peter Xu]

On Mon, May 27, 2019 at 01:41:45PM +0200, Eric Auger wrote:

[...]

> @@ -3368,8 +3368,9 @@ static void vtd_address_space_unmap(VTDAddressSpace 
> *as, IOMMUNotifier *n)
>  {
>      IOMMUTLBEntry entry;
>      hwaddr size;
> -    hwaddr start = n->start;
> -    hwaddr end = n->end;
> +

(extra new line)

> +    hwaddr start = n->iotlb_notifier.start;
> +    hwaddr end = n->iotlb_notifier.end;
>      IntelIOMMUState *s = as->iommu_state;
>      DMAMap map;

[...]

>  typedef void (*IOMMUNotify)(struct IOMMUNotifier *notifier,
>                              IOMMUTLBEntry *data);
>  
> -struct IOMMUNotifier {
> +typedef struct IOMMUIOLTBNotifier {
>      IOMMUNotify notify;

Hi, Eric,

I wasn't following the thread much before so sorry to ask this if too
late - have you thought about using the Notifier struct direct?
Because then it'll (1) allow the user to register with both IOTLB |
CONFIG flags in the same notifier while currently we'll need to
register one for each (and this worries me a bit on when we grow the
types of flags further then one register can have quite a few
notifiers) (2) the notifier part can be shared by different events.
Then when notify the (void *) data can be an union:

struct IOMMUEvent {
  int event; // can be one of the notifier flags
  union {
    struct IOTLBEvent {
      ...
    };
    struct PASIDEvent {
      ...
    };
  }
}

Then the handler hook would be simple too:

handler (data)
{
  switch (data.event) {
    ...
  }
}

I would be fine with current patch if this series is close to be
merged because even if we want that we can do that on top when we
introduce even more notifiers, but just to ask loud first.

> -    IOMMUNotifierFlag notifier_flags;
>      /* Notify for address space range start <= addr <= end */
>      hwaddr start;
>      hwaddr end;
> +} IOMMUIOLTBNotifier;
> +
> +struct IOMMUNotifier {
> +    IOMMUNotifierFlag notifier_flags;
> +    union {
> +        IOMMUIOLTBNotifier iotlb_notifier;
> +    };
>      int iommu_idx;
>      QLIST_ENTRY(IOMMUNotifier) node;
>  };
> @@ -126,15 +132,18 @@ typedef struct IOMMUNotifier IOMMUNotifier;
>  /* RAM is a persistent kind memory */
>  #define RAM_PMEM (1 << 5)
>  
> -static inline void iommu_notifier_init(IOMMUNotifier *n, IOMMUNotify fn,
> -                                       IOMMUNotifierFlag flags,
> -                                       hwaddr start, hwaddr end,
> -                                       int iommu_idx)
> +static inline void iommu_iotlb_notifier_init(IOMMUNotifier *n, IOMMUNotify 
> fn,
> +                                             IOMMUNotifierFlag flags,
> +                                             hwaddr start, hwaddr end,
> +                                             int iommu_idx)
>  {
> -    n->notify = fn;
> +    assert(flags & IOMMU_NOTIFIER_IOTLB_MAP ||
> +           flags & IOMMU_NOTIFIER_IOTLB_UNMAP);

Can use IOMMU_NOTIFIER_IOTLB_ALL directly?

> +    assert(start < end);
>      n->notifier_flags = flags;
> -    n->start = start;
> -    n->end = end;
> +    n->iotlb_notifier.notify = fn;
> +    n->iotlb_notifier.start = start;
> +    n->iotlb_notifier.end = end;
>      n->iommu_idx = iommu_idx;
>  }

Otherwise the patch looks good to me.

Regards,

-- 
Peter Xu