[PATCH] ata.mod under qemu

[PATCH] ata.mod under qemu

[Vladimir 'phcoder' Serbinenko]

Hello. While experimenting with ata.mod on qemu I noticed it doesn't
see the CD when it wasn't already initialised by bios. Just putting
the status check after identify check solves the problem. However I'm
not familiar with ata so I would like someone to have a look at it

-- 
Regards
Vladimir 'phcoder' Serbinenko

Re: [PATCH] ata.mod under qemu

[Vladimir 'phcoder' Serbinenko]

On Mon, May 18, 2009 at 7:59 PM, Vladimir 'phcoder' Serbinenko
<phcoder@gmail.com> wrote:
> Hello. While experimenting with ata.mod on qemu I noticed it doesn't
> see the CD when it wasn't already initialised by bios. Just putting
> the status check after identify check solves the problem. However I'm
> not familiar with ata so I would like someone to have a look at it
Forgotten attachement
>
> --
> Regards
> Vladimir 'phcoder' Serbinenko
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Re: [PATCH] ata.mod under qemu

[Vladimir 'phcoder' Serbinenko]

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

On Mon, May 18, 2009 at 7:59 PM, Vladimir 'phcoder' Serbinenko
<phcoder@gmail.com> wrote:
> On Mon, May 18, 2009 at 7:59 PM, Vladimir 'phcoder' Serbinenko
> <phcoder@gmail.com> wrote:
>> Hello. While experimenting with ata.mod on qemu I noticed it doesn't
>> see the CD when it wasn't already initialised by bios. Just putting
>> the status check after identify check solves the problem. However I'm
>> not familiar with ata so I would like someone to have a look at it
> Forgotten attachement
File was empty and mailer was rejecting it. I haven't forgotten it
actually. Anyway here you go
>>
>> --
>> Regards
>> Vladimir 'phcoder' Serbinenko
>>
>
>
>
> --
> Regards
> Vladimir 'phcoder' Serbinenko
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #2: ata.diff --]
[-- Type: text/x-diff, Size: 1677 bytes --]

diff --git a/ChangeLog b/ChangeLog
index f5bc28b..4dec14d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2009-05-18  Vladimir Serbinenko  <phcoder@gmail.com>
+
+	Fix a false negative for ata disks
+
+	* disk/ata.c (grub_ata_device_initialize): check for status after 
+	identify
+	
 2009-05-17  Vladimir Serbinenko  <phcoder@gmail.com>
 
 	Not fail if unable to retrieve C/H/S on LBA disks
diff --git a/disk/ata.c b/disk/ata.c
index ea42d59..32a314b 100644
--- a/disk/ata.c
+++ b/disk/ata.c
@@ -334,16 +334,6 @@ grub_ata_device_initialize (int port, int device, int addr, int addr2)
   grub_ata_regset (dev, GRUB_ATA_REG_DISK, dev->device << 4);
   grub_ata_wait ();
 
-  /* If status is 0x00, it is safe to assume that there
-     is no device (or only a !READY) device connected.  */
-  grub_int8_t sts = grub_ata_regget (dev, GRUB_ATA_REG_STATUS);
-  grub_dprintf ("ata", "status=0x%x\n", sts);
-  if (sts == 0x00)
-    {
-      grub_free(dev);
-      return 0;
-    }
-
   /* Try to detect if the port is in use by writing to it,
      waiting for a while and reading it again.  If the value
      was preserved, there is a device connected.
@@ -365,6 +355,17 @@ grub_ata_device_initialize (int port, int device, int addr, int addr2)
   if (grub_ata_identify (dev))
     {
       grub_free (dev);
+      grub_errno = GRUB_ERR_NONE;
+      return 0;
+    }
+
+  /* If status is 0x00, it is safe to assume that there
+     is no device (or only a !READY) device connected.  */
+  grub_int8_t sts = grub_ata_regget (dev, GRUB_ATA_REG_STATUS);
+  grub_dprintf ("ata", "status=0x%x\n", sts);
+  if (sts == 0x00)
+    {
+      grub_free(dev);
       return 0;
     }
 

Re: [PATCH] ata.mod under qemu

[Pavel Roskin]

On Mon, 2009-05-18 at 20:04 +0200, Vladimir 'phcoder' Serbinenko wrote:
> On Mon, May 18, 2009 at 7:59 PM, Vladimir 'phcoder' Serbinenko
> <phcoder@gmail.com> wrote:
> > On Mon, May 18, 2009 at 7:59 PM, Vladimir 'phcoder' Serbinenko
> > <phcoder@gmail.com> wrote:
> >> Hello. While experimenting with ata.mod on qemu I noticed it doesn't
> >> see the CD when it wasn't already initialised by bios. Just putting
> >> the status check after identify check solves the problem. However I'm
> >> not familiar with ata so I would like someone to have a look at it
> > Forgotten attachement
> File was empty and mailer was rejecting it. I haven't forgotten it
> actually. Anyway here you go

I did a quick check, and your patch has no effect on my system.  I'm
using Fedora 11 with qemu 0.10.  After inserting the ata module, ls
shows no disks, with or without your patch.

-- 
Regards,
Pavel Roskin

Re: [PATCH] ata.mod under qemu

[Pavel Roskin]

On Mon, 2009-05-18 at 19:36 -0400, Pavel Roskin wrote:

> I did a quick check, and your patch has no effect on my system.  I'm
> using Fedora 11 with qemu 0.10.  After inserting the ata module, ls
> shows no disks, with or without your patch.

Never mind, my "quick check" was too quick.  I forgot to change prefix,
so it's just ls that was failing.

Your patch makes the CD-ROM visible.  Without your patch, only hard
drivers are seen.

-- 
Regards,
Pavel Roskin

Re: [PATCH] ata.mod under qemu

[Christian Franke]

Pavel Roskin wrote:
> On Mon, 2009-05-18 at 19:36 -0400, Pavel Roskin wrote:
> 
> > I did a quick check, and your patch has no effect on my system.  I'm
> > using Fedora 11 with qemu 0.10.  After inserting the ata module, ls
> > shows no disks, with or without your patch.
> > 
> 
> Never mind, my "quick check" was too quick.  I forgot to change
> prefix, so it's just ls that was failing.
> 
> Your patch makes the CD-ROM visible.  Without your patch, only hard
> drivers are seen.
> 

Thanks for the problem report.

According to 'qemu-0.10.4/hw/ide.c', the (READY bit of the) status
register is actually cleared by some commands like RESET and DIAGNOSE if
an ATAPI device is connected. This conforms to ATA parallel transport
standard.

When I added the '(sts == 0x00)' check, I made the false assumption that
device detection through the READY bit is safe. It was added to speed-up
the detection of the missing slave port of some SATA controllers working
in IDE-mode. The check can be safely removed, the only drawback is an
extra ~1s timeout for each SATA controller.

I will commit a fix soon.

-- 
Regards,
Christian Franke

Re: [PATCH] ata.mod under qemu

[Christian Franke]

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

The attached patch should fix this. It should also prevents misleading 
error messages if a device does not exist.

2009-05-20  Christian Franke  <franke@computer.org>

	* disk/ata.c: (grub_ata_wait_not_busy): Add debug output of status
	register.
	(grub_ata_identify): Suppress error message if status register
	is 0x00 after command failure.
	(grub_device_initialize): Remove unsafe status register check.
	Thanks to 'phcoder' for problem report and patch.
	Prevent sign extension in debug message.


-- 
Regards,
Christian Franke



[-- Attachment #2: grub2-ata-status-fix.patch --]
[-- Type: text/x-diff, Size: 3402 bytes --]

diff --git a/disk/ata.c b/disk/ata.c
index ea42d59..651937d 100644
--- a/disk/ata.c
+++ b/disk/ata.c
@@ -41,11 +41,14 @@ grub_ata_wait_not_busy (struct grub_ata_device *dev, int milliseconds)
   grub_millisleep (1);
 
   int i = 1;
-  while (grub_ata_regget (dev, GRUB_ATA_REG_STATUS) & GRUB_ATA_STATUS_BUSY)
+  grub_uint8_t sts;
+  while ((sts = grub_ata_regget (dev, GRUB_ATA_REG_STATUS))
+	 & GRUB_ATA_STATUS_BUSY)
     {
       if (i >= milliseconds)
         {
-	  grub_dprintf ("ata", "timeout: %dms\n", milliseconds);
+	  grub_dprintf ("ata", "timeout: %dms, status=0x%x\n",
+			milliseconds, sts);
 	  return grub_error (GRUB_ERR_TIMEOUT, "ATA timeout");
 	}
 
@@ -259,20 +262,21 @@ grub_ata_identify (struct grub_ata_device *dev)
 
   if (grub_ata_wait_drq (dev, 0, GRUB_ATA_TOUT_STD))
     {
+      grub_free(info);
+      grub_errno = GRUB_ERR_NONE;
+
       if (grub_ata_regget (dev, GRUB_ATA_REG_ERROR) & 0x04) /* ABRT */
-	{
-	  /* Device without ATA IDENTIFY, try ATAPI.  */
-	  grub_free(info);
-	  grub_errno = GRUB_ERR_NONE;
-	  return grub_atapi_identify (dev);
-	}
+	/* Device without ATA IDENTIFY, try ATAPI.  */
+	return grub_atapi_identify (dev);
+
+      else if (grub_ata_regget (dev, GRUB_ATA_REG_STATUS) == 0x00)
+	/* No device, return error but don't print message.  */
+	return GRUB_ERR_UNKNOWN_DEVICE;
+
       else
-	{
-	  /* Error.  */
-	  grub_free(info);
-	  return grub_error (GRUB_ERR_UNKNOWN_DEVICE,
-			     "device can not be identified");
-	}
+	/* Other Error.  */
+	return grub_error (GRUB_ERR_UNKNOWN_DEVICE,
+			   "device can not be identified");
     }
 
   grub_ata_pio_read (dev, info, GRUB_DISK_SECTOR_SIZE);
@@ -334,26 +338,12 @@ grub_ata_device_initialize (int port, int device, int addr, int addr2)
   grub_ata_regset (dev, GRUB_ATA_REG_DISK, dev->device << 4);
   grub_ata_wait ();
 
-  /* If status is 0x00, it is safe to assume that there
-     is no device (or only a !READY) device connected.  */
-  grub_int8_t sts = grub_ata_regget (dev, GRUB_ATA_REG_STATUS);
-  grub_dprintf ("ata", "status=0x%x\n", sts);
-  if (sts == 0x00)
-    {
-      grub_free(dev);
-      return 0;
-    }
-
   /* Try to detect if the port is in use by writing to it,
      waiting for a while and reading it again.  If the value
-     was preserved, there is a device connected.
-     But this tests often detects a second (slave) device
-     connected to a SATA controller which supports only one
-     (master) device.  In this case, the status register
-     check above usually works.  */
+     was preserved, there is a device connected.  */
   grub_ata_regset (dev, GRUB_ATA_REG_SECTORS, 0x5A);  
   grub_ata_wait ();
-  grub_int8_t sec = grub_ata_regget (dev, GRUB_ATA_REG_SECTORS);
+  grub_uint8_t sec = grub_ata_regget (dev, GRUB_ATA_REG_SECTORS);
   grub_dprintf ("ata", "sectors=0x%x\n", sec);
   if (sec != 0x5A)
     {
@@ -361,6 +351,12 @@ grub_ata_device_initialize (int port, int device, int addr, int addr2)
       return 0;
     }
 
+  /* The above test may detect a second (slave) device
+     connected to a SATA controller which supports only one
+     (master) device.  It is not safe to use the status register
+     READY bit to check for controller channel existence.  Some
+     ATAPI commands (RESET, DIAGNOSTIC) may clear this bit.  */
+
   /* Use the IDENTIFY DEVICE command to query the device.  */
   if (grub_ata_identify (dev))
     {

Re: [PATCH] ata.mod under qemu

[Pavel Roskin]

On Wed, 2009-05-20 at 23:40 +0200, Christian Franke wrote: 
> The attached patch should fix this. It should also prevents misleading 
> error messages if a device does not exist.
> 
> 2009-05-20  Christian Franke  <franke@computer.org>
> 
> 	* disk/ata.c: (grub_ata_wait_not_busy): Add debug output of status
> 	register.
> 	(grub_ata_identify): Suppress error message if status register
> 	is 0x00 after command failure.
> 	(grub_device_initialize): Remove unsafe status register check.
> 	Thanks to 'phcoder' for problem report and patch.
> 	Prevent sign extension in debug message.

It helps detect CD-ROM under qemu.  But on the real hardware, it
introduces ghost drives.

Without the patch, I have (ata7) for the SATA hard drive.  With the
patch, I have (ata7) for the SATA hard drive, (ata6) for the SATA DVD-RW
and two bogus unreadable drives (ata0) and (ata1).

-- 
Regards,
Pavel Roskin

Re: [PATCH] ata.mod under qemu

[Christian Franke]

Pavel Roskin wrote:
> On Wed, 2009-05-20 at 23:40 +0200, Christian Franke wrote: 
>   
>> The attached patch should fix this. It should also prevents misleading 
>> error messages if a device does not exist.
>>     
> It helps detect CD-ROM under qemu.  But on the real hardware, it
> introduces ghost drives.
>
> Without the patch, I have (ata7) for the SATA hard drive.  With the
> patch, I have (ata7) for the SATA hard drive, (ata6) for the SATA DVD-RW
> and two bogus unreadable drives (ata0) and (ata1).
>
>   
Thanks for this test. I could not reproduce this yet. I presume that the 
controller behind (ata0) and (ata1) is the chipset's PATA controller.
If no device is connected, these controllers often echo a write to (e.g. 
command) register address back to the next read of (e.g. status) 
register address if the time between is too short. (No VM emulates this 
behavior :-)

Could you possibly repeat this test with 'debug=ata' enabled? If this 
removes the bogus drives, it is likely a problem of too short wait times.

I will prepare a new patch with longer timeouts and more thorough 
register tests during identify.

-- 
Regards,
Christian Franke

Re: [PATCH] ata.mod under qemu

[Christian Franke]

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

Christian Franke wrote:
> Pavel Roskin wrote:
>>>  
>> It helps detect CD-ROM under qemu.  But on the real hardware, it
>> introduces ghost drives.
>>
>> Without the patch, I have (ata7) for the SATA hard drive.  With the
>> patch, I have (ata7) for the SATA hard drive, (ata6) for the SATA DVD-RW
>> and two bogus unreadable drives (ata0) and (ata1).
>>
>>   

New patch attached. Please test on the above hardware if possible.

-- 
Regards,
Christian Franke


[-- Attachment #2: grub2-ata-status-fix-2.patch --]
[-- Type: text/x-diff, Size: 4623 bytes --]

diff --git a/disk/ata.c b/disk/ata.c
index ea42d59..5fa0ef5 100644
--- a/disk/ata.c
+++ b/disk/ata.c
@@ -41,11 +41,14 @@ grub_ata_wait_not_busy (struct grub_ata_device *dev, int milliseconds)
   grub_millisleep (1);
 
   int i = 1;
-  while (grub_ata_regget (dev, GRUB_ATA_REG_STATUS) & GRUB_ATA_STATUS_BUSY)
+  grub_uint8_t sts;
+  while ((sts = grub_ata_regget (dev, GRUB_ATA_REG_STATUS))
+	 & GRUB_ATA_STATUS_BUSY)
     {
       if (i >= milliseconds)
         {
-	  grub_dprintf ("ata", "timeout: %dms\n", milliseconds);
+	  grub_dprintf ("ata", "timeout: %dms, status=0x%x\n",
+			milliseconds, sts);
 	  return grub_error (GRUB_ERR_TIMEOUT, "ATA timeout");
 	}
 
@@ -151,6 +154,7 @@ grub_atapi_identify (struct grub_ata_device *dev)
     return grub_errno;
 
   grub_ata_regset (dev, GRUB_ATA_REG_DISK, 0xE0 | dev->device << 4);
+  grub_ata_wait ();
   if (grub_ata_check_ready (dev))
     {
       grub_free (info);
@@ -248,6 +252,7 @@ grub_ata_identify (struct grub_ata_device *dev)
   info16 = (grub_uint16_t *) info;
 
   grub_ata_regset (dev, GRUB_ATA_REG_DISK, 0xE0 | dev->device << 4);
+  grub_ata_wait ();
   if (grub_ata_check_ready (dev))
     {
       grub_free (info);
@@ -259,24 +264,38 @@ grub_ata_identify (struct grub_ata_device *dev)
 
   if (grub_ata_wait_drq (dev, 0, GRUB_ATA_TOUT_STD))
     {
-      if (grub_ata_regget (dev, GRUB_ATA_REG_ERROR) & 0x04) /* ABRT */
-	{
-	  /* Device without ATA IDENTIFY, try ATAPI.  */
-	  grub_free(info);
-	  grub_errno = GRUB_ERR_NONE;
-	  return grub_atapi_identify (dev);
-	}
+      grub_free(info);
+      grub_errno = GRUB_ERR_NONE;
+      grub_uint8_t sts = grub_ata_regget (dev, GRUB_ATA_REG_STATUS);
+
+      if ((sts & (GRUB_ATA_STATUS_BUSY | GRUB_ATA_STATUS_DRQ
+	  | GRUB_ATA_STATUS_ERR)) == GRUB_ATA_STATUS_ERR
+	  && (grub_ata_regget (dev, GRUB_ATA_REG_ERROR) & 0x04 /* ABRT */))
+	/* Device without ATA IDENTIFY, try ATAPI.  */
+	return grub_atapi_identify (dev);
+
+      else if (sts == 0x00)
+	/* No device, return error but don't print message.  */
+	return GRUB_ERR_UNKNOWN_DEVICE;
+
       else
-	{
-	  /* Error.  */
-	  grub_free(info);
-	  return grub_error (GRUB_ERR_UNKNOWN_DEVICE,
-			     "device can not be identified");
-	}
+	/* Other Error.  */
+	return grub_error (GRUB_ERR_UNKNOWN_DEVICE,
+			   "device can not be identified");
     }
 
   grub_ata_pio_read (dev, info, GRUB_DISK_SECTOR_SIZE);
 
+  /* Re-check status to avoid bogus identify data due to stuck DRQ.  */
+  grub_uint8_t sts = grub_ata_regget (dev, GRUB_ATA_REG_STATUS);
+  if (sts & (GRUB_ATA_STATUS_BUSY | GRUB_ATA_STATUS_DRQ | GRUB_ATA_STATUS_ERR))
+    {
+      grub_dprintf("ata", "bad status=0x%x", sts);
+      grub_free(info);
+      return grub_error (GRUB_ERR_UNKNOWN_DEVICE,
+			 "error reading ATA IDENTIFY data");
+    }
+
   /* Now it is certain that this is not an ATAPI device.  */
   dev->atapi = 0;
 
@@ -334,26 +353,12 @@ grub_ata_device_initialize (int port, int device, int addr, int addr2)
   grub_ata_regset (dev, GRUB_ATA_REG_DISK, dev->device << 4);
   grub_ata_wait ();
 
-  /* If status is 0x00, it is safe to assume that there
-     is no device (or only a !READY) device connected.  */
-  grub_int8_t sts = grub_ata_regget (dev, GRUB_ATA_REG_STATUS);
-  grub_dprintf ("ata", "status=0x%x\n", sts);
-  if (sts == 0x00)
-    {
-      grub_free(dev);
-      return 0;
-    }
-
   /* Try to detect if the port is in use by writing to it,
      waiting for a while and reading it again.  If the value
-     was preserved, there is a device connected.
-     But this tests often detects a second (slave) device
-     connected to a SATA controller which supports only one
-     (master) device.  In this case, the status register
-     check above usually works.  */
+     was preserved, there is a device connected.  */
   grub_ata_regset (dev, GRUB_ATA_REG_SECTORS, 0x5A);  
   grub_ata_wait ();
-  grub_int8_t sec = grub_ata_regget (dev, GRUB_ATA_REG_SECTORS);
+  grub_uint8_t sec = grub_ata_regget (dev, GRUB_ATA_REG_SECTORS);
   grub_dprintf ("ata", "sectors=0x%x\n", sec);
   if (sec != 0x5A)
     {
@@ -361,6 +366,12 @@ grub_ata_device_initialize (int port, int device, int addr, int addr2)
       return 0;
     }
 
+  /* The above test may detect a second (slave) device
+     connected to a SATA controller which supports only one
+     (master) device.  It is not safe to use the status register
+     READY bit to check for controller channel existence.  Some
+     ATAPI commands (RESET, DIAGNOSTIC) may clear this bit.  */
+
   /* Use the IDENTIFY DEVICE command to query the device.  */
   if (grub_ata_identify (dev))
     {

Re: [PATCH] ata.mod under qemu

[Pavel Roskin]

On Sat, 2009-05-23 at 23:04 +0200, Christian Franke wrote:
> Christian Franke wrote:
> > Pavel Roskin wrote:
> >>>  
> >> It helps detect CD-ROM under qemu.  But on the real hardware, it
> >> introduces ghost drives.
> >>
> >> Without the patch, I have (ata7) for the SATA hard drive.  With the
> >> patch, I have (ata7) for the SATA hard drive, (ata6) for the SATA DVD-RW
> >> and two bogus unreadable drives (ata0) and (ata1).
> >>
> >>   
> 
> New patch attached. Please test on the above hardware if possible.

It's working fine.  (ata6) and (ata7) appear, but (ata0) and (ata1)
doesn't.  When inserting ata.mod, I get:

error: error reading ATA IDENTIFY data
error: error reading ATA IDENTIFY data

That must be the "ghost devices".  While it would be nice to get rid of
such messages, I'm fine if your patch is committed as is.

-- 
Regards,
Pavel Roskin

Re: [PATCH] ata.mod under qemu

[Christian Franke]

Pavel Roskin wrote:
> On Sat, 2009-05-23 at 23:04 +0200, Christian Franke wrote:
>   
>>
>> New patch attached. Please test on the above hardware if possible.
>>     
>
> It's working fine.  (ata6) and (ata7) appear, but (ata0) and (ata1)
> doesn't.  When inserting ata.mod, I get:
>
> error: error reading ATA IDENTIFY data
> error: error reading ATA IDENTIFY data
>   

Looks like the (ata0/1) controller returns !BSY, DRQ, !ERR in the status 
register if no device is connected.
This is now detected after the bogus IDENTIFY data is read.

> That must be the "ghost devices".  While it would be nice to get rid of
> such messages, I'm fine if your patch is committed as is.
>
>   

Modified version committed, messages should no longer appear.

Thanks!

-- 
Regards,
Christian Franke

Re: [PATCH] ata.mod under qemu

[Pavel Roskin]

On Mon, 2009-05-25 at 21:47 +0200, Christian Franke wrote:

> > That must be the "ghost devices".  While it would be nice to get rid of
> > such messages, I'm fine if your patch is committed as is.
> >
> >   
> 
> Modified version committed, messages should no longer appear.

The messages are gone, everything is fine.  Thank you!

-- 
Regards,
Pavel Roskin