Re: [edk2-devel] [RFC PATCH v3 30/43] OvmfPkg/Sec: Add #VC exception handling for Sec phase

["Laszlo Ersek" <lersek@redhat.com>]

On 11/22/19 17:30, Tom Lendacky wrote:
> On 11/22/19 6:52 AM, Laszlo Ersek wrote:
>> On 11/21/19 21:46, Tom Lendacky wrote:
>>> On 11/21/19 6:06 AM, Laszlo Ersek wrote:
>>>> On 11/20/19 21:06, Lendacky, Thomas wrote:

>>>>> @@ -737,6 +738,21 @@ SecCoreStartupWithStack (
>>>>>       Table[Index] = 0;
>>>>>     }
>>>>>
>>>>> +  //
>>>>> +  // Initialize IDT
>>>>> +  //
>>>>> +  IdtTableInStack.PeiService = NULL;
>>>>> +  for (Index = 0; Index < SEC_IDT_ENTRY_COUNT; Index ++) {
>>>>> +    CopyMem (&IdtTableInStack.IdtTable[Index], &mIdtEntryTemplate,
>>>>> sizeof (mIdtEntryTemplate));
>>>>> +  }
>>>>> +
>>>>> +  IdtDescriptor.Base  = (UINTN)&IdtTableInStack.IdtTable;
>>>>> +  IdtDescriptor.Limit = (UINT16)(sizeof (IdtTableInStack.IdtTable)
>>>>> - 1);
>>>>> +
>>>>> +  AsmWriteIdtr (&IdtDescriptor);
>>>>> +
>>>>> +  InitializeCpuExceptionHandlers (NULL);
>>>>> +
>>>>>     ProcessLibraryConstructorList (NULL, NULL);
>>>>>
>>>>>     DEBUG ((EFI_D_INFO,
>>>>
>>>> (1) The problem here is that we call multiple library APIs before
>>>> calling ProcessLibraryConstructorList() -- namely CopyMem(),
>>>> AsmWriteIdtr(), and InitializeCpuExceptionHandlers().
> 
> We can reduce this exposure a bit and replace the CopyMem() call with
> something similar to the loop above it.

That would be nice, if you're not too annoyed by the extra busywork.

> I could also use assembler code directly in here to load the IDTR.

I think it would be enough to copy

  MdePkg/Library/BaseLib/Ia32/WriteIdtr.nasm
  MdePkg/Library/BaseLib/X64/WriteIdtr.nasm

under OvmfPkg/Sec, and use a new function name.

> That would leave just InitializeCpuExceptionHandlers(). Is there
> something that can added so as to warn when a library has a
> CONSTRUCTOR added to/part of the definition?

Nothing comes to my mind :(

[...]

>> (1) Append a UINT8 ("BOOLEAN") field to the SEV_ES_AP_JMP_FAR structure
>> (and possibly rename the structure),
>>
>> (2) in the OvmfPkgX64 reset vector, where you determine SEV-ES anyway,
>> explicitly set this field to zero if SEV-ES is disabled, and set it to
>> one, if SEV-ES is enabled,
>>
>> (3) In OvmfPkg/Sec, introduce a new (local) header file declaring the
>> function SevEsIsEnabled(),
>>
>> (4) Provide two C-language implementations (under the Ia32 and X64
>> directories): in the 32-bit version, return constant FALSE; in the
>> 64-bit version, return the value of the new field. Something like:
>>
>>    return ((SEV_ES_AP_JMP_FAR *)FixedPcdGet32
>> (PcdSevEsResetRipBase))->SevEsEnabled;
>>
>> FixedPcdGet32() is explicitly safe to use without library constructors
>> having run.
>>
>> Does this look viable? (It might require you to reshuffle patch 37 vs.
>> patch 30.)
> 
> I think this does. Since this is SEC and the reset vector page isn't
> needed until PEI and later we could even just use the first byte (make a
> union with an SEC usage field) and make this even simpler. Then we don't
> have to worry about positioning it.

Ah, nice.

> Let me work on that and see where I
> get. Anything after the #VC is established would use the current method
> of determine SEV-ES status.

Thanks!
Laszlo