[nvmewin] Intel Byte Enable Patch

Wed Jul 3 09:57:25 PDT 2013

I fell behind a bit in looking at these patches so I apologize for making this comment late.  _WIN32_WINNT is a user-mode #define.  If you want to switch on the version of the WDK environment you're using to build drivers, the check should against the NTDDI_VERSION.  For example:

#if (NTDDI_VERSION >= NTDDI_WIN8)

NTDDI_WIN* are defined in <path to kits>\include\shared\sdkddkver.h
#define NTDDI_WIN6                          0x06000000
#define NTDDI_VISTA                         NTDDI_WIN6
#define NTDDI_WIN7                          0x06010000
#define NTDDI_WIN8                          0x06020000

From: nvmewin-bounces at lists.openfabrics.org [mailto:nvmewin-bounces at lists.openfabrics.org] On Behalf Of Chang, Alex
Sent: Tuesday, July 02, 2013 4:05 PM
To: Murray, Kris R; nvmewin at lists.openfabrics.org
Subject: Re: [nvmewin] Intel Byte Enable Patch

Hi Kris,

When I started to build the patch via WDK 7600, it gave me error and complained:
#if (_WIN32_WINNT > _WIN32_WINNT_WIN7) AND defined(_WIN64)
Can you change it to:
#if (_WIN32_WINNT > _WIN32_WINNT_WIN7) && defined(_WIN64)

Thanks,
Alex

________________________________
From: Murray, Kris R [mailto:kris.r.murray at intel.com]
Sent: Friday, June 28, 2013 11:04 AM
To: nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Cc: Chang, Alex
Subject: RE: Intel Byte Enable Patch
See attached zip file with password: intel1234

In summary, the patch change is whenever the Capabilities register is referenced that is replaced with reading the entire 64-bit register. This is to avoid any Byte Enabled traffic that may be generated across the PCIe bus. The registers are read using 'StorPortReadRegisterUlong()' except on Windows 8 builds with a 64-bit platform.

Thanks,
~Kris

From: Chang, Alex [mailto:Alex.Chang at idt.com]
Sent: Thursday, June 27, 2013 7:14 PM
To: Murray, Kris R; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: Intel Byte Enable Patch

Hi Kris,

The patch from SanDisk had been pushed. You may go ahead re-base and re-send out your patch for review when you're ready.

Thanks,
Alex
________________________________
From: Chang, Alex
Sent: Thursday, June 27, 2013 5:11 PM
To: Murray, Kris R; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: Intel Byte Enable Patch
You may refer to the wrapping for TRIM command support codes. We will release two binary packages. One built with WDK 7600 for Windows 7, Windows Server 2008 R2 and Windows Server 2012. The other one built with WDK 8/VS2012 for Windows 8, where the API is compiled in.

Alex

________________________________
From: Murray, Kris R [mailto:kris.r.murray at intel.com]
Sent: Thursday, June 27, 2013 4:59 PM
To: Chang, Alex; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: Intel Byte Enable Patch
We're using WDK 8.0 and compiling integrated with Visual Studio 2012. I did try my old WDK 7600 compiler and it gave the error for 'storPortReadRegisterUlong64' as undefined.

How about wrapping it with (NTDDI_VERSION >= NTDDI_WIN8)?

~Kris

From: Chang, Alex [mailto:Alex.Chang at idt.com]
Sent: Thursday, June 27, 2013 4:45 PM
To: Murray, Kris R; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: Intel Byte Enable Patch

Per this link, it says so:
http://msdn.microsoft.com/en-us/library/windows/hardware/hh967741(v=vs.85).aspx

I checked the storport.h coming with WDK 7600, it does not define StorPortReadRegisterUlong64. Which WDK version you compile with?

Thanks,
Alex

________________________________
From: Murray, Kris R [mailto:kris.r.murray at intel.com]
Sent: Thursday, June 27, 2013 4:36 PM
To: Chang, Alex; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: Intel Byte Enable Patch
Are you sure? When we looked at the definition in storport.h (and wdm.h) it doesn't appear to be wrapped in any OS version compile switches. Also, I tested it on Windows 7 in QEMU and it ran without error. I verified in WinDbg that correct values were read using this function.

From: Chang, Alex [mailto:Alex.Chang at idt.com]
Sent: Thursday, June 27, 2013 1:44 PM
To: Murray, Kris R; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: Intel Byte Enable Patch

Sounds good to me. However, the API is only available on Windows 8. You might want add:
#if _WIN32_WINNT > _WIN32_WINNT_WIN7

Thanks,
Alex

________________________________
From: Murray, Kris R [mailto:kris.r.murray at intel.com]
Sent: Thursday, June 27, 2013 1:32 PM
To: Chang, Alex; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: Intel Byte Enable Patch
Correct, Ray was going to use an array for the doorbell registers but it turns out that didn't need to happen. I saw that after I submitted, but decided to wait till the rebase to remove it from the patch.

Another change I plan to make is adding a compile option for _WIN64 that will use the StorPortReadUlong64(...) function instead of 2x 32-bit reads.

Thanks,
~Kris

From: Chang, Alex [mailto:Alex.Chang at idt.com]
Sent: Thursday, June 27, 2013 1:25 PM
To: Murray, Kris R; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: Intel Byte Enable Patch

Hi Kris,

After reviewing your patch, I notice that, variable "IODB" (Line# 900 and 1013 in nvmeinit.c) is declared/initialized, but never gets used. I think you meant to read back the initial value of doorbell pointer with it?

Thanks,
Alex

________________________________
From: nvmewin-bounces at lists.openfabrics.org<mailto:nvmewin-bounces at lists.openfabrics.org> [mailto:nvmewin-bounces at lists.openfabrics.org] On Behalf Of Murray, Kris R
Sent: Wednesday, June 12, 2013 4:45 PM
To: nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: [nvmewin] ***UNCHECKED*** Intel Byte Enable Patch
All,

Attached is the patch to fix issues where accessing memory mapped controller register fields directly would generate single byte accesses across the PCIe bus by calling the StorPort functions to read those registers. The 4 places this is done are NVMeFindAdapter, NVMeInitCplQueue, NVMeSubQueue, and NVMeInitCallback.

Password: intel1234

Testing done using IOMeter and SCSI Compliance with logs attached.

Please review and provide feedback in the next couple weeks. Upon acceptance I'll rebase after the other patches make it through.

Thanks,
Kris

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