Re: [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()

["Kinney, Michael D" <michael.d.kinney@intel.com>]

Laszlo,

I agree that the function is better than a macro.

I thought of the alignment issues as well.  CopyMem()
is a good solution.  We could also consider
WriteUnalignedxx() functions in BaseLib.

I was originally thinking this functionality would go
into BaseLib.  But with the use of CopyMem(), we can't
do that.  Maybe we should use WriteUnalignedxx() and
add some ASSERT() checks.

VOID
PatchAssembly (
  VOID    *BufferEnd,
  UINT64  PatchValue,
  UINTN   ValueSize
  )
{
  ASSERT ((UINTN)BufferEnd > ValueSize);
  switch (ValueSize) {
  case 1:
    ASSERT (PatchValue <= MAX_UINT8);
    *((UINT8 *)BufferEnd - 1) = (UINT8)PatchValue;
  case 2:
    ASSERT (PatchValue <= MAX_UINT16);
    WriteUnaligned16 ((UINT16 *)(BufferEnd) - 1, (UINT16)PatchValue));
    break;
  case 4:
    ASSERT (PatchValue <= MAX_UINT32);
    WriteUnaligned32 ((UINT32 *)(BufferEnd) - 1, (UINT32)PatchValue));
    break;
  case 8:
    WriteUnaligned64 ((UINT64 *)(BufferEnd) - 1, PatchValue));
    break;
  default:
    ASSERT (FALSE);
  }
}

Mike

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, January 30, 2018 1:45 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-
> devel-01 <edk2-devel@lists.01.org>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Paolo Bonzini
> <pbonzini@redhat.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
> Subject: Re: [edk2] [PATCH 1/3]
> UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32
> SmmStartup()
> 
> On 01/30/18 21:31, Kinney, Michael D wrote:
> > Laszlo,
> >
> > We have already used this technique in other NASM files
> > to remove DBs.
> 
> OK.
> 
> > Let us know if you have suggestions on how to make the
> > C code that performs the patches easier to read and
> > maintain.
> 
> How about this:
> 
>   VOID
>   PatchAssembly (
>     VOID   *BufferEnd,
>     UINT64 PatchValue,
>     UINTN  ValueSize
>     )
>   {
>     CopyMem (
>       (VOID *)((UINTN)BufferEnd - ValueSize),
>       &PatchValue,
>       ValueSize
>       );
>   }
> 
>   extern UINT8 gAsmSmmCr0;
>   extern UINT8 gAsmSmmCr3;
>   extern UINT8 gAsmSmmCr4;
> 
>   ...
>   {
>     PatchAssembly (&gAsmSmmCr0, AsmReadCr0 (), 4);
>     PatchAssembly (&gAsmSmmCr3, AsmReadCr3 (), 4);
>     PatchAssembly (&gAsmSmmCr4, AsmReadCr4 (), 4);
>     ...
>   }
> 
> (I think it's fine to open-code the last argument as "4",
> rather than
> "sizeof (UINT32)", because for patching, we must have
> intimate knowledge
> of the instruction anyway.)
> 
> To me, this is easier to read, because:
> 
> - there are no complex casts in the "business logic"
> - the size is spelled out once per patching
> - the function name and the variable names make it clear
> we are patching
>   separately compiled assembly code that was linked into
> the same
>   module.
> 
> What do you think?
> 
> Thanks!
> Laszlo
> 
> >> -----Original Message-----
> >> From: edk2-devel [mailto:edk2-devel-
> bounces@lists.01.org]
> >> On Behalf Of Laszlo Ersek
> >> Sent: Tuesday, January 30, 2018 10:17 AM
> >> To: Kinney, Michael D <michael.d.kinney@intel.com>;
> edk2-
> >> devel-01 <edk2-devel@lists.01.org>
> >> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Paolo Bonzini
> >> <pbonzini@redhat.com>; Yao, Jiewen
> >> <jiewen.yao@intel.com>; Dong, Eric
> <eric.dong@intel.com>
> >> Subject: Re: [edk2] [PATCH 1/3]
> >> UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32
> >> SmmStartup()
> >>
> >> On 01/30/18 18:22, Kinney, Michael D wrote:
> >>> Laszlo,
> >>>
> >>> The DBs can be removed if the label is moved after
> >>> the instruction and the patch is done to the label
> >>> minus the size of the patch value.
> >>
> >> Indeed I haven't thought of this.
> >>
> >> If I understand correctly, it means
> >>
> >>   extern UINT8 gSmmCr0;
> >>
> >>   *(UINT32*)(&gSmmCr0 - sizeof (UINT32)) =
> >> (UINT32)AsmReadCr0 ();
> >>
> >> TBH, the DB feels less ugly to me than this :)
> >>
> >> Still, if you think it would be an acceptable price to
> >> pay for removing
> >> the remaining DBs, I can respin.
> >>
> >> Thanks
> >> Laszlo
> >>
> >>>> -----Original Message-----
> >>>> From: edk2-devel [mailto:edk2-devel-
> >> bounces@lists.01.org]
> >>>> On Behalf Of Laszlo Ersek
> >>>> Sent: Tuesday, January 30, 2018 7:34 AM
> >>>> To: edk2-devel-01 <edk2-devel@lists.01.org>
> >>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yao, Jiewen
> >>>> <jiewen.yao@intel.com>; Dong, Eric
> >> <eric.dong@intel.com>;
> >>>> Paolo Bonzini <pbonzini@redhat.com>
> >>>> Subject: [edk2] [PATCH 1/3]
> UefiCpuPkg/PiSmmCpuDxeSmm:
> >>>> update comments in IA32 SmmStartup()
> >>>>
> >>>> The gSmmCr3, gSmmCr4, gSmmCr0 and gSmmJmpAddr global
> >>>> variables  are used
> >>>> for patching assembly instructions, thus we can
> never
> >>>> remove the DB
> >>>> encodings for those instructions. At least we should
> >> add
> >>>> the intended
> >>>> meanings in comments.
> >>>>
> >>>> This patch only changes comments.
> >>>>
> >>>> Cc: Eric Dong <eric.dong@intel.com>
> >>>> Cc: Jian J Wang <jian.j.wang@intel.com>
> >>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
> >>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
> >>>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> >>>> Contributed-under: TianoCore Contribution Agreement
> >> 1.1
> >>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> >>>> ---
> >>>>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 8
> ++++-
> >> ---
> >>>>  1 file changed, 4 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git
> >> a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> >>>> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> >>>> index e96dd8d2392a..08534dba64b7 100644
> >>>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> >>>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> >>>> @@ -44,34 +44,34 @@ global ASM_PFX(SmmStartup)
> >>>>  ASM_PFX(SmmStartup):
> >>>>      DB      0x66
> >>>>      mov     eax, 0x80000001             ; read
> >>>> capability
> >>>>      cpuid
> >>>>      DB      0x66
> >>>>      mov     ebx, edx                    ; rdmsr
> will
> >>>> change edx. keep it in ebx.
> >>>> -    DB      0x66, 0xb8
> >>>> +    DB      0x66, 0xb8                  ; mov eax,
> >> imm32
> >>>>  ASM_PFX(gSmmCr3): DD 0
> >>>>      mov     cr3, eax
> >>>>      DB      0x67, 0x66
> >>>>      lgdt    [cs:ebp + (ASM_PFX(gcSmiInitGdtr) -
> >>>> ASM_PFX(SmmStartup))]
> >>>> -    DB      0x66, 0xb8
> >>>> +    DB      0x66, 0xb8                  ; mov eax,
> >> imm32
> >>>>  ASM_PFX(gSmmCr4): DD 0
> >>>>      mov     cr4, eax
> >>>>      DB      0x66
> >>>>      mov     ecx, 0xc0000080             ; IA32_EFER
> >> MSR
> >>>>      rdmsr
> >>>>      DB      0x66
> >>>>      test    ebx, BIT20                  ; check NXE
> >>>> capability
> >>>>      jz      .1
> >>>>      or      ah, BIT3                    ; set NXE
> bit
> >>>>      wrmsr
> >>>>  .1:
> >>>> -    DB      0x66, 0xb8
> >>>> +    DB      0x66, 0xb8                  ; mov eax,
> >> imm32
> >>>>  ASM_PFX(gSmmCr0): DD 0
> >>>>      DB      0xbf, PROTECT_MODE_DS, 0    ; mov di,
> >>>> PROTECT_MODE_DS
> >>>>      mov     cr0, eax
> >>>> -    DB      0x66, 0xea                   ; jmp far
> >>>> [ptr48]
> >>>> +    DB      0x66, 0xea                  ; jmp far
> >>>> [ptr48]
> >>>>  ASM_PFX(gSmmJmpAddr):
> >>>>      DD      @32bit
> >>>>      DW      PROTECT_MODE_CS
> >>>>  @32bit:
> >>>>      mov     ds, edi
> >>>>      mov     es, edi
> >>>> --
> >>>> 2.14.1.3.gb7cf6e02401b
> >>>>
> >>>>
> >>>> _______________________________________________
> >>>> edk2-devel mailing list
> >>>> edk2-devel@lists.01.org
> >>>> https://lists.01.org/mailman/listinfo/edk2-devel
> >>
> >> _______________________________________________
> >> edk2-devel mailing list
> >> edk2-devel@lists.01.org
> >> https://lists.01.org/mailman/listinfo/edk2-devel