From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web11.101939.1683531378581831737 for ; Mon, 08 May 2023 00:36:19 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=D0t51rCP; spf=pass (domain: redhat.com, ip: 170.10.133.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1683531377; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=q29vY0QHQ+xjnMM/1rYoSwa4OaekkuitVtjNBKFFl7c=; b=D0t51rCP3sB/Ksjs+J6V3vDDapyMvnmRlOBNkKAfLkqp0sfWdB1A7wFIjaghnkFtsEbrsH taeXaoS6I4dmPmgi37s52UpFY6YtHSi+POhOwWk4NiGBSjaCfLfDC3pOgnA+egYxaqhSms fIla/RcCUwi94UsGlhGJM65v9+ZAYoI= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-248-dHxksPIiPvWgHTD7G3jjww-1; Mon, 08 May 2023 03:36:14 -0400 X-MC-Unique: dHxksPIiPvWgHTD7G3jjww-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.rdu2.redhat.com [10.11.54.8]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 377BC84AF30; Mon, 8 May 2023 07:36:12 +0000 (UTC) Received: from [10.39.192.234] (unknown [10.39.192.234]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 0A5C5C15BA0; Mon, 8 May 2023 07:36:10 +0000 (UTC) Message-ID: Date: Mon, 8 May 2023 09:36:09 +0200 MIME-Version: 1.0 Subject: Re: [edk2-devel] purpose of EFI_LOCK To: Vincent Zimmer , devel@edk2.groups.io Cc: "Andrew (EFI) Fish" , Mike Kinney , Michael Brown , Gerd Hoffmann References: <0cf211a5-ddb1-c2a7-1be4-1776c7068496@redhat.com> <607BADB3-C24D-419D-8ECA-AECD6225BBD2@apple.com> <4bccdf95-6700-e1f2-ba8c-a0e946e820b8@redhat.com> From: "Laszlo Ersek" In-Reply-To: X-Scanned-By: MIMEDefang 3.1 on 10.11.54.8 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Thank you Vincent -- I didn't know anything about the MP approach to edk2 networking, but even more interestingly, I never thought there had been (successful) attempts to sneak interrupt driven device drivers (back) into UEFI / edk2. I've always thought (somewhat vaguely perhaps) that the timer tick was a "set in stone" limit for receiving data. Laszlo On 5/6/23 00:25, Vincent Zimmer wrote: > Random and tl;dr fun friday fact. > You can see the efi locks replaced w/ spinlocks in > https://github.com/tianocore/edk2-staging/tree/MpNetworkStack to > support the use case described in > https://uefi.org/sites/default/files/resources/7_Maciej%20Vincent_INTEL_network%20stack%20performance.pdf. > These changes along w/ a bespoke threading protocol allow for running > efi networking code across all cores and still be able to interact w/ > an enlightened dxe core. We didn't push forward to standardize for > various reasons, such as folks pursuing an alternate solution to the > original problem statement (e.g., LinuxBoot for datacenter network > provisioning) and the huge legacy app compat we'd face. So it's > 'possible', just as you 'could' have i/o device interrupt service > routines (isr) w/ their own 'device' tpl's between notify and high > (see "firmware interrupts" > https://uefi.org/specs/UEFI/2.10/07_Services_Boot_Services.html#tpl-usage) > if you were to expose the appropriate registration and dispatch > interface from the core. I don't think we put anything in staging or > elsewhere public w/ isr examples, though, and I expect trying to > retrofit after 2+ decades would face similar app compat issues w/ > folks who already curate their own proprietary device interrupt sol'n. > Vincent > > On Fri, May 5, 2023 at 12:59 PM Laszlo Ersek wrote: >> >> Hi Andrew, >> >> thank you! The background / paragraph on the naming helps! >> >> Cheers, >> Laszlo >> >> On 5/4/23 20:01, Andrew (EFI) Fish wrote: >>> Laszlo, >>> >>> Hope you are doing well! Sorry to top post but I’m just commenting on >>> the big picture, not answering your specific questions yet. >>> >>> The Wiki definition of lock is something like: In computer science >>> locks are a mechanism that enforces limits on access to a resource when >>> there is the possibility for contention between threads of execution. I >>> think the key point here is “threads of execution” is not threads…. >>> >>> UEFI is single threaded with a cooperative event model. There is no >>> scheduler and an event blocks all forward progress of any event at the >>> same or lower TPL. The protocol services have defined TPL Restrictions >>> [1] so that is possible to implement locking. In the context of EFI >>> raising the TPL blocks any “threads of execution” (events) that could >>> preempt the running code from contending with a critical section. >>> >>> Lets thing about what would happen if you use an atomic primitive as a >>> lock in EFI. Let’s say the app is installing a protocol so the DXE Core >>> has the protocol database locked. Any event that fired in that window >>> would not be able to call any UEFI Boot Service that was related to >>> protocols and expect it to succeed. If the event blocked on the lock, >>> the system is dead locked. If the the lock was tested and failed that >>> basically means it would be normal for any UEFI service to fail in >>> events and event code needed to coded to deal with that. Basically the >>> even could we need to defer to a future time the event gets signaled. I >>> think think this quickly devolves in the event code having to implement >>> a simple scheduler for its set. Thus making locks raise the TPL is just >>> better for everyone. >>> >>> Not that we have always been good at naming things, but in the context >>> of EFI a Lock is best implemented as raising TPL so we made an up level >>> look API to make that clear to people, and to help educate people how >>> locks should be implemented in EFI. >>> >>> This is old, but it is a good sumarry of why we did not want BIOS >>> programmers dealing with threads 20+ years ago when we designed EFI. >>> >>> >>> preview.png >>> threads >>> PDF Document · 107 KB >>> >>> >>> >>> >>> >>> [1] >>> https://uefi.org/specs/UEFI/2.10/07_Services_Boot_Services.html#tpl-restrictions >>> >>> Thanks, >>> >>> Andrew Fish >>> >>>> On May 4, 2023, at 1:45 AM, Laszlo Ersek wrote: >>>> >>>> Hi, >>>> >>>> what benefit does EFI_LOCK add, over direct gBS->RaiseTPL() and >>>> gBS->RestoreTPL() calls? >>>> >>>> Considering just the two APIs EfiAcquireLock() and EfiReleaseLock(): >>>> >>>> - The "Lock" field (effectively, lock status field) is useless; it is >>>> only written to, never read (outside of ASSERT()s) >>>> >>>> - The "OwnerTpl" and "Tpl" fields are just convenience storage for the >>>> new TPL we raise to, and the old TPL we restore. >>>> >>>> Considering the EfiAcquireLockOrFail() API as well: >>>> >>>> - This does read the "Lock" (lock status) field, and if it is >>>> EfiLockAcquired, the RaiseTPL() call is refused. So the idea here seems >>>> to be to ensure a *strict* rise in the TPL. Namely, the RaiseTPL() in >>>> EfiAcquireLockOrFail() would still succeed after the RaiseTPL() in >>>> EfiAcquireLock() -- it is valid to "raise" the TPL to the current TPL >>>> --, but the lock status check prevents that. >>>> >>>> - However (#1), this same "strict" raise would be possible by just >>>> calling RaiseTPL() again, and comparing the returned old TPL against the >>>> TPL we've just set as new. If the old TPL is strictly lower, then we've >>>> just "acquired the lock", otherwise we've "already been holding" the >>>> lock. So, from this perspective, EfiAcquireLockOrFail() doesn't add much. >>>> >>>> - Furthermore (#2), if another agent raised the TPL already, but didn't >>>> use our particlar EFI_LOCK object to do that, then the status stored in >>>> "EFI_LOCK.Lock" will not be able to track anything. >>>> >>>> So what is EFI_LOCK good for? >>>> >>>> Effectively I'm disturbed by the *name* "EFI_LOCK". A lock (or mutex) is >>>> supposed to protect some shared resource. If two agents access the >>>> resource without first acquiring the lock, there's trouble. Therefore >>>> the resource itself becomes qualified as "requiring protection by the >>>> lock, at all times". However, the "current TPL" is *not* a resource like >>>> that. It's a UEFI spec level concept; the RaiseTPL and RestoreTPL boot >>>> services are available to any agents; those agents are *not* required to >>>> go through any kind of extra mutual exclusion. >>>> >>>> I've also considered that maybe EFI_LOCK could provide some protection >>>> against inadvertently *lowering* the TPL via RaiseTPL() (which is >>>> undefined behavior, per spec). But I don't see how -- >>>> EfiAcquireLockOrFail() will happily lower the TPL if the Lock field is >>>> EfiLockReleased, but another agent has meanwhile raised the TPL strictly >>>> above the Tpl field. >>>> >>>> Please explain. :) >>>> >>>> Thanks! >>>> Laszlo >>>> >>> >> >> >> >> >> >> >