Re: [Xen-devel] [PATCH v2 09/11] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.04.16 at 16:00, <quan.xu@xxxxxxxxx> wrote:
> --- a/xen/arch/x86/acpi/power.c
> +++ b/xen/arch/x86/acpi/power.c
> @@ -45,19 +45,31 @@ void do_suspend_lowlevel(void);
>  
>  static int device_power_down(void)
>  {
> +    int err;
> +
>      console_suspend();
>  
>      time_suspend();
>  
>      i8259A_suspend();
>  
> -    ioapic_suspend();
> +    err = iommu_suspend();
> +    if ( err )
> +        goto iommu_suspend_error;
>  
>      iommu_suspend();

A little more care please - this is definitely not what you meant.

> + iommu_suspend_error:
> +    ioapic_resume();
> +    i8259A_resume();
> +    time_resume();
> +    console_resume();
> +
> +    return err;

Instead of mostly repeating what device_power_up() does I
wonder whether it wouldn't be better to re-use that function for
the error path here. Either by suitably making information
available to device_power_up() to skip the call to lapic_resume()
or by making lapic_resume() itself recognize it got called without
lapic_suspend() having got called first.

> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -2540,7 +2540,7 @@ static int force_stage = 2;
>   */
>  static u32 platform_features = ARM_SMMU_FEAT_COHERENT_WALK;
>  
> -static void arm_smmu_iotlb_flush_all(struct domain *d)
> +static int arm_smmu_iotlb_flush_all(struct domain *d)
>  {
>       struct arm_smmu_xen_domain *smmu_domain = 
> domain_hvm_iommu(d)->arch.priv;
>       struct iommu_domain *cfg;
> @@ -2557,13 +2557,15 @@ static void arm_smmu_iotlb_flush_all(struct domain *d)
>               arm_smmu_tlb_inv_context(cfg->priv);
>       }
>       spin_unlock(&smmu_domain->lock);
> +
> +    return 0;
>  }

Even if indentation looks inconsistent in this file, please make your
addition match surrounding code.

> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -542,11 +542,12 @@ static int iommu_flush_iotlb_psi(
>      return status;
>  }
>  
> -static void iommu_flush_all(void)
> +static int iommu_flush_all(void)

__must_check

> @@ -554,8 +555,13 @@ static void iommu_flush_all(void)
>          iommu = drhd->iommu;
>          iommu_flush_context_global(iommu, 0);
>          flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0;
> -        iommu_flush_iotlb_global(iommu, 0, flush_dev_iotlb);
> +        rc = iommu_flush_iotlb_global(iommu, 0, flush_dev_iotlb);
> +
> +        if ( rc )
> +            break;

This again violates the best effort flushing principle.

> @@ -2421,16 +2427,20 @@ static int intel_iommu_group_id(u16 seg, u8 bus, u8 
> devfn)
>  }
>  
>  static u32 iommu_state[MAX_IOMMUS][MAX_IOMMU_REGS];
> -static void vtd_suspend(void)
> +static int vtd_suspend(void)

Please take the opportunity to add the missing blank line between
variable and function definition.

> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -157,7 +157,7 @@ struct iommu_ops {
>      unsigned int (*read_apic_from_ire)(unsigned int apic, unsigned int reg);
>      int (*setup_hpet_msi)(struct msi_desc *);
>  #endif /* CONFIG_X86 */
> -    void (*suspend)(void);
> +    int (*suspend)(void);
>      void (*resume)(void);
>      void (*share_p2m)(struct domain *d);
>      void (*crash_shutdown)(void);
> @@ -167,7 +167,7 @@ struct iommu_ops {
>      void (*dump_p2m_table)(struct domain *d);
>  };
>  
> -void iommu_suspend(void);
> +int iommu_suspend(void);

At the very least this one should become __must_check.

Jan


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