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:[email protected]]
>> Sent: Monday, February 5, 2018 2:28 AM
>> To: Kinney, Michael D <[email protected]>; edk2-
>> devel-01 <[email protected]>
>> Cc: Ard Biesheuvel <[email protected]>; Dong,
>> Eric <[email protected]>; Yao, Jiewen
>> <[email protected]>; Leif Lindholm
>> <[email protected]>; Gao, Liming
>> <[email protected]>; Ni, Ruiyu <[email protected]>
>> 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
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
