From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id CE4787803CF for ; Fri, 12 Jan 2024 19:27:04 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=mP9/AFFgkpAdnTOCQz4X/yJo07PN33RDBhx4GbPceUc=; c=relaxed/simple; d=groups.io; h=From:Message-id:MIME-version:Subject:Date:In-reply-to:Cc:To:References:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-type; s=20140610; t=1705087623; v=1; b=dL7wuiEKm8STTTsssJUQj39WCU5Pm3gRCU6i8xDKGbRUQ8MwItNJqoytOVXQ/kyF8U392HjF fnP4Yr1i4zhee1b2x8xN8yo3Vss8+UevRAuPsA2AqDjId+5bjUHn3TG2qrdBgh1CrB+JJb5et+3 DXZXUDsYrBZrySrC5eyL2+AM= X-Received: by 127.0.0.2 with SMTP id 5z1NYY7687511xCLmkmaCPA0; Fri, 12 Jan 2024 11:27:03 -0800 X-Received: from rn-mailsvcp-mx-lapp01.apple.com (rn-mailsvcp-mx-lapp01.apple.com [17.179.253.22]) by mx.groups.io with SMTP id smtpd.web10.1144.1705087622950543252 for ; Fri, 12 Jan 2024 11:27:02 -0800 X-Received: from rn-mailsvcp-mta-lapp04.rno.apple.com (rn-mailsvcp-mta-lapp04.rno.apple.com [10.225.203.152]) by rn-mailsvcp-mx-lapp01.rno.apple.com (Oracle Communications Messaging Server 8.1.0.23.20230328 64bit (built Mar 28 2023)) with ESMTPS id <0S7500BRQY0CE110@rn-mailsvcp-mx-lapp01.rno.apple.com> for devel@edk2.groups.io; Fri, 12 Jan 2024 11:27:02 -0800 (PST) X-Proofpoint-ORIG-GUID: uzHXJfQ0s-8tptvYGUnwKjTMHPXHP7h0 X-Proofpoint-GUID: uzHXJfQ0s-8tptvYGUnwKjTMHPXHP7h0 X-Received: from rn-mailsvcp-policy-lapp01.rno.apple.com (rn-mailsvcp-policy-lapp01.rno.apple.com [17.179.253.18]) by rn-mailsvcp-mta-lapp04.rno.apple.com (Oracle Communications Messaging Server 8.1.0.23.20230328 64bit (built Mar 28 2023)) with ESMTPS id <0S7500EQ6Y0SBPP0@rn-mailsvcp-mta-lapp04.rno.apple.com>; Fri, 12 Jan 2024 11:26:53 -0800 (PST) X-Received: from process_milters-daemon.rn-mailsvcp-policy-lapp01.rno.apple.com by rn-mailsvcp-policy-lapp01.rno.apple.com (Oracle Communications Messaging Server 8.1.0.22.20230228 64bit (built Feb 28 2023)) id <0S7500O00XZ63V00@rn-mailsvcp-policy-lapp01.rno.apple.com>; Fri, 12 Jan 2024 11:26:52 -0800 (PST) X-Va-A: X-Va-T-CD: da3a4df698400084da27c6ab403bcb35 X-Va-E-CD: d38c7c8a8a09a1683f7c6688ae4723e1 X-Va-R-CD: 610f65e57ba77b9f82334b7102a66226 X-Va-ID: 0a071655-bb8b-4b1f-bb7e-5c9cf9a61443 X-Va-CD: 0 X-V-A: X-V-T-CD: da3a4df698400084da27c6ab403bcb35 X-V-E-CD: d38c7c8a8a09a1683f7c6688ae4723e1 X-V-R-CD: 610f65e57ba77b9f82334b7102a66226 X-V-ID: 27d541c0-7d3f-42bf-af66-f852729179fd X-V-CD: 0 X-Received: from smtpclient.apple (unknown [17.234.110.227]) by rn-mailsvcp-policy-lapp01.rno.apple.com (Oracle Communications Messaging Server 8.1.0.22.20230228 64bit (built Feb 28 2023)) with ESMTPSA id <0S7500XHZY0R5700@rn-mailsvcp-policy-lapp01.rno.apple.com>; Fri, 12 Jan 2024 11:26:52 -0800 (PST) From: "Andrew Fish via groups.io" Message-id: <5D4526E4-3C6F-4150-BFF9-2E39B123823B@apple.com> MIME-version: 1.0 (Mac OS X Mail 16.0 \(3774.300.61.1.2\)) Subject: Re: [edk2-devel] Memory Attribute for depex section Date: Fri, 12 Jan 2024 11:26:41 -0800 In-reply-to: Cc: edk2-devel-groups-io , "pedro.falcato@gmail.com" , Laszlo Ersek , "nhi@os.amperecomputing.com" , "ardb+tianocore@kernel.org" To: Mike Kinney References: <44ca139f-4d78-4322-b5b6-8e9788bb7486@os.amperecomputing.com> <2ad16043-754e-3bb9-3a4a-702d9a50bf63@redhat.com> <45b95719-f1fc-dbc6-a4cc-a022d691844c@redhat.com> <30901646-905c-798c-d088-255498028fff@redhat.com> <7659165f-a73b-b628-59fe-c29e67beb850@redhat.com> <35BA9784-D5EC-4287-BEDA-34FC3286F977@apple.com> Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,afish@apple.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: EiDqrXaEkwvm2WbUKqTI90grx7686176AA= Content-type: multipart/alternative; boundary="Apple-Mail=_7246A0D5-4722-4289-A9BC-C845549A6BBF" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=dL7wuiEK; dmarc=none; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io --Apple-Mail=_7246A0D5-4722-4289-A9BC-C845549A6BBF Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > On Jan 12, 2024, at 11:04=E2=80=AFAM, Kinney, Michael D wrote: >=20 > Agreed. Basically every API that takes an EF_HANDLE as input calls that = API to make sure it is a valid handle. > =20 > The first question is if we get value from making sure the EFI_HANDLE is = a member of the active set of handles. > =20 > A simple signature check in the EFI_HANDLE may be enough as long as all f= reed handles clear the signature. > =20 > Then, the only way that the linked list walk adds value is if there it a = call with an invalid handle that happens to > have the matching signature. > =20 MIke, I seem to remember we ended up fixing the way you describe locally. Even if= you convert the list into a tree given the index was a pointer to allocate= memory it could get reused. The tree just made the lookup go faster.=20 If we wanted to get tricky on 64-bit systems we could encode a monotonicall= y increasing number in the non-canonical part of the virtual address, or ab= ove the max physical address if paging is not enabled. To use the EFI_HNDLE= as a pointer you just remove count and replace it with sign extend canonic= al address (zero in our case). We could probably define a HOB to define the= bits to use given the code constructing the page tables for DXE needs to k= now all the rules here.=20 Thanks, Andrew Fish > The=20 > =20 > From: Andrew (EFI) Fish =20 > Sent: Friday, January 12, 2024 10:57 AM > To: edk2-devel-groups-io ; Kinney, Michael D > Cc: pedro.falcato@gmail.com; Laszlo Ersek ; nhi@os.amp= erecomputing.com; ardb+tianocore@kernel.org > Subject: Re: [edk2-devel] Memory Attribute for depex section > =20 > =20 >=20 >=20 > On Jan 12, 2024, at 8:37=E2=80=AFAM, Michael D Kinney > wrote: > =20 > Hi Pedro, >=20 > Thank you for evaluating this idea change from linked list to improve > performance of the handle database. >=20 > The concept of using integers for an EFI_HANDLE has been considered befor= e. > One advantage over pointers is that a guarantee can be made that an EFI_H= ANDLE > value can be guaranteed to be unique. In the current implementation with > EFI_HANDLE being a pointer to an allocated buffer, an EFI_HANDLE value co= uld > potentially be reused if an EFI_HANDLE is freed and later allocated for a= =20 > different EFI_HANDLE. >=20 > If you continue to explore this path I agree that EFI_HANDLE value of 0 s= hould > be reserved and never used. I also recommend that new EFI_HANDLE values = are > always assigned a new unique value that freed EFI_HANDLE values are never= reused. >=20 > The overall linked list performance of the handle database has also been = noted > before, but has rarely raised to the level where it impacts the overall b= oot > performance relative to other optimization opportunities. I look forward= to > the performance data. The PERF_X() macros are the right service to use. = On > x86 it is based on the time stamp counter (TSC) which has very small gran= ularity. >=20 > =20 > Mike, > =20 > We tracked this a while back with the PERF macros when we had some perfor= mance issues running diagnostics on a system with 3,000+ handles. The hot p= ath was CoreValidateHandle(). I think the number of calls to CoreValidateHa= ndle() explodes if you have more handles so it is not just the raw performa= nce of CoreValidateHandle(), but also how many times it gets called.=20 > =20 > Thanks, > =20 > Andrew Fish >=20 >=20 > Mike >=20 >=20 >=20 > -----Original Message----- > From: devel@edk2.groups.io > On Behalf Of Pedro Falcato > Sent: Friday, January 12, 2024 6:47 AM > To: Laszlo Ersek > > Cc: devel@edk2.groups.io ; nhi@os.amperecomp= uting.com ; > ardb+tianocore@kernel.org ; Andrew Fish= > > Subject: Re: [edk2-devel] Memory Attribute for depex section >=20 > On Fri, Jan 12, 2024 at 9:35=E2=80=AFAM Laszlo Ersek > wrote: >=20 >=20 > On 1/12/24 03:10, Pedro Falcato wrote: >=20 > My idea was to make each handle an index - like a file descriptor - > AFAIK there's no reason why it *needs* to be an actual pointer. > We'd allocate indices when creating a handle, and return that (casted to > VOID*). >=20 >=20 > Huh, sneaky. >=20 > I see two potential problems with this. >=20 > First, per C std, these "pointers" would be invalid (they wouldn't > actually point to valid memory), so even just evaluating them (not > dereferencing them!) would invoke undefined behavior. May or may not > matter in practice. With compilers getting smarter about optimization > (and stricter about std conformance), there could be issues, perhaps. >=20 > This is true. Stashing random integers in pointers is > implementation-defined. But it's also super common. Win32 HANDLEs are > exactly this, just integers (stashed in VOID*). The Linux kernel world > also has a bunch of fun tricks with stashing flags in a pointer's > bottom bits, magic pointer values, etc. I severely doubt we can run > into issues with this. EDK2 will not exactly run on the C standard's > abstract machine anyway ;) >=20 >=20 >=20 > The other concern is a bit contrived, but I *guess* there could be code > out there that actually dereferences EFI_HANDLEs. That code would crash. > May or may not matter, because such code is arguably already > semantically invalid. (It would not necessarily be invalid at the > language level -- cf. my previous paragraph --, because passing an > otherwise valid EFI_HANDLE to CopyMem, for copying just 1 byte out of > the underlying opaque data structure, would not violate the language.) >=20 > This is a feature, not a bug! :P >=20 > Seriously though, IHANDLE is not even exposed in semi-public headers, > so any code that's derefing an EFI_HANDLE will need to do something > like >=20 > typedef struct { > /* ... */ > } IHANDLE; >=20 > EFI_HANDLE Handle =3D /* ... */; >=20 > IHANDLE *HandleImpl =3D (IHANDLE *) Handle; >=20 > and I'm a strong believer in "play stupid games, win stupid prizes". > You can definitely make an argument for "this should definitely crash" > instead of just "maybe crashing" (for instance, platforms that still > map the NULL page (like OVMF!), or handles > 4096), so I'm inclined to > think that if we indeed go this route, we should set one or two upper > bits (on 64-bit platforms!) to make handles non-canonical addresses > and therefore necessarily crash on dereference. >=20 >=20 >=20 >=20 > I should note that I find it super hard to get a concrete idea on > performance for EFI firmware without adequate tooling - I meant to > write a standalone flamegraph tool a few weeks back (even posted in > edk2-devel), but, as far as I could tell, the architectural timer > protocol couldn't get me the interrupt frame[1]. Until then, whether > any of this radix tree vs RB tree vs flat array stuff really > matters... I find it hard to say. >=20 > [1] x86 has 3 timers (PIT, LAPIC timer, HPET) and performance > monitoring interrupts, and I couldn't freely use any of them :^) >=20 > Edk2 has some form of profiling already (see > "MdePkg/Include/Library/PerformanceLib.h"). Usually one sees core code > peppered with PERF_CODE_BEGIN and PERF_CODE_END macros. I *think* there > is something like a "display performance" (dp) shell application too, > that can show the collected stats. But I've never used these facilities. >=20 > The wiki seems to have two related articles: >=20 > https://github.com/tianocore/tianocore.github.io/wiki/Edk2-Performance- > Infrastructure >=20 >=20 > https://github.com/tianocore/tianocore.github.io/wiki/PerformancePkg >=20 > The former looks quite comprehensive, at a quick skim. >=20 > /me nods > I've seen those macros around, but I've never used them. > In any case, this problem has piqued my interest, I'll see if I can > find some free time this weekend to hack on a test benchmark and a PoC > :) >=20 > -- > Pedro >=20 >=20 >=20 >=20 >=20 >=20 >=20 >=20 -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113775): https://edk2.groups.io/g/devel/message/113775 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/19134562= 12/xyzzy [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- --Apple-Mail=_7246A0D5-4722-4289-A9BC-C845549A6BBF Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8

On Jan 12, 2024, at 11:04= =E2=80=AFAM, Kinney, Michael D <michael.d.kinney@intel.com> wrote:
Agree= d.  Basically every API that takes an EF_HANDLE as input calls that AP= I to make sure it is a valid handle.
 <= /div>
The first question is if we get value from making sure the EFI_HAN= DLE is a member of the active set of handles.
&nbs= p;
A simple signature check in the EFI_HANDLE may be enough = as long as all freed handles clear the signature.
=  
Then, the only way that the linked list walk adds val= ue is if there it a call with an invalid handle that happens to<= /div>
have the matching signature.
 =

MIke,

I seem to remember we ended up fixing the way you describe locally. = Even if you convert the list into a tree given the index was a pointer to a= llocate memory it could get reused. The tree just made the lookup go faster= . 

If we wanted to get tricky on 64-bit syste= ms we could encode a monotonically increasing number in the non-canonical p= art of the virtual address, or above the max physical address if paging is = not enabled. To use the EFI_HNDLE as a pointer you just remove count and re= place it with sign extend canonical address (zero in our case). We cou= ld probably define a HOB to define the bits to use given the code construct= ing the page tables for DXE needs to know all the rules here. 

Thanks,

Andrew Fish

The 
 
From:<= span class=3D"Apple-converted-space"> Andrew (EFI) Fish <afi= sh@apple.com> 
S= ent: Friday, January = 12, 2024 10:57 AM
To: = edk2-devel-groups-io <devel@edk2.groups.io>; Kinney, Michael D= <michael.d.kinney@intel.com>
Cc: pedro.falcato@gmail.com; Laszlo Ersek <lersek@r= edhat.com>; nhi@os.amperecomputing.com; ardb+tianocore@kernel.org
= Subject: Re: [edk2-de= vel] Memory Attribute for depex section
 
 


=
O= n Jan 12, 2024, at 8:37=E2=80=AFAM, Michael D Kinney <michael.d.kinney@intel.com> wrote:
 
Hi Pedro,

Thank you for evaluating this idea c= hange from linked list to improve
performance of the handle database.
The concept of using integers for an EFI_HANDLE has been considered be= fore.
One advantage over pointers is that a guarantee can be made that a= n EFI_HANDLE
value can be guaranteed to be unique.  In the current = implementation with
EFI_HANDLE being a pointer to an allocated buffer, a= n EFI_HANDLE value could
potentially be reused if an EFI_HANDLE is freed= and later allocated for a 
different EFI_HANDLE.

If you continue to explore this path I a= gree that EFI_HANDLE value of 0 should
be reserved and never used.  = ;I also recommend that new EFI_HANDLE values are
always assigned a new u= nique value that freed EFI_HANDLE values are never reused.

The overa= ll linked list performance of the handle database has also been noted
be= fore, but has rarely raised to the level where it impacts the overall boot<= br>performance relative to other optimization opportunities.  I look f= orward to
the performance data.  The PERF_X() macros are the right = service to use.  On
x86 it is based on the time stamp counter (TSC)= which has very small granularity.

<= div style=3D"margin: 0in; font-size: 11pt; font-family: Calibri, sans-serif= ;"> 
Mike,
<= div style=3D"margin: 0in; font-size: 11pt; font-family: Calibri, sans-serif= ;"> 
We tracked this a while back with = the PERF macros when we had some performance issues running diagnostics on = a system with 3,000+ handles. The hot path was CoreValidateHandle(). I thin= k the number of calls to CoreValidateHandle() explodes if you have more han= dles so it is not just the raw performance of CoreValidateHandle(), but als= o how many times it gets called. 
=  
Thanks,
 
Andrew Fish

Mike



-----Original Message-----
From: devel@edk2.groups.io <devel@edk= 2.groups.io> On Behalf Of Pedro Falcato
Sent: Friday, January 12,= 2024 6:47 AM
To: Laszlo Ersek <lersek@redhat.com>= ;
Cc: = devel@edk2.groups.io; nhi@os.amperecomputing.com;
ardb+tianocore@kernel.org; Andrew Fish <afish@apple.c= om>
Subject: Re: [edk2-devel] Memory Attribute for depex section<= br>
On Fri, Jan 12, 2024 at 9:35=E2=80=AFAM Laszlo Ersek <

On 1/12/24 03:10, Pedro = Falcato wrote:

My idea was to make each handle an index - like = a file descriptor -
AFAIK there's no reason why it *needs* to be an actu= al pointer.
We'd allocate indices when creating a handle, and return tha= t (casted to
Infras= tructure


https://github.com/tianocore/tianocore.github.io/wiki/PerformancePkg=

The former looks quite comprehensive, at a quick skim.


/me nods
I've se= en those macros around, but I've never used them.
In any case, this prob= lem has piqued my interest, I'll see if I can
find some free time this w= eekend to hack on a test benchmark and a PoC
:)

--
Pedro






_._,_._,_
Groups.io Links:

=20 You receive all messages sent to this group. =20 =20

View/Reply Online (#113775) | =20 | Mute= This Topic | New Topic
Your Subscriptio= n | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
--Apple-Mail=_7246A0D5-4722-4289-A9BC-C845549A6BBF--