Thanks, Devkate.
To follow EDKII open forum review process, I add the patch below to see if
others have comments.
Subject: [PATCH] MdeModulePkg/NvmExpressDxe: Correct Prp list creation
algorithm.
Fix the algo of Prp list creation to calculate the number of Prp lists and the
number of entries in the last Prp List correctly.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Feng Tian <feng.t...@intel.com>
Reviewed-by: Baban Devkate <baban.devk...@seagate.com>
---
MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
index 4687858..1b8751d 100644
--- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
+++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
@@ -3,7 +3,7 @@
NVM Express specification.
(C) Copyright 2014 Hewlett-Packard Development Company, L.P.<BR>
- Copyright (c) 2013, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2013 - 2015, Intel Corporation. All rights reserved.<BR>
This program and the accompanying materials
are licensed and made available under the terms and conditions of the BSD
License
which accompanies this distribution. The full text of the license may be
found at
@@ -256,9 +256,15 @@ NvmeCreatePrpList (
//
// Calculate total PrpList number.
//
- *PrpListNo = (UINTN)DivU64x64Remainder ((UINT64)Pages, (UINT64)PrpEntryNo,
&Remainder);
- if (Remainder != 0) {
+ *PrpListNo = (UINTN)DivU64x64Remainder ((UINT64)Pages, (UINT64)PrpEntryNo -
1, &Remainder);
+ if (*PrpListNo == 0) {
+ *PrpListNo = 1;
+ } else if (Remainder != 0) && (Remainder != 1) {
*PrpListNo += 1;
+ } else if (Remainder == 1) {
+ Remainder = PrpEntryNo;
+ } else if (Remainder == 0) {
+ Remainder = PrpEntryNo - 1;
}
Status = PciIo->AllocateBuffer (
@@ -314,7 +320,7 @@ NvmeCreatePrpList (
// Fill last PRP list.
//
PrpListBase = *(UINT64*)PrpListHost + PrpListIndex * EFI_PAGE_SIZE;
- for (PrpEntryIndex = 0; PrpEntryIndex < ((Remainder != 0) ? Remainder :
PrpEntryNo); ++PrpEntryIndex) {
+ for (PrpEntryIndex = 0; PrpEntryIndex < Remainder; ++PrpEntryIndex) {
*((UINT64*)(UINTN)PrpListBase + PrpEntryIndex) = PhysicalAddr;
PhysicalAddr += EFI_PAGE_SIZE;
}
From: Baban Devkate [mailto:baban.devk...@seagate.com]
Sent: Friday, April 24, 2015 14:14
To: Tian, Feng
Cc: edk2-devel@lists.sourceforge.net; baban...@gmail.com
Subject: Re: MdeModulePkg:Fix required in NvmExpressDxe driver
Hi Feng,
This modified code does calculate PrpListNo and Remainder correctly for both
aligned and unaligned cases.
Thanks & regards,
Baban Devkate
On Thu, Apr 23, 2015 at 1:05 PM, Tian, Feng
<feng.t...@intel.com<mailto:feng.t...@intel.com>> wrote:
Hi, Devkate
So the key point should be the *PrpListNo and Remainder doesn’t get calculated
correctly (assume PrpListNo means the number of Prp Lists and Remainder means
the number of entries in last Prp List). Do you agree?
So the correct algo should:
1) If Pages >= 1 and Pages <=512, the algo should return PrpListNo = 1 and
Remainder = Pages.
2) If Pages >= 511*1 + 2 and Pages <= 511*1 + 512, the algo should return
PrpListNo = 2 and Remainder = 2 to 512.
3) …
4) If Pages >= 511*n + 2 and Pages <= 511*n + 512, the algo should return
PrpListNo = n and Remainder = 2 to 512.
I enhanced the code like this, could you help review the attachment?
Thanks
Feng
From: Baban Devkate
[mailto:baban.devk...@seagate.com<mailto:baban.devk...@seagate.com>]
Sent: Wednesday, April 22, 2015 14:22
To: Tian, Feng
Subject: Re: MdeModulePkg:Fix required in NvmExpressDxe driver
Hi Feng,
Actually case 3 was incorrect scenario:(
My concern was with below case....
5) Offset = 0KB, Transfer Length = 4K * 514 blocks = 2MB + 8K
Prp1 = 0K to 4KB
Prp2 = pointer to a Prp List which is followed by another Prp List.
The first Prp List contains 511 entries to read data from offset 4KB to offset
2MB. the last one entry is a pointer to 2nd Prp List. The second Prp List
should contain 2 entry to read data from offset 2MB to offset 2MB +8k.
So for the fifth sample, EFI_SIZE_TO_PAGES(Offset + Bytes) – 1 = 513(i.e
Pages=513). In NvmeCreatePrpList(), PrpListNo = 2 for third sample.
*PrpListNo =2 with Remainder=1 when Pages=513 and PrpEntryNo=512
and this means only first entry of 2nd Prp Lists will get populated and 2nd
entry of second Prp List will not get populated.
So the 2MB + 8K is requested data but 2MB+4KB is the actual payload.
On Wed, Apr 22, 2015 at 10:29 AM, Tian, Feng
<feng.t...@intel.com<mailto:feng.t...@intel.com>> wrote:
Hi, Devkate
I have got feedback from a member of NVMe org and he has confirmed that PRP
Lists shall be minimally sized with packed entries starting with entry 0.
To make us on the same page, I draw two figures to clarify our understanding. I
think you agree the figure 1 is correct and figure 2 is incorrect for a 2M data
buffer.
So for the case 3 you list, why you think it has problem?
3) Offset = 0KB, Transfer Length = 4K * 513 blocks = 2MB + 4K
Prp1 = 0K to 4KB
Prp2 = pointer to a Prp List which contains 512 entries to read data from
offset 4KB to offset 2MB + 4K.
EFI_SIZE_TO_PAGES(Offset + Bytes) – 1 = 512(i.e Pages=512). In
NvmeCreatePrpList(), *PrpListNo =1 with Remainder=0 when Pages=512 and
PrpEntryNo=512.
Thanks
Feng
From: Tian, Feng
Sent: Wednesday, April 15, 2015 09:00
To: Baban Devkate
Cc: baban...@gmail.com<mailto:baban...@gmail.com>;
edk2-devel@lists.sourceforge.net<mailto:edk2-devel@lists.sourceforge.net>;
Tian, Feng
Subject: RE: MdeModulePkg:Fix required in NvmExpressDxe driver
Hi, Devkate
If the last entry in the first Prp list points to the last 4K data range rather
than a next Prp list, how does h/w distinguish the last entry is a pointer to
next Prp list or a valid entry to buffer?
I am consulting NVMe org and if I have some feedbacks I will get back to you.
Thanks
Feng
From: Baban Devkate [mailto:baban.devk...@seagate.com]
Sent: Tuesday, April 14, 2015 13:20
To: Tian, Feng
Cc: baban...@gmail.com<mailto:baban...@gmail.com>;
edk2-devel@lists.sourceforge.net<mailto:edk2-devel@lists.sourceforge.net>
Subject: Re: MdeModulePkg:Fix required in NvmExpressDxe driver
Feng, you were correct that we don't need the 2nd Prp list for such case( case
3) and the last entry in the first Prp list points to last 4K range rather than
a pointer to next Prp List). Which is inline with NVMe spec.
Excerpt from NVM Express 1.1, section 4.3:
The first PRP entry contained within the command may have a non-zero offset
within the memory page. The first PRP List entry (i.e. the first pointer to a
memory page containing additional PRP entries) that if present is contained in
the PRP Entry 2 location within the command, shall be Qword aligned and may
also have a non-zero offset within the memory page. All other PRP and PRP List
entries shall have a memory page offset of 0h, i.e. the entries are memory page
aligned based on the value in CC.MPS. The last entry within a memory page, as
indicated by the memory page size in the CC.MPS field, shall be a PRP List
pointer if there is more than a single memory page of data to be transferred.
PRP Lists shall be minimally sized with packed entries starting with entry 0.
If more PRP List pages are required, then the last entry of the PRP List page
is a pointer to the next PRP List page. The total number of PRP entries is
implied by the command parameters and memory page size.
So with the latest patch, case 4 will generate error.
4) Offset = 0KB, Transfer Length = 4K * 1024 blocks = 4MB
Prp1 = 0K to 4KB
Prp2 = pointer to a Prp List which is followed by another Prp List.
The first Prp List contains 511 entries to read data from offset 4KB to offset
2MB and last one entry is a pointer to 2nd Prp List.
Second Prp List contains next 511 entries to read data from offset 2MB to
offset 4MB-4K and last one entry is a pointer to 3rd Prp List.
But as per NVMe spec, pointer to 3rd Prp List will read data from offset
4MB-4KB to 4MB since there is no more than single memory page data to be
transferred.
So I suggest we should go with below fix which will cater both aligned and
unaligned accesses.
Prp = NvmeCreatePrpList (PciIo, PhyAddr, ((Offset!= 0) ?
EFI_SIZE_TO_PAGES(Offset + Bytes) – 1: EFI_SIZE_TO_PAGES(Offset + Bytes) ),
&PrpListHost, &PrpListNo, &MapPrpList);
On Fri, Apr 10, 2015 at 11:20 AM, Tian, Feng
<feng.t...@intel.com<mailto:feng.t...@intel.com>> wrote:
Got your point now.
My original thought for the 3rd case is we only need a Prp list which contains
512 entries to describe the whole 2M data buffer (I thought we don’t need the
2nd Prp list for such case and the last entry in the first Prp list points to
last 4K range rather than a pointer to next Prp List).
Looks like my original thought is wrong as the h/w will have no way to
distinguish the last entry is a pointer to next Prp list or a valid entry to
buffer.
I made a little adjustment on your proposal. Please help review the attachment.
I think this way may be better to correct callee’s algorithm to create those
right Prp lists, do you agree?
Thanks
Feng
From: Baban Devkate
[mailto:baban.devk...@seagate.com<mailto:baban.devk...@seagate.com>]
Sent: Thursday, April 9, 2015 5:56 PM
To: Tian, Feng; baban...@gmail.com<mailto:baban...@gmail.com>
Cc: edk2-devel@lists.sourceforge.net<mailto:edk2-devel@lists.sourceforge.net>
Subject: Re: MdeModulePkg:Fix required in NvmExpressDxe driver
+ adding self
On Thu, Apr 9, 2015 at 3:19 PM, Baban Devkate
<baban.devk...@seagate.com<mailto:baban.devk...@seagate.com>> wrote:
Let's take one more sample......
3) Offset = 0KB, Transfer Length = 4K * 513 blocks = 2MB + 4K
Prp1 = 0K to 4KB
Prp2 = pointer to a Prp List which is followed by another Prp List.
The first Prp List contains 511 entries to read data from offset 4KB to offset
2MB. the last one entry is a pointer to 2nd Prp List. The second Prp List
should contain 1 entry to read data from offset 2MB to offset 2MB +4k.
So for the third sample, EFI_SIZE_TO_PAGES(Offset + Bytes) – 1 = 512(i.e
Pages=512). In NvmeCreatePrpList(), PrpListNo = 1 for third sample.
*PrpListNo =1 with Remainder=0 when Pages=512 and PrpEntryNo=512
and this means only first Prp Lists entries will get populated and second Prp
List will not come into picture at all.
So the 2MB + 4K is requested data but 2MB is the actual payload.
We should have two cases each for Offset =0 and !=0.
So the fix should be like this:
Prp = NvmeCreatePrpList (PciIo, PhyAddr, ((Offset!= 0) ?
EFI_SIZE_TO_PAGES(Offset + Bytes) – 1: EFI_SIZE_TO_PAGES(Offset + Bytes) ),
&PrpListHost, &PrpListNo, &MapPrpList);
On Thu, Apr 9, 2015 at 1:26 PM, Tian, Feng
<feng.t...@intel.com<mailto:feng.t...@intel.com>> wrote:
Hi, Devkate
I agree the first one is a bug and need to be fixed like your proposal.
For the second one, sorry I couldn’t catch your point.
When the requested buffer size is larger than 2M, it means we need multiple PRP
list pages.
For example, when block size is 4K, memory page size is 4K
1) Offset = 0B, Transfer Length = 4K * 512 blocks = 2MB
Prp1 = 0 to 4KB
Prp2 = pointer to a Prp List which contains 511 entries to read data from
offset 4KB to offset 2MB.
2) Offset = 2KB, Transfer Length = 4K * 513 blocks = 2MB + 4K
Prp1 = 2K to 4KB
Prp2 = pointer to a Prp List which is followed by another Prp List.
The first Prp List contains 511 entries to read data from offset 4KB to offset
2MB. the last one entry is a pointer to 2nd Prp List. The second Prp List
contains 2 entries to read data from offset 2MB to offset 2MB +4k and from
offset 2MB + 4K to offset 2MB + 6K.
So for the first sample, EFI_SIZE_TO_PAGES(Offset + Bytes) – 1 = 511. For the
second sample, EFI_SIZE_TO_PAGES(Offset + Bytes) – 1 = 513. In
NvmeCreatePrpList(), PrpListNo = 1 for first sample and PrpListNo = 2 for
second sample.
So do I misunderstand something?
Thanks
Feng
From: Baban Devkate
[mailto:baban.devk...@seagate.com<mailto:baban.devk...@seagate.com>]
Sent: Thursday, April 09, 2015 09:37
To: edk2-devel@lists.sourceforge.net<mailto:edk2-devel@lists.sourceforge.net>
Cc: Tian, Feng
Subject: MdeModulePkg:Fix required in NvmExpressDxe driver
Hi,
Commit Message
============
MdeModulePkg: Fixed Pointer truncation and incorrect bytes passed for PRP list
creation.
There were two issues in NvmExpressPassthru.c(
MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c)-
1.PRP list entries were zeroed out as "PrpListBase" was getting truncated to
UINT8, changed to UINT64.
Old code- PrpListBase = *(UINT8*)PrpListHost + PrpListIndex * EFI_PAGE_SIZE;
New code- PrpListBase = *(UINT64*)PrpListHost + PrpListIndex * EFI_PAGE_SIZE;
2. Number of bytes passed to PRP list creation function corrected.
Actual payload length will not match requested whenever nested PRP list is
required. With current 1024 max transfer size and 4K Blocksize if the
requested buffer is greater than 2MB then nested PRP list will be required and
NvmeCreatePrpList (.., Pages-1 , ..) will result into above failure.
Old code-
Prp = NvmeCreatePrpList (PciIo, PhyAddr, EFI_SIZE_TO_PAGES(Offset + Bytes) - 1,
&PrpListHost, &PrpListNo, &MapPrpList);
New code-
Prp = NvmeCreatePrpList (PciIo, PhyAddr, EFI_SIZE_TO_PAGES(Offset + Bytes),
&PrpListHost, &PrpListNo, &MapPrpList);
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: baban.devk...@seagate.com<mailto:baban.devk...@seagate.com>
Name-Baban Devkate
email-baban.devk...@seagate.com<mailto:email-baban.devk...@seagate.com>
------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
