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

[Peter Xu]

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 <address@hidden>
> > ---
> >  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

?

[...]

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

-- 
Peter Xu