Jump to content

[RELEASE] Open-source Clover fork with Ryzen support

shawnanastasio

By shawnanastasio,
in General Discussion

 Share
216 posts in this topic

Recommended Posts

Bronya
Bronya
Posted
Posted

Clover is able to differ AMD from Intel CPU. I just need to know what should be corrected for Rysen.

Hi ! 
You See in source code from Shawnanastasio in cpu.c for Ryzen and this works for Ryzen . 
But my cpu.c for Clover is other ! And main.c need change for "QPI" , because this QPI only for Intel  ! 
 
See my cpu.c . You can fix if this wrong ... 

cpu.c.zip

  • Like 4
apianti
apianti
Posted
Posted

I thought that's why Shawnanastasio started this thread to commit these fixes to the Clover developers? What happened?

 

I am a clover developer, I've had multiple people say that they have added support for Ryzen and when I ask for changes that they made there is no response. I don't have this CPU so I can't code changes for it.

 

Github link to source is on the first post, https://github.com/shawnanastasio/Clover-Ryzen

 

Ok, well you look through the repo and determine the changes that need to be made. I have other stuff to do, so does slice, we want just the changes to review and commit for working support. There is no need for all these additional forks to add support for this CPU only the changes that need made.....

 

All of my changes are completely open source. I was waiting for more testers to confirm that it works fine to submit it upstream to Clover.

That's reasonable, you can just do this with the official repo though by just giving changes.

 

Hi ! 

You See in source code from Shawnanastasio in cpu.c for Ryzen and this works for Ryzen . 

But my cpu.c for Clover is other ! And main.c need change for "QPI" , because this QPI only for Intel  ! 

 

See my cpu.c . You can fix if this wrong ...

 

Yeah, this is more reasonable and what I've been expecting for months. I'll review these changes.

 

First, is there a reason you indented almost the whole source? I see immediately though that you used a gcc specific extension __asm__, so that's going to need to be changed. Also, these two spots seem weird:

 

Why divided by two? It only gives logical CPUs and always has SMT? Line 298:

gCPUStructure.Cores = (UINT8)(gCPUStructure.CoresPerPackage & 0xff)/2;
Shouldn't this be before the previous if block where gCPUStructure.CoresPerPackage is being used?? Line 311:

if (gCPUStructure.CoresPerPackage == 0) {
  gCPUStructure.CoresPerPackage = 1;
}
It looks like the actual detection later looks pretty good but it's really hard to tell the differences with the changed indentation as the whole file is changed.
  • Like 3
shawnanastasio
shawnanastasio
Posted
  • Author
Posted

 That's reasonable, you can just do this with the official repo though by just giving changes.

Noted. I wasn't sure if you guys would be okay with accepting a potentially broken/untested patch. If you'd like I can PM you a diff containing all the changes I made for Ryzen support, or explain why the changes needed to be made.

  • Like 3
Bronya
Bronya
Posted
Posted

 

Why divided by two? It only gives logical CPUs and always has SMT? Line 298:

gCPUStructure.Cores = (UINT8)(gCPUStructure.CoresPerPackage & 0xff)/2;

Hi ! 

Yes , this for test . But i changed to this other my code and need test on ryzen ... 

 

 Shouldn't this be before the previous if block where gCPUStructure.CoresPerPackage is being used?? Line 311:

if (gCPUStructure.CoresPerPackage == 0) {
  gCPUStructure.CoresPerPackage = 1;
}

This is just in case )))

  • Like 1
Slice
Slice
Posted
Posted

 

Hi ! 
You See in source code from Shawnanastasio in cpu.c for Ryzen and this works for Ryzen . 
But my cpu.c for Clover is other ! And main.c need change for "QPI" , because this QPI only for Intel  ! 
 
See my cpu.c . You can fix if this wrong ... 

 

Why redefine rdmsr function? It is written by EDK2 developers and must be compatible with AMD CPU.

apianti
apianti
Posted
Posted

Hi ! 

Yes , this for test . But i changed to this other my code and need test on ryzen ... 

 

This is just in case )))

 

But that variable is used before you set it to 1, so all the previous uses it will be zero, meaning that you already used it and the cores and stuff will be zero.

 

 

Why redefine rdmsr function? It is written by EDK2 developers and must be compatible with AMD CPU.

 

This I forgot replace )). But this works ))

 

Nah, it only works for GCC, and only if extensions are enabled.

  • Like 1
Bronya
Bronya
Posted
Posted

But that variable is used before you set it to 1, so all the previous uses it will be zero, meaning that you already used it and the cores and stuff will be zero.

 

 

 

Yes , you right  :lol:  ))

I fixed ))

 

 

Nah, it only works for GCC, and only if extensions are enabled.

 

Why no ? This automatically builds and this working , i tested ))

but this no problem )).

I deleted this rdmsr64 and use  AsmReadMsr64 ...

apianti
apianti
Posted
Posted

The problem is you used a GCC specific extension __asm__, this does not build if you disable GCC extensions, or build with another compiler like VS. It is not a C language keyword. The closest is the c++ keyword asm, which also does not even work in most compilers, as VS just ignores it, and I think GCC doesn't even consider it a keyword.

Bronya
Bronya
Posted
Posted

The problem is you used a GCC specific extension __asm__, this does not build if you disable GCC extensions, or build with another compiler like VS. It is not a C language keyword. The closest is the c++ keyword asm, which also does not even work in most compilers, as VS just ignores it, and I think GCC doesn't even consider it a keyword.

i use c++ and clang ... , i think my compiler automatically build this __asm__.  But can replace __asm__ to asm or __asm ... . 
I speak to you that i deleted this rdmsr64 and i use AsmReadMsr64 . ;)  
Slice
Slice
Posted
Posted

But that variable is used before you set it to 1, so all the previous uses it will be zero, meaning that you already used it and the cores and stuff will be zero.

 

 

 

 

Nah, it only works for GCC, and only if extensions are enabled.

It may be a key why this version is not panic. There are no rdmsr at all!  ;)

  • Like 1
apianti
apianti
Posted
Posted

 

i use c++ and clang ... , i think my compiler automatically build this __asm__.  But can replace __asm__ to asm or __asm ... . 
I speak to you that i deleted this rdmsr64 and i use AsmReadMsr64 . ;)

 

 

Well the previous code doesn't build for me. Also, you must use C for EFI, C++ will not work because there is no runtime library, which is needed to initialize C++ behaviors. And the reason why AsmReadMsr64 exists is to prevent having to use compiler specific extensions, which is the only way to actually inline assembly. That function is compiled from assembly source.

 

Can you please provide just the code you added?

 

 

It may be a key why this version is not panic. There are no rdmsr at all!  ;)

 

What do you mean?

Bronya
Bronya
Posted
Posted

Well the previous code doesn't build for me. Also, you must use C for EFI, C++ will not work because there is no runtime library, which is needed to initialize C++ behaviors. And the reason why AsmReadMsr64 exists is to prevent having to use compiler specific extensions, which is the only way to actually inline assembly. That function is compiled from assembly source.

 

Can you please provide just the code you added?

 

I fixed for cores and thread . 

And fixed for amd family 14h , need test . 

I added cpu.c in archive cpu.c.zip . 

 

Try EFI_4101... and check Min and Max Ratio in menu ...

EFI_4101+ryzen.zip

cpu.c+ryzen.zip

  • Like 4
CEGOCAP
CEGOCAP
Posted
Posted

I fixed for cores and thread . 

And fixed for amd family 14h , need test . 

I added cpu.c in archive cpu.c.zip . 

 

Try EFI_4101... and check Min and Max Ratio in menu ...

Only replace cloverx64.Efi and bootx64. Efi?

post-1348578-0-83544400-1499557348_thumb.jpg

jalavoui
jalavoui
Posted
Posted

I've checked current clover sources at

- https://sourceforge.net/p/cloverefiboot/code/HEAD/tree/rEFIt_UEFI/Platform/cpu.c

 

and by checking other distros like

 

- https://github.com/torvalds/linux/blob/master/arch/x86/kernel/cpu/amd.c

- https://github.com/freebsd/freebsd/blob/master/sys/amd64/amd64/initcpu.c

 

maybe u guys wanna review / add some updates from above

 

i do hope not to see this code again in clover

MACHINE_TYPES GetDefaultModel()
{
  MACHINE_TYPES DefaultType = iMac132;
  if (gCPUStructure.Vendor != CPU_VENDOR_INTEL) {
    return MacPro31;

:)

 

btw Bronya, are u also updating the kernel ?

 

shawnanastasio its a great idea to keep a github 4 this topic - maybe finally we can have a few developers discussing source code.

 

once all this changes are tested we can port all this to clover since we have them here to help us.

 

 

is this the AsmReadMsr64 from http://www.bluestop.org/edk2/docs/trunk/_mde_pkg_2_library_2_base_lib_2_ia32_2_read_msr64_8c_source.html ?

 

 

found this inside https://github.com/freebsd/freebsd/blob/master/sys/amd64/include/cpufunc.h(code seems very clean - maybe useful if we need to fix any 64 bits function):

static __inline uint64_t
rdmsr(u_int msr)
{
	uint32_t low, high;

	__asm __volatile("rdmsr" : "=a" (low), "=d" (high) : "c" (msr));
	return (low | ((uint64_t)high << 32));
}

static __inline uint32_t
rdmsr32(u_int msr)
{
	uint32_t low;

	__asm __volatile("rdmsr" : "=a" (low) : "c" (msr) : "rdx");
	return (low);
}

this code is from init_amd (and fu@#! i wonder if this is why i get random crashes)  - not ryzen but applyes to f16. this is just a piece of code found in https://github.com/freebsd/freebsd/blob/master/sys/amd64/amd64/initcpu.c

guess its easy to browse those sources and decide if they will help us or not. and ofc we need to post a binary and ask ppl to test it b4 thinking on porting this to clover.

if ((cpu_feature2 & CPUID2_HV) == 0) is a check for virtualization (off) - guess is safe to implement this code unless using a virtual machine

/*
	 * Work around Erratum 793: Specific Combination of Writes to Write
	 * Combined Memory Types and Locked Instructions May Cause Core Hang.
	 * See Revision Guide for AMD Family 16h Models 00h-0Fh Processors,
	 * revision 3.04 or later, publication 51810.
	 */
	if (CPUID_TO_FAMILY(cpu_id) == 0x16 && CPUID_TO_MODEL(cpu_id) <= 0xf) {
		if ((cpu_feature2 & CPUID2_HV) == 0) {
			msr = rdmsr(0xc0011020);
			msr |= (uint64_t)1 << 15;
			wrmsr(0xc0011020, msr);
		}
	}
  • Like 2
apianti
apianti
Posted
Posted

That's one of the compiler specific ways that AsmReadMsr64 is implemented, I believe that is for VS IA32. You can find other sources for it in the same places, also in other archs. Like for VS X64 arch, there is an intrinsic compiler function __readmsr that is used to implement the function. I believe it is implemented in assembly for the rest (.asm and .nasm).

 

I will eventually get around to this, but I do not currently have time.... Soon though!  :yes:

jsl
jsl
Posted
Posted (edited)

I fixed for cores and thread . 

And fixed for amd family 14h , need test . 

I added cpu.c in archive cpu.c.zip . 

 

Try EFI_4101... and check Min and Max Ratio in menu ...

Thanks for this new Clover EFI which worked with new kernel of 10.9.5 provided by Bronya on 2017-07-01.

Previous kernel dated on 2017-05-31 always got too fast timer and very poor performance at 10.9.5.

However this new EFI still not working at 10.11.6 or 10.10.5 in my Ryzen 1700X hackintosh, only Bronya's 2667 bootloader worked up to now.

Thanks again Bronya:

Any fix of Clover for 10.11.6 or 10.10.5 ?

Any plan for 10.13 ?

[Edit]

After delete EvOreboot.kext in /S/L/E it can reboot now at 10.9.5.

So it's not the bug of kernel or Clover EFI.

Edited by jsl
Slice
Slice
Posted
Posted

 

 

 

What do you mean?

I mean that some rdmsr command may fail for the AMD CPU. There must be additional check if the CPU has these MSR registers.

And if the command is empty then it will work with some workaround for results...

Namely the MSR #define K10_COFVID_STATUS           0xC0010071

 is not defined for Rysen and will cause Clover red panic.

Slice
Slice
Posted
Posted

 

I've checked current clover sources at

- https://sourceforge.net/p/cloverefiboot/code/HEAD/tree/rEFIt_UEFI/Platform/cpu.c

 

and by checking other distros like

 

- https://github.com/torvalds/linux/blob/master/arch/x86/kernel/cpu/amd.c

- https://github.com/freebsd/freebsd/blob/master/sys/amd64/amd64/initcpu.c

 

maybe u guys wanna review / add some updates from above

 

i do hope not to see this code again in clover

MACHINE_TYPES GetDefaultModel()
{
  MACHINE_TYPES DefaultType = iMac132;
  if (gCPUStructure.Vendor != CPU_VENDOR_INTEL) {
    return MacPro31;

:)

 

btw Bronya, are u also updating the kernel ?

 

shawnanastasio its a great idea to keep a github 4 this topic - maybe finally we can have a few developers discussing source code.

 

once all this changes are tested we can port all this to clover since we have them here to help us.

 

 

is this the AsmReadMsr64 from http://www.bluestop.org/edk2/docs/trunk/_mde_pkg_2_library_2_base_lib_2_ia32_2_read_msr64_8c_source.html ?

 

 

this code is from init_amd (and fu@#! i wonder if this is why i get random crashes)  - not ryzen but applyes to f16

/*
	 * Work around Erratum 793: Specific Combination of Writes to Write
	 * Combined Memory Types and Locked Instructions May Cause Core Hang.
	 * See Revision Guide for AMD Family 16h Models 00h-0Fh Processors,
	 * revision 3.04 or later, publication 51810.
	 */
	if (CPUID_TO_FAMILY(cpu_id) == 0x16 && CPUID_TO_MODEL(cpu_id) <= 0xf) {
		if ((cpu_feature2 & CPUID2_HV) == 0) {
			msr = rdmsr(0xc0011020);
			msr |= (uint64_t)1 << 15;
			wrmsr(0xc0011020, msr);
		}
	}

Those workarounds can be implemented if you make me more investigations what is "cpu_feature2" and "CPUID2_HV".

apianti
apianti
Posted
Posted

I mean that some rdmsr command may fail for the AMD CPU. There must be additional check if the CPU has these MSR registers.

And if the command is empty then it will work with some workaround for results...

Namely the MSR #define K10_COFVID_STATUS           0xC0010071

 is not defined for Rysen and will cause Clover red panic.

 

Ah, right. Wasn't thinking... lol

 Share
Go to topic listing
×
×
  • Create New...