Re: [edk2] [PATCH 0/3] *** Add Nvme.h and NvmExpressLib ***

[Tian, Feng]

Hi, Darbin

We need more internal discussion on NvmExpressLib. So  I would suggest you work 
on #1 & 2 patch at first. I will get back to you if we have conclusion on this.

As for the second concern, it's user choice. We can have a separate header file 
like Pci sample, we also can have a single header file like Smbios & Ata. My 
personal idea is we don't need introduce a new Nvme12.h and we just need update 
those reserved fields according to latest spec in the "IDENTIFY Controller 
Data" case you listed. Anyway, I agree to comply with NVMe 1.1 at first. And 
when we have new request to support NVMe 1.2, we can add new definitions in.

Thanks
Feng

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Reyes, 
Darbin
Sent: Friday, May 13, 2016 7:37 AM
To: Tian, Feng <feng.t...@intel.com>
Cc: Reyes, Darbin <darbin.emm.re...@hpe.com>; edk2-devel@lists.01.org; Gao, 
Liming <liming@intel.com>; Kinney, Michael D <michael.d.kin...@intel.com>; 
Zeng, Star <star.z...@intel.com>
Subject: Re: [edk2] [PATCH 0/3] *** Add Nvme.h and NvmExpressLib ***

Hi Feng,

1. That crossed my mind. Sure, I can do that.

2. Shouldn't v1.2 definitions go in a separate header e.g. Nvme12.h? For 
instance, I see Pci22.h and Pci33.h under IndustryStandard. There are some 
conflicting definitions between the two versions of the spec. Compare v1.1 
Figure 82 with v1.2 Figure 89. NvmExpressDxe is implemented per NVMe spec. 
v1.1, is it safe to use this implementation of the pass thru protocol with v1.2 
commands/structures?

3. The role of the library is to abstract away the details of executing NVMe 
commands. So for instance, GetSmartHealthLog() returns the "Smart Health Log” 
given only a pointer to an instance of the NVMe pass thru protocol. The client 
is free from having to know the NVMe command opcode, command Dword values, Log 
ID, etc.  I only implemented the functions I needed at the time of writing the 
code. I assumed this could be the initial implementation and others could 
implement additional functions. I’m not sure about MdeModulePkg vs. MdePkg. I 
choose MdeModulePkg because NvmExpressDxe resides there.

Regards,
Darbin
> On May 11, 2016, at 8:50 PM, Tian, Feng <feng.t...@intel.com> wrote:
> 
> Hi, Darbin
> 
> As you know, we take extra caution on interface changes. We expect it's 
> stable enough.
> 
> So I have the below questions:
> 1. I only see the NVME_ADMIN cmds are added, could you add NVME_IO cmds to 
> Nvme.h as well?
> 2. Shall we be compliance with NVMe 1.2 spec to add missing cmds?
> 3. what's the role to introduce such NvmExpressLib library class? For 
> example, why only Get Log/ Identify/ firmware download and activate APIs are 
> added? Do we need add other cmd's wrapper to this new library? Do we need put 
> it in MdeModulePkg or MdePkg?
> 
> Thanks
> Feng
> 
> -Original Message-
> From: darbin.emm.re...@hpe.com [mailto:darbin.emm.re...@hpe.com] 
> Sent: Thursday, May 12, 2016 6:59 AM
> To: edk2-devel@lists.01.org
> Cc: Tian, Feng <feng.t...@intel.com>; Zeng, Star <star.z...@intel.com>; 
> Kinney, Michael D <michael.d.kin...@intel.com>; Gao, Liming 
> <liming@intel.com>; samer.el-haj-mahm...@hpe.com; Darbin Reyes 
> <darbin.emm.re...@hpe.com>
> Subject: [PATCH 0/3] *** Add Nvme.h and NvmExpressLib ***
> 
> From: Darbin Reyes <darbin.emm.re...@hpe.com>
> 
> *** Add Nvme.h and NvmExpressLib ***
> 
> Darbin Reyes (3):
>  MdePkg: Add a header for NVMe v1.1 spec. definitions.
>  MdeModulePkg: Move/Replace NvmExpressHci.h definitions to Nvme.h.
>  MdeModulePkg: Add NvmExpressLib.
> 
> MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.h|   2 +
> MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.h | 389 +
> MdeModulePkg/Include/Library/NvmExpressLib.h   | 279 +
> .../Library/DxeNvmExpressLib/DxeNvmExpressLib.c| 637 +
> .../Library/DxeNvmExpressLib/DxeNvmExpressLib.inf  |  41 ++
> MdeModulePkg/MdeModulePkg.dec  |   4 +
> MdePkg/Include/IndustryStandard/Nvme.h | 603 +++
> 7 files changed, 1568 insertions(+), 387 deletions(-)  create mode 100644 
> MdeModulePkg/Include/Library/NvmExpressLib.h
> create mode 100644 MdeModulePkg/Library/DxeNvmExpressLib/DxeNvmExpressLib.c
> create mode 100644 MdeModulePkg/Library/DxeNvmExpressLib/DxeNvmExpressLib.inf
> create mode 100644 MdePkg/Include/IndustryStandard/Nvme.h
> 
> --
> 1.7.11.msysgit.1
> 

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 0/3] *** Add Nvme.h and NvmExpressLib ***

[Reyes, Darbin]

Hi Feng,

1. That crossed my mind. Sure, I can do that.

2. Shouldn't v1.2 definitions go in a separate header e.g. Nvme12.h? For 
instance, I see Pci22.h and Pci33.h under IndustryStandard. There are some 
conflicting definitions between the two versions of the spec. Compare v1.1 
Figure 82 with v1.2 Figure 89. NvmExpressDxe is implemented per NVMe spec. 
v1.1, is it safe to use this implementation of the pass thru protocol with v1.2 
commands/structures?

3. The role of the library is to abstract away the details of executing NVMe 
commands. So for instance, GetSmartHealthLog() returns the "Smart Health Log” 
given only a pointer to an instance of the NVMe pass thru protocol. The client 
is free from having to know the NVMe command opcode, command Dword values, Log 
ID, etc.  I only implemented the functions I needed at the time of writing the 
code. I assumed this could be the initial implementation and others could 
implement additional functions. I’m not sure about MdeModulePkg vs. MdePkg. I 
choose MdeModulePkg because NvmExpressDxe resides there.

Regards,
Darbin
> On May 11, 2016, at 8:50 PM, Tian, Feng  wrote:
> 
> Hi, Darbin
> 
> As you know, we take extra caution on interface changes. We expect it's 
> stable enough.
> 
> So I have the below questions:
> 1. I only see the NVME_ADMIN cmds are added, could you add NVME_IO cmds to 
> Nvme.h as well?
> 2. Shall we be compliance with NVMe 1.2 spec to add missing cmds?
> 3. what's the role to introduce such NvmExpressLib library class? For 
> example, why only Get Log/ Identify/ firmware download and activate APIs are 
> added? Do we need add other cmd's wrapper to this new library? Do we need put 
> it in MdeModulePkg or MdePkg?
> 
> Thanks
> Feng
> 
> -Original Message-
> From: darbin.emm.re...@hpe.com [mailto:darbin.emm.re...@hpe.com] 
> Sent: Thursday, May 12, 2016 6:59 AM
> To: edk2-devel@lists.01.org
> Cc: Tian, Feng ; Zeng, Star ; 
> Kinney, Michael D ; Gao, Liming 
> ; samer.el-haj-mahm...@hpe.com; Darbin Reyes 
> 
> Subject: [PATCH 0/3] *** Add Nvme.h and NvmExpressLib ***
> 
> From: Darbin Reyes 
> 
> *** Add Nvme.h and NvmExpressLib ***
> 
> Darbin Reyes (3):
>  MdePkg: Add a header for NVMe v1.1 spec. definitions.
>  MdeModulePkg: Move/Replace NvmExpressHci.h definitions to Nvme.h.
>  MdeModulePkg: Add NvmExpressLib.
> 
> MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.h|   2 +
> MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.h | 389 +
> MdeModulePkg/Include/Library/NvmExpressLib.h   | 279 +
> .../Library/DxeNvmExpressLib/DxeNvmExpressLib.c| 637 +
> .../Library/DxeNvmExpressLib/DxeNvmExpressLib.inf  |  41 ++
> MdeModulePkg/MdeModulePkg.dec  |   4 +
> MdePkg/Include/IndustryStandard/Nvme.h | 603 +++
> 7 files changed, 1568 insertions(+), 387 deletions(-)  create mode 100644 
> MdeModulePkg/Include/Library/NvmExpressLib.h
> create mode 100644 MdeModulePkg/Library/DxeNvmExpressLib/DxeNvmExpressLib.c
> create mode 100644 MdeModulePkg/Library/DxeNvmExpressLib/DxeNvmExpressLib.inf
> create mode 100644 MdePkg/Include/IndustryStandard/Nvme.h
> 
> --
> 1.7.11.msysgit.1
> 

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 0/3] *** Add Nvme.h and NvmExpressLib ***

[Tian, Feng]

Hi, Darbin

As you know, we take extra caution on interface changes. We expect it's stable 
enough.

So I have the below questions:
1. I only see the NVME_ADMIN cmds are added, could you add NVME_IO cmds to 
Nvme.h as well?
2. Shall we be compliance with NVMe 1.2 spec to add missing cmds?
3. what's the role to introduce such NvmExpressLib library class? For example, 
why only Get Log/ Identify/ firmware download and activate APIs are added? Do 
we need add other cmd's wrapper to this new library? Do we need put it in 
MdeModulePkg or MdePkg?

Thanks
Feng

-Original Message-
From: darbin.emm.re...@hpe.com [mailto:darbin.emm.re...@hpe.com] 
Sent: Thursday, May 12, 2016 6:59 AM
To: edk2-devel@lists.01.org
Cc: Tian, Feng ; Zeng, Star ; Kinney, 
Michael D ; Gao, Liming ; 
samer.el-haj-mahm...@hpe.com; Darbin Reyes 
Subject: [PATCH 0/3] *** Add Nvme.h and NvmExpressLib ***

From: Darbin Reyes 

*** Add Nvme.h and NvmExpressLib ***

Darbin Reyes (3):
  MdePkg: Add a header for NVMe v1.1 spec. definitions.
  MdeModulePkg: Move/Replace NvmExpressHci.h definitions to Nvme.h.
  MdeModulePkg: Add NvmExpressLib.

 MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.h|   2 +
 MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.h | 389 +
 MdeModulePkg/Include/Library/NvmExpressLib.h   | 279 +
 .../Library/DxeNvmExpressLib/DxeNvmExpressLib.c| 637 +
 .../Library/DxeNvmExpressLib/DxeNvmExpressLib.inf  |  41 ++
 MdeModulePkg/MdeModulePkg.dec  |   4 +
 MdePkg/Include/IndustryStandard/Nvme.h | 603 +++
 7 files changed, 1568 insertions(+), 387 deletions(-)  create mode 100644 
MdeModulePkg/Include/Library/NvmExpressLib.h
 create mode 100644 MdeModulePkg/Library/DxeNvmExpressLib/DxeNvmExpressLib.c
 create mode 100644 MdeModulePkg/Library/DxeNvmExpressLib/DxeNvmExpressLib.inf
 create mode 100644 MdePkg/Include/IndustryStandard/Nvme.h

--
1.7.11.msysgit.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 0/3] *** Add Nvme.h and NvmExpressLib ***

[El-Haj-Mahmoud, Samer]

Series Reviewed-By : Samer El-Haj-Mahmoud 



-Original Message-
From: Reyes, Darbin [darbin.emm.re...@hpe.com]
Received: Wednesday, 11 May 2016, 6:07PM
To: edk2-devel@lists.01.org [edk2-devel@lists.01.org]
CC: feng.t...@intel.com [feng.t...@intel.com]; star.z...@intel.com 
[star.z...@intel.com]; michael.d.kin...@intel.com [michael.d.kin...@intel.com]; 
liming@intel.com [liming@intel.com]; El-Haj-Mahmoud, Samer 
[samer.el-haj-mahm...@hpe.com]; Reyes, Darbin [darbin.emm.re...@hpe.com]
Subject: [PATCH 0/3] *** Add Nvme.h and NvmExpressLib ***

From: Darbin Reyes 

*** Add Nvme.h and NvmExpressLib ***

Darbin Reyes (3):
  MdePkg: Add a header for NVMe v1.1 spec. definitions.
  MdeModulePkg: Move/Replace NvmExpressHci.h definitions to Nvme.h.
  MdeModulePkg: Add NvmExpressLib.

 MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.h|   2 +
 MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.h | 389 +
 MdeModulePkg/Include/Library/NvmExpressLib.h   | 279 +
 .../Library/DxeNvmExpressLib/DxeNvmExpressLib.c| 637 +
 .../Library/DxeNvmExpressLib/DxeNvmExpressLib.inf  |  41 ++
 MdeModulePkg/MdeModulePkg.dec  |   4 +
 MdePkg/Include/IndustryStandard/Nvme.h | 603 +++
 7 files changed, 1568 insertions(+), 387 deletions(-)
 create mode 100644 MdeModulePkg/Include/Library/NvmExpressLib.h
 create mode 100644 MdeModulePkg/Library/DxeNvmExpressLib/DxeNvmExpressLib.c
 create mode 100644 MdeModulePkg/Library/DxeNvmExpressLib/DxeNvmExpressLib.inf
 create mode 100644 MdePkg/Include/IndustryStandard/Nvme.h

--
1.7.11.msysgit.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel