Re: [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()

[Leif Lindholm <leif.lindholm@linaro.org>]

On Fri, Feb 02, 2018 at 10:06:07AM +0000, Ard Biesheuvel wrote:
> On 31 January 2018 at 10:40, Laszlo Ersek <lersek@redhat.com> wrote:
> > On 01/30/18 23:25, Kinney, Michael D wrote:
> >> Laszlo,
> >>
> >> I agree that the function is better than a macro.
> >>
> >> I thought of the alignment issues as well.  CopyMem()
> >> is a good solution.  We could also consider
> >> WriteUnalignedxx() functions in BaseLib.
> >
> > IMO, the WriteUnalignedxx functions are a bit pointless in the exact
> > form they are declared (this was discussed earlier esp. with regard to
> > aarch64). The functions take pointers to objects that already have the
> > target type, such as
> >
> > UINT32
> > EFIAPI
> > WriteUnaligned32 (
> >   OUT UINT32                    *Buffer,
> >   IN  UINT32                    Value
> >   )
> >
> > Here the type of Buffer should be (VOID *), not (UINT32 *). Otherwise,
> > the undefined behavior (due to mis-alignment) surfaces as soon as the
> > function is called with an unaligned pointer (i.e. before the target
> > area is actually written).
> >
> >> I was originally thinking this functionality would go
> >> into BaseLib.  But with the use of CopyMem(), we can't
> >> do that.
> >
> > Can we put it in BaseMemoryLib instead (which is where CopyMem() is
> > from)? That library class is still low-level enough. And, while I count
> > 9 library instances, PatchAssembly() is not a large function, we could
> > tolerate adding it to all 9 instances, identically.
> >
> > Let me also ask the opposite question: should we perhaps make the
> > PatchAssembly() API *less* abstract? (Also suggested by your naming of
> > the macro, PATCH_X86_ASM.) If the instruction encoding on e.g. AARCH64
> > doesn't lend itself to such patching (= expressed through the address
> > right after the instruction), then even BaseMemoryLib may be too generic
> > for the API.
> >
> >> Maybe we should use WriteUnalignedxx() and
> >> add some ASSERT() checks.
> >>
> >> VOID
> >> PatchAssembly (
> >>   VOID    *BufferEnd,
> >>   UINT64  PatchValue,
> >>   UINTN   ValueSize
> >>   )
> >> {
> >>   ASSERT ((UINTN)BufferEnd > ValueSize);
> >>   switch (ValueSize) {
> >>   case 1:
> >>     ASSERT (PatchValue <= MAX_UINT8);
> >>     *((UINT8 *)BufferEnd - 1) = (UINT8)PatchValue;
> >>   case 2:
> >>     ASSERT (PatchValue <= MAX_UINT16);
> >>     WriteUnaligned16 ((UINT16 *)(BufferEnd) - 1, (UINT16)PatchValue));
> >>     break;
> >>   case 4:
> >>     ASSERT (PatchValue <= MAX_UINT32);
> >>     WriteUnaligned32 ((UINT32 *)(BufferEnd) - 1, (UINT32)PatchValue));
> >>     break;
> >>   case 8:
> >>     WriteUnaligned64 ((UINT64 *)(BufferEnd) - 1, PatchValue));
> >>     break;
> >>   default:
> >>     ASSERT (FALSE);
> >>   }
> >> }
> >
> > In my opinion:
> >
> > - If Ard and Leif say that PatchAssembly() API makes sense for AARCH64,
> >   then I think we can go with the above generic implementation (for
> >   BaseLib).
> >
> 
> Code patching on ARM/AARCH64 has some hoops to jump through, i.e.,
> clean the D-cache to the point of unification, invalidate the I-cache,
> probably some barriers in case the patching function happened to end
> up in the same cache line as the patchee

Not just the same cache line. Prefetching can happen whenever, for
whatever reason.

> (which may not be a concern
> for this specific use case, but it does need to be taken into account
> if this is turned into a patch-any-assembly-anywhere function)
> 
> So if the PatchAssembly() prototype does end up in a generic library
> class, we'd have to provide ARM and AARCH64 specific implementations
> anyway, and given that I don't see any use for this on ARM/AARCH64 in
> the first place, I think this should belong in an IA32/X64 specific
> package.

I also don't see a specific use for this on ARM* at the moment. But if
this is going to become more widespread, it would be useful to
introduce a higher-level layer with more portable semantics (I don't
know RISC-V, but could imagine they require similar).
However, at that point, we would probably want something
buffer-oriented rather than instruction-oriented, since we'd like to
keep the overhead down if writing more than one register's worth.

/
    Leif