Add CPU Core Count patch to ProvideCurrentCpuInfo #277
Conversation
Set minimum version to High Sierra. Fixed formatting.
|
I understand that you simply replicated our patch structure with your patch, but this is unreviewable for me. Our patch definitions are sparse on information as well, but it's a different thing whether the maintainer or an external contributor adds such code. We understand our own patches because we wrote them, but if you want to merge yours, it needs to be maintained. This means either you'd need to step up and maintain the patch yourself at our pace (this would need team discussion first, this is just a floating variant), or you need to provide enough information that we can do so ourselves easily. For example, I don't want to throw the kernel into IDA to figure out what this even does. By that I don't mean that it will be fine if you provide more information, merely I need the information to think about it. I would guess this is simply a hack to substitute dynamic core count retrieval for AMD with a "hardcoded" (with the dynamically retrieved value by OC) constant, but that is not written down anywhere. If you can provide some reason for non-AMD CPUs why this is helpful, that'd make the merge much more likely to happen. If you can show that you can make good use of the patch engine for AMD, maybe we could rather discuss exposing it to drivers rather than merging AMD patches. But that'd require an overview of all patches with their rationales and implementations. |
| &mProvideCurrentCpuInfoCoreCountReplace[CORE_COUNT_OFFSET], | ||
| &CoreCount, | ||
| sizeof (CoreCount) | ||
| ); |
There was a problem hiding this comment.
I would just use WriteUnaligned32, that should be the fastest on x86.
| ); | ||
| mProvideCurrentCpuInfoCoreCountPatch.ReplaceMask = mProvideCurrentCpuInfoCoreCountMontereyMinReplaceMask; | ||
| } | ||
| CoreCount = (UINT16) (CpuInfo->CoreCount); |
| // CoreCount patch | ||
| // | ||
| if (OcMatchDarwinVersion (KernelVersion, KERNEL_VERSION_HIGH_SIERRA_MIN, 0)) { | ||
| if (OcMatchDarwinVersion (KernelVersion, KERNEL_VERSION_CATALINA_MIN, 0)) { |
| // | ||
| if (OcMatchDarwinVersion (KernelVersion, KERNEL_VERSION_HIGH_SIERRA_MIN, 0)) { | ||
| if (OcMatchDarwinVersion (KernelVersion, KERNEL_VERSION_CATALINA_MIN, 0)) { | ||
| CopyMem ( &mProvideCurrentCpuInfoCoreCountReplace, |
| ); | ||
| } | ||
| if (OcMatchDarwinVersion (KernelVersion, KERNEL_VERSION_MONTEREY_MIN, 0)) { | ||
| CopyMem ( &mProvideCurrentCpuInfoCoreCountReplace, |
| @@ -981,6 +981,82 @@ mProvideCurrentCpuInfoZeroMsrThreadCoreCountPatch = { | |||
| .Limit = 0 | |||
| }; | |||
|
|
|||
| // Offset of value in below patch. | |||
There was a problem hiding this comment.
For proper Doxgen comment emission, please use
///
/// Offset of value in below patch.
///
Also please try to be clearer, this refers to the offset of the CoreCount argument within the instruction.
|
Thank you for the review. We will take this under consideration, we have another approach for this in the works. We will apply your recommendations to this and submit when we are ready. |
|
Any update on this? @Shaneee |
No description provided.