Re: [edk2-devel] Memory Attribute for depex section

["Laszlo Ersek" <lersek@redhat.com>]

On 1/18/24 07:00, Nhi Pham wrote:
> Hi Laszlo,
> 
> On 1/16/2024 2:00 AM, Laszlo Ersek wrote:
>> On 1/15/24 15:04, Ard Biesheuvel wrote:
>>> On Mon, 15 Jan 2024 at 14:07, Nhi Pham <nhi@os.amperecomputing.com>
>>> wrote:
>>>>
>>>> On 1/12/2024 4:45 PM, Laszlo Ersek wrote:
>>>>> (Independently: I think that's a valid thing to do for *SMM* drivers,
>>>>> because the entry point functions of those drivers are permitted to
>>>>> use
>>>>> both SMM and DXE/UEFI protocols. But whether the same is valid for the
>>>>> *standalone* MM drivers -- that looks questionable. Standalone MM
>>>>> drivers should not depend on UEFI/DXE protocols ever, IIUC.)
>>>>>
>>>>>> 3) The issue is patching the grammar in place, why can’t we just
>>>>>> make a
>>>>>> copy for the dispatcher grammer, and operate on the copy. Maybe via a
>>>>>> copy on 1st update strategy?
>>>>>
>>>>> Yes, copying the depex to the heap, and patching it there, was
>>>>> Nhi's #1
>>>>> fix proposal. I think that could be made work. But I'm not sure if the
>>>>> perf savings are worth the additional complexity. The heap allocation
>>>>> (where the writeable depex would exist) would have to be permanently
>>>>> associated with the loaded PE image -- because the dispatcher might
>>>>> need
>>>>> to reevaluate the depex across multiple rounds of dispatching. So
>>>>> that's
>>>>> a new field in some image-related structure, it also needs to be freed
>>>>> upon unload (?), what if the memory allocation fails during depex eval
>>>>> (just consider the depex to eval to FALSE?), etc. Doable, but
>>>>> hairy; not
>>>>> sure if the perf is worth that effort.
>>>>>
>>>>
>>>> Thanks so much, Laszlo for your valuable insights.
>>>>
>>>> The approach #1 works for me. I will do further check for your concerns
>>>> above.
>>>>
>>>> I'm trying your suggested patch and investigating the performance being
>>>> discussed here.
>>>>
>>>
>>> Not sure what approach #1 means,
>>
>> (copying the depex to the heap, and maintaining it there, so that it can
>> be patched)
> 
> Thanks!
> 
>>
>>> but I'd prefer to just remove this
>>> optimization from standalone MM, given that not only a) it shouldn't
>>> have to deal with a large number of protocol GUIDs, but also b) the
>>> driver dispatch is much more straight-forward. (Typically, StMM
>>> drivers can be dispatched in the order they appear in the firmware
>>> volume, in which case each DEPEX is evaluated only once anyway)
>>
>> Sounds like a promising basis for removing the optimization indeed!
>>
> 
> Your patch suggested earlier works for me. And I don't see significant
> performance reduction compared with keeping optimization.
> 
> I don't have strong reason on removing the optimization, but I think it
> would be simply good for now. Could you post your patch to edk2-devel
> for review and merge?

That wouldn't be correct; I don't have any platform for testing StMM. I
proposed the patch purely based on code analysis. I prefer not to post
untested patches, if I can avoid it.

You can however post my patch; simply add your S-o-b at the bottom. You
can also preserve my authorship on the patch with --author=... on
git-commit; but even that is unnecessary for such a simple patch (you
don't even have to pick the patch up from the email, it's trivial to
reimplement from scratch, just reading the email).

Thanks
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113994): https://edk2.groups.io/g/devel/message/113994
Mute This Topic: https://groups.io/mt/103594587/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-