Bug: Freeing dma regions

Bug: Freeing dma regions

[David Kiarie]

Hello,

This patch seems to have introduced a bug -
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=4eeca8c5e72fad752eba9efc293c924d65faa86e

As the commit message says, it should check for regions behind the
next_bit but it checks for regions beyond.

David.

Re: Bug: Freeing dma regions

[Joerg Roedel]

On Wed, Mar 02, 2016 at 03:45:39AM +0300, David Kiarie wrote:
> Hello,
> 
> This patch seems to have introduced a bug -
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=4eeca8c5e72fad752eba9efc293c924d65faa86e
> 
> As the commit message says, it should check for regions behind the
> next_bit but it checks for regions beyond.

Hmm, the patch looks good to me. Do you actually see a reproducible
issue with it?


	Joerg

Re: Bug: Freeing dma regions

[David Kiarie]

On Wed, Mar 2, 2016 at 11:29 AM, Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org> wrote:
> On Wed, Mar 02, 2016 at 03:45:39AM +0300, David Kiarie wrote:
>> Hello,
>>
>> This patch seems to have introduced a bug -
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=4eeca8c5e72fad752eba9efc293c924d65faa86e
>>
>> As the commit message says, it should check for regions behind the
>> next_bit but it checks for regions beyond.
>
> Hmm, the patch looks good to me. Do you actually see a reproducible
> issue with it?

Yep,  I can't tell what exactly is happening but it seems to break
Qemu emulated IOMMU. I haven't tried on real hardware.

>
>
>         Joerg
>

Re: Bug: Freeing dma regions

[David Kiarie]

On Wed, Mar 2, 2016 at 11:37 AM, David Kiarie <davidkiarie4-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Wed, Mar 2, 2016 at 11:29 AM, Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org> wrote:
>> On Wed, Mar 02, 2016 at 03:45:39AM +0300, David Kiarie wrote:
>>> Hello,
>>>
>>> This patch seems to have introduced a bug -
>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=4eeca8c5e72fad752eba9efc293c924d65faa86e
>>>
>>> As the commit message says, it should check for regions behind the
>>> next_bit but it checks for regions beyond.
>>
>> Hmm, the patch looks good to me. Do you actually see a reproducible
>> issue with it?
>
> Yep,  I can't tell what exactly is happening but it seems to break
> Qemu emulated IOMMU. I haven't tried on real hardware.

What effect is setting the value next_bit to last invalidated index
supposed to have ?

>
>>
>>
>>         Joerg
>>

Re: Bug: Freeing dma regions

[Joerg Roedel]

On Wed, Mar 02, 2016 at 11:40:04AM +0300, David Kiarie wrote:
> What effect is setting the value next_bit to last invalidated index
> supposed to have ?

The idea is to safe the IOTLB flush by not re-using the address-range
until the allocator wraps around to 0 again. We only allocate address
ranges between next_bit and end-of-range. This way we don't need a flush
after every unmap operation.



	Joerg

Re: Bug: Freeing dma regions

[David Kiarie]

On Wed, Mar 2, 2016 at 3:35 PM, Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org> wrote:
> On Wed, Mar 02, 2016 at 11:40:04AM +0300, David Kiarie wrote:
>> What effect is setting the value next_bit to last invalidated index
>> supposed to have ?
>
> The idea is to safe the IOTLB flush by not re-using the address-range
> until the allocator wraps around to 0 again. We only allocate address
> ranges between next_bit and end-of-range. This way we don't need a flush
> after every unmap operation.

Okay, I'll look at this issue again.

Last unrelated question. If I have 'iommu=pt' IOMMU doesn't seem to
allocate any addresses and doesn't populate the 'root page table'
until I try to pass-through a device. Why is that set like this ?

>
>
>
>         Joerg
>

Re: Bug: Freeing dma regions

[Joerg Roedel]

On Wed, Mar 02, 2016 at 03:58:18PM +0300, David Kiarie wrote:
> On Wed, Mar 2, 2016 at 3:35 PM, Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org> wrote:
> > On Wed, Mar 02, 2016 at 11:40:04AM +0300, David Kiarie wrote:
> >> What effect is setting the value next_bit to last invalidated index
> >> supposed to have ?
> >
> > The idea is to safe the IOTLB flush by not re-using the address-range
> > until the allocator wraps around to 0 again. We only allocate address
> > ranges between next_bit and end-of-range. This way we don't need a flush
> > after every unmap operation.
> 
> Okay, I'll look at this issue again.
> 
> Last unrelated question. If I have 'iommu=pt' IOMMU doesn't seem to
> allocate any addresses and doesn't populate the 'root page table'
> until I try to pass-through a device. Why is that set like this ?

The pt stands for passthrough. In this mode the IOMMU is enabled, but
configured in way as if it is disabled. This means that all devices get
access to the physical address space and no remapping through the
DMA-API is done at all. Often this is used for performance reasons.

This only changes when a device is is assigned to a guest. In that case
it will only see the guest-physical address space via a page-table.


	Joerg

Re: Bug: Freeing dma regions

[David Kiarie]

On Wed, Mar 2, 2016 at 4:45 PM, Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org> wrote:
> On Wed, Mar 02, 2016 at 03:58:18PM +0300, David Kiarie wrote:
>> On Wed, Mar 2, 2016 at 3:35 PM, Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org> wrote:
>> > On Wed, Mar 02, 2016 at 11:40:04AM +0300, David Kiarie wrote:
>> >> What effect is setting the value next_bit to last invalidated index
>> >> supposed to have ?
>> >
>> > The idea is to safe the IOTLB flush by not re-using the address-range
>> > until the allocator wraps around to 0 again. We only allocate address
>> > ranges between next_bit and end-of-range. This way we don't need a flush
>> > after every unmap operation.
>>
>> Okay, I'll look at this issue again.
>>
>> Last unrelated question. If I have 'iommu=pt' IOMMU doesn't seem to
>> allocate any addresses and doesn't populate the 'root page table'
>> until I try to pass-through a device. Why is that set like this ?
>
> The pt stands for passthrough. In this mode the IOMMU is enabled, but
> configured in way as if it is disabled. This means that all devices get
> access to the physical address space and no remapping through the
> DMA-API is done at all. Often this is used for performance reasons.
>
> This only changes when a device is is assigned to a guest. In that case
> it will only see the guest-physical address space via a page-table.

Thanks for the insight!

>
>
>         Joerg
>