Re: [RFC PATCH v2 01/25] iommu/dma: Introduce CONFIG_PISA and metadata table

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] iommu/dma: CONFIG_PISA breaks builds on 64-bit UML and
  !SPARSEMEM
- [Medium] iommu/dma: pisa_meta_memory_cb() ignores pisa_meta_prealloc()
  error
--

commit 4571e9a5a4e068f2cb559f5c16306bf2b33d6515
Author: Luigi Rizzo <lrizzo@google.com>

iommu/dma: Introduce CONFIG_PISA and metadata table

This commit introduces the CONFIG_PISA build option, a sparse per-2MB-frame
metadata table in drivers/iommu/pisa-meta.c, and KUnit tests for metadata
table operations.

> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index 85a8d7fdfa30b..15e06eaed0924 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -423,4 +423,37 @@ config IOMMU_DEBUG_PAGEALLOC
>  	  If unsure, say N.
>  endif # IOMMU_SUPPORT
>  
> +# Pre-mapped IOVA Superpage Allocator
> +config PISA
> +	bool "Pre-mapped IOVA Superpage Allocator"
> +	depends on X86_64 || ARM64
> +	depends on !PREEMPT_RT
> +	default y

[Severity: Medium]
Can enabling CONFIG_PISA by default break builds on 64-bit UML (ARCH=um on
x86_64) or configurations without CONFIG_SPARSEMEM?

PISA is placed outside the IOMMU_SUPPORT block, and drivers/Makefile enters
drivers/iommu/ unconditionally.  In drivers/iommu/pisa-meta.c,
static_assert(PISA_SHIFT <= SUBSECTION_SHIFT) and pisa_meta_init()'s use of
MAX_PHYSMEM_BITS both rely on symbols that are only defined when
CONFIG_SPARSEMEM is enabled, whereas 64-bit UML sets CONFIG_X86_64=y with
CONFIG_FLATMEM=y.

> diff --git a/drivers/iommu/pisa-meta.c b/drivers/iommu/pisa-meta.c
> new file mode 100644
> index 0000000000000..5ceea91e4d5e8
> --- /dev/null
> +++ b/drivers/iommu/pisa-meta.c
[ ... ]
> +/* Page tables for memory about to go online; failure keeps it out of the allocators. */
> +static int pisa_meta_memory_cb(struct notifier_block *nb, unsigned long action, void *arg)
> +{
> +	struct memory_notify *mn = arg;
> +
> +	if (action != MEM_GOING_ONLINE)
> +		return NOTIFY_OK;
> +
> +	mutex_lock(&pisa_meta_mutex);
> +	if (pisa_meta_array)
> +		pisa_meta_prealloc(pisa_meta_array, pisa_meta_nframes, mn->start_pfn,
> +				   mn->start_pfn + mn->nr_pages);
> +	mutex_unlock(&pisa_meta_mutex);
> +	return NOTIFY_OK;
> +}

[Severity: Medium]
Should pisa_meta_memory_cb() propagate errors from pisa_meta_prealloc() via
notifier_from_errno()?

Returning NOTIFY_OK unconditionally allows online_pages() to bring hotplugged
memory online even when page-table preallocation fails.  Any subsequent
non-blocking pisa_meta_ensure_pfn(pfn, false) call on a page from that range
cannot allocate the missing page table and fails with -EINVAL in
pisa_meta_back().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109182533.3594984-1-lrizzo@google.com?part=1