[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3] Fix ATA SMART and CHECK POWER MODE
From: |
Brian Wheeler |
Subject: |
Re: [Qemu-devel] [PATCH v3] Fix ATA SMART and CHECK POWER MODE |
Date: |
Thu, 10 Feb 2011 09:28:19 -0500 |
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