From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=51355 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PnXVW-0008UF-Vm for qemu-devel@nongnu.org; Thu, 10 Feb 2011 09:28:32 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PnXVP-0006VU-4a for qemu-devel@nongnu.org; Thu, 10 Feb 2011 09:28:24 -0500 Received: from hartman.uits.indiana.edu ([129.79.1.194]:46175 helo=internal-relay.indiana.edu) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PnXVO-0006VN-TI for qemu-devel@nongnu.org; Thu, 10 Feb 2011 09:28:23 -0500 Subject: Re: [Qemu-devel] [PATCH v3] Fix ATA SMART and CHECK POWER MODE From: Brian Wheeler In-Reply-To: <20110209232230.GE26301@us.ibm.com> References: <1297289526.31675.2.camel@nibbler.dlib.indiana.edu> <20110209232230.GE26301@us.ibm.com> Content-Type: text/plain; charset="UTF-8" Date: Thu, 10 Feb 2011 09:28:19 -0500 Message-ID: <1297348099.24995.16.camel@nibbler.dlib.indiana.edu> Mime-Version: 1.0 Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Ryan Harper Cc: qemu-devel On Wed, 2011-02-09 at 17:22 -0600, Ryan Harper wrote: > * Brian Wheeler [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 > > > > -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