Re: [PATCH v5 12/12] xen/arm: Suspend/resume IOMMU on Xen suspend/resume

[Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>]

Hi,

Mykola Kvach <xakep.amatop@gmail.com> writes:

> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>
> This is done using generic iommu_suspend/resume functions that cause
> IOMMU driver specific suspend/resume handlers to be called for enabled
> IOMMU (if one has suspend/resume driver handlers implemented).
>
> These handlers work only when IPMMU-VMSA is used; otherwise,
> we return an error during system suspend requests.
>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
> Tested-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> ---
>  xen/arch/arm/suspend.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/xen/arch/arm/suspend.c b/xen/arch/arm/suspend.c
> index b5398e5ca6..cb86426ebd 100644
> --- a/xen/arch/arm/suspend.c
> +++ b/xen/arch/arm/suspend.c
> @@ -4,6 +4,7 @@
>  #include <asm/suspend.h>
>  #include <xen/console.h>
>  #include <xen/cpu.h>
> +#include <xen/iommu.h>
>  #include <xen/llc-coloring.h>
>  #include <xen/sched.h>
>  
> @@ -49,6 +50,13 @@ static long system_suspend(void *data)
>  
>      time_suspend();
>  
> +    status = iommu_suspend();
> +    if ( status )
> +    {
> +        system_state = SYS_STATE_resume;
> +        goto resume_time;
> +    }
> +
>      local_irq_save(flags);
>      status = gic_suspend();
>      if ( status )
> @@ -105,6 +113,10 @@ static long system_suspend(void *data)
>  
>   resume_irqs:
>      local_irq_restore(flags);
> +
> +    iommu_resume();
> +
> + resume_time:
>      time_resume();
>  
>   resume_nonboot_cpus:
> @@ -140,6 +152,15 @@ int host_system_suspend(void)
>          return -ENOSYS;
>      }
>  
> +    /* TODO: drop check once suspend/resume support for SMMU is implemented */
> +#ifndef CONFIG_IPMMU_VMSA

This check is meaningless, as you can have CONFIG_IPMMU_VMSA enabled
along with CONFIG_ARM_SMMU on a system with SMMU. I think it is enough
that iommu_suspend() will fail during system_suspend(), isn't it?


> +    if ( iommu_enabled )
> +    {
> +        dprintk(XENLOG_ERR, "IOMMU is enabled, suspend not supported yet\n");
> +        return -ENOSYS;
> +    }
> +#endif
> +
>      /*
>       * system_suspend should be called when Dom0 finalizes the suspend
>       * procedure from its boot core (VCPU#0). However, Dom0's VCPU#0 could

-- 
WBR, Volodymyr