[nvmewin] SGL issue

[Chang, Alex]

In addition, the logic seems assuming numImplicitEntries as 1 when the size is less than or equal to PAGE_SIZE.

Thanks,
Alex

________________________________
From: Luse, Paul E [mailto:paul.e.luse at intel.com]
Sent: Wednesday, April 18, 2012 8:18 AM
To: Chang, Alex; nvmewin at lists.openfabrics.org
Subject: SGL issue

Ugh, replied to the wrong email subject name again.  Anyway, I think I see the issue here.  I believe we intended the mod operation below should be done w/the address not the size... as I did in my example below and then I realized the code would have done that mod on 0x100 and gotten a 0 back.

From: Luse, Paul E
Sent: Wednesday, April 18, 2012 8:07 AM
To: 'Chang, Alex'; nvmewin at lists.openfabrics.org
Subject: RE: [nvmewin] ***UNCHECKED*** NEW patch for review based on some recent HW bring up work

Alex,

Where you state: In SntiTranslateSglToPrp, numImplicitEntries is assigned as 1 (Line 4238).

I don't understand why numImplicitEntries doesn't become 2 with this:
            numImplicitEntries += (((sgElementSize % PAGE_SIZE) != 0) ? 1 : 0);
since 0x1`12345600 mod 0x1000 = 0x600

What am I missing here?

Thx
Paul

From: Chang, Alex [mailto:Alex.Chang at idt.com]<mailto:[mailto:Alex.Chang at idt.com]>
Sent: Monday, April 16, 2012 5:09 PM
To: Chang, Alex; Luse, Paul E; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: [nvmewin] ***UNCHECKED*** NEW patch for review based on some recent HW bring up work

My typo the size of rest of the page should be 0x600 instead of 0xa00.

________________________________
From: nvmewin-bounces at lists.openfabrics.org<mailto:nvmewin-bounces at lists.openfabrics.org> [mailto:nvmewin-bounces at lists.openfabrics.org]<mailto:[mailto:nvmewin-bounces at lists.openfabrics.org]> On Behalf Of Chang, Alex
Sent: Monday, April 16, 2012 5:03 PM
To: Luse, Paul E; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: Re: [nvmewin] ***UNCHECKED*** NEW patch for review based on some recent HW bring up work
Hi Paul,

I believe there is an error in SntiTranslateSglToPrp routine and it can be easily reproduced in non-quick drive format if the driver reports the size of LBA is 4096 bytes. The failing request I've seen is :
1. Reading LBA0 for one LBA.
2. Only one SG element included.
3. The starting physical address is not page-aligned, i.e., 0x1`12345600

In SntiTranslateSglToPrp, numImplicitEntries is assigned as 1 (Line 4238). After entering 2nd for loop (Line 4248), pSrbExt->numberofPrpEntries becomes 1 (Line 4253), which causes pPrp1 is programmed as localPrpEntry (Line 4256) and then jumps out of the for loop without programming pPrp2. However, pPrp2 should be programmed as 0x1`12346000 to transfer the rest of the page (0xa00 bytes).

I am not sure why it works for us for such a long period. May be the different sizes of LBA, and 4096 is simply much easier to replicate the problem.

Thanks,
Alex


________________________________
From: nvmewin-bounces at lists.openfabrics.org<mailto:nvmewin-bounces at lists.openfabrics.org> [mailto:nvmewin-bounces at lists.openfabrics.org]<mailto:[mailto:nvmewin-bounces at lists.openfabrics.org]> On Behalf Of Luse, Paul E
Sent: Monday, April 16, 2012 10:04 AM
To: nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: [nvmewin] ***UNCHECKED*** NEW patch for review based on some recent HW bring up work
There's only a few changes here that need to be reviewed, mostly this is Chatham specific and, again, the only reason we're allowing these HW specific changes in the driver is because its currently the only HW vehicle available to Msft and to UNH who is working on some test suites.  We have an updated version of Chatham which required a few changes but also identified a few issues in the driver that I think are critical enough that I wanted to get them in before we build the release:


-         We weren't handling the CC register correctly in a few places.  Details are below, but this one is critical for new HW.

-         We were exposing LBA ranges that are identified by the controller as hidden

-         We weren't correctly handing the report_luns command for multiple luns

-         We weren't correctly checking status on init state machine completion handlers

As usual, pw is ofanvme123 and the faster folks can check this out the faster we can build our first release J

Thx
Paul


Sources:

-         New switch for chatham2, removed inclusion of chatham switch by default


Nvmestd.h

-         Chatham stuff

-         Moved some NVME register defines to nvmereg.h

-         For DBG mode, changed the timeout state machine to something more reasonable (first it was too short, then too long)

Nvmestd.c

-         Chatham stuff

-         Added readback of CC before we write it again (to toggle EN for initial reset).  Without this some HW will surely fail to init correctly

Nvmestat.c

-         Chatham stuff


Nvmesnti.ch

-         Chatham stuff


Nvmesnti.c

-         Changes to honor the ExposeNamespace field we learn from LBA ranges.  Previously we were not hiding namespaces that were supposed to be hidden

-         Fixes to SntiTranslateReportLuns() for multiple LUNs we were incorrectly failing the call when the allocated buffer was too small, the correct handling is to fill out what we can and return the size we needed in the response and we'll get  second call with the correctly sized buffer.  Tested, of course, and it works as expected

-         Chatham stuff

Nvmereg.h

-         Moved reg defines to here from the other .h file mentioned above

Nveinit.c

-         In NVMeResetAdapter() we were clobbering the CC register, changed so we correctly read it, clear EN, and write it

-         In NVMeEnableAdapter() we were not fully setting up CC, added additional fields and deifnes

-         in NVMeSetFeaturesCompletion(), I mentioned this before I think, I updated LBA range completion handler to assume that an all zero response means that we should expose it, also added a check for what we should be looking for to see if >1 ns is available, the NLB field.  Qemu I think is wrong with its LBA range response right now, once its fixed I'll update this to remove the zero check

-         chatham stuff

-         in NVMeInitCallback() added checks for status code type as well as status code, just the status code is not sufficient in all cases to determine success or not


____________________________________
Paul Luse
Sr. Staff Engineer
PCG Server Software Engineering
Desk: 480.554.3688, Mobile: 480.334.4630

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openfabrics.org/pipermail/nvmewin/attachments/20120418/7cb42f7c/attachment.html>