On 02/02/18 11:06, Ard Biesheuvel wrote:
> On 31 January 2018 at 10:40, Laszlo Ersek <ler...@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 (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 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 <michael.d.kin...@intel.com>; edk2-
>>>> devel-01 <edk2-devel@lists.01.org>
>>>> Cc: Ni, Ruiyu <ruiyu...@intel.com>; Paolo Bonzini
>>>> <pbonz...@redhat.com>; Yao, Jiewen
>>>> <jiewen....@intel.com>; Dong, Eric <eric.d...@intel.com>
>>>> 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 <michael.d.kin...@intel.com>;
>>>> edk2-
>>>>>> devel-01 <edk2-devel@lists.01.org>
>>>>>> Cc: Ni, Ruiyu <ruiyu...@intel.com>; Paolo Bonzini
>>>>>> <pbonz...@redhat.com>; Yao, Jiewen
>>>>>> <jiewen....@intel.com>; Dong, Eric
>>>> <eric.d...@intel.com>
>>>>>> 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 <edk2-devel@lists.01.org>
>>>>>>>> Cc: Ni, Ruiyu <ruiyu...@intel.com>; Yao, Jiewen
>>>>>>>> <jiewen....@intel.com>; Dong, Eric
>>>>>> <eric.d...@intel.com>;
>>>>>>>> Paolo Bonzini <pbonz...@redhat.com>
>>>>>>>> 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 <eric.d...@intel.com>
>>>>>>>> Cc: Jian J Wang <jian.j.w...@intel.com>
>>>>>>>> Cc: Jiewen Yao <jiewen....@intel.com>
>>>>>>>> Cc: Paolo Bonzini <pbonz...@redhat.com>
>>>>>>>> Cc: Ruiyu Ni <ruiyu...@intel.com>
>>>>>>>> Contributed-under: TianoCore Contribution Agreement
>>>>>> 1.1
>>>>>>>> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
>>>>>>>> ---
>>>>>>>> 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, 0x80000001 ; 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, 0xc0000080 ; IA32_EFER
>>>>>> MSR
>>>>>>>> rdmsr
>>>>>>>> DB 0x66
>>>>>>>> test ebx, 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
>>>
>>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
