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

[Laszlo Ersek <lersek@redhat.com>]

On 01/30/18 23:25, Kinney, Michael D wrote:
> 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.

IMO, the WriteUnalignedxx functions are a bit pointless in the exact
form they are declared (this was discussed earlier esp. with regard to
aarch64). The functions take pointers to objects that already have the
target type, such as

UINT32
EFIAPI
WriteUnaligned32 (
  OUT UINT32                    *Buffer,
  IN  UINT32                    Value
  )

Here the type of Buffer should be (VOID *), not (UINT32 *). Otherwise,
the undefined behavior (due to mis-alignment) surfaces as soon as the
function is called with an unaligned pointer (i.e. before the target
area is actually written).

> I was originally thinking this functionality would go
> into BaseLib.  But with the use of CopyMem(), we can't
> do that.

Can we put it in BaseMemoryLib instead (which is where CopyMem() is
from)? That library class is still low-level enough. And, while I count
9 library instances, PatchAssembly() is not a large function, we could
tolerate adding it to all 9 instances, identically.

Let me also ask the opposite question: should we perhaps make the
PatchAssembly() API *less* abstract? (Also suggested by your naming of
the macro, PATCH_X86_ASM.) If the instruction encoding on e.g. AARCH64
doesn't lend itself to such patching (= expressed through the address
right after the instruction), then even BaseMemoryLib may be too generic
for the API.

> 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);
>   }
> }

In my opinion:

- If Ard and Leif say that PatchAssembly() API makes sense for AARCH64,
  then I think we can go with the above generic implementation (for
  BaseLib).

- If Ard and Leif say the API is only useful on x86, then I suggest that
  we implement the API separately for all arches (still in BaseLib):

  - On x86, we should simply open-code the unaligned accesses (like you
    originall suggested). The pointer arithmetic will look a bit wild,
    but it's safely hidden behind a BaseLib API, so client code will
    look nice.

  - On all other arches, we should implement the function with
    ASSERT(FALSE).

Thanks!
Laszlo

> 
> 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
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>