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!)
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 (#61130): https://edk2.groups.io/g/devel/message/61130
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]
-=-=-=-=-=-=-=-=-=-=-=-
