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

[Brian Wheeler]

On Wed, 2011-02-09 at 17:22 -0600, Ryan Harper wrote:
> * Brian Wheeler <address@hidden> [2011-02-09 16:13]:
> > 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.
> > 
> > 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 dd63664..b0b0b35 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 taking from a running system, specifically a
> > +   Seagate ST3500418AS */
> 
> 
> These values ought to have meaning for your hardware, but won't for either the
> virtual disk, nor the underlying storage that the virtual disk is
> running on.  Since we're not attempting to pass any of that info, nor
> keep it in-sync, it probably doesn't matter that much that we're just
> copying device specific data.  I'm open to discussion on how much we
> care about the attribute values[1].
> 
> 1. 
> https://secure.wikimedia.org/wikipedia/en/wiki/S.M.A.R.T.#ATA_S.M.A.R.T._attributes
> 
> 

The main reason for this patch was to make sure the disk tools and
smartd on linux were happy and returned reasonable values.  At some
point I may add on the ability to trigger a smart failure (by jumping
the sectors remapped or something) but for now its read only and not
really meaningful.

> > +static const int smart_attributes[][12] = {
> > +    /* id,  flags, hflags, val, wrst, raw (6 bytes), threshold */
> > +    /* raw read error rate*/
> > +    { 0x01, 0x03, 0x00, 0x74, 0x63, 0x31, 0x6d, 0x3f, 0x0d, 0x00, 0x00, 
> > 0x06},
> 
> probably fine, but this is vendor hardware specific.  I can't think of a
> better number other than 0.
> 

I've set it to zero.

> > +    /* spin up */
> > +    { 0x03, 0x03, 0x00, 0x61, 0x61, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 
> > 0x00},
> 
> default is probably fine as well, though it's dependent upon the
> hardware as well.  Could be zero as well.
> 

I've set it to 16ms so skdump returns something other than 'n/a'


> > +    /* start stop count */
> > +    { 0x04, 0x02, 0x00, 0x64, 0x64, 0x64, 0x00, 0x00, 0x00, 0x00, 0x00, 
> > 0x14},
> 
> depends on hardware and power mgmt, any count is probably fine.
> 
> > +    /* remapped sectors */
> > +    { 0x05, 0x03, 0x00, 0x64, 0x64, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 
> > 0x24},
> 
> Probably should be zero.
> 

When dumping it via skdump or smartctl out it reads as 0 sectors
remapped (as indicated by the raw value).  The value looks like its a
countdown of sectors remaining, so setting it to the 'worst' value is
equivalent to no sectors remapped.


> > +    /* power on hours */
> > +    { 0x09, 0x03, 0x00, 0x61, 0x61, 0x68, 0x0a, 0x00, 0x00, 0x00, 0x00, 
> > 0x00},
> 
> Zero.
> 

I'm going to set it to 1 (hour) so skdump returns something other than
'n/a'

> > +    /* power cycle count */
> > +    { 0x0c, 0x03, 0x00, 0x64, 0x64, 0x32, 0x00, 0x00, 0x00, 0x00, 0x00, 
> > 0x00},
> 
> Zero

I've set it to zero.

> 
> > +    /* airflow-temperature-celsius */
> > +    { 190,  0x03, 0x00, 0x64, 0x64, 0x1f, 0x00, 0x16, 0x22, 0x00, 0x00, 
> > 0x32},
> 
> Something resonably ambient 20-30C, current value is probably fine.
> 

it reads at 31.0C.  I've set the value (and worst)so it matches the raw
value.  (100C - 31C = 69C (0x45)).  I've also adjusted the raw value so
it shows the Min/Max is 31C


> > +    /* 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];
> 
> Needs one more indent for s->io_b, per CODING_STYLE (4. Block structure)
> 

OK

> > +               }
> >             }
> >             s->io_buffer[362] = 0x02 | (s->smart_autosave?0x80:0x00);
> >             if (s->smart_selftest_count == 0) {
> > 
> > 
> 

I've normalized the Value/Worst at 100 (except for airflow).  The new
patch is forthcoming.

Thanks for the feedback!
Brian