[PATCH] iommu/amd: Fix loop timeout issue in iommu_ga_log_enable()

[PATCH] iommu/amd: Fix loop timeout issue in iommu_ga_log_enable()

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

The polling loop for the register change in iommu_ga_log_enable() needs
to have a udelay() in it.  Otherwise the CPU might be faster than the
IOMMU hardware and wrongly trigger the WARN_ON() further down the code
stream.

Fixes: 8bda0cfbdc1a ("iommu/amd: Detect and initialize guest vAPIC log")
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/amd/init.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index dc338acf3338..d2e09d53851f 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -834,6 +834,7 @@ static int iommu_ga_log_enable(struct amd_iommu *iommu)
 		status = readl(iommu->mmio_base + MMIO_STATUS_OFFSET);
 		if (status & (MMIO_STATUS_GALOG_RUN_MASK))
 			break;
+		udelay(1);
 	}
 
 	if (WARN_ON(i >= LOOP_TIMEOUT))
-- 
2.34.1

Re: [PATCH] iommu/amd: Fix loop timeout issue in iommu_ga_log_enable()

[John Garry]

On 31/01/2022 16:17, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> The polling loop for the register change in iommu_ga_log_enable() needs
> to have a udelay() in it.  Otherwise the CPU might be faster than the
> IOMMU hardware and wrongly trigger the WARN_ON() further down the code
> stream.
> 
> Fixes: 8bda0cfbdc1a ("iommu/amd: Detect and initialize guest vAPIC log")
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>   drivers/iommu/amd/init.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> index dc338acf3338..d2e09d53851f 100644
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c
> @@ -834,6 +834,7 @@ static int iommu_ga_log_enable(struct amd_iommu *iommu)
>   		status = readl(iommu->mmio_base + MMIO_STATUS_OFFSET);
>   		if (status & (MMIO_STATUS_GALOG_RUN_MASK))
>   			break;
> +		udelay(1);

Maybe readl_relaxed_poll_timeout_atomic() could be used instead

Thanks,
John

>   	}
>   
>   	if (WARN_ON(i >= LOOP_TIMEOUT))
> 

Re: [PATCH] iommu/amd: Fix loop timeout issue in iommu_ga_log_enable()

[kernel test robot]

Hi Joerg,

I love your patch! Yet something to improve:

[auto build test ERROR on joro-iommu/next]
[also build test ERROR on v5.17-rc2 next-20220131]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Joerg-Roedel/iommu-amd-Fix-loop-timeout-issue-in-iommu_ga_log_enable/20220201-002214
base:   https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git next
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20220201/202202011448.2UFIiZHD-lkp@intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/5b5785526da0e2a6e793d2107d09afc9f310f17f
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Joerg-Roedel/iommu-amd-Fix-loop-timeout-issue-in-iommu_ga_log_enable/20220201-002214
        git checkout 5b5785526da0e2a6e793d2107d09afc9f310f17f
        # save the config file to linux build tree
        mkdir build_dir
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/iommu/amd/init.c: In function 'iommu_ga_log_enable':
>> drivers/iommu/amd/init.c:837:3: error: implicit declaration of function 'udelay' [-Werror=implicit-function-declaration]
     837 |   udelay(1);
         |   ^~~~~~
   cc1: some warnings being treated as errors


vim +/udelay +837 drivers/iommu/amd/init.c

   804	
   805	static int iommu_ga_log_enable(struct amd_iommu *iommu)
   806	{
   807	#ifdef CONFIG_IRQ_REMAP
   808		u32 status, i;
   809		u64 entry;
   810	
   811		if (!iommu->ga_log)
   812			return -EINVAL;
   813	
   814		/* Check if already running */
   815		status = readl(iommu->mmio_base + MMIO_STATUS_OFFSET);
   816		if (WARN_ON(status & (MMIO_STATUS_GALOG_RUN_MASK)))
   817			return 0;
   818	
   819		entry = iommu_virt_to_phys(iommu->ga_log) | GA_LOG_SIZE_512;
   820		memcpy_toio(iommu->mmio_base + MMIO_GA_LOG_BASE_OFFSET,
   821			    &entry, sizeof(entry));
   822		entry = (iommu_virt_to_phys(iommu->ga_log_tail) &
   823			 (BIT_ULL(52)-1)) & ~7ULL;
   824		memcpy_toio(iommu->mmio_base + MMIO_GA_LOG_TAIL_OFFSET,
   825			    &entry, sizeof(entry));
   826		writel(0x00, iommu->mmio_base + MMIO_GA_HEAD_OFFSET);
   827		writel(0x00, iommu->mmio_base + MMIO_GA_TAIL_OFFSET);
   828	
   829	
   830		iommu_feature_enable(iommu, CONTROL_GAINT_EN);
   831		iommu_feature_enable(iommu, CONTROL_GALOG_EN);
   832	
   833		for (i = 0; i < LOOP_TIMEOUT; ++i) {
   834			status = readl(iommu->mmio_base + MMIO_STATUS_OFFSET);
   835			if (status & (MMIO_STATUS_GALOG_RUN_MASK))
   836				break;
 > 837			udelay(1);
   838		}
   839	
   840		if (WARN_ON(i >= LOOP_TIMEOUT))
   841			return -EINVAL;
   842	#endif /* CONFIG_IRQ_REMAP */
   843		return 0;
   844	}
   845	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH] iommu/amd: Fix loop timeout issue in iommu_ga_log_enable()

[Joerg Roedel]

On Mon, Jan 31, 2022 at 05:06:03PM +0000, John Garry wrote:
> > diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> > index dc338acf3338..d2e09d53851f 100644
> > --- a/drivers/iommu/amd/init.c
> > +++ b/drivers/iommu/amd/init.c
> > @@ -834,6 +834,7 @@ static int iommu_ga_log_enable(struct amd_iommu *iommu)
> >   		status = readl(iommu->mmio_base + MMIO_STATUS_OFFSET);
> >   		if (status & (MMIO_STATUS_GALOG_RUN_MASK))
> >   			break;
> > +		udelay(1);
> 
> Maybe readl_relaxed_poll_timeout_atomic() could be used instead

I sent another version of this patch which uses
readl_poll_timeout_atomic(), but it didn't fix the issue. I take this
approach for now and leave using the helper as a future improvement.

Thanks,

	Joerg