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

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

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?

Mike

> -----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