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

[Laszlo Ersek <lersek@redhat.com>]

On 02/02/18 11:06, Ard Biesheuvel wrote:
> On 31 January 2018 at 10:40, Laszlo Ersek <lersek@redhat.com> wrote:
>> 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).
>>
> 
> Code patching on ARM/AARCH64 has some hoops to jump through, i.e.,
> clean the D-cache to the point of unification, invalidate the I-cache,
> probably some barriers in case the patching function happened to end
> up in the same cache line as the patchee (which may not be a concern
> for this specific use case, but it does need to be taken into account
> if this is turned into a patch-any-assembly-anywhere function)
> 
> So if the PatchAssembly() prototype does end up in a generic library
> class, we'd have to provide ARM and AARCH64 specific implementations
> anyway, and given that I don't see any use for this on ARM/AARCH64 in
> the first place, I think this should belong in an IA32/X64 specific
> package.

Thank you for the response!

For now I'm going to post the series with the function introduced as
PatchInstructionX86() to BaseLib, visibly only to IA32 and X64 edk2
platforms. If a better place than BaseLib looks necessary, I can move it
according to review comments.

Thanks!
Laszlo

> 
>> - 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
>>>
>>