Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MdeModulePkg: FMP support for CXL devices #6296

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

abhi-narvaria
Copy link

@abhi-narvaria abhi-narvaria commented Oct 8, 2024

Description

Enables the detection of CXL devices in UEFI environment and to update the Firmware of the detected CXL devices

  • Breaking change?
    • Breaking change - Does this PR cause a break in build or boot behavior?
    • Examples: Does it add a new library class or move a module to a different repo.
  • Impacts security?
    • Security - Does this PR have a direct security impact?
    • Examples: Crypto algorithm change or buffer overflow fix.
  • Includes tests?
    • Tests - Does this PR include any explicit test code?
    • Examples: Unit tests or integration tests.

How This Was Tested

Using OVMF of QEMU

The following commands emulate a CXL type 3 device in QEMU

#!/bin/bash

# Requirements:
#  git clone https://github.com/tianocore/edk2.git  /code/edk2
#  cd /code/edk2; build -t GCC5 -a X64 -p OvmfPkg/OvmfPkgX64.dsc -D FD_SIZE_4MB -D NVME

export WORKSPACE=/code/edk2
export CODE=${WORKSPACE}/Build/OvmfX64/DEBUG_GCC5/FV/OVMF_CODE.fd
export VARS=${WORKSPACE}/Build/OvmfX64/DEBUG_GCC5/FV/OVMF_VARS.fd

qemu-system-x86_64 -drive if=none,id=code,format=raw,file=${CODE},readonly=on \
		   -drive if=none,id=vars,format=raw,file=${VARS},snapshot=on \
		   -machine q35,cxl=on,pflash0=code,pflash1=vars \
                   -object memory-backend-ram,id=m1,size=1G \
		   -object memory-backend-ram,id=m2,size=1G \
                   -object memory-backend-file,id=cxl-lsa0,share=on,mem-path=lsa0.raw,size=1k \
		   -object memory-backend-file,id=cxl-lsa1,share=on,mem-path=lsa1.raw,size=1k \
		   -device pxb-cxl,bus_nr=12,bus=pcie.0,id=cxl.1 \
                   -device cxl-rp,port=0,bus=cxl.1,id=root_port_cxl1,chassis=0,slot=2 \
		   -device cxl-rp,port=1,bus=cxl.1,id=root_port_cxl2,chassis=1,slot=2 \
                   -device cxl-type3,bus=root_port_cxl1,memdev=m1,id=cxl-mem0,lsa=cxl-lsa0 \
		   -device cxl-type3,bus=root_port_cxl2,memdev=m2,id=cxl-mem1,lsa=cxl-lsa1 \
		   -M cxl-fmw.0.targets.0=cxl.1,cxl-fmw.0.size=10G \
	 	   -display none -nographic

CxlFirmwareMgmt.efi is the Application to Send / Receive the FMP commands.
The following UEFI shell commands showcase how to use the FMP commands.

Display Help Menu: CxlFirmwareMgmt.efi
Fw Image Info on BDF value 13, 0, 0: CxlFirmwareMgmt.efi -fimginfo 13 0 0
SetImage on BDF value 13, 0, 0 and slot 2: CxlFirmwareMgmt.efi -fsetimg 13 0 0 2 hello.bin
GetImage on BDF value 13, 0, 0 and slot 2: CxlFirmwareMgmt.efi -fgetimg 13 0 0 2
SetPackageInfo on BDF value 13, 0, 0: CxlFirmwareMgmt.efi -fsetpack 13 0 0
GetPackageInfo on BDF value 13, 0, 0: CxlFirmwareMgmt.efi -fgetpack 13 0 0

Integration Instructions

N/A

@github-actions github-actions bot added the impact:testing This contribution includes tests such as unit and/or integration tests. label Oct 8, 2024
@mdkinney
Copy link
Member

mdkinney commented Oct 8, 2024

Defines and structs from the CXL Specification should do into MdePkg/Include/IndustryStandard.

I see use of libc like functions. Please refer to MdePkg/Include/Library/BaseLib.h and consider using those instead of providing new implementations here.

@abhi-narvaria
Copy link
Author

Defines and structs from the CXL Specification should do into MdePkg/Include/IndustryStandard.

I see use of libc like functions. Please refer to MdePkg/Include/Library/BaseLib.h and consider using those instead of providing new implementations here.

Hi,
I have taken care review comment, Kindly verify this.

@abhi-narvaria
Copy link
Author

Hi All,
Review comments are taken care. kindly review the updated patches. Let me know if something needs to be done from my side.

@abhi-narvaria
Copy link
Author

abhi-narvaria commented Nov 12, 2024

Hi @mdkinney / @LiuZhiguang001 ,
I have taken care the review comments and updated the code. Kindly verify the changes. let me know if something else to be done from my side.

Copy link
Member

@mdkinney mdkinney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of the code does not follow the coding style and there are many function header comment blocks missing and very few comments to explain the logic..

Please see:

MdePkg/Include/IndustryStandard/Cxl20.h Outdated Show resolved Hide resolved
MdePkg/Include/IndustryStandard/Cxl20.h Outdated Show resolved Hide resolved
MdeModulePkg/Bus/Pci/CxlDxe/CxlDxe_util.c Outdated Show resolved Hide resolved
MdeModulePkg/Bus/Pci/CxlDxe/CxlDxe_util.c Outdated Show resolved Hide resolved
MdeModulePkg/Bus/Pci/CxlDxe/CxlDxe_util.c Outdated Show resolved Hide resolved
MdeModulePkg/Bus/Pci/CxlDxe/CxlDxe.h Outdated Show resolved Hide resolved
MdeModulePkg/Bus/Pci/CxlDxe/CxlDxe.h Outdated Show resolved Hide resolved
MdeModulePkg/Bus/Pci/CxlDxe/CxlDxe.h Outdated Show resolved Hide resolved
@abhi-narvaria
Copy link
Author

Hi @mdkinney ,
Thanks for your input, I will work on review comments provided.

@abhi-narvaria
Copy link
Author

Hi @mdkinney @lgao4 ,
Review comments are taken care. Kindly verify the patch.
Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact:testing This contribution includes tests such as unit and/or integration tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants