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

[Laszlo Ersek <lersek@redhat.com>]

On 02/02/18 14:28, Leif Lindholm wrote:
> On Fri, Feb 02, 2018 at 10:06:07AM +0000, 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
> 
> Not just the same cache line. Prefetching can happen whenever, for
> whatever reason.
> 
>> (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.
> 
> I also don't see a specific use for this on ARM* at the moment. But if
> this is going to become more widespread, it would be useful to
> introduce a higher-level layer with more portable semantics (I don't
> know RISC-V, but could imagine they require similar).
> However, at that point, we would probably want something
> buffer-oriented rather than instruction-oriented, since we'd like to
> keep the overhead down if writing more than one register's worth.

I'll CC you and Ard on the BaseLib patches; hopefully
PatchInstructionX86() will be possible to reimplement in terms of the
more generic, buffer-oriented API, once we introduce that.

Thanks!
Laszlo