[Qemu-devel] PATCH: ide.c: send irq for WIN_DIAGNOSE

[Qemu-devel] PATCH: ide.c: send irq for WIN_DIAGNOSE

[Tristan Gingold]

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

Hi,

according to ATA std:
   The pending interrupt condition shall be set by:
   − the completion of a command; or

This patch sends an irq for WIN_DIAGNOSE (and WIN_SRST)

Tristan.



[-- Attachment #2: qemu.diff --]
[-- Type: application/octet-stream, Size: 1283 bytes --]

Index: hw/ide.c
===================================================================
RCS file: /sources/qemu/qemu/hw/ide.c,v
retrieving revision 1.72
diff -c -r1.72 ide.c
*** hw/ide.c	18 Nov 2007 01:44:37 -0000	1.72
--- hw/ide.c	29 Nov 2007 12:57:51 -0000
***************
*** 2038,2054 ****
              }
              ide_set_irq(s);
              break;
-         case WIN_DIAGNOSE:
-             ide_set_signature(s);
-             s->status = 0x00; /* NOTE: READY is _not_ set */
-             s->error = 0x01;
-             break;
          case WIN_SRST:
              if (!s->is_cdrom)
                  goto abort_cmd;
              ide_set_signature(s);
              s->status = 0x00; /* NOTE: READY is _not_ set */
              s->error = 0x01;
              break;
          case WIN_PACKETCMD:
              if (!s->is_cdrom)
--- 2038,2051 ----
              }
              ide_set_irq(s);
              break;
          case WIN_SRST:
              if (!s->is_cdrom)
                  goto abort_cmd;
+         case WIN_DIAGNOSE:
              ide_set_signature(s);
              s->status = 0x00; /* NOTE: READY is _not_ set */
              s->error = 0x01;
+ 	    ide_set_irq(s);
              break;
          case WIN_PACKETCMD:
              if (!s->is_cdrom)

Re: [Qemu-devel] PATCH: ide.c: send irq for WIN_DIAGNOSE

[Carlo Marcelo Arenas Belon]

On Thu, Nov 29, 2007 at 02:05:36PM +0100, Tristan Gingold wrote:
> according to ATA std:

which ATA std?

>   The pending interrupt condition shall be set by:
>   ??? the completion of a command; or
> 
> This patch sends an irq for WIN_DIAGNOSE (and WIN_SRST)

DEVICE RESET or DEVICE DIAGNOSTIC in T13/1153D revision 18 don't ask for an
irq.

what is the use case you are trying to solve? which guest OS?

Carlo

Re: [Qemu-devel] PATCH: ide.c: send irq for WIN_DIAGNOSE

[Tristan Gingold]

On Nov 29, 2007, at 4:07 PM, Carlo Marcelo Arenas Belon wrote:

> On Thu, Nov 29, 2007 at 02:05:36PM +0100, Tristan Gingold wrote:
>> according to ATA std:
>
> which ATA std?

A rather old one: ATA-3

>>   The pending interrupt condition shall be set by:
>>   ??? the completion of a command; or
>>
>> This patch sends an irq for WIN_DIAGNOSE (and WIN_SRST)
>
> DEVICE RESET or DEVICE DIAGNOSTIC in T13/1153D revision 18 don't  
> ask for an
> irq.

Well, not just after the command is executed.  But according to 9.5.1  
of 1153D:

l) After completing the above steps, Device 0 shall assert INTRQ if  
nIEN is cleared to zero.

So the IRQ is asserted at the end of diagnostic.

> what is the use case you are trying to solve? which guest OS?

The OS timeout during diagnostic.

Tristan.

Re: [Qemu-devel] PATCH: ide.c: send irq for WIN_DIAGNOSE

[Carlo Marcelo Arenas Belon]

On Thu, Nov 29, 2007 at 05:46:08PM +0100, Tristan Gingold wrote:
> On Nov 29, 2007, at 4:07 PM, Carlo Marcelo Arenas Belon wrote:
> >On Thu, Nov 29, 2007 at 02:05:36PM +0100, Tristan Gingold wrote:
> 
> >>  The pending interrupt condition shall be set by:
> >>  ??? the completion of a command; or
> >>
> >>This patch sends an irq for WIN_DIAGNOSE (and WIN_SRST)
> >
> >DEVICE RESET or DEVICE DIAGNOSTIC in T13/1153D revision 18 don't  
> >ask for an
> >irq.
> 
> Well, not just after the command is executed.  But according to 9.5.1  
> of 1153D:
> 
> l) After completing the above steps, Device 0 shall assert INTRQ if  
> nIEN is cleared to zero.
> 
> So the IRQ is asserted at the end of diagnostic.

right my bad, missed that on my copy of ATA-4 while looking for a match to
your description of the mis-implementation, but why are you asserting one
also for the reset?

If I am reading the spec and the code right, the patch should be instead :

Index: hw/ide.c
===================================================================
RCS file: /sources/qemu/qemu/hw/ide.c,v
retrieving revision 1.72
diff -u -r1.72 ide.c
--- hw/ide.c	18 Nov 2007 01:44:37 -0000	1.72
+++ hw/ide.c	30 Nov 2007 14:02:33 -0000
@@ -2042,6 +2053,7 @@
             ide_set_signature(s);
             s->status = 0x00; /* NOTE: READY is _not_ set */
             s->error = 0x01;
+            ide_set_irq(s);
             break;
         case WIN_SRST:
             if (!s->is_cdrom)

> >what is the use case you are trying to solve? which guest OS?
> 
> The OS timeout during diagnostic.

is there a way to reproduce that timeout on the guest OS you are using?
if using Linux and smartctl, you will get a timeout but not because of this
but because SMART is not supported (which might be also a bug, but at least
not this bug)

Carlo

Re: [Qemu-devel] PATCH: ide.c: send irq for WIN_DIAGNOSE

[Tristan Gingold]

On Nov 30, 2007, at 3:12 PM, Carlo Marcelo Arenas Belon wrote:
> right my bad, missed that on my copy of ATA-4 while looking for a  
> match to
> your description of the mis-implementation, but why are you  
> asserting one
> also for the reset?

My fault.  I tried to make code common too quickly!

> If I am reading the spec and the code right, the patch should be  
> instead :

Looks ok to me.

>>> what is the use case you are trying to solve? which guest OS?
>>
>> The OS timeout during diagnostic.
>
> is there a way to reproduce that timeout on the guest OS you are  
> using?

Sure.  I will try asap.

Tristan.

Re: [Qemu-devel] PATCH: ide.c: send irq for WIN_DIAGNOSE

[Tristan Gingold]

> Index: hw/ide.c
> ===================================================================
> RCS file: /sources/qemu/qemu/hw/ide.c,v
> retrieving revision 1.72
> diff -u -r1.72 ide.c
> --- hw/ide.c	18 Nov 2007 01:44:37 -0000	1.72
> +++ hw/ide.c	30 Nov 2007 14:02:33 -0000
> @@ -2042,6 +2053,7 @@
>              ide_set_signature(s);
>              s->status = 0x00; /* NOTE: READY is _not_ set */
>              s->error = 0x01;
> +            ide_set_irq(s);
>              break;
>          case WIN_SRST:
>              if (!s->is_cdrom)


> is there a way to reproduce that timeout on the guest OS you are  
> using?

I confirm this fixes the timeout issue.
Tristan.