Hi folks,

These two patches fix a nasty double allocation problem in swiotlb_alloc()
and add a diagnostic to help catch any similar issues in future. This was
a royal pain to track down and I've had to make a bit of a leap at the
correct alignment semantics (i.e. iotlb_align_mask vs alloc_align_mask).

Without these changes, we've been observing random vsock hangs when
communicating with virtual machines in Android.

Please have a look!

Cheers,

Will

Cc: iommu@lists.linux.dev
Cc: Christoph Hellwig <hch@lst.de>
Cc: Marek Szyprowski <m.szyprowski@samsung.com>
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Petr Tesarik <petr.tesarik1@huawei-partners.com>
Cc: Dexuan Cui <decui@microsoft.com>

--->8

Will Deacon (2):
  swiotlb: Fix allocation alignment requirement when searching slots
  swiotlb: Enforce page alignment in swiotlb_alloc()

 kernel/dma/swiotlb.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

-- 
2.43.0.429.g432eaa2c6b-goog

Hi Will,

On Fri, 26 Jan 2024 15:19:54 +0000
Will Deacon <will@kernel.org> wrote:

> Hi folks,
> 
> These two patches fix a nasty double allocation problem in swiotlb_alloc()
> and add a diagnostic to help catch any similar issues in future. This was
> a royal pain to track down and I've had to make a bit of a leap at the
> correct alignment semantics (i.e. iotlb_align_mask vs alloc_align_mask).

Welcome to the club. I believe you had to re-discover what I described here:

  https://lore.kernel.org/linux-iommu/20231108101347.77cab795@meshulam.tesarici.cz/

The relevant part would be this:

  To sum it up, there are two types of alignment:

  1. specified by a device's min_align_mask; this says how many low
     bits of a buffer's physical address must be preserved,

  2. specified by allocation size and/or the alignment parameter;
     this says how many low bits in the first IO TLB slot's physical
     address must be zero.  

Fix for that has been sitting on my TODO list for too long. :-(

Petr T

> Without these changes, we've been observing random vsock hangs when
> communicating with virtual machines in Android.
> 
> Please have a look!
> 
> Cheers,
> 
> Will
> 
> Cc: iommu@lists.linux.dev
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: Petr Tesarik <petr.tesarik1@huawei-partners.com>
> Cc: Dexuan Cui <decui@microsoft.com>
> 
> --->8  
> 
> Will Deacon (2):
>   swiotlb: Fix allocation alignment requirement when searching slots
>   swiotlb: Enforce page alignment in swiotlb_alloc()
> 
>  kernel/dma/swiotlb.c | 32 ++++++++++++++++++++++----------
>  1 file changed, 22 insertions(+), 10 deletions(-)
>

On Fri, Jan 26, 2024 at 05:20:59PM +0100, Petr Tesařík wrote:
> On Fri, 26 Jan 2024 15:19:54 +0000
> Will Deacon <will@kernel.org> wrote:
> 
> > Hi folks,
> > 
> > These two patches fix a nasty double allocation problem in swiotlb_alloc()
> > and add a diagnostic to help catch any similar issues in future. This was
> > a royal pain to track down and I've had to make a bit of a leap at the
> > correct alignment semantics (i.e. iotlb_align_mask vs alloc_align_mask).
> 
> Welcome to the club. I believe you had to re-discover what I described here:
> 
>   https://lore.kernel.org/linux-iommu/20231108101347.77cab795@meshulam.tesarici.cz/

Lucky me...

> The relevant part would be this:
> 
>   To sum it up, there are two types of alignment:
> 
>   1. specified by a device's min_align_mask; this says how many low
>      bits of a buffer's physical address must be preserved,
> 
>   2. specified by allocation size and/or the alignment parameter;
>      this says how many low bits in the first IO TLB slot's physical
>      address must be zero.  
> 
> Fix for that has been sitting on my TODO list for too long. :-(

FWIW, it did _used_ to work (or appear to work), so it would be good to
at least get it back to the old behaviour if nothing else.

Anyway, cheers for reviewing the patches. I'll go through your comments
now...

Will

On Mon, 29 Jan 2024 18:42:55 +0000
Will Deacon <will@kernel.org> wrote:

> On Fri, Jan 26, 2024 at 05:20:59PM +0100, Petr Tesařík wrote:
> > On Fri, 26 Jan 2024 15:19:54 +0000
> > Will Deacon <will@kernel.org> wrote:
> >   
> > > Hi folks,
> > > 
> > > These two patches fix a nasty double allocation problem in swiotlb_alloc()
> > > and add a diagnostic to help catch any similar issues in future. This was
> > > a royal pain to track down and I've had to make a bit of a leap at the
> > > correct alignment semantics (i.e. iotlb_align_mask vs alloc_align_mask).  
> > 
> > Welcome to the club. I believe you had to re-discover what I described here:
> > 
> >   https://lore.kernel.org/linux-iommu/20231108101347.77cab795@meshulam.tesarici.cz/  
> 
> Lucky me...
> 
> > The relevant part would be this:
> > 
> >   To sum it up, there are two types of alignment:
> > 
> >   1. specified by a device's min_align_mask; this says how many low
> >      bits of a buffer's physical address must be preserved,
> > 
> >   2. specified by allocation size and/or the alignment parameter;
> >      this says how many low bits in the first IO TLB slot's physical
> >      address must be zero.  
> > 
> > Fix for that has been sitting on my TODO list for too long. :-(  
> 
> FWIW, it did _used_ to work (or appear to work), so it would be good to
> at least get it back to the old behaviour if nothing else.

Yes, now that I look at the code, it was probably misunderstanding on
my side as to how the three different alignment requirements are
supposed to work together.

AFAICT your patch addresses everything that has ever worked. The rest
needs some more thought, and before I touch this loop again, I'll write
a proper test case.

Petr T

> Anyway, cheers for reviewing the patches. I'll go through your
> comments now...
> 
> Will

On Mon, Jan 29, 2024 at 08:26:19PM +0100, Petr Tesařík wrote:
> On Mon, 29 Jan 2024 18:42:55 +0000
> Will Deacon <will@kernel.org> wrote:
> 
> > On Fri, Jan 26, 2024 at 05:20:59PM +0100, Petr Tesařík wrote:
> > > On Fri, 26 Jan 2024 15:19:54 +0000
> > > Will Deacon <will@kernel.org> wrote:
> > >   
> > > > Hi folks,
> > > > 
> > > > These two patches fix a nasty double allocation problem in swiotlb_alloc()
> > > > and add a diagnostic to help catch any similar issues in future. This was
> > > > a royal pain to track down and I've had to make a bit of a leap at the
> > > > correct alignment semantics (i.e. iotlb_align_mask vs alloc_align_mask).  
> > > 
> > > Welcome to the club. I believe you had to re-discover what I described here:
> > > 
> > >   https://lore.kernel.org/linux-iommu/20231108101347.77cab795@meshulam.tesarici.cz/  
> > 
> > Lucky me...
> > 
> > > The relevant part would be this:
> > > 
> > >   To sum it up, there are two types of alignment:
> > > 
> > >   1. specified by a device's min_align_mask; this says how many low
> > >      bits of a buffer's physical address must be preserved,
> > > 
> > >   2. specified by allocation size and/or the alignment parameter;
> > >      this says how many low bits in the first IO TLB slot's physical
> > >      address must be zero.  
> > > 
> > > Fix for that has been sitting on my TODO list for too long. :-(  
> > 
> > FWIW, it did _used_ to work (or appear to work), so it would be good to
> > at least get it back to the old behaviour if nothing else.
> 
> Yes, now that I look at the code, it was probably misunderstanding on
> my side as to how the three different alignment requirements are
> supposed to work together.
> 
> AFAICT your patch addresses everything that has ever worked. The rest
> needs some more thought, and before I touch this loop again, I'll write
> a proper test case.

Thanks, that would be much appreciated!

Will