Worrisome IDE PIO transfers...

Worrisome IDE PIO transfers...

[Jeff Garzik]

Looking at the function that is used to transfer data when in PIO mode...

void taskfile_output_data (ide_drive_t *drive, void *buffer, u32 wcount)
{
         if (drive->bswap) {
                 ata_bswap_data(buffer, wcount);
                 HWIF(drive)->ata_output_data(drive, buffer, wcount);
                 ata_bswap_data(buffer, wcount);
         } else {
                 HWIF(drive)->ata_output_data(drive, buffer, wcount);
         }
}

Swapping the data in-place is very, very wrong...   you don't want to be 
touching the data that userspace might have mmap'd ...  Additionally, 
byteswapping back and forth for each PIO sector chews unnecessary CPU.

Seems to me the architecture's OUTS[WL] hook (or a new, similar hook) 
that swaps as it writes would be _much_ preferred, and eliminate this 
possible data corruption issue.

	Jeff

Re: Worrisome IDE PIO transfers...

[Bartlomiej Zolnierkiewicz]

[ Geert added to cc: ]

On Sunday 29 of February 2004 00:24, Jeff Garzik wrote:
> Looking at the function that is used to transfer data when in PIO mode...
>
> void taskfile_output_data (ide_drive_t *drive, void *buffer, u32 wcount)
> {
>          if (drive->bswap) {
>                  ata_bswap_data(buffer, wcount);
>                  HWIF(drive)->ata_output_data(drive, buffer, wcount);
>                  ata_bswap_data(buffer, wcount);
>          } else {
>                  HWIF(drive)->ata_output_data(drive, buffer, wcount);
>          }
> }
>
> Swapping the data in-place is very, very wrong...   you don't want to be
> touching the data that userspace might have mmap'd ...  Additionally,
> byteswapping back and forth for each PIO sector chews unnecessary CPU.

This is used for accessing "normal" disks on beasts with byte-swapped IDE
bus (Atari/Q40/TiVo) and "byteswapped" disks on normal machines.

[ Hm. actually I don't see how it can be used for accessing "normal" disks,
  as data is byteswapped by IDE bus and then swapped back by IDE driver. ]

Manfred noticed the same issue some time ago:
http://www.ussg.iu.edu/hypermail/linux/kernel/0201.0/0768.html
but discussion ended without final conclusion.

I like Alan's idea to use loopback instead of "bswap".

> Seems to me the architecture's OUTS[WL] hook (or a new, similar hook)
> that swaps as it writes would be _much_ preferred, and eliminate this
> possible data corruption issue.

I think something similar has been already done
(grep for insw_swapw/outsw_swapw in ide-iops.c and asm-m68k/ide.h).

Bartlomiej

Re: Worrisome IDE PIO transfers...

[Jeff Garzik]

Bartlomiej Zolnierkiewicz wrote:
> [ Geert added to cc: ]
> 
> On Sunday 29 of February 2004 00:24, Jeff Garzik wrote:
> 
>>Looking at the function that is used to transfer data when in PIO mode...
>>
>>void taskfile_output_data (ide_drive_t *drive, void *buffer, u32 wcount)
>>{
>>         if (drive->bswap) {
>>                 ata_bswap_data(buffer, wcount);
>>                 HWIF(drive)->ata_output_data(drive, buffer, wcount);
>>                 ata_bswap_data(buffer, wcount);
>>         } else {
>>                 HWIF(drive)->ata_output_data(drive, buffer, wcount);
>>         }
>>}
>>
>>Swapping the data in-place is very, very wrong...   you don't want to be
>>touching the data that userspace might have mmap'd ...  Additionally,
>>byteswapping back and forth for each PIO sector chews unnecessary CPU.
> 
> 
> This is used for accessing "normal" disks on beasts with byte-swapped IDE
> bus (Atari/Q40/TiVo) and "byteswapped" disks on normal machines.
> 
> [ Hm. actually I don't see how it can be used for accessing "normal" disks,
>   as data is byteswapped by IDE bus and then swapped back by IDE driver. ]

Yeah, just byteswapped disks are affected.


> Manfred noticed the same issue some time ago:
> http://www.ussg.iu.edu/hypermail/linux/kernel/0201.0/0768.html
> but discussion ended without final conclusion.
> 
> I like Alan's idea to use loopback instead of "bswap".

Neat but no more zerocopy that way.  I much prefer a swap-as-you-go...


>>Seems to me the architecture's OUTS[WL] hook (or a new, similar hook)
>>that swaps as it writes would be _much_ preferred, and eliminate this
>>possible data corruption issue.
> 
> I think something similar has been already done
> (grep for insw_swapw/outsw_swapw in ide-iops.c and asm-m68k/ide.h).

Yeah, but this would need to be per-device...  I agree all the other 
pieces are already present.

	Jeff

Re: Worrisome IDE PIO transfers...

[Bartlomiej Zolnierkiewicz]

On Sunday 29 of February 2004 01:58, Jeff Garzik wrote:

> > I like Alan's idea to use loopback instead of "bswap".
>
> Neat but no more zerocopy that way.  I much prefer a swap-as-you-go...

Okay, better solution:

- on Atari/Q40:
  if drive->bswap use insw/outsw instead of swapping variants
 
- on others:
  use device mapper as suggested by Matt Mackall
  (no extra copying and you can use DMA to read disk from TiVo!)

Bartlomiej

Re: Worrisome IDE PIO transfers...

[Geert Uytterhoeven]

On Sun, 29 Feb 2004, Bartlomiej Zolnierkiewicz wrote:
> On Sunday 29 of February 2004 01:58, Jeff Garzik wrote:
> > > I like Alan's idea to use loopback instead of "bswap".
> >
> > Neat but no more zerocopy that way.  I much prefer a swap-as-you-go...
>
> Okay, better solution:
>
> - on Atari/Q40:
>   if drive->bswap use insw/outsw instead of swapping variants

Yep, that sounds the most logical. Richard?

> - on others:
>   use device mapper as suggested by Matt Mackall
>   (no extra copying and you can use DMA to read disk from TiVo!)

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: Worrisome IDE PIO transfers...

[Richard Zidlicky]

On Sun, Feb 29, 2004 at 09:52:08AM +0100, Geert Uytterhoeven wrote:
> On Sun, 29 Feb 2004, Bartlomiej Zolnierkiewicz wrote:
> > On Sunday 29 of February 2004 01:58, Jeff Garzik wrote:
> > > > I like Alan's idea to use loopback instead of "bswap".
> > >
> > > Neat but no more zerocopy that way.  I much prefer a swap-as-you-go...
> >
> > Okay, better solution:
> >
> > - on Atari/Q40:
> >   if drive->bswap use insw/outsw instead of swapping variants
> 
> Yep, that sounds the most logical. Richard?

looks good. 

However it appears to fix only part of the problem -  we need some
logic to ensure only disk data is swapped.
Bswapping WIN_DOWNLOAD_MICROCODE data would not be very
clever I guess.

Richard

Re: Worrisome IDE PIO transfers...

[Bartlomiej Zolnierkiewicz]

On Sunday 29 of February 2004 20:23, Richard Zidlicky wrote:
> On Sun, Feb 29, 2004 at 09:52:08AM +0100, Geert Uytterhoeven wrote:
> > On Sun, 29 Feb 2004, Bartlomiej Zolnierkiewicz wrote:
> > > On Sunday 29 of February 2004 01:58, Jeff Garzik wrote:
> > > > > I like Alan's idea to use loopback instead of "bswap".
> > > >
> > > > Neat but no more zerocopy that way.  I much prefer a
> > > > swap-as-you-go...
> > >
> > > Okay, better solution:
> > >
> > > - on Atari/Q40:
> > >   if drive->bswap use insw/outsw instead of swapping variants
> >
> > Yep, that sounds the most logical. Richard?
>
> looks good.
>
> However it appears to fix only part of the problem -  we need some
> logic to ensure only disk data is swapped.
> Bswapping WIN_DOWNLOAD_MICROCODE data would not be very
> clever I guess.

Actually drive->bswap should die as I overlooked the fact that we are
_not_ swapping disk data (byteswapped data is used for FS) on Atari/Q40.

Therefore the real solution is to use device-mapper instead of drive->bswap
and on Atari/Q40 use standard insw/outs only if blk_fs_request(drive->rq),
for everything else insw_swapw/outsw_swapw should be used.

Does it make any sense? :)

Bartlomiej

Re: Worrisome IDE PIO transfers...

[Richard Zidlicky]

On Sun, Feb 29, 2004 at 09:36:33PM +0100, Bartlomiej Zolnierkiewicz wrote:
> On Sunday 29 of February 2004 20:23, Richard Zidlicky wrote:
> > On Sun, Feb 29, 2004 at 09:52:08AM +0100, Geert Uytterhoeven wrote:
> > > On Sun, 29 Feb 2004, Bartlomiej Zolnierkiewicz wrote:
> > > > On Sunday 29 of February 2004 01:58, Jeff Garzik wrote:
> > > > > > I like Alan's idea to use loopback instead of "bswap".
> > > > >
> > > > > Neat but no more zerocopy that way.  I much prefer a
> > > > > swap-as-you-go...
> > > >
> > > > Okay, better solution:
> > > >
> > > > - on Atari/Q40:
> > > >   if drive->bswap use insw/outsw instead of swapping variants
> > >
> > > Yep, that sounds the most logical. Richard?
> >
> > looks good.
> >
> > However it appears to fix only part of the problem -  we need some
> > logic to ensure only disk data is swapped.
> > Bswapping WIN_DOWNLOAD_MICROCODE data would not be very
> > clever I guess.
> 
> Actually drive->bswap should die as I overlooked the fact that we are
> _not_ swapping disk data (byteswapped data is used for FS) on Atari/Q40.

correct, on those machines the IDE bus is wired "reversed" and we take
the data without any correction, except for IDENTIFY and atapi requests.
That means that quite a few ioctls (SMART etc) are most likely broken
right now.

> Therefore the real solution is to use device-mapper instead of drive->bswap
> and on Atari/Q40 use standard insw/outs only if blk_fs_request(drive->rq),
> for everything else insw_swapw/outsw_swapw should be used.
> 
> Does it make any sense? :)

that would be the perfect solution. Note that atapi transfers are
already correct the way they are - nothing to fix here.

Richard

Re: Worrisome IDE PIO transfers...

[Matt Mackall]

On Sun, Feb 29, 2004 at 01:21:30AM +0100, Bartlomiej Zolnierkiewicz wrote:
> I like Alan's idea to use loopback instead of "bswap".

Or, more likely, device mapper.

-- 
Matt Mackall : http://www.selenic.com : Linux development and consulting

Re: Worrisome IDE PIO transfers...

[Jeff Garzik]

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

Matt Mackall wrote:
> On Sun, Feb 29, 2004 at 01:21:30AM +0100, Bartlomiej Zolnierkiewicz wrote:
> 
>>I like Alan's idea to use loopback instead of "bswap".
> 
> 
> Or, more likely, device mapper.


Somehow I doubt anybody cares enough to write a whole driver just for 
this unlikely case.

For now let's at least record the knowledge...  (patch attached)

	Jeff



[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 826 bytes --]

===== drivers/ide/ide-taskfile.c 1.28 vs edited =====
--- 1.28/drivers/ide/ide-taskfile.c	Thu Feb 26 12:11:20 2004
+++ edited/drivers/ide/ide-taskfile.c	Sat Feb 28 21:18:27 2004
@@ -29,6 +29,7 @@
 
 #include <linux/config.h>
 #include <linux/module.h>
+#include <linux/compiler.h>
 #include <linux/types.h>
 #include <linux/string.h>
 #include <linux/kernel.h>
@@ -81,7 +82,12 @@
 
 void taskfile_output_data (ide_drive_t *drive, void *buffer, u32 wcount)
 {
-	if (drive->bswap) {
+	if (unlikely(drive->bswap)) {
+		/* FIXME: Besides the inefficiency each sector
+		 * twice, this can lead to data corruption on
+		 * SMP.  Fortunately drives that need this swapping
+		 * are quite uncommon.
+		 */
 		ata_bswap_data(buffer, wcount);
 		HWIF(drive)->ata_output_data(drive, buffer, wcount);
 		ata_bswap_data(buffer, wcount);

Re: Worrisome IDE PIO transfers...

[Bartlomiej Zolnierkiewicz]

On Sunday 29 of February 2004 03:41, Jeff Garzik wrote:
> Matt Mackall wrote:
> > On Sun, Feb 29, 2004 at 01:21:30AM +0100, Bartlomiej Zolnierkiewicz wrote:
> >>I like Alan's idea to use loopback instead of "bswap".
> >
> > Or, more likely, device mapper.
>
> Somehow I doubt anybody cares enough to write a whole driver just for
> this unlikely case.

Any TiVo hacker reading this? :)

> For now let's at least record the knowledge...  (patch attached)

Ok, thanks.

Bartlomiej

Re: Worrisome IDE PIO transfers...

[Aubin LaBrosse]

On Sat, 2004-02-28 at 22:08, Bartlomiej Zolnierkiewicz wrote:
> On Sunday 29 of February 2004 03:41, Jeff Garzik wrote:
> > Matt Mackall wrote:
> > > On Sun, Feb 29, 2004 at 01:21:30AM +0100, Bartlomiej Zolnierkiewicz wrote:
> > >>I like Alan's idea to use loopback instead of "bswap".
> > >
> > > Or, more likely, device mapper.
> >
> > Somehow I doubt anybody cares enough to write a whole driver just for
> > this unlikely case.
> 
> Any TiVo hacker reading this? :)
> 

ahem.  now for the whole list instead of just Bart, (sorry Bart) in case
any of you gurus have insights for me (even if it's just to tell me I'm
biting off more than I can chew.)

I'm definitely reading, but not necessarily comprehending.  I upgraded a
60GB TiVo to 2x80GB in about 2002 using 2.4 and some userland tools.  in
fact i was just contemplating doing another upgrade since the 80s in it
now are on their way out i think, so i'll even have some spare TiVo
drives to fool around with during the development of this thing. I'm
probably not qualified to begin an undertaking like this, I'm just a
student, but....I've always wanted to be a kernel hacker. ;-) but i'm
not sure I understand what would be involved. And, does this mean I'll
have to use 2.4 to do my next upcoming drive replacement? (replacing the
drives in the tivo requires a host linux box on which to do it)

If someone would be kind enough to give me a little more info about the
dm driver for tivo drives, i'd at least entertain the idea...

-Aubin

Re: Worrisome IDE PIO transfers...

[Bartlomiej Zolnierkiewicz]

On Sunday 29 of February 2004 03:41, Jeff Garzik wrote:
> Matt Mackall wrote:
> > On Sun, Feb 29, 2004 at 01:21:30AM +0100, Bartlomiej Zolnierkiewicz wrote:
> >>I like Alan's idea to use loopback instead of "bswap".
> >
> > Or, more likely, device mapper.
>
> Somehow I doubt anybody cares enough to write a whole driver just for
> this unlikely case.

http://www.kernel.org/pub/linux/kernel/people/bart/dm-byteswap-2.6.4-rc1.patch

Guess what's this? :)

[ Not that I care so much but I always wanted to learn more about
  device-mapper and crypto API. ]

It is simply a stripped down dm-crypt.c, so all credits go to Christophe.
I have tested it quickly with loop device and it seems to work.

Bartlomiej

Re: Worrisome IDE PIO transfers...

[Christophe Saout]

Am Mo, den 01.03.2004 schrieb Bartlomiej Zolnierkiewicz um 01:47:

> http://www.kernel.org/pub/linux/kernel/people/bart/dm-byteswap-2.6.4-rc1.patch
> 
> Guess what's this? :)

The thieves... they've stolen my precioussss. ;)

> It is simply a stripped down dm-crypt.c, so all credits go to Christophe.
> I have tested it quickly with loop device and it seems to work.

Yes, it's not that complicated. Looks good.
BTW: You don't need the km_types voodoo as the conversion routine is
never called from a softirq context and you are allowed (but should try
to avoid it) to sleep. You could add a conditional reschedule after
kunmapping the buffers to keep the latency low on non-preempt kernels.

BTW: I've got some cleanups and a small fix in Andrew's latest tree
(using a #define for the log prefix and I bvec array thingy).

Re: Worrisome IDE PIO transfers...

[Bartlomiej Zolnierkiewicz]

On Monday 01 of March 2004 14:23, Christophe Saout wrote:
> Am Mo, den 01.03.2004 schrieb Bartlomiej Zolnierkiewicz um 01:47:
> > http://www.kernel.org/pub/linux/kernel/people/bart/dm-byteswap-2.6.4-rc1.
> >patch
> >
> > Guess what's this? :)
>
> The thieves... they've stolen my precioussss. ;)
>
> > It is simply a stripped down dm-crypt.c, so all credits go to Christophe.
> > I have tested it quickly with loop device and it seems to work.
>
> Yes, it's not that complicated. Looks good.
> BTW: You don't need the km_types voodoo as the conversion routine is
> never called from a softirq context and you are allowed (but should try
> to avoid it) to sleep. You could add a conditional reschedule after
> kunmapping the buffers to keep the latency low on non-preempt kernels.

Yes, you are of course right.  I will remove "km_types voodoo".

> BTW: I've got some cleanups and a small fix in Andrew's latest tree
> (using a #define for the log prefix and I bvec array thingy).

Yep, I've seen them already.

Thanks,
Bartlomiej

Re: Worrisome IDE PIO transfers...

[Geert Uytterhoeven]

On Sun, 29 Feb 2004, Bartlomiej Zolnierkiewicz wrote:
> [ Geert added to cc: ]
>
> On Sunday 29 of February 2004 00:24, Jeff Garzik wrote:
> > Looking at the function that is used to transfer data when in PIO mode...
> >
> > void taskfile_output_data (ide_drive_t *drive, void *buffer, u32 wcount)
> > {
> >          if (drive->bswap) {
> >                  ata_bswap_data(buffer, wcount);
> >                  HWIF(drive)->ata_output_data(drive, buffer, wcount);
> >                  ata_bswap_data(buffer, wcount);
> >          } else {
> >                  HWIF(drive)->ata_output_data(drive, buffer, wcount);
> >          }
> > }
> >
> > Swapping the data in-place is very, very wrong...   you don't want to be
> > touching the data that userspace might have mmap'd ...  Additionally,
> > byteswapping back and forth for each PIO sector chews unnecessary CPU.
>
> This is used for accessing "normal" disks on beasts with byte-swapped IDE
> bus (Atari/Q40/TiVo) and "byteswapped" disks on normal machines.
>
> [ Hm. actually I don't see how it can be used for accessing "normal" disks,
>   as data is byteswapped by IDE bus and then swapped back by IDE driver. ]

Why not? The only difference between `normal' and `byteswapped' disks is that
the bytes in a 16-bit word are swapped (sic :-), so you can convert in both
directions by swapping the bytes. Normal disks have been used on Atari before,
so it should (still) work.

BTW, the generic tree misses this patch, which was deemed inappropriate before,
but is needed to make sure the drive identification block is correct on those
machines:

--- linux-2.6.3/drivers/ide/ide-iops.c	Mon Sep 16 09:49:17 2002
+++ linux-m68k-2.6.3/drivers/ide/ide-iops.c	Wed Oct  2 23:01:40 2002
@@ -360,6 +360,23 @@
 	int i;
 	u16 *stringcast;

+#ifdef __mc68000__
+	if (!MACH_IS_AMIGA && !MACH_IS_MAC && !MACH_IS_Q40 && !MACH_IS_ATARI)
+		return;
+
+#ifdef M68K_IDE_SWAPW
+	if (M68K_IDE_SWAPW) {	/* fix bus byteorder first */
+		u_char *p = (u_char *)id;
+		u_char t;
+		for (i = 0; i < 512; i += 2) {
+			t = p[i];
+			p[i] = p[i+1];
+			p[i+1] = t;
+		}
+	}
+#endif
+#endif /* __mc68000__ */
+
 	id->config         = __le16_to_cpu(id->config);
 	id->cyls           = __le16_to_cpu(id->cyls);
 	id->reserved2      = __le16_to_cpu(id->reserved2);


Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: Worrisome IDE PIO transfers...

[Bartlomiej Zolnierkiewicz]

On Sunday 29 of February 2004 09:50, Geert Uytterhoeven wrote:
> On Sun, 29 Feb 2004, Bartlomiej Zolnierkiewicz wrote:
> > [ Geert added to cc: ]
> >
> > On Sunday 29 of February 2004 00:24, Jeff Garzik wrote:
> > > Looking at the function that is used to transfer data when in PIO
> > > mode...
> > >
> > > void taskfile_output_data (ide_drive_t *drive, void *buffer, u32
> > > wcount) {
> > >          if (drive->bswap) {
> > >                  ata_bswap_data(buffer, wcount);
> > >                  HWIF(drive)->ata_output_data(drive, buffer, wcount);
> > >                  ata_bswap_data(buffer, wcount);
> > >          } else {
> > >                  HWIF(drive)->ata_output_data(drive, buffer, wcount);
> > >          }
> > > }
> > >
> > > Swapping the data in-place is very, very wrong...   you don't want to
> > > be touching the data that userspace might have mmap'd ... 
> > > Additionally, byteswapping back and forth for each PIO sector chews
> > > unnecessary CPU.
> >
> > This is used for accessing "normal" disks on beasts with byte-swapped IDE
> > bus (Atari/Q40/TiVo) and "byteswapped" disks on normal machines.
> >
> > [ Hm. actually I don't see how it can be used for accessing "normal"
> > disks, as data is byteswapped by IDE bus and then swapped back by IDE
> > driver. ]
>
> Why not? The only difference between `normal' and `byteswapped' disks is
> that the bytes in a 16-bit word are swapped (sic :-), so you can convert in
> both directions by swapping the bytes. Normal disks have been used on Atari
> before, so it should (still) work.

Oh yes, IDE driver fixes byteorder on Atari/Q40 only when "bswap" is used.

> BTW, the generic tree misses this patch, which was deemed inappropriate
> before, but is needed to make sure the drive identification block is
> correct on those machines:

Thanks.  I can't see a smarter way to fix this now. :/

Bartlomiej