From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=53866 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PnJMx-0004cG-C6 for qemu-devel@nongnu.org; Wed, 09 Feb 2011 18:22:44 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PnJMu-0000qm-0J for qemu-devel@nongnu.org; Wed, 09 Feb 2011 18:22:41 -0500 Received: from e34.co.us.ibm.com ([32.97.110.152]:34161) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PnJMt-0000py-Or for qemu-devel@nongnu.org; Wed, 09 Feb 2011 18:22:39 -0500 Received: from d03relay03.boulder.ibm.com (d03relay03.boulder.ibm.com [9.17.195.228]) by e34.co.us.ibm.com (8.14.4/8.13.1) with ESMTP id p19NBFRT008422 for ; Wed, 9 Feb 2011 16:11:16 -0700 Received: from d03av02.boulder.ibm.com (d03av02.boulder.ibm.com [9.17.195.168]) by d03relay03.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p19NMZW8111004 for ; Wed, 9 Feb 2011 16:22:35 -0700 Received: from d03av02.boulder.ibm.com (loopback [127.0.0.1]) by d03av02.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p19NMZ9a014116 for ; Wed, 9 Feb 2011 16:22:35 -0700 Date: Wed, 9 Feb 2011 17:22:30 -0600 From: Ryan Harper Subject: Re: [Qemu-devel] [PATCH v3] Fix ATA SMART and CHECK POWER MODE Message-ID: <20110209232230.GE26301@us.ibm.com> References: <1297289526.31675.2.camel@nibbler.dlib.indiana.edu> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1297289526.31675.2.camel@nibbler.dlib.indiana.edu> List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Brian Wheeler Cc: qemu-devel * 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 > +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