* [Qemu-devel] [PATCH v3] Fix ATA SMART and CHECK POWER MODE
@ 2011-02-09 22:12 Brian Wheeler
2011-02-09 23:22 ` Ryan Harper
0 siblings, 1 reply; 3+ messages in thread
From: Brian Wheeler @ 2011-02-09 22:12 UTC (permalink / raw)
To: qemu-devel
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: bdwheele@indiana.edu
----------------
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 */
+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},
+ /* spin up */
+ { 0x03, 0x03, 0x00, 0x61, 0x61, 0x00, 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, 0x61, 0x61, 0x68, 0x0a, 0x00, 0x00, 0x00, 0x00, 0x00},
+ /* power cycle count */
+ { 0x0c, 0x03, 0x00, 0x64, 0x64, 0x32, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00},
+ /* airflow-temperature-celsius */
+ { 190, 0x03, 0x00, 0x64, 0x64, 0x1f, 0x00, 0x16, 0x22, 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) {
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH v3] Fix ATA SMART and CHECK POWER MODE
2011-02-09 22:12 [Qemu-devel] [PATCH v3] Fix ATA SMART and CHECK POWER MODE Brian Wheeler
@ 2011-02-09 23:22 ` Ryan Harper
2011-02-10 14:28 ` Brian Wheeler
0 siblings, 1 reply; 3+ messages in thread
From: Ryan Harper @ 2011-02-09 23:22 UTC (permalink / raw)
To: Brian Wheeler; +Cc: qemu-devel
* Brian Wheeler <bdwheele@indiana.edu> [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: bdwheele@indiana.edu
> ----------------
> 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
> +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.
> + /* 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.
> + /* 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.
> + /* power on hours */
> + { 0x09, 0x03, 0x00, 0x61, 0x61, 0x68, 0x0a, 0x00, 0x00, 0x00, 0x00, 0x00},
Zero.
> + /* power cycle count */
> + { 0x0c, 0x03, 0x00, 0x64, 0x64, 0x32, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00},
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.
> + /* 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)
> + }
> }
> s->io_buffer[362] = 0x02 | (s->smart_autosave?0x80:0x00);
> if (s->smart_selftest_count == 0) {
>
>
--
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
ryanh@us.ibm.com
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH v3] Fix ATA SMART and CHECK POWER MODE
2011-02-09 23:22 ` Ryan Harper
@ 2011-02-10 14:28 ` Brian Wheeler
0 siblings, 0 replies; 3+ messages in thread
From: Brian Wheeler @ 2011-02-10 14:28 UTC (permalink / raw)
To: Ryan Harper; +Cc: qemu-devel
On Wed, 2011-02-09 at 17:22 -0600, Ryan Harper wrote:
> * Brian Wheeler <bdwheele@indiana.edu> [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: bdwheele@indiana.edu
> > ----------------
> > 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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2011-02-10 14:28 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-02-09 22:12 [Qemu-devel] [PATCH v3] Fix ATA SMART and CHECK POWER MODE Brian Wheeler
2011-02-09 23:22 ` Ryan Harper
2011-02-10 14:28 ` Brian Wheeler
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.