Re: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
On 02/02/18 14:28, Leif Lindholm wrote:
> On Fri, Feb 02, 2018 at 10:06:07AM +, Ard Biesheuvel wrote:
>> On 31 January 2018 at 10:40, Laszlo Ersek 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 UINT32Value
>>> )
>>>
>>> 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.
I'll CC you and Ard on the BaseLib patches; hopefully
PatchInstructionX86() will be possible to reimplement in terms of the
more generic, buffer-oriented API, once we introduce that.
Thanks!
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
On Fri, Feb 02, 2018 at 10:06:07AM +, Ard Biesheuvel wrote:
> On 31 January 2018 at 10:40, Laszlo Ersek 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 UINT32Value
> > )
> >
> > 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
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
On 02/02/18 11:06, Ard Biesheuvel wrote:
> On 31 January 2018 at 10:40, Laszlo Ersek 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 UINT32Value
>> )
>>
>> 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 (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.
Thank you for the response!
For now I'm going to post the series with the function introduced as
PatchInstructionX86() to BaseLib, visibly only to IA32 and X64 edk2
platforms. If a better place than BaseLib looks necessary, I can move it
according to review comments.
Thanks!
Laszlo
>
>> - If Ard and Leif say the API is only useful on x86, then I suggest that
>> we implement the API separately for all arches (still in BaseLib):
>>
>> - On x86, we should simply open-code the unaligned accesses (like you
>> originall suggested). The pointer arithmetic will look a bit wild,
>> but it's safely hidden behind a BaseLib API, so client code will
>> look nice.
>>
>> - On all other arches, we should implem
Re: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
On 31 January 2018 at 10:40, Laszlo Ersek 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 UINT32Value
> )
>
> 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 (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.
> - If Ard and Leif say the API is only useful on x86, then I suggest that
> we implement the API separately for all arches (still in BaseLib):
>
> - On x86, we should simply open-code the unaligned accesses (like you
> originall suggested). The pointer arithmetic will look a bit wild,
> but it's safely hidden behind a BaseLib API, so client code will
> look nice.
>
> - On all other arches, we should implement the function with
> ASSERT(FALSE).
>
> Thanks!
> Laszlo
>
>>
>> Mike
>>
>>> -Original Message-
>>> From: Laszlo Ersek [mailto:ler...@redhat.com]
>>> Sent: Tuesday, January 30, 2018 1:45 PM
>>> To: Kinney, Michael D ; edk2-
>>> devel-01
>>> Cc: Ni, Ruiyu ; Paolo Bonzini
>>> ; Yao, Jiewen
>>> ; Dong, Eric
>>> Subject: Re: [edk2] [PATCH 1/3]
>>> UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32
>>> SmmStartup()
>>>
>>> On 01/30/18 21:31, Kinney, Michael D wrote:
>>>> Laszlo,
>>>>
>>>> We have already used this technique in other NASM files
>
Re: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
On 01/31/18 23:11, Kinney, Michael D wrote:
> Laszlo,
>
> I agree the Unaligned functions have issues.
> We should see if we could change the param type.
> It should be a backwards compatible change to
> go from a type specific pointer to VOID *. But
> need to check with all supported compilers.
>
> We can have arch specific functions and macros.
> There are many in BaseLib.h. This way, if a macro
> or function is used by an unsupported arch, the
> build will fail. I also like some of the name
> change suggestions. Maybe PatchInstructionX86()
> and change the parameter name to InstructionEnd.
>
> BaseLib.h
> ==
> #if defined (MDE_CPU_IA32) || defined (MDE_CPU_X64)
>
> VOID
> EFIAPI
> PatchInstructionX86 (
> VOID*InstructionEnd,
> UINT64 PatchValue,
> UINTN ValueSize
> );
>
> #endif
>
> BaseLib Instance
> ==
> VOID
> EFIAPI
> PatchInstructionX86 (
> VOID*InstructionEnd,
> UINT64 PatchValue,
> UINTN ValueSize
> )
> {
> ASSERT ((UINTN)InstructionEnd > ValueSize);
> switch (ValueSize) {
> case 1:
> ASSERT (PatchValue <= MAX_UINT8);
> *((UINT8 *)InstructionEnd - 1) = (UINT8)PatchValue;
> case 2:
> ASSERT (PatchValue <= MAX_UINT16);
> WriteUnaligned16 ((UINT16 *)(InstructionEnd) - 1, (UINT16)PatchValue));
> break;
> case 4:
> ASSERT (PatchValue <= MAX_UINT32);
> WriteUnaligned32 ((UINT32 *)(InstructionEnd) - 1, (UINT32)PatchValue));
> break;
> case 8:
> WriteUnaligned64 ((UINT64 *)(InstructionEnd) - 1, PatchValue));
> break;
> default:
> ASSERT (FALSE);
> }
> }
I managed to remove all instruction DBs from PiSmmCpuDxeSmm. I plan to
post the patches this week or the next.
Thanks!
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
Laszlo,
I agree the Unaligned functions have issues.
We should see if we could change the param type.
It should be a backwards compatible change to
go from a type specific pointer to VOID *. But
need to check with all supported compilers.
We can have arch specific functions and macros.
There are many in BaseLib.h. This way, if a macro
or function is used by an unsupported arch, the
build will fail. I also like some of the name
change suggestions. Maybe PatchInstructionX86()
and change the parameter name to InstructionEnd.
BaseLib.h
==
#if defined (MDE_CPU_IA32) || defined (MDE_CPU_X64)
VOID
EFIAPI
PatchInstructionX86 (
VOID*InstructionEnd,
UINT64 PatchValue,
UINTN ValueSize
);
#endif
BaseLib Instance
==
VOID
EFIAPI
PatchInstructionX86 (
VOID*InstructionEnd,
UINT64 PatchValue,
UINTN ValueSize
)
{
ASSERT ((UINTN)InstructionEnd > ValueSize);
switch (ValueSize) {
case 1:
ASSERT (PatchValue <= MAX_UINT8);
*((UINT8 *)InstructionEnd - 1) = (UINT8)PatchValue;
case 2:
ASSERT (PatchValue <= MAX_UINT16);
WriteUnaligned16 ((UINT16 *)(InstructionEnd) - 1, (UINT16)PatchValue));
break;
case 4:
ASSERT (PatchValue <= MAX_UINT32);
WriteUnaligned32 ((UINT32 *)(InstructionEnd) - 1, (UINT32)PatchValue));
break;
case 8:
WriteUnaligned64 ((UINT64 *)(InstructionEnd) - 1, PatchValue));
break;
default:
ASSERT (FALSE);
}
}
Mike
> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Wednesday, January 31, 2018 2:40 AM
> To: Kinney, Michael D ; edk2-
> devel-01
> Cc: Ni, Ruiyu ; Paolo Bonzini
> ; Yao, Jiewen
> ; Dong, Eric ;
> Ard Biesheuvel ; Leif Lindholm
> (Linaro address)
> Subject: Re: [edk2] [PATCH 1/3]
> UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32
> SmmStartup()
>
> 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 UINT32Value
> )
>
> 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
Re: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
On 01/31/18 06:54, Ni, Ruiyu wrote: > Laszlo, Mike, > Considering this patch doesn't make the code worse, > actually improved a tiny bit, can we firstly check in the three patches? I agree; the PatchAssembly() discussion is taking quite a bit of thought, meanwhile IA32 SMM is broken on KVM -- and even on QEMU! (Paolo helped me test that, and yes, QEMU/TCG is affected the exact same way.) I will go ahead and push the patches with the reviews from Paolo and Ray, with the following small modifications: * In the commit message of the first patch, I'll change we can never remove to we can't yet remove in the commit message. * In the third patch, I will change the commit message from SMM emulation under KVM to SMM emulation under both KVM and QEMU (TCG) * In the third patch, I will update the code comments as requested by Ray. > Reviewed-by: Ruiyu Ni Thanks! Laszlo ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
On 01/31/18 06:44, Ni, Ruiyu wrote:
> Mike, Laszlo,
> Does the patch only apply to the operand?
> If so, PatchAssembly looks too general.
> How about the name like PatchAssemblyOperand?
Originally I meant to call the function "PatchInstruction", but then I
thought it could be used for patching any other artifacts too, in object
code that comes from NASM.
I'm also fine calling it PatchAssemblyOperand, or even
PatchInstructionTrailingOperand :)
Thanks
Laszlo
> On 1/31/2018 6:25 AM, 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.
>>
>> I was originally thinking this functionality would go
>> into BaseLib. But with the use of CopyMem(), we can't
>> do that. 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);
>> }
>> }
>>
>> Mike
>>
>>> -Original Message-
>>> From: Laszlo Ersek [mailto:ler...@redhat.com]
>>> Sent: Tuesday, January 30, 2018 1:45 PM
>>> To: Kinney, Michael D ; edk2-
>>> devel-01
>>> Cc: Ni, Ruiyu ; Paolo Bonzini
>>> ; Yao, Jiewen
>>> ; Dong, Eric
>>> Subject: Re: [edk2] [PATCH 1/3]
>>> UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32
>>> SmmStartup()
>>>
>>> On 01/30/18 21:31, Kinney, Michael D wrote:
>>>> Laszlo,
>>>>
>>>> We have already used this technique in other NASM files
>>>> to remove DBs.
>>>
>>> OK.
>>>
>>>> Let us know if you have suggestions on how to make the
>>>> C code that performs the patches easier to read and
>>>> maintain.
>>>
>>> How about this:
>>>
>>> VOID
>>> PatchAssembly (
>>> VOID *BufferEnd,
>>> UINT64 PatchValue,
>>> UINTN ValueSize
>>> )
>>> {
>>> CopyMem (
>>> (VOID *)((UINTN)BufferEnd - ValueSize),
>>> &PatchValue,
>>> ValueSize
>>> );
>>> }
>>>
>>> extern UINT8 gAsmSmmCr0;
>>> extern UINT8 gAsmSmmCr3;
>>> extern UINT8 gAsmSmmCr4;
>>>
>>> ...
>>> {
>>> PatchAssembly (&gAsmSmmCr0, AsmReadCr0 (), 4);
>>> PatchAssembly (&gAsmSmmCr3, AsmReadCr3 (), 4);
>>> PatchAssembly (&gAsmSmmCr4, AsmReadCr4 (), 4);
>>> ...
>>> }
>>>
>>> (I think it's fine to open-code the last argument as "4",
>>> rather than
>>> "sizeof (UINT32)", because for patching, we must have
>>> intimate knowledge
>>> of the instruction anyway.)
>>>
>>> To me, this is easier to read, because:
>>>
>>> - there are no complex casts in the "business logic"
>>> - the size is spelled out once per patching
>>> - the function name and the variable names make it clear
>>> we are patching
>>> separately compiled assembly code that was linked into
>>> the same
>>> module.
>>>
>>> What do you think?
>>>
>>> Thanks!
>>> Laszlo
>>>
>>>>> -Original Message-
>>>>> From: edk2-devel [mailto:edk2-devel-
>>> boun...@lists.01.org]
>>>>> On Behalf Of Laszlo Ersek
>>>>> Sent: Tuesday, January 30, 2018 10:17 AM
>>>>> To: Kinney, Michael D ;
>>> edk2-
>>>>> devel-01
>>>>> Cc: Ni, Ruiyu ; Paolo Bonzini
>>>>&g
Re: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
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 UINT32Value
)
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).
- If Ard and Leif say the API is only useful on x86, then I suggest that
we implement the API separately for all arches (still in BaseLib):
- On x86, we should simply open-code the unaligned accesses (like you
originall suggested). The pointer arithmetic will look a bit wild,
but it's safely hidden behind a BaseLib API, so client code will
look nice.
- On all other arches, we should implement the function with
ASSERT(FALSE).
Thanks!
Laszlo
>
> Mike
>
>> -Original Message-
>> From: Laszlo Ersek [mailto:ler...@redhat.com]
>> Sent: Tuesday, January 30, 2018 1:45 PM
>> To: Kinney, Michael D ; edk2-
>> devel-01
>> Cc: Ni, Ruiyu ; Paolo Bonzini
>> ; Yao, Jiewen
>> ; Dong, Eric
>> Subject: Re: [edk2] [PATCH 1/3]
>> UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32
>> SmmStartup()
>>
>> On 01/30/18 21:31, Kinney, Michael D wrote:
>>> Laszlo,
>>>
>>> We have already used this technique in other NASM files
>>> to remove DBs.
>>
>> OK.
>>
>>> Let us know if you have suggestions on how to make the
>>> C code that performs the patches easier to read and
>>> maintain.
>>
>> How about this:
>>
>> VOID
>> PatchAssembly (
>> VOID *BufferEnd,
>> UINT64 PatchValue,
>> UINTN ValueSize
>> )
>> {
>> CopyMem (
>> (VOID *)((UINTN)BufferEnd - ValueSize),
>> &PatchValue,
>> ValueSize
>> );
>> }
>>
>> extern UINT8 gAsmSmmCr0;
>> extern UINT8 gAsmSmmCr3;
>> extern UINT8 gAsmSmmCr4;
>>
>> ...
>> {
>> PatchAssembly (&gAsmSmmCr0, AsmReadCr0 (), 4);
>> PatchAssembly (&gAsmSmmCr3, AsmReadCr3 (), 4);
>> PatchAssembly (&gAsmSmmCr4, AsmReadCr4 (), 4);
>> ...
>> }
>>
>> (I think it's fine to open-code the last argument as "4",
>> rather than
>> "sizeof (UINT32)", because for patching, we must have
>> intimate knowledge
>> of the instruction anyway.)
>>
>> To me, this is ea
Re: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
On 1/31/2018 1:44 PM, Ni, Ruiyu wrote:
Mike, Laszlo,
Does the patch only apply to the operand?
If so, PatchAssembly looks too general.
How about the name like PatchAssemblyOperand?
On 1/31/2018 6:25 AM, 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.
I was originally thinking this functionality would go
into BaseLib. But with the use of CopyMem(), we can't
do that. 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);
}
}
Mike
-Original Message-
From: Laszlo Ersek [mailto:ler...@redhat.com]
Sent: Tuesday, January 30, 2018 1:45 PM
To: Kinney, Michael D ; edk2-
devel-01
Cc: Ni, Ruiyu ; Paolo Bonzini
; Yao, Jiewen
; Dong, Eric
Subject: Re: [edk2] [PATCH 1/3]
UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32
SmmStartup()
On 01/30/18 21:31, Kinney, Michael D wrote:
Laszlo,
We have already used this technique in other NASM files
to remove DBs.
OK.
Let us know if you have suggestions on how to make the
C code that performs the patches easier to read and
maintain.
How about this:
VOID
PatchAssembly (
VOID *BufferEnd,
UINT64 PatchValue,
UINTN ValueSize
)
{
CopyMem (
(VOID *)((UINTN)BufferEnd - ValueSize),
&PatchValue,
ValueSize
);
}
extern UINT8 gAsmSmmCr0;
extern UINT8 gAsmSmmCr3;
extern UINT8 gAsmSmmCr4;
...
{
PatchAssembly (&gAsmSmmCr0, AsmReadCr0 (), 4);
PatchAssembly (&gAsmSmmCr3, AsmReadCr3 (), 4);
PatchAssembly (&gAsmSmmCr4, AsmReadCr4 (), 4);
...
}
(I think it's fine to open-code the last argument as "4",
rather than
"sizeof (UINT32)", because for patching, we must have
intimate knowledge
of the instruction anyway.)
To me, this is easier to read, because:
- there are no complex casts in the "business logic"
- the size is spelled out once per patching
- the function name and the variable names make it clear
we are patching
separately compiled assembly code that was linked into
the same
module.
What do you think?
Thanks!
Laszlo
-Original Message-
From: edk2-devel [mailto:edk2-devel-
boun...@lists.01.org]
On Behalf Of Laszlo Ersek
Sent: Tuesday, January 30, 2018 10:17 AM
To: Kinney, Michael D ;
edk2-
devel-01
Cc: Ni, Ruiyu ; Paolo Bonzini
; Yao, Jiewen
; Dong, Eric
Subject: Re: [edk2] [PATCH 1/3]
UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32
SmmStartup()
On 01/30/18 18:22, Kinney, Michael D wrote:
Laszlo,
The DBs can be removed if the label is moved after
the instruction and the patch is done to the label
minus the size of the patch value.
Indeed I haven't thought of this.
If I understand correctly, it means
extern UINT8 gSmmCr0;
*(UINT32*)(&gSmmCr0 - sizeof (UINT32)) =
(UINT32)AsmReadCr0 ();
TBH, the DB feels less ugly to me than this :)
Still, if you think it would be an acceptable price to
pay for removing
the remaining DBs, I can respin.
Thanks
Laszlo
-Original Message-
From: edk2-devel [mailto:edk2-devel-
boun...@lists.01.org]
On Behalf Of Laszlo Ersek
Sent: Tuesday, January 30, 2018 7:34 AM
To: edk2-devel-01
Cc: Ni, Ruiyu ; Yao, Jiewen
; Dong, Eric
;
Paolo Bonzini
Subject: [edk2] [PATCH 1/3]
UefiCpuPkg/PiSmmCpuDxeSmm:
update comments in IA32 SmmStartup()
The gSmmCr3, gSmmCr4, gSmmCr0 and gSmmJmpAddr global
variables are used
for patching assembly instructions, thus we can
never
remove the DB
encodings for those instructions. At least we should
add
the intended
meanings in comments.
This patch only changes comments.
Cc: Eric Dong
Cc: Jian J Wang
Cc: Jiewen Yao
Cc: Paolo Bonzini
Cc: Ruiyu Ni
Contributed-under: TianoCore Contribution Agreement
1.1
Signed-off-by: Laszlo Ersek
---
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 8
-
---
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git
a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
index e96dd8d2392a..08534dba64b7 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
@@ -44,34 +44,34 @@ global ASM_PFX(SmmSta
Re: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
Mike, Laszlo,
Does the patch only apply to the operand?
If so, PatchAssembly looks too general.
How about the name like PatchAssemblyOperand?
On 1/31/2018 6:25 AM, 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.
I was originally thinking this functionality would go
into BaseLib. But with the use of CopyMem(), we can't
do that. 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);
}
}
Mike
-Original Message-
From: Laszlo Ersek [mailto:ler...@redhat.com]
Sent: Tuesday, January 30, 2018 1:45 PM
To: Kinney, Michael D ; edk2-
devel-01
Cc: Ni, Ruiyu ; Paolo Bonzini
; Yao, Jiewen
; Dong, Eric
Subject: Re: [edk2] [PATCH 1/3]
UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32
SmmStartup()
On 01/30/18 21:31, Kinney, Michael D wrote:
Laszlo,
We have already used this technique in other NASM files
to remove DBs.
OK.
Let us know if you have suggestions on how to make the
C code that performs the patches easier to read and
maintain.
How about this:
VOID
PatchAssembly (
VOID *BufferEnd,
UINT64 PatchValue,
UINTN ValueSize
)
{
CopyMem (
(VOID *)((UINTN)BufferEnd - ValueSize),
&PatchValue,
ValueSize
);
}
extern UINT8 gAsmSmmCr0;
extern UINT8 gAsmSmmCr3;
extern UINT8 gAsmSmmCr4;
...
{
PatchAssembly (&gAsmSmmCr0, AsmReadCr0 (), 4);
PatchAssembly (&gAsmSmmCr3, AsmReadCr3 (), 4);
PatchAssembly (&gAsmSmmCr4, AsmReadCr4 (), 4);
...
}
(I think it's fine to open-code the last argument as "4",
rather than
"sizeof (UINT32)", because for patching, we must have
intimate knowledge
of the instruction anyway.)
To me, this is easier to read, because:
- there are no complex casts in the "business logic"
- the size is spelled out once per patching
- the function name and the variable names make it clear
we are patching
separately compiled assembly code that was linked into
the same
module.
What do you think?
Thanks!
Laszlo
-Original Message-
From: edk2-devel [mailto:edk2-devel-
boun...@lists.01.org]
On Behalf Of Laszlo Ersek
Sent: Tuesday, January 30, 2018 10:17 AM
To: Kinney, Michael D ;
edk2-
devel-01
Cc: Ni, Ruiyu ; Paolo Bonzini
; Yao, Jiewen
; Dong, Eric
Subject: Re: [edk2] [PATCH 1/3]
UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32
SmmStartup()
On 01/30/18 18:22, Kinney, Michael D wrote:
Laszlo,
The DBs can be removed if the label is moved after
the instruction and the patch is done to the label
minus the size of the patch value.
Indeed I haven't thought of this.
If I understand correctly, it means
extern UINT8 gSmmCr0;
*(UINT32*)(&gSmmCr0 - sizeof (UINT32)) =
(UINT32)AsmReadCr0 ();
TBH, the DB feels less ugly to me than this :)
Still, if you think it would be an acceptable price to
pay for removing
the remaining DBs, I can respin.
Thanks
Laszlo
-Original Message-
From: edk2-devel [mailto:edk2-devel-
boun...@lists.01.org]
On Behalf Of Laszlo Ersek
Sent: Tuesday, January 30, 2018 7:34 AM
To: edk2-devel-01
Cc: Ni, Ruiyu ; Yao, Jiewen
; Dong, Eric
;
Paolo Bonzini
Subject: [edk2] [PATCH 1/3]
UefiCpuPkg/PiSmmCpuDxeSmm:
update comments in IA32 SmmStartup()
The gSmmCr3, gSmmCr4, gSmmCr0 and gSmmJmpAddr global
variables are used
for patching assembly instructions, thus we can
never
remove the DB
encodings for those instructions. At least we should
add
the intended
meanings in comments.
This patch only changes comments.
Cc: Eric Dong
Cc: Jian J Wang
Cc: Jiewen Yao
Cc: Paolo Bonzini
Cc: Ruiyu Ni
Contributed-under: TianoCore Contribution Agreement
1.1
Signed-off-by: Laszlo Ersek
---
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 8
-
---
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git
a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
index e96dd8d2392a..08534dba64b7 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
@@ -44,34 +44,34 @@ global ASM_PFX(SmmStartup)
ASM_PFX(SmmStartup):
DB
Re: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
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.
I was originally thinking this functionality would go
into BaseLib. But with the use of CopyMem(), we can't
do that. 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);
}
}
Mike
> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Tuesday, January 30, 2018 1:45 PM
> To: Kinney, Michael D ; edk2-
> devel-01
> Cc: Ni, Ruiyu ; Paolo Bonzini
> ; Yao, Jiewen
> ; Dong, Eric
> Subject: Re: [edk2] [PATCH 1/3]
> UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32
> SmmStartup()
>
> On 01/30/18 21:31, Kinney, Michael D wrote:
> > Laszlo,
> >
> > We have already used this technique in other NASM files
> > to remove DBs.
>
> OK.
>
> > Let us know if you have suggestions on how to make the
> > C code that performs the patches easier to read and
> > maintain.
>
> How about this:
>
> VOID
> PatchAssembly (
> VOID *BufferEnd,
> UINT64 PatchValue,
> UINTN ValueSize
> )
> {
> CopyMem (
> (VOID *)((UINTN)BufferEnd - ValueSize),
> &PatchValue,
> ValueSize
> );
> }
>
> extern UINT8 gAsmSmmCr0;
> extern UINT8 gAsmSmmCr3;
> extern UINT8 gAsmSmmCr4;
>
> ...
> {
> PatchAssembly (&gAsmSmmCr0, AsmReadCr0 (), 4);
> PatchAssembly (&gAsmSmmCr3, AsmReadCr3 (), 4);
> PatchAssembly (&gAsmSmmCr4, AsmReadCr4 (), 4);
> ...
> }
>
> (I think it's fine to open-code the last argument as "4",
> rather than
> "sizeof (UINT32)", because for patching, we must have
> intimate knowledge
> of the instruction anyway.)
>
> To me, this is easier to read, because:
>
> - there are no complex casts in the "business logic"
> - the size is spelled out once per patching
> - the function name and the variable names make it clear
> we are patching
> separately compiled assembly code that was linked into
> the same
> module.
>
> What do you think?
>
> Thanks!
> Laszlo
>
> >> -----Original Message-
> >> From: edk2-devel [mailto:edk2-devel-
> boun...@lists.01.org]
> >> On Behalf Of Laszlo Ersek
> >> Sent: Tuesday, January 30, 2018 10:17 AM
> >> To: Kinney, Michael D ;
> edk2-
> >> devel-01
> >> Cc: Ni, Ruiyu ; Paolo Bonzini
> >> ; Yao, Jiewen
> >> ; Dong, Eric
>
> >> Subject: Re: [edk2] [PATCH 1/3]
> >> UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32
> >> SmmStartup()
> >>
> >> On 01/30/18 18:22, Kinney, Michael D wrote:
> >>> Laszlo,
> >>>
> >>> The DBs can be removed if the label is moved after
> >>> the instruction and the patch is done to the label
> >>> minus the size of the patch value.
> >>
> >> Indeed I haven't thought of this.
> >>
> >> If I understand correctly, it means
> >>
> >> extern UINT8 gSmmCr0;
> >>
> >> *(UINT32*)(&gSmmCr0 - sizeof (UINT32)) =
> >> (UINT32)AsmReadCr0 ();
> >>
> >> TBH, the DB feels less ugly to me than this :)
> >>
> >> Still, if you think it would be an acceptable price to
> >> pay for removing
> >> the remaining DBs, I can respin.
> >>
> >> Thanks
> >> Laszlo
> >>
> >>>> -Original Message-
> >>>> From: edk2-devel [mailto:edk2-devel-
> >> boun...@lists.01.org]
> >>>> On Behalf Of Laszlo Ersek
> >>>> Sent: Tuesday, January 30, 2018 7:34 AM
> >>>> To: edk2-devel-01
> >>>> Cc: Ni, Ruiyu ; Yao, Jiewen
> >>>> ; Dong, Eric
> >> ;
> >>>> Paolo Bonzini
> >>>> Subject: [edk2] [PATCH 1/3]
> UefiCpuPkg/PiSmmCpuDxeSmm:
Re: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
On 01/30/18 22:26, Kinney, Michael D wrote: > Laszlo, > > Maybe we can add a macro to help: > > #define PATCH_X86_ASM(Label,Type,Value) *((Type *)(&Label) - 1) = > (Type)(Value) > > PATCH_X86_ASM (gSmmCr0, UINT32, AsmReadCr0()); Before sending my previous email, I thought of something like this. Here I slightly dislike: - the unaligned access to a wider-than-byte object - the larger potential for triggering undefined behavior in the C language implementation (due to type punning) -- doing the arithmetic in UINTN and using CopyMem for the data movement should mitigate that - the requirement that the object to patch has to be followed by the same number of bytes (otherwise linking might fail, I think). For example if we'd like to patch 4 bytes at the very end of the assembly code, e.g. in a jmp instruction, we'd have to match that with a DD, just so the extern UINT32 global variable could be declared and linked. I think requiring just one byte trailing padding and making all such markers UINT8 is easier to handle - the requirement that the declared type of "gSmmCr0" match the type passed to the macro as 2nd argument That said, I'm fine using either of PATCH_X86_ASM() and PatchAssembly(). Thanks! Laszlo > > Mike > >> -Original Message- >> From: Kinney, Michael D >> Sent: Tuesday, January 30, 2018 12:31 PM >> To: Laszlo Ersek ; edk2-devel-01 >> ; Kinney, Michael D >> >> Cc: Ni, Ruiyu ; Paolo Bonzini >> ; Yao, Jiewen >> ; Dong, Eric >> Subject: RE: [edk2] [PATCH 1/3] >> UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 >> SmmStartup() >> >> Laszlo, >> >> We have already used this technique in other NASM files >> to remove DBs. >> >> Let us know if you have suggestions on how to make the >> C code that performs the patches easier to read and >> maintain. >> >> Mike >> >>> -Original Message- >>> From: edk2-devel [mailto:edk2-devel- >> boun...@lists.01.org] >>> On Behalf Of Laszlo Ersek >>> Sent: Tuesday, January 30, 2018 10:17 AM >>> To: Kinney, Michael D ; >> edk2- >>> devel-01 >>> Cc: Ni, Ruiyu ; Paolo Bonzini >>> ; Yao, Jiewen >>> ; Dong, Eric >> >>> Subject: Re: [edk2] [PATCH 1/3] >>> UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 >>> SmmStartup() >>> >>> On 01/30/18 18:22, Kinney, Michael D wrote: >>>> Laszlo, >>>> >>>> The DBs can be removed if the label is moved after >>>> the instruction and the patch is done to the label >>>> minus the size of the patch value. >>> >>> Indeed I haven't thought of this. >>> >>> If I understand correctly, it means >>> >>> extern UINT8 gSmmCr0; >>> >>> *(UINT32*)(&gSmmCr0 - sizeof (UINT32)) = >>> (UINT32)AsmReadCr0 (); >>> >>> TBH, the DB feels less ugly to me than this :) >>> >>> Still, if you think it would be an acceptable price to >>> pay for removing >>> the remaining DBs, I can respin. >>> >>> Thanks >>> Laszlo >>> >>>>> -Original Message- >>>>> From: edk2-devel [mailto:edk2-devel- >>> boun...@lists.01.org] >>>>> On Behalf Of Laszlo Ersek >>>>> Sent: Tuesday, January 30, 2018 7:34 AM >>>>> To: edk2-devel-01 >>>>> Cc: Ni, Ruiyu ; Yao, Jiewen >>>>> ; Dong, Eric >>> ; >>>>> Paolo Bonzini >>>>> Subject: [edk2] [PATCH 1/3] >> UefiCpuPkg/PiSmmCpuDxeSmm: >>>>> update comments in IA32 SmmStartup() >>>>> >>>>> The gSmmCr3, gSmmCr4, gSmmCr0 and gSmmJmpAddr global >>>>> variables are used >>>>> for patching assembly instructions, thus we can >> never >>>>> remove the DB >>>>> encodings for those instructions. At least we should >>> add >>>>> the intended >>>>> meanings in comments. >>>>> >>>>> This patch only changes comments. >>>>> >>>>> Cc: Eric Dong >>>>> Cc: Jian J Wang >>>>> Cc: Jiewen Yao >>>>> Cc: Paolo Bonzini >>>>> Cc: Ruiyu Ni >>>>> Contributed-under: TianoCore Contribution Agreement >>> 1.1 >>>>> Signed-off-by: Laszlo Ersek >>>>> --- >>>>> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 8 >> - >>> -
Re: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
On 01/30/18 21:31, Kinney, Michael D wrote:
> Laszlo,
>
> We have already used this technique in other NASM files
> to remove DBs.
OK.
> Let us know if you have suggestions on how to make the
> C code that performs the patches easier to read and
> maintain.
How about this:
VOID
PatchAssembly (
VOID *BufferEnd,
UINT64 PatchValue,
UINTN ValueSize
)
{
CopyMem (
(VOID *)((UINTN)BufferEnd - ValueSize),
&PatchValue,
ValueSize
);
}
extern UINT8 gAsmSmmCr0;
extern UINT8 gAsmSmmCr3;
extern UINT8 gAsmSmmCr4;
...
{
PatchAssembly (&gAsmSmmCr0, AsmReadCr0 (), 4);
PatchAssembly (&gAsmSmmCr3, AsmReadCr3 (), 4);
PatchAssembly (&gAsmSmmCr4, AsmReadCr4 (), 4);
...
}
(I think it's fine to open-code the last argument as "4", rather than
"sizeof (UINT32)", because for patching, we must have intimate knowledge
of the instruction anyway.)
To me, this is easier to read, because:
- there are no complex casts in the "business logic"
- the size is spelled out once per patching
- the function name and the variable names make it clear we are patching
separately compiled assembly code that was linked into the same
module.
What do you think?
Thanks!
Laszlo
>> -Original Message-
>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org]
>> On Behalf Of Laszlo Ersek
>> Sent: Tuesday, January 30, 2018 10:17 AM
>> To: Kinney, Michael D ; edk2-
>> devel-01
>> Cc: Ni, Ruiyu ; Paolo Bonzini
>> ; Yao, Jiewen
>> ; Dong, Eric
>> Subject: Re: [edk2] [PATCH 1/3]
>> UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32
>> SmmStartup()
>>
>> On 01/30/18 18:22, Kinney, Michael D wrote:
>>> Laszlo,
>>>
>>> The DBs can be removed if the label is moved after
>>> the instruction and the patch is done to the label
>>> minus the size of the patch value.
>>
>> Indeed I haven't thought of this.
>>
>> If I understand correctly, it means
>>
>> extern UINT8 gSmmCr0;
>>
>> *(UINT32*)(&gSmmCr0 - sizeof (UINT32)) =
>> (UINT32)AsmReadCr0 ();
>>
>> TBH, the DB feels less ugly to me than this :)
>>
>> Still, if you think it would be an acceptable price to
>> pay for removing
>> the remaining DBs, I can respin.
>>
>> Thanks
>> Laszlo
>>
>>>> -Original Message-
>>>> From: edk2-devel [mailto:edk2-devel-
>> boun...@lists.01.org]
>>>> On Behalf Of Laszlo Ersek
>>>> Sent: Tuesday, January 30, 2018 7:34 AM
>>>> To: edk2-devel-01
>>>> Cc: Ni, Ruiyu ; Yao, Jiewen
>>>> ; Dong, Eric
>> ;
>>>> Paolo Bonzini
>>>> Subject: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm:
>>>> update comments in IA32 SmmStartup()
>>>>
>>>> The gSmmCr3, gSmmCr4, gSmmCr0 and gSmmJmpAddr global
>>>> variables are used
>>>> for patching assembly instructions, thus we can never
>>>> remove the DB
>>>> encodings for those instructions. At least we should
>> add
>>>> the intended
>>>> meanings in comments.
>>>>
>>>> This patch only changes comments.
>>>>
>>>> Cc: Eric Dong
>>>> Cc: Jian J Wang
>>>> Cc: Jiewen Yao
>>>> Cc: Paolo Bonzini
>>>> Cc: Ruiyu Ni
>>>> Contributed-under: TianoCore Contribution Agreement
>> 1.1
>>>> Signed-off-by: Laszlo Ersek
>>>> ---
>>>> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 8 -
>> ---
>>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git
>> a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
>>>> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
>>>> index e96dd8d2392a..08534dba64b7 100644
>>>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
>>>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
>>>> @@ -44,34 +44,34 @@ global ASM_PFX(SmmStartup)
>>>> ASM_PFX(SmmStartup):
>>>> DB 0x66
>>>> mov eax, 0x8001 ; read
>>>> capability
>>>> cpuid
>>>> DB 0x66
>>>> mov ebx, edx; rdmsr will
>>>> change edx. keep it in ebx.
>>>> -DB 0x66, 0xb8
>>>> +DB 0x66, 0xb8 ; mov eax,
>> imm32
>>>> ASM_PFX(gSmmCr3): DD 0
>>>> mov cr3, eax
>>>> DB 0x67,
Re: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
Laszlo, Maybe we can add a macro to help: #define PATCH_X86_ASM(Label,Type,Value) *((Type *)(&Label) - 1) = (Type)(Value) PATCH_X86_ASM (gSmmCr0, UINT32, AsmReadCr0()); Mike > -Original Message- > From: Kinney, Michael D > Sent: Tuesday, January 30, 2018 12:31 PM > To: Laszlo Ersek ; edk2-devel-01 > ; Kinney, Michael D > > Cc: Ni, Ruiyu ; Paolo Bonzini > ; Yao, Jiewen > ; Dong, Eric > Subject: RE: [edk2] [PATCH 1/3] > UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 > SmmStartup() > > Laszlo, > > We have already used this technique in other NASM files > to remove DBs. > > Let us know if you have suggestions on how to make the > C code that performs the patches easier to read and > maintain. > > Mike > > > -Original Message- > > From: edk2-devel [mailto:edk2-devel- > boun...@lists.01.org] > > On Behalf Of Laszlo Ersek > > Sent: Tuesday, January 30, 2018 10:17 AM > > To: Kinney, Michael D ; > edk2- > > devel-01 > > Cc: Ni, Ruiyu ; Paolo Bonzini > > ; Yao, Jiewen > > ; Dong, Eric > > > Subject: Re: [edk2] [PATCH 1/3] > > UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 > > SmmStartup() > > > > On 01/30/18 18:22, Kinney, Michael D wrote: > > > Laszlo, > > > > > > The DBs can be removed if the label is moved after > > > the instruction and the patch is done to the label > > > minus the size of the patch value. > > > > Indeed I haven't thought of this. > > > > If I understand correctly, it means > > > > extern UINT8 gSmmCr0; > > > > *(UINT32*)(&gSmmCr0 - sizeof (UINT32)) = > > (UINT32)AsmReadCr0 (); > > > > TBH, the DB feels less ugly to me than this :) > > > > Still, if you think it would be an acceptable price to > > pay for removing > > the remaining DBs, I can respin. > > > > Thanks > > Laszlo > > > > >> -Original Message- > > >> From: edk2-devel [mailto:edk2-devel- > > boun...@lists.01.org] > > >> On Behalf Of Laszlo Ersek > > >> Sent: Tuesday, January 30, 2018 7:34 AM > > >> To: edk2-devel-01 > > >> Cc: Ni, Ruiyu ; Yao, Jiewen > > >> ; Dong, Eric > > ; > > >> Paolo Bonzini > > >> Subject: [edk2] [PATCH 1/3] > UefiCpuPkg/PiSmmCpuDxeSmm: > > >> update comments in IA32 SmmStartup() > > >> > > >> The gSmmCr3, gSmmCr4, gSmmCr0 and gSmmJmpAddr global > > >> variables are used > > >> for patching assembly instructions, thus we can > never > > >> remove the DB > > >> encodings for those instructions. At least we should > > add > > >> the intended > > >> meanings in comments. > > >> > > >> This patch only changes comments. > > >> > > >> Cc: Eric Dong > > >> Cc: Jian J Wang > > >> Cc: Jiewen Yao > > >> Cc: Paolo Bonzini > > >> Cc: Ruiyu Ni > > >> Contributed-under: TianoCore Contribution Agreement > > 1.1 > > >> Signed-off-by: Laszlo Ersek > > >> --- > > >> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 8 > - > > --- > > >> 1 file changed, 4 insertions(+), 4 deletions(-) > > >> > > >> diff --git > > a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm > > >> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm > > >> index e96dd8d2392a..08534dba64b7 100644 > > >> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm > > >> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm > > >> @@ -44,34 +44,34 @@ global ASM_PFX(SmmStartup) > > >> ASM_PFX(SmmStartup): > > >> DB 0x66 > > >> mov eax, 0x8001 ; read > > >> capability > > >> cpuid > > >> DB 0x66 > > >> mov ebx, edx; rdmsr > will > > >> change edx. keep it in ebx. > > >> -DB 0x66, 0xb8 > > >> +DB 0x66, 0xb8 ; mov eax, > > imm32 > > >> ASM_PFX(gSmmCr3): DD 0 > > >> mov cr3, eax > > >> DB 0x67, 0x66 > > >> lgdt[cs:ebp + (ASM_PFX(gcSmiInitGdtr) - > > >> ASM_PFX(SmmStartup))] > > >> -DB 0x66, 0xb8 > > >> +DB 0x66, 0xb8 ; mov eax, > > imm32 > > >> ASM_PFX(gSmmCr4): DD 0 > > >> mov
Re: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
Laszlo, We have already used this technique in other NASM files to remove DBs. Let us know if you have suggestions on how to make the C code that performs the patches easier to read and maintain. Mike > -Original Message- > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] > On Behalf Of Laszlo Ersek > Sent: Tuesday, January 30, 2018 10:17 AM > To: Kinney, Michael D ; edk2- > devel-01 > Cc: Ni, Ruiyu ; Paolo Bonzini > ; Yao, Jiewen > ; Dong, Eric > Subject: Re: [edk2] [PATCH 1/3] > UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 > SmmStartup() > > On 01/30/18 18:22, Kinney, Michael D wrote: > > Laszlo, > > > > The DBs can be removed if the label is moved after > > the instruction and the patch is done to the label > > minus the size of the patch value. > > Indeed I haven't thought of this. > > If I understand correctly, it means > > extern UINT8 gSmmCr0; > > *(UINT32*)(&gSmmCr0 - sizeof (UINT32)) = > (UINT32)AsmReadCr0 (); > > TBH, the DB feels less ugly to me than this :) > > Still, if you think it would be an acceptable price to > pay for removing > the remaining DBs, I can respin. > > Thanks > Laszlo > > >> -Original Message- > >> From: edk2-devel [mailto:edk2-devel- > boun...@lists.01.org] > >> On Behalf Of Laszlo Ersek > >> Sent: Tuesday, January 30, 2018 7:34 AM > >> To: edk2-devel-01 > >> Cc: Ni, Ruiyu ; Yao, Jiewen > >> ; Dong, Eric > ; > >> Paolo Bonzini > >> Subject: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: > >> update comments in IA32 SmmStartup() > >> > >> The gSmmCr3, gSmmCr4, gSmmCr0 and gSmmJmpAddr global > >> variables are used > >> for patching assembly instructions, thus we can never > >> remove the DB > >> encodings for those instructions. At least we should > add > >> the intended > >> meanings in comments. > >> > >> This patch only changes comments. > >> > >> Cc: Eric Dong > >> Cc: Jian J Wang > >> Cc: Jiewen Yao > >> Cc: Paolo Bonzini > >> Cc: Ruiyu Ni > >> Contributed-under: TianoCore Contribution Agreement > 1.1 > >> Signed-off-by: Laszlo Ersek > >> --- > >> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 8 - > --- > >> 1 file changed, 4 insertions(+), 4 deletions(-) > >> > >> diff --git > a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm > >> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm > >> index e96dd8d2392a..08534dba64b7 100644 > >> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm > >> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm > >> @@ -44,34 +44,34 @@ global ASM_PFX(SmmStartup) > >> ASM_PFX(SmmStartup): > >> DB 0x66 > >> mov eax, 0x8001 ; read > >> capability > >> cpuid > >> DB 0x66 > >> mov ebx, edx; rdmsr will > >> change edx. keep it in ebx. > >> -DB 0x66, 0xb8 > >> +DB 0x66, 0xb8 ; mov eax, > imm32 > >> ASM_PFX(gSmmCr3): DD 0 > >> mov cr3, eax > >> DB 0x67, 0x66 > >> lgdt[cs:ebp + (ASM_PFX(gcSmiInitGdtr) - > >> ASM_PFX(SmmStartup))] > >> -DB 0x66, 0xb8 > >> +DB 0x66, 0xb8 ; mov eax, > imm32 > >> ASM_PFX(gSmmCr4): DD 0 > >> mov cr4, eax > >> DB 0x66 > >> mov ecx, 0xc080 ; IA32_EFER > MSR > >> rdmsr > >> DB 0x66 > >> testebx, BIT20 ; check NXE > >> capability > >> jz .1 > >> or ah, BIT3; set NXE bit > >> wrmsr > >> .1: > >> -DB 0x66, 0xb8 > >> +DB 0x66, 0xb8 ; mov eax, > imm32 > >> ASM_PFX(gSmmCr0): DD 0 > >> DB 0xbf, PROTECT_MODE_DS, 0; mov di, > >> PROTECT_MODE_DS > >> mov cr0, eax > >> -DB 0x66, 0xea ; jmp far > >> [ptr48] > >> +DB 0x66, 0xea ; jmp far > >> [ptr48] > >> ASM_PFX(gSmmJmpAddr): > >> DD @32bit > >> DW PROTECT_MODE_CS > >> @32bit: > >> mov ds, edi > >> mov es, edi > >> -- > >> 2.14.1.3.gb7cf6e02401b > >> > >> > >> ___ > >> edk2-devel mailing list > >> edk2-devel@lists.01.org > >> https://lists.01.org/mailman/listinfo/edk2-devel > > ___ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
On 01/30/18 18:22, Kinney, Michael D wrote: > Laszlo, > > The DBs can be removed if the label is moved after > the instruction and the patch is done to the label > minus the size of the patch value. Indeed I haven't thought of this. If I understand correctly, it means extern UINT8 gSmmCr0; *(UINT32*)(&gSmmCr0 - sizeof (UINT32)) = (UINT32)AsmReadCr0 (); TBH, the DB feels less ugly to me than this :) Still, if you think it would be an acceptable price to pay for removing the remaining DBs, I can respin. Thanks Laszlo >> -Original Message- >> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] >> On Behalf Of Laszlo Ersek >> Sent: Tuesday, January 30, 2018 7:34 AM >> To: edk2-devel-01 >> Cc: Ni, Ruiyu ; Yao, Jiewen >> ; Dong, Eric ; >> Paolo Bonzini >> Subject: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: >> update comments in IA32 SmmStartup() >> >> The gSmmCr3, gSmmCr4, gSmmCr0 and gSmmJmpAddr global >> variables are used >> for patching assembly instructions, thus we can never >> remove the DB >> encodings for those instructions. At least we should add >> the intended >> meanings in comments. >> >> This patch only changes comments. >> >> Cc: Eric Dong >> Cc: Jian J Wang >> Cc: Jiewen Yao >> Cc: Paolo Bonzini >> Cc: Ruiyu Ni >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Laszlo Ersek >> --- >> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 8 >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm >> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm >> index e96dd8d2392a..08534dba64b7 100644 >> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm >> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm >> @@ -44,34 +44,34 @@ global ASM_PFX(SmmStartup) >> ASM_PFX(SmmStartup): >> DB 0x66 >> mov eax, 0x8001 ; read >> capability >> cpuid >> DB 0x66 >> mov ebx, edx; rdmsr will >> change edx. keep it in ebx. >> -DB 0x66, 0xb8 >> +DB 0x66, 0xb8 ; mov eax, imm32 >> ASM_PFX(gSmmCr3): DD 0 >> mov cr3, eax >> DB 0x67, 0x66 >> lgdt[cs:ebp + (ASM_PFX(gcSmiInitGdtr) - >> ASM_PFX(SmmStartup))] >> -DB 0x66, 0xb8 >> +DB 0x66, 0xb8 ; mov eax, imm32 >> ASM_PFX(gSmmCr4): DD 0 >> mov cr4, eax >> DB 0x66 >> mov ecx, 0xc080 ; IA32_EFER MSR >> rdmsr >> DB 0x66 >> testebx, BIT20 ; check NXE >> capability >> jz .1 >> or ah, BIT3; set NXE bit >> wrmsr >> .1: >> -DB 0x66, 0xb8 >> +DB 0x66, 0xb8 ; mov eax, imm32 >> ASM_PFX(gSmmCr0): DD 0 >> DB 0xbf, PROTECT_MODE_DS, 0; mov di, >> PROTECT_MODE_DS >> mov cr0, eax >> -DB 0x66, 0xea ; jmp far >> [ptr48] >> +DB 0x66, 0xea ; jmp far >> [ptr48] >> ASM_PFX(gSmmJmpAddr): >> DD @32bit >> DW PROTECT_MODE_CS >> @32bit: >> mov ds, edi >> mov es, edi >> -- >> 2.14.1.3.gb7cf6e02401b >> >> >> ___ >> edk2-devel mailing list >> edk2-devel@lists.01.org >> https://lists.01.org/mailman/listinfo/edk2-devel ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
Laszlo, The DBs can be removed if the label is moved after the instruction and the patch is done to the label minus the size of the patch value. Mike > -Original Message- > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] > On Behalf Of Laszlo Ersek > Sent: Tuesday, January 30, 2018 7:34 AM > To: edk2-devel-01 > Cc: Ni, Ruiyu ; Yao, Jiewen > ; Dong, Eric ; > Paolo Bonzini > Subject: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: > update comments in IA32 SmmStartup() > > The gSmmCr3, gSmmCr4, gSmmCr0 and gSmmJmpAddr global > variables are used > for patching assembly instructions, thus we can never > remove the DB > encodings for those instructions. At least we should add > the intended > meanings in comments. > > This patch only changes comments. > > Cc: Eric Dong > Cc: Jian J Wang > Cc: Jiewen Yao > Cc: Paolo Bonzini > Cc: Ruiyu Ni > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Laszlo Ersek > --- > UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm > b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm > index e96dd8d2392a..08534dba64b7 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm > @@ -44,34 +44,34 @@ global ASM_PFX(SmmStartup) > ASM_PFX(SmmStartup): > DB 0x66 > mov eax, 0x8001 ; read > capability > cpuid > DB 0x66 > mov ebx, edx; rdmsr will > change edx. keep it in ebx. > -DB 0x66, 0xb8 > +DB 0x66, 0xb8 ; mov eax, imm32 > ASM_PFX(gSmmCr3): DD 0 > mov cr3, eax > DB 0x67, 0x66 > lgdt[cs:ebp + (ASM_PFX(gcSmiInitGdtr) - > ASM_PFX(SmmStartup))] > -DB 0x66, 0xb8 > +DB 0x66, 0xb8 ; mov eax, imm32 > ASM_PFX(gSmmCr4): DD 0 > mov cr4, eax > DB 0x66 > mov ecx, 0xc080 ; IA32_EFER MSR > rdmsr > DB 0x66 > testebx, BIT20 ; check NXE > capability > jz .1 > or ah, BIT3; set NXE bit > wrmsr > .1: > -DB 0x66, 0xb8 > +DB 0x66, 0xb8 ; mov eax, imm32 > ASM_PFX(gSmmCr0): DD 0 > DB 0xbf, PROTECT_MODE_DS, 0; mov di, > PROTECT_MODE_DS > mov cr0, eax > -DB 0x66, 0xea ; jmp far > [ptr48] > +DB 0x66, 0xea ; jmp far > [ptr48] > ASM_PFX(gSmmJmpAddr): > DD @32bit > DW PROTECT_MODE_CS > @32bit: > mov ds, edi > mov es, edi > -- > 2.14.1.3.gb7cf6e02401b > > > ___ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
