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