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

["Kinney, Michael D" <michael.d.kinney@intel.com>]

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:lersek@redhat.com]
> Sent: Monday, February 5, 2018 11:23 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/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