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

[Peter Xu]

On Thu, May 17, 2018 at 12:10:41PM +0200, Auger Eric wrote:
> 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 <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
> Hum I missed not all of them belonged to that list. a bit long now?
> vtd_as_with_notifiers?

Okay I can use that.  Regarding to the other "s"s issues - I think
I'll just drop those comments since they aren't really helpful after
all.  Thanks,

-- 
Peter Xu