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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.05.16 at 10:08, <quan.xu@xxxxxxxxx> wrote:
> --- a/xen/arch/x86/acpi/power.c
> +++ b/xen/arch/x86/acpi/power.c
> @@ -43,36 +43,68 @@ struct acpi_sleep_info acpi_sinfo;
>  
>  void do_suspend_lowlevel(void);
>  
> +enum dev_power_saved
> +{
> +    SAVED_NONE,    /* None of device power saved */
> +    SAVED_CONSOLE, /* Device power of console saved */
> +    SAVED_TIME,    /* Device power of time saved */
> +    SAVED_I8259A,  /* Device power of I8259A saved */
> +    SAVED_IOAPIC,  /* Device power of IOAPIC saved */
> +    SAVED_IOMMU,   /* Device power of IOMMU saved */
> +    SAVED_LAPIC,   /* Device power of LAPIC saved */
> +};

Please avoid comments saying nothing substantially different than
the code they accompany, and also not helping the reader to
understand the commented code.

>  static int device_power_down(void)
>  {
> -    console_suspend();
> +    if ( console_suspend() )
> +        return SAVED_CONSOLE;

I said so on the previous round, and I need to repeat it now: If
console_suspend() fails, you saved _nothing_.

> -    time_suspend();
> +    if ( time_suspend() )
> +        return SAVED_TIME;
>  
> -    i8259A_suspend();
> +    if ( i8259A_suspend() )
> +        return SAVED_I8259A;
>  
> +    /* ioapic_suspend cannot fail */
>      ioapic_suspend();
>  
> -    iommu_suspend();
> +    if ( iommu_suspend() )
> +        return SAVED_IOMMU;
>  
> -    lapic_suspend();
> +    if ( lapic_suspend() )
> +        return SAVED_LAPIC;
>  
>      return 0;

And this silently means SAVED_NONE, whereas here you saved
everything. Yielding clearly bogus code ...

> -static void device_power_up(void)
> +static void device_power_up(enum dev_power_saved saved)
>  {
> -    lapic_resume();
> -
> -    iommu_resume();
> -
> -    ioapic_resume();
> -
> -    i8259A_resume();
> -
> -    time_resume();
> -
> -    console_resume();
> +    switch ( saved )
> +    {
> +    case SAVED_NONE:
> +        lapic_resume();

... here and ...

> @@ -196,7 +232,7 @@ static int enter_state(u32 state)
>      write_cr4(cr4 & ~X86_CR4_MCE);
>      write_efer(read_efer());
>  
> -    device_power_up();
> +    device_power_up(SAVED_NONE);

... here.

> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -541,20 +541,28 @@ static int iommu_flush_iotlb_psi(
>      return status;
>  }
>  
> -static void iommu_flush_all(void)
> +static int __must_check iommu_flush_all(void)
>  {
>      struct acpi_drhd_unit *drhd;
>      struct iommu *iommu;
>      int flush_dev_iotlb;
> +    int rc = 0;
>  
>      flush_all_cache();
>      for_each_drhd_unit ( drhd )
>      {
> +        int ret;
> +
>          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);
> +        ret = iommu_flush_iotlb_global(iommu, 0, flush_dev_iotlb);
> +
> +        if ( unlikely(ret) )
> +            rc = ret;

Same as for earlier patches - "if ( unlikely(!rc) )" please.

Also, having come here - did I miss iommu_flush_iotlb_global() gaining
a __must_check annotation somewhere? And the struct iommu_flush
pointers and handlers? And, by analogy, iommu_flush_context_*()?

Jan


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