Laszlo, I do like this typedef idea.
typedef VOID (X86_ASSEMBLY_LABEL) (VOID); Maybe change the name so it is clearer that this should never be used in a call. A comment block about the typedef can also clarify the expected usage. typedef VOID (X86_ASSEMBLY_PATCH_LABEL) (VOID); Thanks, Mike > -----Original Message----- > From: Laszlo Ersek [mailto:[email protected]] > Sent: Monday, February 5, 2018 11:23 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/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
