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

[Auger Eric]

Hi Peter,

On 5/28/19 6:48 AM, Peter Xu wrote:
> 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 {
>       ...
>     };
>   }
> }

I am currently prototyping your suggestion. I think this would clarify
some parts of the code to see clearly the type of event that is
propagated. I will send a separate RFC for this change.

Thanks!

Eric
> 
> 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,
>