[nvmewin] HGST Changes to Enable WMI, Driver Trace and various fixes

Tue Oct 7 09:12:26 PDT 2014

Hi Tom,

I have a quick question about the data type of “Id” in class NVMe_DataType defined in nvmeMofData.mof. Is there any specific reason you defined it as “sint32” instead of “uint32”?

Thanks,
Alex

From: Thomas Freeman [mailto:thomas.freeman at hgst.com]
Sent: Monday, October 06, 2014 1:27 PM
To: Alex Chang; Parag Sheth
Cc: nvmewin at lists.openfabrics.org
Subject: RE: [nvmewin] HGST Changes to Enable WMI, Driver Trace and various fixes

Hi Alex…. Now that all the comments are in, I’ll merge in the changes from the review and run a test. I should have the new patch out in the next day.

Tom Freeman

Software Engineer, Device Manager and Driver Development
HGST, a Western Digital company
Thomas.Freeman at hgst.com<mailto:Thomas.Freeman at hgst.com>
507-322-2311

From: Alex Chang [mailto:Alex.Chang at pmcs.com]
Sent: Monday, October 6, 2014 10:51 AM
To: Thomas Freeman; Parag Sheth
Cc: nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: [nvmewin] HGST Changes to Enable WMI, Driver Trace and various fixes

Good morning, Tom,

Will you please send out the revised patch soon? We still have three patches to go before our November release.

Thank you very much!
Alex

From: Thomas Freeman [mailto:thomas.freeman at hgst.com]
Sent: Wednesday, October 01, 2014 12:30 PM
To: Alex Chang; Parag Sheth
Cc: nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: [nvmewin] HGST Changes to Enable WMI, Driver Trace and various fixes

Thanks for the clarification. I’ll change to using CACHING_MOE_PAGE.

Tom Freeman

Software Engineer, Device Manager and Driver Development
HGST, a Western Digital company
Thomas.Freeman at hgst.com<mailto:Thomas.Freeman at hgst.com>
507-322-2311

From: Alex Chang [mailto:Alex.Chang at pmcs.com]
Sent: Wednesday, October 1, 2014 2:17 PM
To: Parag Sheth; Thomas Freeman
Cc: nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: [nvmewin] HGST Changes to Enable WMI, Driver Trace and various fixes

Hi Tom,

If we don’t support VS 2012 anymore, the we certainly can use PMODE_CACHING_PAGE_EX for all windows with VS 2013. Because the storport.h included in VS 2012 doesn’t define PMODE_CACHING_PAGE_EX. The safest bet to fix it, I agree to use CACHING_MODE_PAGE defined in nvmesnti.h.
Sorry Tom, I did not exam it carefully when we discussed the issue.

Thanks,
Alex

From: Parag Sheth [mailto:parag.sheth at seagate.com]
Sent: Wednesday, October 01, 2014 11:30 AM
To: Thomas Freeman
Cc: Alex Chang; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: Re: [nvmewin] HGST Changes to Enable WMI, Driver Trace and various fixes

Hi Tom,

The real solution to this problem is to use CACHING_MODE_PAGE defined in nvmesnti.h and not to use MODE_CACHING_PAGE defined in storport.h. The names are confusing.

Thanks
Parag Sheth

On Wed, Oct 1, 2014 at 8:00 AM, Thomas Freeman <thomas.freeman at hgst.com<mailto:thomas.freeman at hgst.com>> wrote:
Hi Paraq… thanks for the feedback.

I’ll remove the reference to SetWmiDataBlock.

In my notes, I forgot to mention the change I made for mode SntiHardCodeCacheModePage. With the change from the last patch, SCSI compliance was failing on Windows 7. (The failure is shown below).  Using PMODE_CACHING_PAGE_EX for all Windows versions allowed us to successfully complete that test. I conferred with Alex offline about this fix – he indicated that he had intended to include this in Patch#27.
Alex, do you have any comments on this issue?

Policy: REQUIRED
Start: MODE SELECT 6: MODE SENSE (6) Checking Caching Mode Page Length., TUID=
Description: Checking the Caching Mode Page is 20 bytes.
Reference: SCSI Primary Commands - 3 (SPC-3) Revision 23 (or published) specification Section 6.7 http://www.t10.org/ftp/t10/drafts/spc3/spc3r23.pdf
Expectation: cachePageLength == 20 bytes.
MODE: Cache page length is 0xc.

Tom Freeman

Software Engineer, Device Manager and Driver Development
HGST, a Western Digital company
Thomas.Freeman at hgst.com<mailto:Thomas.Freeman at hgst.com>
507-322-2311<tel:507-322-2311>

From: Parag Sheth [mailto:parag.sheth at seagate.com<mailto:parag.sheth at seagate.com>]
Sent: Tuesday, September 30, 2014 6:07 PM
To: Alex Chang
Cc: Thomas Freeman; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: Re: [nvmewin] HGST Changes to Enable WMI, Driver Trace and various fixes

Hi Tom,

1. Your change in function SntiHardCodeCacheModePage (nvmesnti.c) overturns the fix made in last patch.
2. Function SetWmiDataBlock (nvmeWmi.c & .h) is never used. It can be removed.

Other than these 2, the patch looks fine.

Thanks
Parag Sheth

On Tue, Sep 30, 2014 at 8:18 AM, Alex Chang <Alex.Chang at pmcs.com<mailto:Alex.Chang at pmcs.com>> wrote:
Thank you, Parag.
I know we allocate one extra page intentionally. When people don’t know the trick, the original order simply avoids some concern.

Alex

From: Thomas Freeman [mailto:thomas.freeman at hgst.com<mailto:thomas.freeman at hgst.com>]
Sent: Tuesday, September 30, 2014 7:50 AM
To: Parag Sheth; Alex Chang
Cc: nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: [nvmewin] HGST Changes to Enable WMI, Driver Trace and various fixes

Thank you Paraq… I like your explanation.
In addition to your explanation, I’d also say the drivers uses pCplQStart -> (pCplQstart + queueSize -1) for the completion queue.
That is also the address range used in that memset. If those address are wrong, then the CompletionQueue itself is wrong.

Alex…. Yes, I’ll change the RtlZeroMemory to memset. I’m not sure why I didn’t use memset to begin with.

Tom Freeman

Software Engineer, Device Manager and Driver Development
HGST, a Western Digital company
Thomas.Freeman at hgst.com<mailto:Thomas.Freeman at hgst.com>
507-322-2311<tel:507-322-2311>

From: Parag Sheth [mailto:parag.sheth at seagate.com]
Sent: Monday, September 29, 2014 8:32 PM
To: Alex Chang
Cc: Thomas Freeman; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: Re: [nvmewin] HGST Changes to Enable WMI, Driver Trace and various fixes

Hi Alex,

Tom's change related to your point # 2 seems right.

Here is my explanation.
- While allocating memory for submission queue + completion queue + command entries, there is 1 additional memory page allocated.
- Because this allocation is page aligned to begin with, sub queue start is always page aligned.
- making completion queue page aligned might use 1 additionally allocated page.
- command entries need not be page aligned.
- And all these 3 things are initialized in the above order.

So I think clearing memory for completion queue must be done only after page align macro. Let me know if I am missing anything here.

Thanks
Parag Sheth

On Mon, Sep 29, 2014 at 11:18 AM, Alex Chang <Alex.Chang at pmcs.com<mailto:Alex.Chang at pmcs.com>> wrote:
[cid:image001.gif at 01CFE20E.B4F1F8D0]
Hi Tom,

I have the following comments after browsing the changes you’ve made:

1.       In line 184 of nvmestd.c, can RtlZeroMemory be replaced by memset ?

2.       In line 1195 of nvmeinit.c, you moved down memset after page align macro for completion queue start. In case of the original completion queue start wasn’t page aligned, memset will clear the memory it shouldn’t.

Thanks,

Alex

From: nvmewin-bounces at lists.openfabrics.org<mailto:nvmewin-bounces at lists.openfabrics.org> [mailto:nvmewin-bounces at lists.openfabrics.org<mailto:nvmewin-bounces at lists.openfabrics.org>] On Behalf Of Thomas Freeman
Sent: Wednesday, September 24, 2014 11:55 AM
To: nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: [nvmewin] HGST Changes to Enable WMI, Driver Trace and various fixes

Content-Type: text/plain; charset=UTF-8

Content-Transfer-Encoding: 8bit

Date: %%SENT_DATE%%

Subject: Suspect Message Quarantined

WARNING: The virus scanner was unable to scan an attachment in an email message sent to you.  This attachment could possibly contain viruses or other malicious programs.  The attachment could not be scanned for the following reasons:

%%DESC%%

The full message and the attachment have been stored in the quarantine.

The identifier for this message is '%%QID%%'.

Access the quarantine at:

https://puremessage.pmc-sierra.bc.ca:28443/

For more information on PMC's Anti-Spam system:

http://pmc-intranet/wiki/index.php/Outlook:Anti-Spam_FAQ

IT Services

PureMessage Admin

The attached zip file contains source code, project files and instructional documents for WMI and Driver tracing.
The password is “hgst1234”

We tested the changes on w7, w8 and w8.1 – 64-bit
Testing included sdstress, iometer, quick and slow format: MBR and GPT and scsi compliance.

Here is a description of the various changes:

WMI (WindowsWmiDesc.pdf in the attached zip file gives details on using the change)
nvme.vcxproj - Menu for "Mof Compiler"
                -Create Binary Mof File = "../../source/%(FileName).bmf"
                -WMI Syntax Check = Yes (-WMI)
   menu for Linker->Input
                -added $(DDK_LIB_PATH)\scsiwmi.lib
   directly in the file. This directive causes VS to create nvmeMofData.h:
  <ItemGroup>
    <Wmimofck Include="../../source/nvmeMofData.bmf">
      <HeaderOuputFile>"$(IntDir)nvmeMofData.h"</HeaderOuputFile>
      <AdditionalOptions>-u -h"$(IntDir)nvmeMofData.h"</AdditionalOptions>
    </Wmimofck>
  </ItemGroup>

nvme.rc - Added "mofResource" statement
nvmeMofData.bmf - Visual studio generates this file on every compile. Exists in the "source" directory.
nvmeMofData.mof – Used to define WMI obects. Exists in the "source" directory
nvmeMofData.h - Visual studio generates during compile. It is created based on nvmeMofData.mof. It exists in the project's intermediate directory (e.g. nvme/x64/Win7Debug).

nvmeStd.c
                -NVMeFindAdapter - indicate that the driver is a wmiProvider and initialize the wmiContext.
                -NVMeBuildIo - Allow calls to WMI
        -NVMeStartIo - Allow calls to WMI

nvmeStd.h
                -WmiLibContext - Added this structure to the device extension. The WmiLibContext structure provides registration information for a miniport driver's data and event blocks and defines entry points for the miniport driver's HwScsiWmiXxx callback routines.
                -WmiReqContext - Added this structure to the srb extension. When processing a WMI command, this structure is used to save context information for that particular command.

precomp.h
                -added guiddef.h, scsiwmi.h, initguid.h, nvmeMofData.h, nmeWmi.h

WPP - Driver tracing (WindowsTraceCollection.pdf in the attached zip file gives details on using the change)
Relied heavily on the document "Using Storage Tracing Support In Miniport Drivers" - http://download.microsoft.com/download/8/e/8/8e85288f-b718-47d9-a7b2-af19d4c8f031/Storage-tracing-for-miniports.doc

nvme.vcxproj - in the Menu for "Wpp Tracing"
                -Func to Gen Trace = StorStorPortDebugPrint(LEVEL,MSG,...)
                -Run Wpp Tracing = Yes
                -Scan Configuration Data = ..\..\source\nvme_tracing.h
                -Specify Template File = {km-StorDefault.tpl}*.tmh
                -Trace Kernel-Mode Components = Yes (-km)

All c-files: Added include for "nvmeinit.tmh". A Visual Studio generated file to support driver tracing.

nvmeStd.c - create a global variable for storing the trace context information. At cleanup time, the Port Driver calls a cleanup routine that uses this global variable.
                -DriverEntry - Include code to initialize driver tracing through wpp.
                -WppCleanupRoutine - Added code to be called by the port driver to clean up driver tracing.
nvmeStd.h -
                -add function prototype for Wpp Cleanup routine.
        -Changed values for debug print levels to match values in evntrace.h
    INFO = TRACE_LEVEL_INFORMATION,
    WARNING = TRACE_LEVEL_WARNING,
    ERROR = TRACE_LEVEL_ERROR,
    TRACE = TRACE_LEVEL_VERBOSE

precomp.h
                -added nvme_tracing.h

Various Fixes
nvme.vcxproj
                -Changed c/c++ compiler warning level to W3/WX
nvmeSnti.c
   SntiTranslateReadCapacity16 - The caller’s response buffer was being cleared and set up without regard to its size. I put in changes to allocate a read capacity buffer on the stack. Once that temporary buffer was filled in, I used the response buffer size to determine how much data to copy into the caller’s response buffer.
NOTE: It appears this same issue occurs in the handling of other scsi commands (e.g. in SntiTranslateDeviceIdentificationPage, if the response buffer was too short, setting the fields in pIdDescriptor would overrun the response buffer). This may be a fix to be implemented in the future. The issue in SntiTranslateReadCapacity16 issue was seen during testing. The potential issue with SntiTranslateDeviceIdentificationPage has not occurred in our testing.

   SnitTranslateUnmap
                -Changes to ensure the DataSetManagement buffer was always 16-byte aligned.
                -Use prp2 to allow the prplist to cross a page boundary
                -When building the dsm-deallocate command, cdw10->NR was used to count the number of ranges - starting with the value 1. Since NR is a zero-based value, the count was decremented when command building was completed. Unfortunately, when 256 ranges are specified, NR has a value of 0 before decrementing. The value of 0 prevented the code from decrementing NR. A change was made to use a local uint16 variable to cound the number of ranges.
    SntiTranslateWriteBuffer - The existing code set dword10 as a 1-based value. A change was made to correctly set this as a zero-based value.
    SntiTranslateLogSense - A pointer to the data buffer is saved in the srb extension. A change was made to also save the data buffer length in the srb extension. The value is used when the data buffer is freed.

nvmeStd.h
                -Increase size of dsmBuffer to allow for 16-byte alignment.
        -add dataBufferSize to store size of data buffer allocated for log sense command.

Tom Freeman

Software Engineer, Device Manager and Driver Development
HGST, a Western Digital company
Thomas.Freeman at hgst.com<mailto:Thomas.Freeman at hgst.com>
507-322-2311<tel:507-322-2311>

_______________________________________________
nvmewin mailing list
nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
http://lists.openfabrics.org/mailman/listinfo/nvmewin

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openfabrics.org/pipermail/nvmewin/attachments/20141007/678791b7/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: image001.gif
Type: image/gif
Size: 1117 bytes
Desc: image001.gif
URL: <http://lists.openfabrics.org/pipermail/nvmewin/attachments/20141007/678791b7/attachment.gif>