On 06/11/20 11:24, Laszlo Ersek wrote:
> On 06/05/20 15:27, Tom Lendacky wrote:
>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
>>
>> Under SEV-ES, a DR7 read or write intercept generates a #VC exception.
>> The #VC handler must provide special support to the guest for this. On
>> a DR7 write, the #VC handler must cache the value and issue a VMGEXIT
>> to notify the hypervisor of the write. However, the #VC handler must
>> not actually set the value of the DR7 register. On a DR7 read, the #VC
>> handler must return the cached value of the DR7 register to the guest.
>> VMGEXIT is not invoked for a DR7 register read.
>>
>> The caching of the DR7 values will make use of the per-CPU data pages
>> that are allocated along with the GHCB pages. The per-CPU page for a
>> vCPU is the page that immediately follows the vCPU's GHCB page. Since
>> each GHCB page is unique for a vCPU, the page that follows becomes
>> unique for that vCPU. The SEC phase will reserves an area of memory for
>> a single GHCB and per-CPU page for use by the BSP. After transitioning
>> to the PEI phase, new GHCB and per-CPU pages are allocated for the BSP
>> and all APs.
>>
>> Cc: Jordan Justen <jordan.l.jus...@intel.com>
>> Cc: Laszlo Ersek <ler...@redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheu...@arm.com>
>> Signed-off-by: Tom Lendacky <thomas.lenda...@amd.com>
>> ---
>> OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c | 114 ++++++++++++++++++++
>> 1 file changed, 114 insertions(+)
>
> The patch looks good to me:
>
> Acked-by: Laszlo Ersek <ler...@redhat.com>
>
> Just one question: in the v8 review, I wrote:
>
> """
> With your explanation above, about platform reset, I think I'm happy
> with the current handling of "Dr7Cached". So I'd like to leave the
> choice to you: please either add the clearing, or document in the commit
> message and/or the code that platform reset will not happen. Whichever
> you like more.
> """
>
> So what have you chosen ultimately? I haven't found a comment to the
> effect of "An SEV-ES guest can't be rebooted/reset without restarting
> Qemu" in this patch, in the commit message or in the code. Did you
> implement the clearing, in the end? (Sorry if I should have noticed it
> already!)
If I understand correctly, it's the clearGhcbMemoryLoop part (moved to
the new, correct, location) in patch#29. (For SEC.)
For PEI, we have a ZeroMem() call in patch#31.
I'm happy with those. (Hopefully I understand the code enough to be
*justifiedly* happy. :))
Thanks!
Laszlo
>
>> diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
>> b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
>> index 9d93e30a8ea4..e8f9d3fa01a8 100644
>> --- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
>> +++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
>> @@ -126,6 +126,14 @@ UINT64
>> SEV_ES_INSTRUCTION_DATA *InstructionData
>> );
>>
>> +//
>> +// Per-CPU data mapping structure
>> +//
>> +typedef struct {
>> + BOOLEAN Dr7Cached;
>> + UINT64 Dr7;
>> +} SEV_ES_PER_CPU_DATA;
>> +
>>
>> /**
>> Checks the GHCB to determine if the specified register has been marked
>> valid.
>> @@ -1478,6 +1486,104 @@ RdtscExit (
>> return 0;
>> }
>>
>> +/**
>> + Handle a DR7 register write event.
>> +
>> + Use the VMGEXIT instruction to handle a DR7 write event.
>> +
>> + @param[in, out] Ghcb Pointer to the Guest-Hypervisor
>> Communication
>> + Block
>> + @param[in, out] Regs x64 processor context
>> + @param[in] InstructionData Instruction parsing context
>> +
>> + @return 0 Event handled successfully
>> + @return Others New exception value to propagate
>> +
>> +**/
>> +STATIC
>> +UINT64
>> +Dr7WriteExit (
>> + IN OUT GHCB *Ghcb,
>> + IN OUT EFI_SYSTEM_CONTEXT_X64 *Regs,
>> + IN SEV_ES_INSTRUCTION_DATA *InstructionData
>> + )
>> +{
>> + SEV_ES_INSTRUCTION_OPCODE_EXT *Ext;
>> + SEV_ES_PER_CPU_DATA *SevEsData;
>> + UINT64 *Register;
>> + UINT64 Status;
>> +
>> + Ext = &InstructionData->Ext;
>> + SevEsData = (SEV_ES_PER_CPU_DATA *) (Ghcb + 1);
>> +
>> + DecodeModRm (Regs, InstructionData);
>> +
>> + //
>> + // MOV DRn always treats MOD == 3 no matter how encoded
>> + //
>> + Register = GetRegisterPointer (Regs, Ext->ModRm.Rm);
>> +
>> + //
>> + // Using a value of 0 for ExitInfo1 means RAX holds the value
>> + //
>> + Ghcb->SaveArea.Rax = *Register;
>> + GhcbSetRegValid (Ghcb, GhcbRax);
>> +
>> + Status = VmgExit (Ghcb, SVM_EXIT_DR7_WRITE, 0, 0);
>> + if (Status != 0) {
>> + return Status;
>> + }
>> +
>> + SevEsData->Dr7 = *Register;
>> + SevEsData->Dr7Cached = TRUE;
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + Handle a DR7 register read event.
>> +
>> + Use the VMGEXIT instruction to handle a DR7 read event.
>> +
>> + @param[in, out] Ghcb Pointer to the Guest-Hypervisor
>> Communication
>> + Block
>> + @param[in, out] Regs x64 processor context
>> + @param[in] InstructionData Instruction parsing context
>> +
>> + @return 0 Event handled successfully
>> +
>> +**/
>> +STATIC
>> +UINT64
>> +Dr7ReadExit (
>> + IN OUT GHCB *Ghcb,
>> + IN OUT EFI_SYSTEM_CONTEXT_X64 *Regs,
>> + IN SEV_ES_INSTRUCTION_DATA *InstructionData
>> + )
>> +{
>> + SEV_ES_INSTRUCTION_OPCODE_EXT *Ext;
>> + SEV_ES_PER_CPU_DATA *SevEsData;
>> + UINT64 *Register;
>> +
>> + Ext = &InstructionData->Ext;
>> + SevEsData = (SEV_ES_PER_CPU_DATA *) (Ghcb + 1);
>> +
>> + DecodeModRm (Regs, InstructionData);
>> +
>> + //
>> + // MOV DRn always treats MOD == 3 no matter how encoded
>> + //
>> + Register = GetRegisterPointer (Regs, Ext->ModRm.Rm);
>> +
>> + //
>> + // If there is a cached valued for DR7, return that. Otherwise return the
>> + // DR7 standard reset value of 0x400 (no debug breakpoints set).
>> + //
>> + *Register = (SevEsData->Dr7Cached) ? SevEsData->Dr7 : 0x400;
>> +
>> + return 0;
>> +}
>> +
>> /**
>> Handle a #VC exception.
>>
>> @@ -1522,6 +1628,14 @@ VmgExitHandleVc (
>>
>> ExitCode = Regs->ExceptionData;
>> switch (ExitCode) {
>> + case SVM_EXIT_DR7_READ:
>> + NaeExit = Dr7ReadExit;
>> + break;
>> +
>> + case SVM_EXIT_DR7_WRITE:
>> + NaeExit = Dr7WriteExit;
>> + break;
>> +
>> case SVM_EXIT_RDTSC:
>> NaeExit = RdtscExit;
>> break;
>>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#61131): https://edk2.groups.io/g/devel/message/61131
Mute This Topic: https://groups.io/mt/74692440/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
