Push 2025 05 05

[rdementi]

No description provided.

[Copilot]

Pull Request Overview

This PR cleans up outdated or redundant code for Windows‐based PCI discovery and extends CPU topology support by incorporating L3 cache identification.

• Removed unused Windows-specific pcm-lspci files.
• Extended topology structures and core mask initialization to include L3 cache.
• Refactored logging in utils and PMU modules and updated CSV row-building logic.

Reviewed Changes

Copilot reviewed 9 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/windows/pcm-lspci-win.cpp	Removed obsolete Windows entry point implementation.
src/utils.cpp	Added a conditional branch that logs a message when cpuBusValid is false, ensuring continued execution.
src/topologyentry.h	Introduced a new l3_cache_id field and updated initCoreMasks to accept a new parameter for L3 cache.
src/pcm-lspci.cpp	Removed the entire PCI tree file as it is no longer needed.
src/pcm-iio-pmu.cpp	Replaced a backspace trimming mechanism with std::string::erase to remove trailing characters.
src/lspci.h & src/lspci.cpp	Removed deprecated iio_skx structure and print_pci function to reflect updated PCI handling design.
src/cpucounters.cpp	Enhanced CPU topology discovery by capturing L3 cache details and updated related debug logging.

Files not reviewed (2)

• pcm.spec: Language not supported
• src/CMakeLists.txt: Language not supported

Comments suppressed due to low confidence (1)

src/utils.cpp:1345

• [nitpick] The message 'CPUBUSNO_VALID is 0' is logged as an error even though the function returns true to allow further processing. It might be clearer to use a debug or warning log level, or adjust the message to better reflect that this is a non-fatal condition.

if (!cpuBusValid)

[Copilot]

Pull Request Overview

This PR, titled "Push 2025 05 05", streamlines code by removing obsolete files/functions and extending topology detection to include L3 cache information while refining string‐manipulation and error handling in various utility modules.

• Removed obsolete Windows-specific lspci file and the print_pci function.
• Augmented topology structures and functions to include L3 cache mask processing and added corresponding logging.
• Updated string manipulation in pcm-iio-pmu.cpp and enhanced error handling in PCI configuration PMU discovery.

Reviewed Changes

Copilot reviewed 9 out of 11 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/windows/pcm-lspci-win.cpp	Entire file removal to deprecate Windows-specific lspci entry point.
src/utils.cpp	Added a check for cpuBusValid with appropriate logging and early return.
src/topologyentry.h	New field l3_cache_id added and initCoreMasks updated to accept an L3 mask shift parameter.
src/lspci.cpp	Removed obsolete print_pci definition.
src/pcm-iio-pmu.cpp	Replaced backspace-based string trimming with an erase call for clarity.
src/cpucounters.cpp	Updated initCoreMasks calls to include l3CacheMaskShift; added L3 cache validations and logging.
src/lspci.h	Removed the obsolete iio_skx structure and declaration of print_pci.

Files not reviewed (2)

• pcm.spec: Language not supported
• src/CMakeLists.txt: Language not supported

Comments suppressed due to low confidence (3)

src/topologyentry.h:29

• The addition of 'l3_cache_id' improves topology representation. Ensure that its default initializer (-1) is consistent with the rest of the topology handling.

int32 l3_cache_id = -1;

src/topologyentry.h:1220

• Since l3CacheMaskShift is of an unsigned type, the check 'l3CacheMaskShift < 0' is redundant. Consider removing it to simplify the condition.

if (l3CacheMaskShift < 0 || l3CacheMaskShift > 31)

src/lspci.cpp:94

• The 'print_pci' function has been removed; please verify that there are no remaining calls to this function elsewhere in the code.

void print_pci(struct pci p, const PCIDB & pciDB)

[Copilot]

The negative check for l3CacheMaskShift is unnecessary for unsigned values, and there is a duplicate check for values greater than 31. Consolidate these validations.

Suggested change

	if (l3CacheMaskShift < 0 || l3CacheMaskShift > 31)
	{
	throw std::runtime_error("Invalid l3CacheMaskShift: must be between 0 and 31");
	}
	if (l3CacheMaskShift > 31)
	{
	throw std::runtime_error("Invalid bit range for L3 cache ID extraction");
	if (l3CacheMaskShift > 31)
	{
	throw std::runtime_error("Invalid l3CacheMaskShift: must be between 0 and 31 inclusive");

[Copilot]

Ensure that the string 's' contains at least 2 characters before calling s.erase(s.size() - 2) to avoid potential out-of-range access.

Suggested change

	s.erase(s.size() - 2);
	if (s.size() >= 2) {
	s.erase(s.size() - 2);
	}

[Copilot]

[nitpick] The check for the PCI_ENABLE bit not being set now logs an error and returns an empty shared pointer. Consider whether failing fast or using a more robust error handling strategy is more appropriate.

Suggested change

	std::cerr << "PCM Error: PCI_ENABLE bit not set in address 0x" << std::hex << Addr.raw << std::dec << "\n";
	std::cerr << "This is likely a bug in the uncore PMU discovery BIOS table. Contact your BIOS vendor.\n";
	std::cerr << "Socket: " << s << "\n";
	std::cerr << "Box type: " << BoxType << "\n";
	std::cerr << "Box position: " << pos << "/" << numBoxes << "\n";
	std::cerr << "Address: " << Addr.getStr() << "\n";
	return std::shared_ptr<PCICFGRegister64>();
	std::ostringstream errorMsg;
	errorMsg << "PCM Error: PCI_ENABLE bit not set in address 0x" << std::hex << Addr.raw << std::dec << "\n"
	<< "This is likely a bug in the uncore PMU discovery BIOS table. Contact your BIOS vendor.\n"
	<< "Socket: " << s << "\n"
	<< "Box type: " << BoxType << "\n"
	<< "Box position: " << pos << "/" << numBoxes << "\n"
	<< "Address: " << Addr.getStr() << "\n";
	throw std::runtime_error(errorMsg.str());

[Copilot]

Pull Request Overview

This PR removes unused Windows‑specific LSPCI code and updates topology extraction logic to include L3 cache support while also refining related logging and string formatting behaviors. Key changes include:

• Removal of pcm‑lspci‑win.cpp and pcm‑lspci.cpp files.
• Changes in topology and core mask handling (adding an l3_cache_id field and updating initCoreMasks calls).
• Adjustments in utils.cpp and pcm‑iio‑pmu.cpp for improved logging and string manipulation.

Reviewed Changes

Copilot reviewed 11 out of 13 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/windows/pcm-lspci-win.cpp	Removed Windows-specific LSPCI implementation.
src/utils.cpp	Updated CPU bus validation block with an added log message and early return condition.
src/types.h	Added necessary includes for standard exceptions and string support.
src/topologyentry.h	Introduced l3_cache_id field and updated initCoreMasks signature for L3 cache extraction.
src/pcm-lspci.cpp	Entire file removed to eliminate redundant code.
src/pcm-iio-pmu.cpp	Updated string trailing comma removal using erase instead of backspaces.
src/lspci.h and src/lspci.cpp	Removed legacy functions and structures no longer in use.
src/cpucounters.cpp	Updated topology discovery to incorporate L3 cache mask computations.
src/MacMSRDriver/PcmMsr/PcmMsr.cpp	Modified topology initialization to support L3 cache extraction.

Files not reviewed (2)

• pcm.spec: Language not supported
• src/CMakeLists.txt: Language not supported

[Copilot]

The log message 'CPUBUSNO_VALID is 0' printed in this branch could be misleading since the function returns true in this case. Consider clarifying the message to indicate that this is an expected condition under specific configurations.

[Copilot]

Ensure that the string has at least two characters before erasing the last two characters to avoid potential underflow. A guard or assert could prevent unexpected behavior if the string is shorter than expected.

Suggested change

	s.erase(s.size() - 2);
	if (s.size() >= 2) {
	s.erase(s.size() - 2);
	}