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

Thomas Freeman thomas.freeman at hgst.com
Wed Oct 1 08:31:44 PDT 2014 

Hi Alex…. I’m a little confused on this.

Here is my understanding of how it works:
For this discussion I’ll define 3 values – CQ, SQE and CQS

1.       QS: The size of the queue (pSQI->SubQEntries * sizeof(NVMe_COMPLETION_QUEUE_ENTRY)

2.       SQE: A pointer to the the first byte after the the submission queue(PtrTemp + queueSize)

3.       CQS: a pointer to the Completion Queue (calculated by rounding the SQE to the next page boundary).

The actual Completion queue uses memory CQS through (CQS+QS-1)
But, the memset in the old code clears memory SQE through (SQE+QS+1)

Using this memset does not clear the entire Completion Queue. Memory from (SQE+QS+1) through (CQS+QS+1) is not initialized.
The change I made clears memory CQS through (CQS+QS+1). If this is clearing invalid memory, then the Completion Queue itself it using invalid memory.

What are your thoughts on this?
Thanks,
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: Tuesday, September 30, 2014 10:19 AM
To: Thomas Freeman; Parag Sheth
Cc: nvmewin at lists.openfabrics.org
Subject: RE: [nvmewin] HGST Changes to Enable WMI, Driver Trace and various fixes

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]
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

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 01CFDD60.45761840]
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/20141001/912f545e/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/20141001/912f545e/attachment.gif>

More information about the nvmewin mailing list