Re: [Qemu-devel] [PATCH v5] PING: Fix ATA SMART and CHECK POWER MODE

[Stefan Hajnoczi]

On Mon, Mar 7, 2011 at 8:28 AM, Kevin Wolf <address@hidden> wrote:
> Am 06.03.2011 20:26, schrieb Aurelien Jarno:
>> You should Cc: the IDE maintainer (Kevin Wolf) so that this patch can
>> get acked. Done with this mail.
>
> Thanks. In fact, I saw the patch, but it still needs a review. Can
> whoever commented on the previous versions give it the review and post
> an Acked-by? If not, I'll try to get to it myself.

I think it was Ryan.  Original email below:

>> On Tue, Mar 01, 2011 at 08:30:23AM -0500, Brian Wheeler wrote:
>>> This patch fixes two things:
>>>
>>>  1) CHECK POWER MODE
>>>
>>> The error return value wasn't always zero, so it would show up as
>>> offline.  Error is now explicitly set to zero.
>>>
>>>  2) SMART
>>>
>>> The smart values that were returned were invalid and tools like skdump
>>> would not recognize that the smart data was actually valid and would
>>> dump weird output.  The data has been fixed up and raw value support
>>> was added.  Tools like skdump and palimpsest work as expected.
>>>
>>> v5 changes:  rebase
>>> v4 changes:  incorporate changes from Ryan Harper
>>> v3 changes:  don't reformat code I didn't change
>>> v2 changes:  use single structure instead of one for thresholds and one
>>> for data.
>>>
>>> Signed-off-by: address@hidden
>>> ----------------
>>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>>> index 9c91a49..1ffca56 100644
>>> --- a/hw/ide/core.c
>>> +++ b/hw/ide/core.c
>>> @@ -34,13 +34,26 @@
>>>
>>>  #include <hw/ide/internal.h>
>>>
>>> -static const int smart_attributes[][5] = {
>>> -    /* id,  flags, val, wrst, thrsh */
>>> -    { 0x01, 0x03, 0x64, 0x64, 0x06}, /* raw read */
>>> -    { 0x03, 0x03, 0x64, 0x64, 0x46}, /* spin up */
>>> -    { 0x04, 0x02, 0x64, 0x64, 0x14}, /* start stop count */
>>> -    { 0x05, 0x03, 0x64, 0x64, 0x36}, /* remapped sectors */
>>> -    { 0x00, 0x00, 0x00, 0x00, 0x00}
>>> +/* These values were based on a Seagate ST3500418AS but have been modified
>>> +   to make more sense in QEMU */
>>> +static const int smart_attributes[][12] = {
>>> +    /* id,  flags, hflags, val, wrst, raw (6 bytes), threshold */
>>> +    /* raw read error rate*/
>>> +    { 0x01, 0x03, 0x00, 0x64, 0x64, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 
>>> 0x06},
>>> +    /* spin up */
>>> +    { 0x03, 0x03, 0x00, 0x64, 0x64, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 
>>> 0x00},
>>> +    /* start stop count */
>>> +    { 0x04, 0x02, 0x00, 0x64, 0x64, 0x64, 0x00, 0x00, 0x00, 0x00, 0x00, 
>>> 0x14},
>>> +    /* remapped sectors */
>>> +    { 0x05, 0x03, 0x00, 0x64, 0x64, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 
>>> 0x24},
>>> +    /* power on hours */
>>> +    { 0x09, 0x03, 0x00, 0x64, 0x64, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 
>>> 0x00},
>>> +    /* power cycle count */
>>> +    { 0x0c, 0x03, 0x00, 0x64, 0x64, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 
>>> 0x00},
>>> +    /* airflow-temperature-celsius */
>>> +    { 190,  0x03, 0x00, 0x45, 0x45, 0x1f, 0x00, 0x1f, 0x1f, 0x00, 0x00, 
>>> 0x32},
>>> +    /* end of list */
>>> +    { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 
>>> 0x00}
>>>  };
>>>
>>>  /* XXX: DVDs that could fit on a CD will be reported as a CD */
>>> @@ -1843,6 +1856,7 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
>>>          break;
>>>      case WIN_CHECKPOWERMODE1:
>>>      case WIN_CHECKPOWERMODE2:
>>> +        s->error = 0;
>>>          s->nsector = 0xff; /* device active or idle */
>>>          s->status = READY_STAT | SEEK_STAT;
>>>          ide_set_irq(s->bus);
>>> @@ -2097,7 +2111,7 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
>>>              if (smart_attributes[n][0] == 0)
>>>                      break;
>>>              s->io_buffer[2+0+(n*12)] = smart_attributes[n][0];
>>> -            s->io_buffer[2+1+(n*12)] = smart_attributes[n][4];
>>> +            s->io_buffer[2+1+(n*12)] = smart_attributes[n][11];
>>>              }
>>>              for (n=0; n<511; n++) /* checksum */
>>>              s->io_buffer[511] += s->io_buffer[n];
>>> @@ -2110,12 +2124,13 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
>>>              memset(s->io_buffer, 0, 0x200);
>>>              s->io_buffer[0] = 0x01; /* smart struct version */
>>>              for (n=0; n<30; n++) {
>>> -            if (smart_attributes[n][0] == 0)
>>> +                if (smart_attributes[n][0] == 0) {
>>>                      break;
>>> -            s->io_buffer[2+0+(n*12)] = smart_attributes[n][0];
>>> -            s->io_buffer[2+1+(n*12)] = smart_attributes[n][1];
>>> -            s->io_buffer[2+3+(n*12)] = smart_attributes[n][2];
>>> -            s->io_buffer[2+4+(n*12)] = smart_attributes[n][3];
>>> +                }
>>> +                int i;
>>> +                for(i = 0; i < 11; i++) {
>>> +                    s->io_buffer[2+i+(n*12)] = smart_attributes[n][i];
>>> +                }
>>>              }
>>>              s->io_buffer[362] = 0x02 | (s->smart_autosave?0x80:0x00);
>>>              if (s->smart_selftest_count == 0) {