Re: [Xen-devel] Ping: [PATCH] VT-d: drop bogus checks

[Keir Fraser <keir@xxxxxxx>]

On 15/10/2012 15:45, "Jan Beulich" <JBeulich@xxxxxxxx> wrote:

> There were a number of cases where an "iommu" retrieved got passed to
> another function before being NULL-checked. While this by itself was
> not a problem as the called function did the checks, it is confusing to
> the reader and redundant in several cases (particularly with NULL-
> checking the return value of iommu_ir_ctrl()). Drop the redundant
> checks (also ones where the sole caller of a function did the checking
> already), and at once make the three similar functions proper inline
> instead of extern ones (they were prototyped in the wrong header file
> anyway, so would have needed touching sooner or later).
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Yes, I like this sort of cleanup.

Acked-by: Keir Fraser <keir@xxxxxxx>

> --- a/xen/drivers/passthrough/vtd/intremap.c
> +++ b/xen/drivers/passthrough/vtd/intremap.c
> @@ -217,13 +217,6 @@ static int remap_entry_to_ioapic_rte(
>      unsigned long flags;
>      struct ir_ctrl *ir_ctrl = iommu_ir_ctrl(iommu);
>  
> -    if ( ir_ctrl == NULL )
> -    {
> -        dprintk(XENLOG_ERR VTDPREFIX,
> -                "remap_entry_to_ioapic_rte: ir_ctl is not ready\n");
> -        return -EFAULT;
> -    }
> -
>      if ( index < 0 || index > IREMAP_ENTRY_NR - 1 )
>      {
>          dprintk(XENLOG_ERR VTDPREFIX,
> @@ -358,8 +351,7 @@ unsigned int io_apic_read_remap_rte(
>      struct iommu *iommu = ioapic_to_iommu(IO_APIC_ID(apic));
>      struct ir_ctrl *ir_ctrl = iommu_ir_ctrl(iommu);
>  
> -    if ( !iommu || !ir_ctrl || ir_ctrl->iremap_maddr == 0 ||
> -        (ir_ctrl->iremap_num == 0) ||
> +    if ( !ir_ctrl || !ir_ctrl->iremap_maddr || !ir_ctrl->iremap_num ||
>          ( (index = apic_pin_2_ir_idx[apic][ioapic_pin]) < 0 ) )
>          return __io_apic_read(apic, reg);
>  
> @@ -385,7 +377,7 @@ void io_apic_write_remap_rte(
>      struct ir_ctrl *ir_ctrl = iommu_ir_ctrl(iommu);
>      int saved_mask;
>  
> -    if ( !iommu || !ir_ctrl || ir_ctrl->iremap_maddr == 0 )
> +    if ( !ir_ctrl || !ir_ctrl->iremap_maddr )
>      {
>          __io_apic_write(apic, reg, value);
>          return;
> @@ -475,13 +467,6 @@ static int remap_entry_to_msi_msg(
>      unsigned long flags;
>      struct ir_ctrl *ir_ctrl = iommu_ir_ctrl(iommu);
>  
> -    if ( ir_ctrl == NULL )
> -    {
> -        dprintk(XENLOG_ERR VTDPREFIX,
> -                "remap_entry_to_msi_msg: ir_ctl == NULL");
> -        return -EFAULT;
> -    }
> -
>      remap_rte = (struct msi_msg_remap_entry *) msg;
>      index = (remap_rte->address_lo.index_15 << 15) |
>               remap_rte->address_lo.index_0_14;
> @@ -644,7 +629,7 @@ void msi_msg_read_remap_rte(
>      iommu = drhd->iommu;
>  
>      ir_ctrl = iommu_ir_ctrl(iommu);
> -    if ( !iommu || !ir_ctrl || ir_ctrl->iremap_maddr == 0 )
> +    if ( !ir_ctrl || !ir_ctrl->iremap_maddr )
>          return;
>  
>      remap_entry_to_msi_msg(iommu, msg);
> @@ -663,7 +648,7 @@ void msi_msg_write_remap_rte(
>      iommu = drhd->iommu;
>  
>      ir_ctrl = iommu_ir_ctrl(iommu);
> -    if ( !iommu || !ir_ctrl || ir_ctrl->iremap_maddr == 0 )
> +    if ( !ir_ctrl || !ir_ctrl->iremap_maddr )
>          return;
>  
>      msi_msg_to_remap_entry(iommu, pdev, msi_desc, msg);
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -153,21 +153,6 @@ static void __init free_intel_iommu(stru
>      xfree(intel);
>  }
>  
> -struct qi_ctrl *iommu_qi_ctrl(struct iommu *iommu)
> -{
> -    return iommu ? &iommu->intel->qi_ctrl : NULL;
> -}
> -
> -struct ir_ctrl *iommu_ir_ctrl(struct iommu *iommu)
> -{
> -    return iommu ? &iommu->intel->ir_ctrl : NULL;
> -}
> -
> -struct iommu_flush *iommu_get_flush(struct iommu *iommu)
> -{
> -    return iommu ? &iommu->intel->flush : NULL;
> -}
> -
>  static int iommus_incoherent;
>  static void __iommu_flush_cache(void *addr, unsigned int size)
>  {
> --- a/xen/drivers/passthrough/vtd/iommu.h
> +++ b/xen/drivers/passthrough/vtd/iommu.h
> @@ -20,7 +20,7 @@
>  #ifndef _INTEL_IOMMU_H_
>  #define _INTEL_IOMMU_H_
>  
> -#include <xen/types.h>
> +#include <xen/iommu.h>
>  
>  /*
>   * Intel IOMMU register specification per version 1.0 public spec.
> @@ -510,6 +510,21 @@ struct intel_iommu {
>      struct acpi_drhd_unit *drhd;
>  };
>  
> +static inline struct qi_ctrl *iommu_qi_ctrl(struct iommu *iommu)
> +{
> +    return iommu ? &iommu->intel->qi_ctrl : NULL;
> +}
> +
> +static inline struct ir_ctrl *iommu_ir_ctrl(struct iommu *iommu)
> +{
> +    return iommu ? &iommu->intel->ir_ctrl : NULL;
> +}
> +
> +static inline struct iommu_flush *iommu_get_flush(struct iommu *iommu)
> +{
> +    return iommu ? &iommu->intel->flush : NULL;
> +}
> +
>  #define INTEL_IOMMU_DEBUG(fmt, args...) \
>      do  \
>      {   \
> --- a/xen/drivers/passthrough/vtd/utils.c
> +++ b/xen/drivers/passthrough/vtd/utils.c
> @@ -267,8 +267,7 @@ static void dump_iommu_info(unsigned cha
>          {
>              iommu = ioapic_to_iommu(mp_ioapics[apic].mpc_apicid);
>              ir_ctrl = iommu_ir_ctrl(iommu);
> -            if ( !iommu || !ir_ctrl || ir_ctrl->iremap_maddr == 0 ||
> -                ir_ctrl->iremap_num == 0 )
> +            if ( !ir_ctrl || !ir_ctrl->iremap_maddr || !ir_ctrl->iremap_num )
>                  continue;
>  
>              printk( "\nRedirection table of IOAPIC %x:\n", apic);
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -106,9 +106,6 @@ struct msi_desc;
>  struct msi_msg;
>  void msi_msg_read_remap_rte(struct msi_desc *msi_desc, struct msi_msg *msg);
>  void msi_msg_write_remap_rte(struct msi_desc *msi_desc, struct msi_msg *msg);
> -struct qi_ctrl *iommu_qi_ctrl(struct iommu *iommu);
> -struct ir_ctrl *iommu_ir_ctrl(struct iommu *iommu);
> -struct iommu_flush *iommu_get_flush(struct iommu *iommu);
>  void hvm_dpci_isairq_eoi(struct domain *d, unsigned int isairq);
>  struct hvm_irq_dpci *domain_get_irq_dpci(const struct domain *);
>  void free_hvm_irq_dpci(struct hvm_irq_dpci *dpci);
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel