OcAppleKernelLib: Added patch for MSR MISC_PWR_MGMT (1AAh)

[mrmiller]

Patch kernel writes to MISC_PWR_MGMT (1AAh) if the system firmware has locked that register (flag 0x2000).

This change allows Skylake-SP (Xeon Scalable) CPUs to boot with their native CPU ID rather than emulating an older chip. I suspect it will also work for Xeon W chips which share the same ID and capabilities.

I'm not sure if this should be part of AppleXcpmExtraMsrs or not but starting with it there based on @vit9696's suggestion. See acidanthera/bugtracker#365.

CCing @PMheart who I believe was involved in similar discussions working around issues with Xeon W, e.g. this CPUID patch.

This discussion thread about Xeon Ws may also be relevant.

[vit9696]

@mrmiller please make the patch unconditional. If this MSR is locked, all the others are pretty much certainly locked as well. Even if we put aside our firmware logic knowledge, there is one more reason:

MSR values are local per core, and GIGABYTE once forgot to update BSP (primary core) E2h register value in the past. Proper detection will require MP protocol usage, as done in VerifyMsrE2, but it is neither guaranteed to be properly implemented, nor has valid timeout value that is guaranteed to work on any amount of cores without stalling. Current timeout has failed thrice in VerifyMsrE2 already.

[mrmiller]

Sounds good! Removed the check for whether the MSR is locked or not.

[vit9696]

Sorry for not noticing this earlier, but actually there are two more issues with this patch.

1. Debug kernel. This sequence is simply not present for debug kernel, so we also need to patch BF AA 01 00 00 E8 → EB 08 90 90 90 E8, just like for CFG Lock (E2h) above.
2. Replacement count and base. Make the patch localised, so that we do not patch something else.
   1. For debug patch set Base to "_xcpm_hwp_enable" for debug patch, Limit to 4096, and Count to 2 as we cannot distinguish read and write
   2. For release patch set Count to 3 as the function is inlined thrice. Leave Base and Limit unchanged.

[mrmiller]

1. Debug kernel. This sequence is simply not present for debug kernel, so we also need to patch `BF AA 01 00 00 E8` → `EB 08 90 90 90 E8`, just like for CFG Lock (`E2h`) above.

Will do.

2. Replacement count and base. Make the patch localised, so that we do not patch something else.
   
   1. For debug patch set Base to `"_xcpm_hwp_enable"` for debug patch, Limit to 4096, and Count to 2 as we cannot distinguish read and write

Will do.

   2. For release patch set Count to 3 as the function is inlined thrice. Leave Base and Limit unchanged.

I count 4 calls to wrmsr with 1AAh, including the original, non-inlined_xcpm_hwp_enable (though it's unclear if it's ever called). Should we be trying to specify the exact count in this case? It seems somewhat brittle as if Apple ends up adding even another inlined call, the patch may silently start failing on the release kernel. I guess we can cross that bridge if we ever come to it.

The risk seems to be that we match against a sequence we shouldn't have and change something unrelated. It seems somewhat unlikely for this 7 byte sequence, but I'm not sure Count does a good job protecting us from a bad replacement anyways.

[vit9696]

Should be ++Replacements; without space probably, and add a DEBUG_INFO log here, so that we have in the log that the patch has happened. Thanks!

[mrmiller]

Looking at the debug kernel, there seems to be an additional function that writes to MSR 1AAh: _xcpm_enable_hw_coordination (which is inlined on the release build). I can either make another patch to hit that or we can tweak the existing debug patch so it differentiates between the reads and the writes. Since the reads only take one argument, we can look for the use of a second argument. Both instances look the same in the debug build:

mov        rsi, qword [rbp-8]
mov        edi, 0x1aa
call       _xcpm_wrmsr64

So we could look for the sequence 48 8B ?? ?? BF AA 01 00 00 E8 instead and only affect the (currently two) writes.

[PMheart]

Hello,

Could it be better to change the replacement pattern of Debug to
90 90 90 EB 05
which seems kind of clearer?

Or we could also use bitmasking to NOP out the rdmsr call as well I guess.

[PMheart]

As for the call under _xcpm_enable_hw_coordination, well, should that be patched as well? (together with the two sequences under _xcpm_hwp_enable , I mean)

If so, then there seems to be no need to do the precise matching, just remove .Base as there are only FOUR ones (two under _xcpm_hwp_enable and the other two under _xcpm_enable_hw_coordination)

EDIT: 4 ones. Sorry for my blindness.

[mrmiller]

As for the call under _xcpm_enable_hw_coordination, well, should that be patched as well? (together with the two sequences under _xcpm_hwp_enable , I mean)

Yes, it definitely should be. I hadn't looked at the debug kernel until just now and noticed the same thing.

If so, then there seems to be no need to do the precise matching, just remove .Base as there are only THREE ones (two under _xcpm_hwp_enable and the rest under _xcpm_enable_hw_coordination)

I agree. We don't really need to patch the reads for that MSR, but we currently are as well. What do you think about expanding the Find sequence slightly to look for the mov into rsi so we only patch the writes? And then apply that to the entire kernel with Count 2 (if we're trying to be exact).

(To be pedantic, there are 4 matches to the current sequence... 2 call _xcpm_rdmsr64 which we don't need to patch. The other two, which we need to patch, call _xcpm_wrmsr64.)

[vit9696]

@mrmiller of course it was 4, I just managed to overlook it. But you made a good claim for 0 vs 4, and with the second debug hit it is probably better to use count 0 for both.

xcpm_enable_hw_coordination is actually new, I did not have it in my idb, but we of course need to patch it. The writes are roughly the same for both:

void __cdecl xcpm_enable_hw_coordination()
{
  uint64_t_0 msrval; // [rsp+8h] [rbp-8h]

  msrval = xcpm_rdmsr64(0x1AAu) & 0xFFFFFFFFFFFFFFFELL;
  xcpm_wrmsr64(0x1AAu, msrval);
}

void __cdecl xcpm_hwp_enable()
{
  uint64_t_0 msrval; // [rsp+8h] [rbp-8h]

  msrval = xcpm_rdmsr64(0x1AAu) | 0x40;
  xcpm_wrmsr64(0x1AAu, msrval);
  xcpm_wrmsr64(0x770u, 1uLL);
  if ( xcpm_config.xcpm_hwp_pkg_mode )
    xcpm_wrmsr64(0x774u, 0x40000FFFF07uLL);
}

So I think patching reads and writes is ok. It is worse to extend the patch as there are many ways to assign rsi. So please just remove Base, Limit, Count, and use the original debug patch.

I also noted that you print nothing on success. Please do it for both debug and release patches separately, so that we have a log entry for debugging.

[vit9696]

Hello,

Could it be better to change the replacement pattern of Debug to
90 90 90 EB 05
which seems kind of clearer?

Or we could also use bitmasking to NOP out the rdmsr call as well I guess.

Better not, so that we are consistent with CFG Lock patch.

[mrmiller]

Alright, removed the limitations on the patch applications for both and added a log line for success.

[PMheart]

Better not, so that we are consistent with CFG Lock patch.

Well, I guess the reason why I thought 05 was clearer was that it simply jumped the next call that takes 5 bytes. Isn't it?

Regarding CFG lock debug patch, better to change it to 90 90 90 EB 05 as well I guess.

Anyway... It simply is worth nothing. Up to you. ^^

[mrmiller]

Regarding CFG lock debug patch, better to change it to 90 90 90 EB 05 as well I guess.

Anyway... It simply is worth nothing. Up to you. ^^

I'm not an opcode wizard so I'll leave this to you two to debate which is more clear and elegant. They're functionally equivalent so I'm happy to change both or neither!

[PMheart]

Well, whatever, let's ignore it.

Finally, the changes look good enough to me. Let's wait vit for the last review and we are about to merge it. :)

[vit9696]

Thanks, merged!

[ornulf]

Tested this patch on my Xenon W-2175 and it works like a charm.

Thank´s all of you for your contribution.

opencore-2019-08-27-074119.txt

[mrmiller]

Tested this patch on my Xenon W-2175 and it works like a charm.

Thank´s all of you for your contribution.

opencore-2019-08-27-074119.txt

Hmm... are you sure that log was generated using the latest version? I don't see any entries related to the new change and the other entries like 15:722 00:006 Replacing _xcpm_SMT_scope_msrs data 46 29660 should be prefixed with OCAK: in the latest.

[ornulf]

No it was generated with the patch above.
I will compile the latest version of OC with the merged changes and come back with a new log file.

[mrmiller]

No worries! I started by testing it as a patch in my config too. Just was curious because the log didn’t look like it came from the latest OC so wanted to confirm. So happy to hear it works for you too!

[ornulf]

here we go.
opencore-2019-08-27-083024.txt

[mrmiller]

Beautiful. Thanks for testing it!