Re: [PATCH] ide-scsi: DMA alignment bug fixed

Re: [PATCH] ide-scsi: DMA alignment bug fixed

[Bartlomiej Zolnierkiewicz]

Why can't you simply change alignment in ide-scsi to 32 bytes?

On Mon, 8 Nov 2004 23:22:39 -0500 (EST), Terry Kyriacopoulos
<terryk@echo-on.net> wrote:
> 
> 
> 
> 
> On Fri, 5 Nov 2004, Jens Axboe wrote:
> 
> > I don't think this is the right way to go. The design of ide-scsi's dma
> > setup is based on the old buffer_heads, where you can only have a single
> > page at the time so you need to string them manually. We actually have
> > kernel helpers for mapping _user_ data to a request, we could add one
> > for mapping kernel data to a request as well.
> >
> > Here's a patch that I did some time ago for mapping a kernel pointer to
> > a bio. bio_copy_user() would be good inspiration, too. Mapping an sglist
> > to a new bio should be even simpler and it would clean up the ide-scsi
> > stuff a lot. It's not exactly pretty.
> 
> ...
> 
> 
> 
> >
> > +struct bio *bio_map_data(request_queue_t *q, unsigned char *ptr,
> > +                      unsigned int len, int read)
> > +{
> > +
> > +     unsigned long data = (unsigned long) ptr;
> > +     unsigned long end = (data + len + PAGE_SIZE - 1) >> PAGE_SHIFT;
> > +     unsigned long start = data >> PAGE_SHIFT;
> > +     const int nr_pages = end - start;
> > +     unsigned int bytes, offset;
> > +     struct bio *bio;
> > +     struct page *page;
> > +     int i;
> > +
> > +     if (!len || !nr_pages)
> > +             return ERR_PTR(-EINVAL);
> > +
> > +     bio = bio_alloc(GFP_KERNEL, nr_pages);
> > +     if (!bio)
> > +             return ERR_PTR(-ENOMEM);
> > +
> > +     for (i = 0; i < nr_pages; i++) {
> > +             if (len <= 0)
> > +                     break;
> > +
> > +             page = virt_to_page(ptr);
> > +             offset = offset_in_page(ptr);
> > +
> > +             bytes = PAGE_SIZE - offset;
> > +             if (bytes > len)
> > +                     bytes = len;
> > +
> > +             if (__bio_add_page(q, bio, page, bytes, offset) < bytes)
> > +                     break;
> > +
> > +             len -= bytes;
> > +             offset = 0;
> > +             ptr += bytes;
> > +     }
> > +
> > +     bio->bi_end_io = bio_map_data_endio;
> > +     bio->bi_rw |= (!read << BIO_RW);
> > +     return bio;
> > +}
> > +
> 
> This code maps a pointer+length into a bio, but ide-scsi is fed an sglist,
> just like all low-level SCSI drivers, so this doesn't really help.
> 
> Please understand the way I handle misalignment.  Originally I used
> bounce buffers for the entire transfer if there was any misalignment,
> but this doesn't give optimal performance.  To maintain security, the
> bounce buffers must be zeroed before the I/O, otherwise a data scavenger
> can read potentially sensitive data if a transfer doesn't fully overwrite
> them.
> 
> This is where I got the idea to minimize the size of the extra bounce
> buffers by using the largest aligned subset of the user area.  If the
> base isn't aligned, the data must be shifted into place, hence the need
> for the overlapped copying in 'idescsi_bounce_dma'.  If only the length
> is misaligned, a small bounce buffer is appended to the bio just for the
> partial block.  Notice what it does for performance:
> 
> - Extra memory allocation is minimized
> - The user area doesn't need to be zeroed
> - Copying is limited to the last block if the base is aligned
> 
> cdda2wav/cdrecord/cdrdao use buffers much larger than 512 bytes and
> typically provide base aligned buffers, so this advanced technique
> really pays off.
> 
> Yes, some code is ugly, but the bug it fixes is uglier.
> 
> Another goal of mine was to confine the patch to the ide-scsi module.
> Making changes to high-level SCSI drivers or to IDE services would
> definitely not be a good approach.
> 
> Given the above, the code you presented can't really simplify ide-scsi.
> (If you have something that maps an sglist to a single contiguous block,
> I could use it.:-)