Jump to content

Clover Problems and Solutions

ErmaC

By ErmaC,
in Clover Development

 Share
3,206 posts in this topic

Recommended Posts

vit9696
vit9696
Posted
Posted

I attach a quick and dirty patch working as an imminent solution to the problem. Due to very short timespan, little hardware to test this on, and varying vendor-specific form of managing RTC memory I put the changes under a special config option:

Boot → RtcHibernateAware = YES

This option is HIGHLY recommended if you use hibernation, as it reduces the risks of leaking the key.

 

What it does is basically explained above: upon booting a hibernated volume it reads the relevant RTC memory and erases it. This technically allows to boot from hibernated volumes when NVRAM variable is not available.   

 

I additionally attach a patched prebuilt Clover based on r4449 and a couple of tools:

— EraseHibernateRtc.efi (erases the affected RTC region to ensure that no encryption key is stored there)

— ReadHibernateState.efi (reads and prints the RTC region along with the nvram variable, useful for debugging)

hibernate-rtc.patch.zip

clover-and-tools.zip

  • Like 7
Link to comment
Share on other sites
vit9696
vit9696
Posted
Posted

Sure, the source code for the second one is in the spoiler above, and here it is for the first one: 


STATIC
VOID
CleanupIORTCVariable (
  )
{
  UINT8    Data[EXPECTED_HIBERNATE_VAR_SIZE];
  UINTN    Index;

  for (Index = 0; Index < EXPECTED_HIBERNATE_VAR_SIZE; Index++) {
    Data[Index] = SimpleRtcRead (Index + 128);
  }

  Print (L"IOHibernateRTCVariables from RTC is as follows:\n");
  for (Index = 0; Index < EXPECTED_HIBERNATE_VAR_SIZE; Index++) {
    Print (L"%02X ", Data[Index]);
  }
  Print (L"\n");

  ZeroMem (Data, EXPECTED_HIBERNATE_VAR_SIZE);

  Data[0] = 'D';
  Data[1] = 'E';
  Data[2] = 'A';
  Data[3] = 'D';

  for (Index = 0; Index < EXPECTED_HIBERNATE_VAR_SIZE; Index++) {
    SimpleRtcWrite (Index + 128, Data[Index]);
  }

  Print (L"IOHibernateRTCVariables after cleanup is as follows:\n");
  for (Index = 0; Index < EXPECTED_HIBERNATE_VAR_SIZE; Index++) {
    Print (L"%02X ", Data[Index]);
  }
  Print (L"\n");
}

 

I cannot say they are of great importance though.

  • Like 1
Link to comment
Share on other sites
Slice
Slice
Posted
Posted
18 hours ago, vit9696 said:

I mentioned this in the updates topic, but will say again that this variable is written neither to NVRAM, nor to RTC when RTC has less than 256 bytes of RAM. If you have AppleRTC patches or anything alike it will be the case.

Yes, you are right. Althouh I switched off AppleRTC patch in Clover but I used prepatched DSDT with RTC this kind 

                Device (RTC)
                {
                    Name (_HID, EisaId ("PNP0B00") /* AT Real-Time Clock */)  // _HID: Hardware ID
                    Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
                    {
                        IO (Decode16,
                            0x0070,             // Range Minimum
                            0x0070,             // Range Maximum
                            0x00,               // Alignment
                            0x02,               // Length
                            )
                    })
                }

while initially it was 

                    Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
                    {
                        IO (Decode16,
                            0x0070,             // Range Minimum
                            0x0070,             // Range Maximum
                            0x01,               // Alignment
                            0x08,               // Length
                            )
                    })

Now I changed back and got 

Variable NV+RT+BS '7C436110-AB2A-4BBB-A880-FE41995C9F82:IOHibernateRTCVariables' DataSize = 0x2C
  00000000: 41 41 50 4C 01 00 00 00-F7 C8 0F 43 E1 ED B3 92  *AAPL.......C....*
  00000010: EA DA F3 77 A6 DD D4 15-58 79 87 CE EB 97 72 38  *...w....Xy....r8*
  00000020: 05 0D A5 78 E3 82 7E D1-7B 5A C3 68              *...x..~.{Z.h*

I still have no working hibernation but this is a great step forward.

Link to comment
Share on other sites
  • 3 weeks later...
RehabMan
RehabMan
Posted
Posted (edited)

@Slice

 

The commit for r4468 breaks the ACPI patcher.  On my NUC, had to roll back to previously working Clover as it was not bootable with those changes you made.

 

Here are my fixes:

https://github.com/RehabMan/Clover/commit/d0507a6b352fb2c8f85da70836b1bac08ecf0cec 


NUC6i7KYK:Clover rehabman$ git diff  rEFIt_UEFI/Platform/AcpiPatcher.c

diff --git a/rEFIt_UEFI/Platform/AcpiPatcher.c b/rEFIt_UEFI/Platform/AcpiPatcher.c

index f0b20eee..03535d04 100644

--- a/rEFIt_UEFI/Platform/AcpiPatcher.c

+++ b/rEFIt_UEFI/Platform/AcpiPatcher.c

@@ -487,8 +487,8 @@ VOID PatchAllTables()

   UINT32 Count = XsdtTableCount();

   UINT64* Ptr = XsdtEntryPtrFromIndex(0);

   UINT64* EndPtr = XsdtEntryPtrFromIndex(Count);

-  BOOLEAN Patched = FALSE;

   for (; Ptr < EndPtr; Ptr++) {

+    BOOLEAN Patched = FALSE;

     EFI_ACPI_DESCRIPTION_HEADER* Table = (EFI_ACPI_DESCRIPTION_HEADER*)(UINTN)ReadUnaligned64(Ptr);

     if (!Table) {

       // skip NULL entry

@@ -498,10 +498,6 @@ VOID PatchAllTables()

       // may be also EFI_ACPI_4_0_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE?

       continue; // will be patched elsewhere

     }

-    if (!CheckTableHeader(Table)) {

-      // header does not need patching

-      continue;

-    }

     if (IsXsdtEntryMerged(IndexFromXsdtEntryPtr(Ptr))) {

       // table header already patched

       continue;

@@ -511,7 +507,7 @@ VOID PatchAllTables()

     EFI_PHYSICAL_ADDRESS BufferPtr = EFI_SYSTEM_TABLE_MAX_ADDRESS;

     EFI_STATUS Status = gBS->AllocatePages(AllocateMaxAddress,

                                            EfiACPIReclaimMemory,

-                                           EFI_SIZE_TO_PAGES(Len + 19),

+                                           EFI_SIZE_TO_PAGES(Len + 4096),

                                            &BufferPtr);

     if(EFI_ERROR(Status)) {

       //DBG(" ... not patched\n");

@@ -520,12 +516,12 @@ VOID PatchAllTables()

     EFI_ACPI_DESCRIPTION_HEADER* NewTable = (EFI_ACPI_DESCRIPTION_HEADER*)(UINTN)BufferPtr;

     CopyMem(NewTable, Table, Len);

     if ((gSettings.FixDsdt & FIX_HEADERS) || gSettings.FixHeaders) {

-      CopyMem(NewTable, Table, Len);

-      PatchTableHeader(NewTable);

-      Patched = TRUE;

+      if (CheckTableHeader(NewTable)) {

+        PatchTableHeader(NewTable);

+        Patched = TRUE;

+      }

     }

     if (NewTable->Signature == EFI_ACPI_4_0_SECONDARY_SYSTEM_DESCRIPTION_TABLE_SIGNATURE) {

-      CopyMem(NewTable, Table, Len);

       if (gSettings.PatchDsdtNum > 0) {

         //DBG("Patching SSDT:\n");

         UINT32 i;

@@ -548,20 +544,24 @@ VOID PatchAllTables()

       NewTable->Length = Len;

       RenameDevices((UINT8*)NewTable);

       GetBiosRegions((UINT8*)NewTable);  //take Regions from SSDT even if they will be dropped

-      Patched = TRUE;;

+      Patched = TRUE;

     }

     if (NewTable->Signature == MCFG_SIGN && gSettings.FixMCFG) {

       INTN Len1 = ((Len + 4 - 1) / 16 + 1) * 16 - 4;

-      CopyMem(NewTable, Table, Len1); //Len increased but less then EFI_PAGE

+      CopyMem(NewTable, Table, Len1); //Len increased but less than EFI_PAGE

       NewTable->Length = Len1;

       Patched = TRUE;

     }

     if (Patched) {

+      // in case we need to free it, keep track of table size

+      SaveMergedXsdtEntrySize(IndexFromXsdtEntryPtr(Ptr), Len + 4096);

+

+      // write patched table pointer into the XSDT

       WriteUnaligned64(Ptr, BufferPtr);

       FixChecksum(NewTable);

     }

     else {

-      gBS->FreePages(BufferPtr, EFI_SIZE_TO_PAGES(Len + 19));

+      gBS->FreePages(BufferPtr, EFI_SIZE_TO_PAGES(Len + 4096));

     }

   }

 }

 

Edited by RehabMan
  • Like 1
  • Thanks 1
Link to comment
Share on other sites
Zenith432
Zenith432
Posted
Posted (edited)

@Slice:

How about making Patches_for_UDK2018 using "svn cp" from Patches_for_EDK2 and using that for UDK2018 while keeping Patches_for_EDK2 for EDK2?

 

PS: If it's acceptable, I can make the change.

Edited by Zenith432
  • Like 3
Link to comment
Share on other sites
apianti
apianti
Posted
Posted

A better choice would be to remove the patches all together, move the modified source modules into clover and modify the dsc/dec/inf files to use that source module instead of the one in the edks. Then it will build without having to copy patches or worry about the edk version.

Link to comment
Share on other sites
Zenith432
Zenith432
Posted
Posted

@apianti:

Not everything can be handled this way, because some of the patches are to include files that are included in many project in EDK2, not to just to local source files.  Moreover, keeping the patches in EDK2 tree makes the patches easy to keep up to date with git-rebase, but I'm the only one doing that.  If patches are moved to Clover tree, they need more complex merge.

Link to comment
Share on other sites
Slice
Slice
Posted
Posted
On 5/19/2018 at 7:59 PM, Zenith432 said:

@Slice:

How about making Patches_for_UDK2018 using "svn cp" from Patches_for_EDK2 and using that for UDK2018 while keeping Patches_for_EDK2 for EDK2?

 

PS: If it's acceptable, I can make the change.

Yes, acceptable.

Link to comment
Share on other sites
apianti
apianti
Posted
Posted
20 hours ago, Zenith432 said:

@apianti:

Not everything can be handled this way, because some of the patches are to include files that are included in many project in EDK2, not to just to local source files.  Moreover, keeping the patches in EDK2 tree makes the patches easy to keep up to date with git-rebase, but I'm the only one doing that.  If patches are moved to Clover tree, they need more complex merge.

 

No, you've misunderstood. Take the entire source module and move it into clover, modify with the patches, then use that module instead of whatever module was originally being used from the edk. It's so that you don't need to use patches at all, you can easily diff and rebase only that module, I'm confused as to why it would be any different....? It's certainly what you're supposed to do when using the edk or it wouldn't have library classes where you can choose different modules to satisfy the class reference. Clover does this for some modules already so unsure why it wouldn't do so for every module that it needs patched, you can also have a variable when defined build with the unpatched edk modules instead as well.

Link to comment
Share on other sites
Slice
Slice
Posted
Posted
3 hours ago, apianti said:

 

No, you've misunderstood. Take the entire source module and move it into clover, modify with the patches, then use that module instead of whatever module was originally being used from the edk. It's so that you don't need to use patches at all, you can easily diff and rebase only that module, I'm confused as to why it would be any different....? It's certainly what you're supposed to do when using the edk or it wouldn't have library classes where you can choose different modules to satisfy the class reference. Clover does this for some modules already so unsure why it wouldn't do so for every module that it needs patched, you can also have a variable when defined build with the unpatched edk modules instead as well.

Yes, it is a way. Take all headers from EDK2 and place them into Clover/Include. Take all Libraries and all Modules, including cross dependencies. 

It just requires efforts.

Link to comment
Share on other sites
Zenith432
Zenith432
Posted
Posted
On 5/21/2018 at 8:27 AM, Slice said:

Take all headers from EDK2 and place them into Clover/Include. Take all Libraries and all Modules, including cross dependencies.

includepath for a module is taken from .dec file specified in .inf file for module.  It is why PeCoffLib.h ended up being patched in edk2 tree, remember?  So headers cannot be moved under Clover unless all modules that use them also get moved and their .inf file modified.

Link to comment
Share on other sites
Slice
Slice
Posted
Posted
35 minutes ago, Zenith432 said:

includepath for a module is taken from .dec file specified in .inf file for module.  It is why PeCoffLib.h ended up being patched in edk2 tree, remember?  So headers cannot be moved under Clover unless all modules that use them also get moved and their .inf file modified.

Yes, all modules and all libraries should be modified.

I partially made the job. DUET is a part of Clover tree now being very old and modified version instead of patches_for_recent_duet.

 

32 minutes ago, STLVNUB said:

Why doesn't Clover go with the flow and use EDK2 as is, no need for patches IMHO

EDK2 as is? You are laughing.

Link to comment
Share on other sites
mhaeuser
mhaeuser
Posted
Posted (edited)
3 hours ago, Slice said:

EDK2 as is? You are laughing.

I went quite well without commenting out ASSERTs, adding extern declarations to headers not defining that GUID, or effectively removing safety checks from functions declared "safe" in their very names while adding false comments.

But this GetBestLanguage ([...], "", [...]); hack really got to me, you should submit that.

 

On 5/20/2018 at 2:06 AM, apianti said:

A better choice would be to remove the patches all together, move the modified source modules into clover and modify the dsc/dec/inf files to use that source module instead of the one in the edks. Then it will build without having to copy patches or worry about the edk version.

 

not even Base.h was safe, do you really want to fork MdePkg? :P

 

@Zenith432 Didn't I see you submit va-args fixes on edk2-devel? Those look like the only changes that might ("might" because idk about the validity) be worth anything.

Edited by Download-Fritz
Link to comment
Share on other sites
Slice
Slice
Posted
Posted

I also have to mention that LoadImage() is allowed only from FV, not from HDD as Clover did.

There are other discrepancies with pure EDK2.

There is a bug with Thunk.s which is not correctly compiled by clang. Other clang problems still not submitted to EDK2.

There was a strange bug with SafeString which made Clover not working. We resolve it but a hacking way.

Link to comment
Share on other sites
mhaeuser
mhaeuser
Posted
Posted

I'm aware, that is an actual security feature implemented this way so it is not accidentially bypassed or the unsafe variant is used because it is more convenient. Except for maybe va-args and one variable assigned but not used, the rest looks like garbage.

Link to comment
Share on other sites
nms
nms
Posted
Posted (edited)
4 hours ago, Slice said:

There is a bug with Thunk.s which is not correctly compiled by clang.

If one follows edk2 mailing list, then there are patches in pipe to remove all non nasm assembler sources.

Edited by nms
missing words
Link to comment
Share on other sites
Zenith432
Zenith432
Posted
Posted (edited)
20 hours ago, Download-Fritz said:

@Zenith432 Didn't I see you submit va-args fixes on edk2-devel? Those look like the only changes that might ("might" because idk about the validity) be worth anything.

That was a long time ago - there was a bug of iterating a VA_LIST twice without intervening VA_END+reVA_START, plus other (less-lethal) cases of not using VA_END properly.

The more recent submits were fixes to problems in BaseTools.

The problem with GetBestLanguage/VariableGetBestLanguage are fixed in EDK2 by using -Wno-varargs globally with clang.  This isn't such a good idea, because it silences all clang -Wvarargs which may detect potential bugs.  However, the package maintainer seems content with it - and fixing all cases in EDK2 of disallowed type of 2nd argument to VA_START requires a change in the official PEI documentation.  I'm also unsure that the solution in Clover of naming the first variadic parameter works in all cases in EDK2, because the first variadic parameter may also have a type which is not a result of default arument promotion (this by itself is also UB :)).  I don't have patience to sort it all out.

Anyways, I recently changed the implementation in Clover of GetBestLanguage so adding the redundant "" is not needed.  The named (formerly first variadic) parameter is processed along all the variadic.

Edited by Zenith432
Link to comment
Share on other sites
mhaeuser
mhaeuser
Posted
Posted
That was a long time ago - there was a bug of iterating a VA_LIST twice without intervening VA_END+reVA_START, plus other (less-lethal) cases of not using VA_END properly.
The more recent submits were fixes to problems in BaseTools.
The problem with GetBestLanguage/VariableGetBestLanguage are fixed in EDK2 by using -Wno-varargs globally with clang.  This isn't such a good idea, because it silences all clang -Wvarargs which may detect potential bugs.  However, the package maintainer seems content with it - and fixing all cases in EDK2 of disallowed type of 2nd argument to VA_START requires a change in the official PEI documentation.  I'm also unsure that the solution in Clover of naming the first variadic parameter works in all cases in EDK2, because the first variadic parameter may also have a type which is not a result of default arument promotion (this by itself is also UB ).  I don't have patience to sort it all out.
Anyways, I recently changed the implementation in Clover of GetBestLanguage so adding the redundant "" is not needed.  The named (formerly first variadic) parameter is processed along all the variadic.
Oh sorry, I meant the changes in Base.h which had something about "MS no builtin vaargs" (don't have access atm). GetBestLanguage will be fixed once the PI spec is updated.
Link to comment
Share on other sites
Zenith432
Zenith432
Posted
Posted (edited)
1 hour ago, Download-Fritz said:

Oh sorry, I meant the changes in Base.h which had something about "MS no builtin vaargs" (don't have access atm). GetBestLanguage will be fixed once the PI spec is updated.

I see lgao4 just fixed it...

Quote

77ca824c (origin/master, origin/HEAD) MdePkg/IndustryStandard: Add header file for SPMI ACPI table
1d4f84f9 BaseTools/Workspace: Fix ValueChain set
180ac200 MdeModulePkg Variable: Fix XCODE5 varargs warning
cb96e7d4 IntelFrameworkPkg UefiLib: Fix XCODE5 varargs warning
d2aafe1e MdePkg UefiLib: Fix XCODE5 varargs warning

And you also patched something in 1d4f84.

EDK2 fixed it by changing the BOOLEAN to UINTN.

Anyhow, EDK2 doesn't need the changes I made to MdePkg/Include/Base.h.

They have a macro NO_MSABI_VA_FUNCS to disable the __builtin_ms_va_* variants.  It is used for XCOODE5 in tools_def.template.

In GCC, __builtin_ms_va_* always work the same (as Microsoft) whether you use mixed ABI or not.

In clang, with x86_64-windows-macho, they can't be used except on some specific older versions identified in Clover mod to Base.h.  The implementation of __builtin_va_* works (= is STDC compliant), but is incompatible with Microsoft.

In Clover, we have XCODE5 which uses x86_64-windows-macho, and XCODE8 which uses x86_64-apple-darwin, and also some of the macOS support apps shipped with Clover end up including Base.h even though they're not EDK2 EFI binaries.  They also use x86_64-apple-darwin.  So Clover Base.h needs to handle cases that EDK2 doesn't.

Edited by Zenith432
  • Like 1
Link to comment
Share on other sites
 Share
Go to topic listing
×
×
  • Create New...