Re: [PATCH 00/14] rid PiSmmCpuDxeSmm of DB-encoded instructions

[Laszlo Ersek <lersek@redhat.com>]

On 02/05/18 19:22, Kinney, Michael D wrote:
> Laszlo,
> 
> Let's see if we can close on the timeline for 
> the .S/.asm RFC this week.
> 
> I am concerned about making them UINT8 from C code
> because future maintainer may think that the patch 
> value type is UINT8.
> 
> Labels in assembly that are defined to be a function
> that is callable from C code does not have a storage
> type.  Why can't we make these labels the same way?

To my understanding, the labels in the NASM source code for functions
and variables look the same; the actual declaration only comes from the
C code.

(Assuming we declare a NASM label as a function in the C source, nothing
in the toolchain enforces an actual match between caller and callee; it
is possible to call the function (from C) through a declaration that
doesn't match the actual assembly implementation. IOW it's up to us to
avoid such bugs.)

If I understand correctly, you are suggesting that we take a label from
the NASM source that stands right after an instruction to patch, and we
declare it as a function in the C source. (With what prototype though?
The label does not actually introduce a function definition in the
assembly code; it would make no sense to call it.) Then, for the
patching, I presume your suggestion is to convert the address of the
function to UINTN, perform the subtraction, etc. Something like:

  typedef VOID (X86_ASSEMBLY_LABEL) (VOID);

(This is not a pointer-to-function type, but a function type.)

A declaration using the typedef would be

  extern X86_ASSEMBLY_LABEL gPatchCr3;

(This declares an extern function, not a pointer to a function.)

The patching function would take a pointer to a function:

  VOID
  EFIAPI
  PatchInstructionX86 (
    OUT X86_ASSEMBLY_LABEL *InstructionEnd,
    IN  UINT64             PatchValue,
    IN  UINTN              ValueSize
    );

and the implementation would have to do e.g.

  WriteUnaligned32 (
    (UINT32 *)(UINTN)InstructionEnd - 1,
    (UINT32)PatchValue
    );

It would be called like

  PatchInstructionX86 (&gPatchCr3, Value, 4);


But, what does this buy us in comparison to just:

  typedef UINT8 X86_ASSEMBLY_LABEL;

?

If you worry that a future maintainer misunderstands the UINT8, then we
can as well hide the UINT8 behind a typedef; X86_ASSEMBLY_LABEL doesn't
have to be a function type for the hiding. (Conversely, when using a
function type as underlying type, I worry that a future maintainer might
be tempted to call them :) )

Thanks,
Laszlo

>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Monday, February 5, 2018 2:28 AM
>> To: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-
>> devel-01 <edk2-devel@lists.01.org>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Dong,
>> Eric <eric.dong@intel.com>; Yao, Jiewen
>> <jiewen.yao@intel.com>; Leif Lindholm
>> <leif.lindholm@linaro.org>; Gao, Liming
>> <liming.gao@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
>> Subject: Re: [PATCH 00/14] rid PiSmmCpuDxeSmm of DB-
>> encoded instructions
>>
>> On 02/03/18 01:45, Kinney, Michael D wrote:
>>> Laszlo,
>>>
>>> Thanks for all the work on this series and the very
>>> detailed commit messages.
>>>
>>> Liming's email on removing the .S and .asm files is an
>>> RFC.  We need to see this RFC approved before we can
>>> commit changes to remove .S and .asm files.  This
>> should
>>> be a separate activity.
>>
>> Sure, I can drop that patch, but then the PiSmmCpuDxeSmm
>> changes in the
>> other patches will divert the NASM files from the .S and
>> .asm files. Is
>> that (temporary) non-uniformity better than removing the
>> .S and .asm files?
>>
>>> One odd thing I see in this series is that the
>> instruction
>>> patch label in the .nasm file is just a label and does
>> not
>>> have any storage associated with it.
>>
>> No, this is not correct; the storage that is associated
>> with each of
>> these "patch labels" is the one byte (UINT8) directly
>> following the
>> label -- whatever that byte might be. It is generally
>> part of a totally
>> unrelated instruction.
>>
>> In case we had to patch an immediate operand that
>> happened to comprise
>> the very last byte(s) of a NASM source file, *then* we'd
>> have to add one
>> dummy DB at the end, just so there was something that the
>> label directly
>> refered to.
>>
>> This is why UINT8 is a good type here, because it
>> requires us to add the
>> least amount of padding.
>>
>>> But in the C code
>>> the type UINT8 is used with the label which implies
>> some
>>> storage.  Can we make the globals in C code be a
>> pointer
>>> (maybe VOID *) instead of UINT8?
>>
>> I don't think so. For building the addresses, we rely on
>> the linker, and
>> the linker needs definitions (allocations) of objects.
>> Your above
>> observation is correct (i.e. that storage is required),
>> my addition to
>> that is that storage is *already* allocated (one UINT8
>> per patch label /
>> symbol).
>>
>> Thanks!
>> Laszlo