[edk2-devel] [PATCH v8 17/46] OvmfPkg/VmgExitLib: Add support for NPF NAE events (MMIO)
Lendacky, Thomas
thomas.lendacky at amd.com
Fri May 22 20:41:57 UTC 2020
On 5/22/20 9:14 AM, Laszlo Ersek wrote:
> On 05/19/20 23:50, Lendacky, Thomas wrote:
>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2198&data=02%7C01%7Cthomas.lendacky%40amd.com%7Cfdd2325c2e5341d8ae5408d7fe5a75f5%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637257536808864483&sdata=RICgoxIHQzIwTS0UWB0gFK39ENqFaoSH%2FeX6DU0h0VI%3D&reserved=0
>>
>> Under SEV-ES, a NPF intercept for an NPT entry with a reserved bit set
>> generates a #VC exception. This condition is assumed to be an MMIO access.
>> VMGEXIT must be used to allow the hypervisor to handle this intercept.
>>
>> Add support to construct the required GHCB values to support a NPF NAE
>> event for MMIO. Parse the instruction that generated the #VC exception,
>> setting the required register values in the GHCB and creating the proper
>> SW_EXIT_INFO1, SW_EXITINFO2 and SW_SCRATCH values in the GHCB.
>>
>> Cc: Jordan Justen <jordan.l.justen at intel.com>
>> Cc: Laszlo Ersek <lersek at redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel at arm.com>
>> Signed-off-by: Tom Lendacky <thomas.lendacky at amd.com>
>> ---
>> .../Library/VmgExitLib/X64/VmgExitVcHandler.c | 436 ++++++++++++++++++
>> 1 file changed, 436 insertions(+)
>>
>> diff --git a/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c
>> index 1c6b472a47c4..50199845ceef 100644
>> --- a/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c
>> +++ b/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c
>> @@ -224,6 +224,263 @@ GhcbSetRegValid (
>> Ghcb->SaveArea.ValidBitmap[RegIndex] |= (1 << RegBit);
>> }
>>
>> +/**
>> + Return a pointer to the contents of the specified register.
>> +
>> + Based upon the input register, return a pointer to the registers contents
>> + in the x86 processor context.
>> +
>> + @param[in] Regs x64 processor context
>> + @param[in] Register Register to obtain pointer for
>> +
>> + @retval Pointer to the contents of the requested register
>> +
>> +**/
>> +STATIC
>> +INT64 *
>
> (1) Please change the return type from (INT64*) to (UINT64*).
>
> My request will look more justified once I get to the rest of my points
> below.
>
>> +GetRegisterPointer (
>> + IN EFI_SYSTEM_CONTEXT_X64 *Regs,
>> + IN UINT8 Register
>> + )
>> +{
>> + UINT64 *Reg;
>> +
>> + switch (Register) {
>> + case 0:
>> + Reg = &Regs->Rax;
>> + break;
>> + case 1:
>> + Reg = &Regs->Rcx;
>> + break;
>> + case 2:
>> + Reg = &Regs->Rdx;
>> + break;
>> + case 3:
>> + Reg = &Regs->Rbx;
>> + break;
>> + case 4:
>> + Reg = &Regs->Rsp;
>> + break;
>> + case 5:
>> + Reg = &Regs->Rbp;
>> + break;
>> + case 6:
>> + Reg = &Regs->Rsi;
>> + break;
>> + case 7:
>> + Reg = &Regs->Rdi;
>> + break;
>> + case 8:
>> + Reg = &Regs->R8;
>> + break;
>> + case 9:
>> + Reg = &Regs->R9;
>> + break;
>> + case 10:
>> + Reg = &Regs->R10;
>> + break;
>> + case 11:
>> + Reg = &Regs->R11;
>> + break;
>> + case 12:
>> + Reg = &Regs->R12;
>> + break;
>> + case 13:
>> + Reg = &Regs->R13;
>> + break;
>> + case 14:
>> + Reg = &Regs->R14;
>> + break;
>> + case 15:
>> + Reg = &Regs->R15;
>> + break;
>> + default:
>> + Reg = NULL;
>> + }
>> + ASSERT (Reg != NULL);
>> +
>> + return (INT64 *) Reg;
>> +}
>
> (2) Please remove the cast in the "return" statement.
>
>> +
>> +/**
>> + Update the instruction parsing context for displacement bytes.
>> +
>> + @param[in, out] InstructionData Instruction parsing context
>> + @param[in] Size The instruction displacement size
>> +
>> +**/
>> +STATIC
>> +VOID
>> +UpdateForDisplacement (
>> + IN OUT SEV_ES_INSTRUCTION_DATA *InstructionData,
>> + IN UINTN Size
>> + )
>> +{
>> + InstructionData->DisplacementSize = Size;
>> + InstructionData->Immediate += Size;
>> + InstructionData->End += Size;
>> +}
>> +
>> +/**
>> + Determine if an instruction address if RIP relative.
>> +
>> + Examine the instruction parsing context to determine if the address offset
>> + is relative to the instruction pointer.
>> +
>> + @param[in] InstructionData Instruction parsing context
>> +
>> + @retval TRUE Instruction addressing is RIP relative
>> + @retval FALSE Instruction addressing is not RIP relative
>> +
>> +**/
>> +STATIC
>> +BOOLEAN
>> +IsRipRelative (
>> + IN SEV_ES_INSTRUCTION_DATA *InstructionData
>> + )
>> +{
>> + SEV_ES_INSTRUCTION_OPCODE_EXT *Ext;
>> +
>> + Ext = &InstructionData->Ext;
>> +
>> + return ((InstructionData->Mode == LongMode64Bit) &&
>> + (Ext->ModRm.Mod == 0) &&
>> + (Ext->ModRm.Rm == 5) &&
>> + (InstructionData->SibPresent == FALSE));
>> +}
>> +
>> +/**
>> + Return the effective address of a memory operand.
>> +
>> + Examine the instruction parsing context to obtain the effective memory
>> + address of a memory operand.
>> +
>> + @param[in] Regs x64 processor context
>> + @param[in] InstructionData Instruction parsing context
>> +
>> + @retval The memory operand effective address
>> +
>> +**/
>> +STATIC
>> +UINTN
>
> (3) Please make the return type UINT64.
>
> It doesn't change behavior at all, as this is X64-only code, but it will
> make our reasoning easier.
>
> (The return value of GetEffectiveMemoryAddress() is assigned to
> Ext->RmData (SEV_ES_INSTRUCTION_OPCODE_EXT.RmData) later, which has type
> UINTN. But this is X64-only code, so that assignment is fine.)
>
>> +GetEffectiveMemoryAddress (
>> + IN EFI_SYSTEM_CONTEXT_X64 *Regs,
>> + IN SEV_ES_INSTRUCTION_DATA *InstructionData
>> + )
>> +{
>> + SEV_ES_INSTRUCTION_OPCODE_EXT *Ext;
>> + INTN EffectiveAddress;
>
> (4) Please make this a UINT64 too.
>
>> +
>> + Ext = &InstructionData->Ext;
>> + EffectiveAddress = 0;
>> +
>> + if (IsRipRelative (InstructionData)) {
>> + /* RIP-relative displacement is a 32-bit signed value */
>
> (5) Please update the comment style.
>
>> + INT32 RipRelative;
>> +
>> + RipRelative = *(INT32 *) InstructionData->Displacement;
>
> OK.
>
>> +
>> + UpdateForDisplacement (InstructionData, 4);
>> + return (UINTN) ((INTN) Regs->Rip + RipRelative);
>
> So, casting "Regs->Rip" (of type UINT64) to INTN is where I start
> fidgeting :) The C standard says in "6.3.1.3 Signed and unsigned
> integers", paragraph 3:
>
> Otherwise, the new type is signed and the value cannot be represented
> in it; either the result is implementation-defined or an
> implementation-defined signal is raised.
>
> Now I *do* realize that our particular C language implementation(s) *do*
> define the behavior here. If Rip is in the upper half of the address
> space, we flip to negative (in two's complement representation), perform
> the signed addition, then flip back to positive (which is *not*
> implementation defined but standard-defined, but will do the right thing
> here).
>
> But that's way too hard to follow if you actually want to pay attention
> to the signed/unsigned conversions. We can do this without relying on
> the implementation-dependent two's complement representation. Here's
> what I suggest:
>
> RipRelative is an INT32, and may be negative. Consider the cast
>
> (UINT64)RipRelative
>
> If RipRelative is non-negative, then the value doesn't change (we'll
> perform a plain increment).
>
> If RipRelative is negative, we'll get the following value from the cast,
> mathematically speaking:
>
> (MAX_UINT64 + 1) - (-RipRelative) [*]
>
> which is just a different way of writing
>
> (MAX_UINT64 + 1) + RipRelative
>
> And the latter comes straight from the C standard, 6.3.1.3p2:
>
> Otherwise, if the new type is unsigned, the value is converted by
> repeatedly adding or subtracting one more than the maximum value that
> can be represented in the new type until the value is in the range of
> the new type.
>
> Now consider what happens when we add [*] to Regs->Rip (which is itself
> a UINT64):
>
> Regs->Rip + ((MAX_UINT64 + 1) - (-RipRelative))
>
> Unpack the outer parens:
>
> Regs->Rip + (MAX_UINT64 + 1) - (-RipRelative)
>
> making for
>
> (Regs->Rip + (MAX_UINT64 + 1)) - (-RipRelative)
>
> The middle term falls away, per "6.2.5 Types", paragraph 9:
>
> [...] A computation involving unsigned operands can never overflow,
> because a result that cannot be represented by the resulting unsigned
> integer type is reduced modulo the number that is one greater than the
> largest value that can be represented by the resulting type.
>
> Therefore we get:
>
> Regs->Rip - (-RipRelative)
>
> which is exactly what we want, for a negative RipRelative.
>
> (6) Thus, the return statement should be:
>
> //
> // Negative displacement is handled by standard UINT64 wrap-around.
> //
> return Regs->Rip + (UINT64)RipRelative;
>
> (Technically, we could even drop the explicit (UINT64) cast --
> RipRelative would be converted automatically to UINT64 --, but we should
> keep the (UINT64) cast for documentation purposes.)
Impressive! I'll make all those changes.
>
>> + }
>> +
>> + switch (Ext->ModRm.Mod) {
>> + case 1:
>> + UpdateForDisplacement (InstructionData, 1);
>> + EffectiveAddress += (INT8) (*(INT8 *) (InstructionData->Displacement));
>
> Considering the patch as-is, the outer (INT8) cast is redundant. But,
> that's not really my point. My point is how we should update this, after
> changing the type of EffectiveAddress to UINT64:
>
> (7) Replace the outer (INT8) cast with (UINT64).
>
> EffectiveAddress += (UINT64) (*(INT8 *) (InstructionData->Displacement));
>
> The reasoning is the same as for (6). If the displacement is negative,
> the value we get on the right hand side is
>
> (MAX_UINT64 + 1) - (-Displacement)
>
> And when we add that to EffectiveAddress (also of type UINT64), the
> (MAX_UINT64 + 1) term falls away, and we get
>
> EffectiveAddress - (-Displacement)
>
> (The UINT64 conversion would happen anyway, per the "usual arithmetic
> conversions", given the new UINT64 type of EffectiveAddress; so the cast
> is mainly for documentation, again.)
>
>> + break;
>> + case 2:
>> + switch (InstructionData->AddrSize) {
>> + case Size16Bits:
>> + UpdateForDisplacement (InstructionData, 2);
>> + EffectiveAddress += (INT16) (*(INT16 *) (InstructionData->Displacement));
>
> (8) Same as (7); please change the outer cast to (UINT64).
>
>> + break;
>> + default:
>> + UpdateForDisplacement (InstructionData, 4);
>> + EffectiveAddress += (INT32) (*(INT32 *) (InstructionData->Displacement));
>
> (9) Same as (7); please change the outer cast to (UINT64).
>
>> + break;
>> + }
>> + break;
>> + }
>> +
>> + if (InstructionData->SibPresent) {
>> + if (Ext->Sib.Index != 4) {
>> + EffectiveAddress += (*GetRegisterPointer (Regs, Ext->Sib.Index) << Ext->Sib.Scale);
>
> In the patch, as-is, we're left-shifting an INT64 that may be negative.
> That's not defined by the standard; see "6.5.7 Bitwise shift operators",
> p4:
>
> [...] If E1 has a signed type and nonnegative value, and E1 * 2^E2 is
> representable in the result type, then that is the resulting value;
> otherwise, the behavior is undefined.
>
> (10) Therefore we should do:
>
> INT64 Displacement;
>
> CopyMem (&Displacement, GetRegisterPointer (Regs, Ext->Sib.Index),
> sizeof Displacement);
> Displacement *= (1 << Ext->Sib.Scale);
> //
> // Negative displacement is handled by standard UINT64 wrap-around.
> //
> EffectiveAddress += (UINT64)Displacement;
>
> Assuming that the instruction we're decoding isn't malformed in the
> first place, this is safe.
>
> (10a) The CopyMem could be replaced with
>
> Displacement = *(INT64 *)GetRegisterPointer (Regs, Ext->Sib.Index);
>
> but the CopyMem() is cleaner. (It is where we *explicitly* rely on two's
> complement representation.)
>
> (10b) "Ext->Sib.Scale" is at most 3 (from DecodeModRm() below -- it
> comes from a 2-bits wide bitfield), so left-shifting value 1 (of type
> INT32) is OK.
>
> (10c) Multiplying a negative INT64 by 1, 2, 4, or 8 is well-defined
> (assuming again that the initial Displacement value is small enough,
> which depends on the original instruction).
>
> If we wanted to be super-safe, we could replace this open-coded
> INT64 multiplication with a call to SafeInt64Mult(), from
> <Library/SafeIntLib.h>, and hang here, if the call fails.
>
> Up to you.
>
> (10d) The final addition follows the same argument as above. We could
> again drop the UINT64 cast (the INT64 operand would be converted to
> UINT64 via the "usual arithmetic conversions"), but we should keep it
> for documentation purposes.
>
>> + }
>> +
>> + if ((Ext->Sib.Base != 5) || Ext->ModRm.Mod) {
>> + EffectiveAddress += *GetRegisterPointer (Regs, Ext->Sib.Base);
>
> This will just continue working, with EffectiveAddress and
> (*GetRegisterPointer()) being both UINT64's. A negative displacement
> will be encoded within the register that (*GetRegisterPointer()) reads
> out.
>
>> + } else {
>> + UpdateForDisplacement (InstructionData, 4);
>> + EffectiveAddress += (INT32) (*(INT32 *) (InstructionData->Displacement));
>
> (11) Same as (9) -- please change the outer (INT32) cast to (UINT64),
> for documentation.
>
>> + }
>> + } else {
>> + EffectiveAddress += *GetRegisterPointer (Regs, Ext->ModRm.Rm);
>
> Continues working fine.
>
>> + }
>> +
>> + return (UINTN) EffectiveAddress;
>
> (12) Please drop the cast.
Ditto here.
>
>> +}
>> +
>> +/**
>> + Decode a ModRM byte.
>> +
>> + Examine the instruction parsing context to decode a ModRM byte and the SIB
>> + byte, if present.
>> +
>> + @param[in] Regs x64 processor context
>> + @param[in, out] InstructionData Instruction parsing context
>> +
>> +**/
>> +STATIC
>> +VOID
>> +DecodeModRm (
>> + IN EFI_SYSTEM_CONTEXT_X64 *Regs,
>> + IN OUT SEV_ES_INSTRUCTION_DATA *InstructionData
>> + )
>> +{
>> + SEV_ES_INSTRUCTION_REX_PREFIX *RexPrefix;
>> + SEV_ES_INSTRUCTION_OPCODE_EXT *Ext;
>> + SEV_ES_INSTRUCTION_MODRM *ModRm;
>> + SEV_ES_INSTRUCTION_SIB *Sib;
>> +
>> + RexPrefix = &InstructionData->RexPrefix;
>> + Ext = &InstructionData->Ext;
>> + ModRm = &InstructionData->ModRm;
>> + Sib = &InstructionData->Sib;
>> +
>> + InstructionData->ModRmPresent = TRUE;
>> + ModRm->Uint8 = *(InstructionData->End);
>> +
>> + InstructionData->Displacement++;
>> + InstructionData->Immediate++;
>> + InstructionData->End++;
>> +
>> + Ext->ModRm.Mod = ModRm->Bits.Mod;
>> + Ext->ModRm.Reg = (RexPrefix->Bits.BitR << 3) | ModRm->Bits.Reg;
>> + Ext->ModRm.Rm = (RexPrefix->Bits.BitB << 3) | ModRm->Bits.Rm;
>> +
>> + Ext->RegData = *GetRegisterPointer (Regs, Ext->ModRm.Reg);
>> +
>> + if (Ext->ModRm.Mod == 3) {
>> + Ext->RmData = *GetRegisterPointer (Regs, Ext->ModRm.Rm);
>
> Both of these UINTN field assignments will continue working, with
> GetRegisterPointer() returning (UINT64*).
>
>> + } else {
>> + if (ModRm->Bits.Rm == 4) {
>> + InstructionData->SibPresent = TRUE;
>> + Sib->Uint8 = *(InstructionData->End);
>> +
>> + InstructionData->Displacement++;
>> + InstructionData->Immediate++;
>> + InstructionData->End++;
>> +
>> + Ext->Sib.Scale = Sib->Bits.Scale;
>> + Ext->Sib.Index = (RexPrefix->Bits.BitX << 3) | Sib->Bits.Index;
>> + Ext->Sib.Base = (RexPrefix->Bits.BitB << 3) | Sib->Bits.Base;
>> + }
>> +
>> + Ext->RmData = GetEffectiveMemoryAddress (Regs, InstructionData);
>> + }
>> +}
>> +
>> /**
>> Decode instruction prefixes.
>>
>> @@ -411,6 +668,181 @@ UnsupportedExit (
>> return Status;
>> }
>>
>> +/**
>> + Handle an MMIO event.
>> +
>> + Use the VMGEXIT instruction to handle either an MMIO read or an MMIO write.
>> +
>> + @param[in, out] Ghcb Pointer to the Guest-Hypervisor Communication
>> + Block
>> + @param[in, out] Regs x64 processor context
>> + @param[in, out] InstructionData Instruction parsing context
>> +
>> + @retval 0 Event handled successfully
>> + @retval Others New exception value to propagate
>> +
>> +**/
>> +STATIC
>> +UINT64
>> +MmioExit (
>> + IN OUT GHCB *Ghcb,
>> + IN OUT EFI_SYSTEM_CONTEXT_X64 *Regs,
>> + IN OUT SEV_ES_INSTRUCTION_DATA *InstructionData
>> + )
>> +{
>> + UINT64 ExitInfo1, ExitInfo2, Status;
>> + UINTN Bytes;
>> + INTN *Register;
>
> (13) Please change this to (UINT64 *).
>
>> + UINT8 OpCode, SignByte;
>> +
>> + Bytes = 0;
>> +
>> + OpCode = *(InstructionData->OpCodes);
>> + if (OpCode == 0x0F) {
>> + OpCode = *(InstructionData->OpCodes + 1);
>> + }
>
> (14) Can you add a comment regarding the 0x0F constant?
I'll create a #define (TWO_BYTE_OPCODE_ESCAPE) that should (hopefully) be
self commenting.
>
>> +
>> + switch (OpCode) {
>> + /* MMIO write */
>
> (15) Please update the comment style.
>
> Also, can we be more explicit about the opcodes, with comments?
Can do.
>
>> + case 0x88:
>> + Bytes = 1;
>
> (16) Please add a "fall through" comment.
For this and remaining comments: Will do.
Thanks!
Tom
>
>> + case 0x89:
>> + DecodeModRm (Regs, InstructionData);
>> + Bytes = (Bytes) ? Bytes
>
> (17) Please use an explicit (Bytes > 0) comparison.
>
>> + : (InstructionData->DataSize == Size16Bits) ? 2
>> + : (InstructionData->DataSize == Size32Bits) ? 4
>> + : (InstructionData->DataSize == Size64Bits) ? 8
>> + : 0;
>
> I struggled for a while to figure out what bothered me about this syntax
> :)
>
> (18) The colons ":" should be at the ends of the lines.
>
> Bytes = ((Bytes > 0) ? Bytes :
> (InstructionData->DataSize == Size16Bits) ? 2 :
> (InstructionData->DataSize == Size32Bits) ? 4 :
> (InstructionData->DataSize == Size64Bits) ? 8 :
> 0);
>
> (I recommend the outermost parens only for supporting the indentation.)
>
>> +
>> + if (InstructionData->Ext.ModRm.Mod == 3) {
>> + /* NPF on two register operands??? */
>
> (19) Please update the comment style.
>
>> + return UnsupportedExit (Ghcb, Regs, InstructionData);
>> + }
>> +
>> + ExitInfo1 = InstructionData->Ext.RmData;
>> + ExitInfo2 = Bytes;
>> + CopyMem (Ghcb->SharedBuffer, &InstructionData->Ext.RegData, Bytes);
>> +
>> + Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
>> + Status = VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, ExitInfo1, ExitInfo2);
>> + if (Status) {
>
> (20) please write (Status > 0) or (Status != 0)
>
>> + return Status;
>> + }
>> + break;
>> +
>> + case 0xC6:
>> + Bytes = 1;
>
> (21) Please add a "fall through" comment.
>
>> + case 0xC7:
>> + DecodeModRm (Regs, InstructionData);
>> + Bytes = (Bytes) ? Bytes
>> + : (InstructionData->DataSize == Size16Bits) ? 2
>> + : (InstructionData->DataSize == Size32Bits) ? 4
>> + : 0;
>
> (22) please see (17) and (18)
>
>> +
>> + InstructionData->ImmediateSize = Bytes;
>> + InstructionData->End += Bytes;
>> +
>> + ExitInfo1 = InstructionData->Ext.RmData;
>> + ExitInfo2 = Bytes;
>> + CopyMem (Ghcb->SharedBuffer, InstructionData->Immediate, Bytes);
>> +
>> + Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
>> + Status = VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, ExitInfo1, ExitInfo2);
>> + if (Status) {
>
> (23) please write (Status > 0) or (Status != 0)
>
>> + return Status;
>> + }
>> + break;
>> +
>> + /* MMIO read */
>
> (24) pls see (15)
>
>> + case 0x8A:
>> + Bytes = 1;
>
> (25) Please add a "fall through" comment.
>
>> + case 0x8B:
>> + DecodeModRm (Regs, InstructionData);
>> + Bytes = (Bytes) ? Bytes
>> + : (InstructionData->DataSize == Size16Bits) ? 2
>> + : (InstructionData->DataSize == Size32Bits) ? 4
>> + : (InstructionData->DataSize == Size64Bits) ? 8
>> + : 0;
>
> (26) please see (17) and (18)
>
>> + if (InstructionData->Ext.ModRm.Mod == 3) {
>> + /* NPF on two register operands??? */
>
> (27) Please update the comment style.
>
>> + return UnsupportedExit (Ghcb, Regs, InstructionData);
>> + }
>> +
>> + ExitInfo1 = InstructionData->Ext.RmData;
>> + ExitInfo2 = Bytes;
>> +
>> + Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
>> + Status = VmgExit (Ghcb, SVM_EXIT_MMIO_READ, ExitInfo1, ExitInfo2);
>> + if (Status) {
>
> (28) please write (Status > 0) or (Status != 0)
>
>> + return Status;
>> + }
>> +
>> + Register = GetRegisterPointer (Regs, InstructionData->Ext.ModRm.Reg);
>> + if (Bytes == 4) {
>> + /* Zero-extend for 32-bit operation */
>
> (29) Please update the comment style.
>
>> + *Register = 0;
>
> Continues working with Register having type (UINT64 *).
>
>> + }
>> + CopyMem (Register, Ghcb->SharedBuffer, Bytes);
>> + break;
>> +
>> + /* MMIO Read w/ zero-extension */
>
> (30) Please see (15)
>
>> + case 0xB6:
>> + Bytes = 1;
>
> (31) Please add a "fall through" comment.
>
>> + case 0xB7:
>> + Bytes = (Bytes) ? Bytes : 2;
>
> (32) Please use an explicit (Bytes > 0) comparison.
>
>> +
>> + ExitInfo1 = InstructionData->Ext.RmData;
>> + ExitInfo2 = Bytes;
>> +
>> + Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
>> + Status = VmgExit (Ghcb, SVM_EXIT_MMIO_READ, ExitInfo1, ExitInfo2);
>> + if (Status) {
>
> (33) please write (Status > 0) or (Status != 0)
>
>> + return Status;
>> + }
>> +
>> + Register = GetRegisterPointer (Regs, InstructionData->Ext.ModRm.Reg);
>> + SetMem (Register, InstructionData->DataSize, 0);
>> + CopyMem (Register, Ghcb->SharedBuffer, Bytes);
>> + break;
>> +
>> + /* MMIO Read w/ sign-extension */
>
> (34) Please see (15)
>
>> + case 0xBE:
>> + Bytes = 1;
>
> (35) Please add a "fall through" comment.
>
>> + case 0xBF:
>> + Bytes = (Bytes) ? Bytes : 2;
>
> (36) Please see (17)
>
>> +
>> + ExitInfo1 = InstructionData->Ext.RmData;
>> + ExitInfo2 = Bytes;
>> +
>> + Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
>> + Status = VmgExit (Ghcb, SVM_EXIT_MMIO_READ, ExitInfo1, ExitInfo2);
>> + if (Status) {
>
> (37) please write (Status > 0) or (Status != 0)
>
>> + return Status;
>> + }
>> +
>> + if (Bytes == 1) {
>> + UINT8 *Data = (UINT8 *) Ghcb->SharedBuffer;
>> +
>> + SignByte = (*Data & 0x80) ? 0xFF : 0x00;
>
> (38) Please use BIT7 (or if there's an even better dedicated macro, then
> that), rather than 0x80.
>
> (39) Usual comment about bitmask used in logical context.
>
>> + } else {
>> + UINT16 *Data = (UINT16 *) Ghcb->SharedBuffer;
>> +
>> + SignByte = (*Data & 0x8000) ? 0xFF : 0x00;
>> + }
>
> (40) Same two comments as (38) and (39).
>
>> +
>> + Register = GetRegisterPointer (Regs, InstructionData->Ext.ModRm.Reg);
>> + SetMem (Register, InstructionData->DataSize, SignByte);
>> + CopyMem (Register, Ghcb->SharedBuffer, Bytes);
>> + break;
>> +
>> + default:
>> + Status = GP_EXCEPTION;
>> + ASSERT (FALSE);
>> + }
>> +
>> + return Status;
>> +}
>> +
>> /**
>> Handle an MSR event.
>>
>> @@ -806,6 +1238,10 @@ VmgExitHandleVc (
>> NaeExit = MsrExit;
>> break;
>>
>> + case SVM_EXIT_NPF:
>> + NaeExit = MmioExit;
>> + break;
>> +
>> default:
>> NaeExit = UnsupportedExit;
>> }
>>
>
> Thanks!
> Laszlo
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#60172): https://edk2.groups.io/g/devel/message/60172
Mute This Topic: https://groups.io/mt/74336571/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list