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

["Lendacky, Thomas" <thomas.lendacky@amd.com>]

On 11/22/19 3:10 PM, Laszlo Ersek wrote:
> 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.

I haven't done the WriteIdtr() stuff, yet, but the passing of the SEV-ES
status from the ResetVector code to the SEC code via the SEV-ES page at
0x0080_B000 is working nicely.

Thanks,
Tom

> 
>> 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
>