OvmfPkg/Sec: Refactor SevEsIsEnabled condition in SecCoreStartupWithS…

[khaliid2040]

Description

This PR makes small refactor in SecCoreStartupWithStack() function particularly combines two SEV condition check one for loading IDT and initializing exceptions, and the other is enabling caches for SEV guests.

Since AsmWriteIdtr() and AsmEnableCache() don't depend on any library constructor, it is safe to combine those two conditions into one which makes code flow like this:

If it is SEV guest :

• Write to IDTR
• Initialize exceptions.
• Then enable caches.

else :

• for non SEV guests just load IDT into IDTR

And then call ProcessLibraryConstructorList (). Thus eliminating the condition below ProcessLibraryConstructorList(), because it is not needed anymore.

• Breaking change?
  • No breakage observed
• Impacts security?
  • NO
• Includes tests?
  • NO

How This Was Tested

On OVMF image with platform architecture is X64

Integration Instructions

N/A

[tlendacky]

While I agree that the refactor is more straightforward, the reason for the way it currently is coded is specified in the big comment above the AsmWriteIdtr() call in the SEV-ES path. It will be up to the maintainers to determine if they want to make this change.

By the way, I haven't tested this, but I'll assume you tested all types of guests?

[mikebeaton]

I think this may be too small a change to be worth making as a pull request, if it doesn't fix a bug or make the code significantly cleaner or easier to work with.

[ardbiesheuvel]

Let's keep the code as-is, given that there doesn't appear to be anything wrong with it. Thanks.

[khaliid2040]

I think this may be too small a change to be worth making as a pull request, if it doesn't fix a bug or make the code significantly cleaner or easier to work with.

@mikebeaton The function is quite long and i thought any change that makes it less and more readable without introducing any regressions is worth. But of course this PR with it's changes should be least priority.

While I agree that the refactor is more straightforward, the reason for the way it currently is coded is specified in the big comment above the AsmWriteIdtr() call in the SEV-ES path. It will be up to the maintainers to determine if they want to make this change.

By the way, I haven't tested this, but I'll assume you tested all types of guests?

@tlendacky Given my tests, all assembly functions (with Asm prefix) worked fine without any library constructors being initialized.

Let's keep the code as-is, given that there doesn't appear to be anything wrong with it. Thanks.

You are welcome @ardbiesheuvel.

Also i would appriciate more tests to be done with this changes. I was also relying on CI tests