[PATCH kernel] powerpc/powernv/ioda: Relax max DMA window size check

DMA windows can only have a size of power of two on IODA2 hardware and
using memory_hotplug_max() to determine the upper limit won't work
correcly if it returns not power of two value.

This relaxes the check by rounding up the value returned by
memory_hotplug_max().

It is expected to impact DPDK on machines with non-power-of-two RAM size,
mostly. KVM guests are less likely to be affected as usually guests get
less than half of hosts RAM.

Signed-off-by: Alexey Kardashevskiy <redacted>
---
 arch/powerpc/platforms/powernv/pci-ioda.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
index 269f119e4b3c..4c62162da181 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c

@@ -2769,7 +2769,8 @@ static long pnv_pci_ioda2_table_alloc_pages(int nid, __u64 bus_offset,
 	if (!levels || (levels > POWERNV_IOMMU_MAX_LEVELS))
 		return -EINVAL;
 
-	if ((window_size > memory_hotplug_max()) || !is_power_of_2(window_size))
+	if ((window_size > roundup_pow_of_two(memory_hotplug_max())) ||
+			!is_power_of_2(window_size))
 		return -EINVAL;
 
 	/* Adjust direct table size from window_size and levels */

-- 
2.11.0

On 31/10/17 15:04, Alexey Kardashevskiy wrote:

DMA windows can only have a size of power of two on IODA2 hardware and
using memory_hotplug_max() to determine the upper limit won't work
correcly if it returns not power of two value.

This relaxes the check by rounding up the value returned by
memory_hotplug_max().

It is expected to impact DPDK on machines with non-power-of-two RAM size,
mostly. KVM guests are less likely to be affected as usually guests get
less than half of hosts RAM.

It was pointed out that this check is quite useless anyway as the vm_locked
memory limit should hit first, and if that is not set or the user got the
root privilege level, then there are easier ways to crash the host so I am
thinking of:

diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c

b/arch/powerpc/platforms/powernv/pci-ioda.c
index 269f119e4b3c..a47e4cf343b2 100644

--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c

@@ -2769,7 +2769,7 @@ static long pnv_pci_ioda2_table_alloc_pages(int nid,

__u64 bus_offset,
        if (!levels || (levels > POWERNV_IOMMU_MAX_LEVELS))
                return -EINVAL;

-       if ((window_size > memory_hotplug_max()) ||
!is_power_of_2(window_size))
+       if (!is_power_of_2(window_size))
                return -EINVAL;



Makes sense?

-- 
Alexey

Alexey Kardashevskiy [off-list ref] writes:

Sounds reasonable.

Execpt where is the vm_locked check? I think it's in the VFIO driver? If
so I guess the only concern is that this code might be called via some
other path that doesn't do that check.

cheers

Michael Ellerman [off-list ref] wrote on 11/06/2017 11:45:34 AM:

From: Michael Ellerman <mpe@ellerman.id.au>
To: Alexey Kardashevskiy <redacted>, David Gibson
[off-list ref]
Cc: linuxppc-dev@lists.ozlabs.org, Jonas Pfefferle1
[off-list ref], Nicholas Piggin [off-list ref]
Date: 11/06/2017 11:45 AM
Subject: Re: [PATCH kernel] powerpc/powernv/ioda: Relax max DMA
window size check

Alexey Kardashevskiy [off-list ref] writes:

quoted

On 31/10/17 15:04, Alexey Kardashevskiy wrote:

quoted

DMA windows can only have a size of power of two on IODA2 hardware and
using memory_hotplug_max() to determine the upper limit won't work
correcly if it returns not power of two value.

This relaxes the check by rounding up the value returned by
memory_hotplug_max().

It is expected to impact DPDK on machines with non-power-of-two RAM

size,

quoted

quoted

mostly. KVM guests are less likely to be affected as usually guests

get

quoted

quoted

less than half of hosts RAM.

It was pointed out that this check is quite useless anyway as the

vm_locked

quoted

memory limit should hit first, and if that is not set or the user got

the

quoted

root privilege level, then there are easier ways to crash the host so I

am

quoted

thinking of:

diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c

b/arch/powerpc/platforms/powernv/pci-ioda.c
index 269f119e4b3c..a47e4cf343b2 100644

--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c

@@ -2769,7 +2769,7 @@ static long pnv_pci_ioda2_table_alloc_pages(int

nid,

quoted

__u64 bus_offset,
        if (!levels || (levels > POWERNV_IOMMU_MAX_LEVELS))
                return -EINVAL;

-       if ((window_size > memory_hotplug_max()) ||
!is_power_of_2(window_size))
+       if (!is_power_of_2(window_size))
                return -EINVAL;



Makes sense?

Sounds reasonable.

Execpt where is the vm_locked check? I think it's in the VFIO driver? If
so I guess the only concern is that this code might be called via some
other path that doesn't do that check.

cheers

The vm_locked is incremented here:
http://elixir.free-electrons.com/linux/v4.13.11/source/drivers/vfio/vfio_iommu_spapr_tce.c#L176
resp.
http://elixir.free-electrons.com/linux/v4.13.11/source/arch/powerpc/mm/mmu_context_iommu.c#L124
on VFIO_IOMMU_SPAPR_REGISTER_MEMORY. From my understanding only pages
that have been registered through here can be mapped with MAP_DMA.

Cheers,
Jonas

On 06/11/17 21:45, Michael Ellerman wrote:

Alexey Kardashevskiy [off-list ref] writes:

quoted

On 31/10/17 15:04, Alexey Kardashevskiy wrote:

quoted

DMA windows can only have a size of power of two on IODA2 hardware and
using memory_hotplug_max() to determine the upper limit won't work
correcly if it returns not power of two value.

This relaxes the check by rounding up the value returned by
memory_hotplug_max().

It is expected to impact DPDK on machines with non-power-of-two RAM size,
mostly. KVM guests are less likely to be affected as usually guests get
less than half of hosts RAM.

It was pointed out that this check is quite useless anyway as the vm_locked
memory limit should hit first, and if that is not set or the user got the
root privilege level, then there are easier ways to crash the host so I am
thinking of:

diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c

b/arch/powerpc/platforms/powernv/pci-ioda.c
index 269f119e4b3c..a47e4cf343b2 100644

--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c

@@ -2769,7 +2769,7 @@ static long pnv_pci_ioda2_table_alloc_pages(int nid,

__u64 bus_offset,
        if (!levels || (levels > POWERNV_IOMMU_MAX_LEVELS))
                return -EINVAL;

-       if ((window_size > memory_hotplug_max()) ||
!is_power_of_2(window_size))
+       if (!is_power_of_2(window_size))
                return -EINVAL;



Makes sense?

Sounds reasonable.

Execpt where is the vm_locked check? I think it's in the VFIO driver?

Yes, as Jonas already said.

If
so I guess the only concern is that this code might be called via some
other path that doesn't do that check.

It is also called from pnv_pci_ioda2_setup_default_config() to create a
32bit DMA window which is limited by
__rounddown_pow_of_two(memory_hotplug_max()). I'll repost. Thanks.


-- 
Alexey