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

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

[Alexey Kardashevskiy]

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 <aik@ozlabs.ru>
---
 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

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

[Alexey Kardashevskiy]

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?


> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  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 */
> 


-- 
Alexey

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

[Michael Ellerman]

Alexey Kardashevskiy <aik@ozlabs.ru> writes:

> 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?

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

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

[Jonas Pfefferle1]

[-- Attachment #1: Type: text/plain, Size: 2648 bytes --]


Michael Ellerman <mpe@ellerman.id.au> wrote on 11/06/2017 11:45:34 AM:

> From: Michael Ellerman <mpe@ellerman.id.au>
> To: Alexey Kardashevskiy <aik@ozlabs.ru>, David Gibson
> <david@gibson.dropbear.id.au>
> Cc: linuxppc-dev@lists.ozlabs.org, Jonas Pfefferle1
> <JPF@zurich.ibm.com>, Nicholas Piggin <npiggin@gmail.com>
> Date: 11/06/2017 11:45 AM
> Subject: Re: [PATCH kernel] powerpc/powernv/ioda: Relax max DMA
> window size check
>
> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
>
> > 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?
>
> 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

[-- Attachment #2: Type: text/html, Size: 4034 bytes --]

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

[Alexey Kardashevskiy]

On 06/11/17 21:45, Michael Ellerman wrote:
> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
> 
>> 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?
> 
> 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