[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 3/6] x86/vtd: Drop struct qi_ctrl



> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx]
> Sent: 22 February 2019 19:13
> To: Xen-devel <xen-devel@xxxxxxxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Jan Beulich 
> <JBeulich@xxxxxxxx>; Paul Durrant
> <Paul.Durrant@xxxxxxxxxx>; Kevin Tian <kevin.tian@xxxxxxxxx>
> Subject: [PATCH 3/6] x86/vtd: Drop struct qi_ctrl
> 
> It is unclear why this abstraction exists, but iommu_qi_ctrl() returns
> possibly NULL and every user unconditionally dereferences the result.  In
> practice, I can't spot a path where iommu is NULL, so I think it is mostly
> dead.
> 
> Move the sole member into struct vtd_iommu, and delete iommu_qi_ctrl().
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Paul Durrant <paul.durrant@xxxxxxxxxx>
> CC: Kevin Tian <kevin.tian@xxxxxxxxx>
> ---
>  xen/drivers/passthrough/vtd/iommu.h  | 13 +++--------
>  xen/drivers/passthrough/vtd/qinval.c | 42 
> +++++++++++++++---------------------
>  2 files changed, 20 insertions(+), 35 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/vtd/iommu.h 
> b/xen/drivers/passthrough/vtd/iommu.h
> index 556b3d6..12b531c 100644
> --- a/xen/drivers/passthrough/vtd/iommu.h
> +++ b/xen/drivers/passthrough/vtd/iommu.h
> @@ -506,10 +506,6 @@ extern struct list_head acpi_drhd_units;
>  extern struct list_head acpi_rmrr_units;
>  extern struct list_head acpi_ioapic_units;
> 
> -struct qi_ctrl {
> -    u64 qinval_maddr;  /* queue invalidation page machine address */
> -};
> -
>  struct ir_ctrl {
>      u64 iremap_maddr;            /* interrupt remap table machine address */
>      int iremap_num;              /* total num of used interrupt remap entry 
> */
> @@ -527,7 +523,6 @@ struct iommu_flush {
>  };
> 
>  struct intel_iommu {
> -    struct qi_ctrl qi_ctrl;
>      struct ir_ctrl ir_ctrl;
>      struct iommu_flush flush;
>      struct acpi_drhd_unit *drhd;
> @@ -545,16 +540,14 @@ struct vtd_iommu {
>      u64 root_maddr; /* root entry machine address */
>      struct msi_desc msi;
>      struct intel_iommu *intel;
> +
> +    uint64_t qinval_maddr;   /* queue invalidation page machine address */
> +
>      struct list_head ats_devices;
>      unsigned long *domid_bitmap;  /* domain id bitmap */
>      u16 *domid_map;               /* domain id mapping array */
>  };
> 
> -static inline struct qi_ctrl *iommu_qi_ctrl(struct vtd_iommu *iommu)
> -{
> -    return iommu ? &iommu->intel->qi_ctrl : NULL;
> -}
> -
>  static inline struct ir_ctrl *iommu_ir_ctrl(struct vtd_iommu *iommu)
>  {
>      return iommu ? &iommu->intel->ir_ctrl : NULL;
> diff --git a/xen/drivers/passthrough/vtd/qinval.c 
> b/xen/drivers/passthrough/vtd/qinval.c
> index 3ddb9b6..f6fcee5 100644
> --- a/xen/drivers/passthrough/vtd/qinval.c
> +++ b/xen/drivers/passthrough/vtd/qinval.c
> @@ -84,7 +84,7 @@ static int __must_check 
> queue_invalidate_context_sync(struct vtd_iommu *iommu,
> 
>      spin_lock_irqsave(&iommu->register_lock, flags);
>      index = qinval_next_index(iommu);
> -    entry_base = iommu_qi_ctrl(iommu)->qinval_maddr +
> +    entry_base = iommu->qinval_maddr +
>                   ((index >> QINVAL_ENTRY_ORDER) << PAGE_SHIFT);

^ This calculation looks worthy of a macro or an inline. It is repeated a lot.

  Paul

>      qinval_entries = map_vtd_domain_page(entry_base);
>      qinval_entry = &qinval_entries[index % (1 << QINVAL_ENTRY_ORDER)];
> @@ -118,7 +118,7 @@ static int __must_check 
> queue_invalidate_iotlb_sync(struct vtd_iommu *iommu,
> 
>      spin_lock_irqsave(&iommu->register_lock, flags);
>      index = qinval_next_index(iommu);
> -    entry_base = iommu_qi_ctrl(iommu)->qinval_maddr +
> +    entry_base = iommu->qinval_maddr +
>                   ((index >> QINVAL_ENTRY_ORDER) << PAGE_SHIFT);
>      qinval_entries = map_vtd_domain_page(entry_base);
>      qinval_entry = &qinval_entries[index % (1 << QINVAL_ENTRY_ORDER)];
> @@ -155,7 +155,7 @@ static int __must_check queue_invalidate_wait(struct 
> vtd_iommu *iommu,
> 
>      spin_lock_irqsave(&iommu->register_lock, flags);
>      index = qinval_next_index(iommu);
> -    entry_base = iommu_qi_ctrl(iommu)->qinval_maddr +
> +    entry_base = iommu->qinval_maddr +
>                   ((index >> QINVAL_ENTRY_ORDER) << PAGE_SHIFT);
>      qinval_entries = map_vtd_domain_page(entry_base);
>      qinval_entry = &qinval_entries[index % (1 << QINVAL_ENTRY_ORDER)];
> @@ -201,9 +201,7 @@ static int __must_check queue_invalidate_wait(struct 
> vtd_iommu *iommu,
> 
>  static int __must_check invalidate_sync(struct vtd_iommu *iommu)
>  {
> -    struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu);
> -
> -    ASSERT(qi_ctrl->qinval_maddr);
> +    ASSERT(iommu->qinval_maddr);
> 
>      return queue_invalidate_wait(iommu, 0, 1, 1, 0);
>  }
> @@ -211,10 +209,9 @@ static int __must_check invalidate_sync(struct vtd_iommu 
> *iommu)
>  static int __must_check dev_invalidate_sync(struct vtd_iommu *iommu,
>                                              struct pci_dev *pdev, u16 did)
>  {
> -    struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu);
>      int rc;
> 
> -    ASSERT(qi_ctrl->qinval_maddr);
> +    ASSERT(iommu->qinval_maddr);
>      rc = queue_invalidate_wait(iommu, 0, 1, 1, 1);
>      if ( rc == -ETIMEDOUT )
>      {
> @@ -248,7 +245,7 @@ int qinval_device_iotlb_sync(struct vtd_iommu *iommu, 
> struct pci_dev *pdev,
>      ASSERT(pdev);
>      spin_lock_irqsave(&iommu->register_lock, flags);
>      index = qinval_next_index(iommu);
> -    entry_base = iommu_qi_ctrl(iommu)->qinval_maddr +
> +    entry_base = iommu->qinval_maddr +
>                   ((index >> QINVAL_ENTRY_ORDER) << PAGE_SHIFT);
>      qinval_entries = map_vtd_domain_page(entry_base);
>      qinval_entry = &qinval_entries[index % (1 << QINVAL_ENTRY_ORDER)];
> @@ -282,7 +279,7 @@ static int __must_check queue_invalidate_iec_sync(struct 
> vtd_iommu *iommu,
> 
>      spin_lock_irqsave(&iommu->register_lock, flags);
>      index = qinval_next_index(iommu);
> -    entry_base = iommu_qi_ctrl(iommu)->qinval_maddr +
> +    entry_base = iommu->qinval_maddr +
>                   ((index >> QINVAL_ENTRY_ORDER) << PAGE_SHIFT);
>      qinval_entries = map_vtd_domain_page(entry_base);
>      qinval_entry = &qinval_entries[index % (1 << QINVAL_ENTRY_ORDER)];
> @@ -325,9 +322,8 @@ static int __must_check flush_context_qi(void *_iommu, 
> u16 did,
>                                           bool_t flush_non_present_entry)
>  {
>      struct vtd_iommu *iommu = _iommu;
> -    struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu);
> 
> -    ASSERT(qi_ctrl->qinval_maddr);
> +    ASSERT(iommu->qinval_maddr);
> 
>      /*
>       * In the non-present entry flush case, if hardware doesn't cache
> @@ -355,9 +351,8 @@ static int __must_check flush_iotlb_qi(void *_iommu, u16 
> did, u64 addr,
>      u8 dr = 0, dw = 0;
>      int ret = 0, rc;
>      struct vtd_iommu *iommu = _iommu;
> -    struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu);
> 
> -    ASSERT(qi_ctrl->qinval_maddr);
> +    ASSERT(iommu->qinval_maddr);
> 
>      /*
>       * In the non-present entry flush case, if hardware doesn't cache
> @@ -397,7 +392,6 @@ static int __must_check flush_iotlb_qi(void *_iommu, u16 
> did, u64 addr,
>  int enable_qinval(struct vtd_iommu *iommu)
>  {
>      struct acpi_drhd_unit *drhd;
> -    struct qi_ctrl *qi_ctrl;
>      struct iommu_flush *flush;
>      u32 sts;
>      unsigned long flags;
> @@ -405,24 +399,22 @@ int enable_qinval(struct vtd_iommu *iommu)
>      if ( !ecap_queued_inval(iommu->ecap) || !iommu_qinval )
>          return -ENOENT;
> 
> -    qi_ctrl = iommu_qi_ctrl(iommu);
>      flush = iommu_get_flush(iommu);
> 
>      /* Return if already enabled by Xen */
>      sts = dmar_readl(iommu->reg, DMAR_GSTS_REG);
> -    if ( (sts & DMA_GSTS_QIES) && qi_ctrl->qinval_maddr )
> +    if ( (sts & DMA_GSTS_QIES) && iommu->qinval_maddr )
>          return 0;
> 
> -    if ( qi_ctrl->qinval_maddr == 0 )
> +    if ( iommu->qinval_maddr == 0 )
>      {
>          drhd = iommu_to_drhd(iommu);
> -        qi_ctrl->qinval_maddr = alloc_pgtable_maddr(drhd, 
> QINVAL_ARCH_PAGE_NR);
> -        if ( qi_ctrl->qinval_maddr == 0 )
> -        {
> -            dprintk(XENLOG_WARNING VTDPREFIX,
> -                    "Cannot allocate memory for qi_ctrl->qinval_maddr\n");
> +
> +        iommu->qinval_maddr =
> +            alloc_pgtable_maddr(drhd, QINVAL_ARCH_PAGE_NR);
> +
> +        if ( iommu->qinval_maddr == 0 )
>              return -ENOMEM;
> -        }
>      }
> 
>      flush->context = flush_context_qi;
> @@ -438,7 +430,7 @@ int enable_qinval(struct vtd_iommu *iommu)
>       * to IQA register.
>       */
>      dmar_writeq(iommu->reg, DMAR_IQA_REG,
> -                qi_ctrl->qinval_maddr | QINVAL_PAGE_ORDER);
> +                iommu->qinval_maddr | QINVAL_PAGE_ORDER);
> 
>      dmar_writeq(iommu->reg, DMAR_IQT_REG, 0);
> 
> --
> 2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.