timberland-sig/edk2

NvmeOfPassthru protocol is not upstreamable

Opened this issue · 6 comments

mrabeda commented

Link: https://github.com/timberland-sig/edk2/blob/timberland_1.0_final/MdePkg/Include/Protocol/NvmeOFPassthru.h

  1. Protocol structure definition is not compliant with all other protocol definitions in EDK2. Example: https://github.com/timberland-sig/edk2-private/blob/3f693ec4853d89e0ab19700f34b7fe19c11b1d92/MdePkg/Include/Protocol/Http.h
  2. Protocol utilizes definitions from SPDK. Protocol definition file needs to be moved to NetworkPkg/Include/Protocol in order to utilize SPDK include headers. SPDK header include statement has to be added.
  3. Protocol GUID also must move from MdePkg to NetworkPkg DEC file.
  4. Protocol function naming is inconsistent. It will be easier to have protocol structure field names like Read/Write/Disconnect.
  5. Each protocol function prototype must have a pointer to the protocol as first argument ('This').
  6. Structures must be passed to protocol functions via pointers.
  7. Each function prototype must have a comment describing its expected behavior.
Douglas-Farley commented

fyi - @swamy-kadaba / @Ajay-Khadolia / @amit-jain9
This in theory is a blocker for nvmeofcli.efi as an app in the upstream.

mrabeda commented

It is not only for CLI.
It seems we are using Passthru protocol in UefiBootManagerLib as well.
image

mrabeda commented

Considering the above, we need to think through how to solve this.
If UefiBootManagerLib requires protocol GUID, then protocol definition should be in MdePkg.
However, by doing so, using SPDK structures in the protocol will not be possible.
Structures within protocol definition header will have to be introduced to abstract SPDK structures.

amit-jain9 commented

Considering the above, we need to think through how to solve this. If UefiBootManagerLib requires protocol GUID, then protocol definition should be in MdePkg. However, by doing so, using SPDK structures in the protocol will not be possible. Structures within protocol definition header will have to be introduced to abstract SPDK structures.

GUID and protocol declarations (in include/Protocol/NvmeOfPassthru.h) are already part of MdePkg and the SPDK structures are available here. So only change required is to make the passthru protocol compliant to EDK2 standards. We have started working on it.

mrabeda commented

the SPDK structures are available here.

This is simply not true.

If a driver/library INF contains the following statement:

[Packages]
  MdePkg/MdePkg.dsc

that driver/library should be able to utilize any headers from MdePkg. A header file in MdePkg referencing a header file in NetworkPkg will not work, because that driver/library will not have NetworkPkg include paths. It would be required to include NetworkPkg DEC file under [Packages] as well, which from package isolation perspective is undesirable.

amit-jain9 commented

the SPDK structures are available here.

This is simply not true.

If a driver/library INF contains the following statement:

[Packages]
  MdePkg/MdePkg.dsc

that driver/library should be able to utilize any headers from MdePkg. A header file in MdePkg referencing a header file in NetworkPkg will not work, because that driver/library will not have NetworkPkg include paths. It would be required to include NetworkPkg DEC file under [Packages] as well, which from package isolation perspective is undesirable.

Ok. In that case we will try to abstract the SPDK structures from NvmeOfPassthru.h.