Re: [Qemu-devel] [PATCH v2 02/10] intel-iommu: remove IntelIOMMUNotifierNode

[Auger Eric <eric.auger@redhat.com>]

Hi Peter,

On 05/17/2018 12:02 PM, Peter Xu wrote:
> On Thu, May 17, 2018 at 11:46:22AM +0200, Auger Eric wrote:
>> Hi Peter,
>>
>> On 05/04/2018 05:08 AM, Peter Xu wrote:
>>> That is not really necessary.  Removing that node struct and put the
>>> list entry directly into VTDAddressSpace.  It simplfies the code a lot.
>>>
>>> Signed-off-by: Peter Xu <peterx@redhat.com>
>>> ---
>>>  include/hw/i386/intel_iommu.h |  9 ++------
>>>  hw/i386/intel_iommu.c         | 41 ++++++++++-------------------------
>>>  2 files changed, 14 insertions(+), 36 deletions(-)
>>>
>>> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
>>> index 45ec8919b6..220697253f 100644
>>> --- a/include/hw/i386/intel_iommu.h
>>> +++ b/include/hw/i386/intel_iommu.h
>>> @@ -67,7 +67,6 @@ typedef union VTD_IR_TableEntry VTD_IR_TableEntry;
>>>  typedef union VTD_IR_MSIAddress VTD_IR_MSIAddress;
>>>  typedef struct VTDIrq VTDIrq;
>>>  typedef struct VTD_MSIMessage VTD_MSIMessage;
>>> -typedef struct IntelIOMMUNotifierNode IntelIOMMUNotifierNode;
>>>  
>>>  /* Context-Entry */
>>>  struct VTDContextEntry {
>>> @@ -93,6 +92,7 @@ struct VTDAddressSpace {
>>>      MemoryRegion iommu_ir;      /* Interrupt region: 0xfeeXXXXX */
>>>      IntelIOMMUState *iommu_state;
>>>      VTDContextCacheEntry context_cache_entry;
>>> +    QLIST_ENTRY(VTDAddressSpace) next;
>>>  };
>>>  
>>>  struct VTDBus {
>>> @@ -253,11 +253,6 @@ struct VTD_MSIMessage {
>>>  /* When IR is enabled, all MSI/MSI-X data bits should be zero */
>>>  #define VTD_IR_MSI_DATA          (0)
>>>  
>>> -struct IntelIOMMUNotifierNode {
>>> -    VTDAddressSpace *vtd_as;
>>> -    QLIST_ENTRY(IntelIOMMUNotifierNode) next;
>>> -};
>>> -
>>>  /* The iommu (DMAR) device state struct */
>>>  struct IntelIOMMUState {
>>>      X86IOMMUState x86_iommu;
>>> @@ -295,7 +290,7 @@ struct IntelIOMMUState {
>>>      GHashTable *vtd_as_by_busptr;   /* VTDBus objects indexed by PCIBus* reference */
>>>      VTDBus *vtd_as_by_bus_num[VTD_PCI_BUS_MAX]; /* VTDBus objects indexed by bus number */
>>>      /* list of registered notifiers */
>>> -    QLIST_HEAD(, IntelIOMMUNotifierNode) notifiers_list;
>>> +    QLIST_HEAD(, VTDAddressSpace) notifiers_list;
>> Wouldn't it make sense to rename notifiers_list into something more
>> understandable like address_spaces?
> 
> But address_spaces might be a bit confusing too on the other side as
> "a list of all VT-d address spaces".  How about something like:
> 
>      address_spaces_with_notifiers
Hum I missed not all of them belonged to that list. a bit long now?
vtd_as_with_notifiers?

Thanks

Eric
> 
> ?
> 
> [...]
> 
>>> -    /* update notifier node with new flags */
>>> -    QLIST_FOREACH_SAFE(node, &s->notifiers_list, next, next_node) {
>>> -        if (node->vtd_as == vtd_as) {
>>> -            if (new == IOMMU_NOTIFIER_NONE) {
>>> -                QLIST_REMOVE(node, next);
>>> -                g_free(node);
>>> -            }
>>> -            return;
>>> -        }
>>> +        /* Insert new ones */
>> s/ones/one
>>
>>> +        QLIST_INSERT_HEAD(&s->notifiers_list, vtd_as, next);
>>> +    } else if (new == IOMMU_NOTIFIER_NONE) {
>>> +        /* Remove old ones */
>> same. Not sure the comments are worth.
> 
> Will remove two "s"s there.  Thanks,
>