On Tue, Jul 25, 2017 at 5:01 PM, Heinrich Schuchardt <xypron.g...@gmx.de> wrote: > On 07/25/2017 09:42 PM, Rob Clark wrote: >> On Tue, Jul 25, 2017 at 3:12 PM, Heinrich Schuchardt <xypron.g...@gmx.de> >> wrote: >>> On 07/25/2017 07:52 PM, Rob Clark wrote: >>>> On Tue, Jul 25, 2017 at 12:56 PM, Heinrich Schuchardt >>>> <xypron.g...@gmx.de> wrote: >>>>> On 07/25/2017 04:28 PM, Alexander Graf wrote: >>>>>> >>>>>> >>>>>> On 25.07.17 14:28, Rob Clark wrote: >>>>>>> On Tue, Jul 25, 2017 at 8:16 AM, Alexander Graf <ag...@suse.de> wrote: >>>>>>>> >>>>>>>> >>>>>>>> On 25.07.17 14:08, Rob Clark wrote: >>>>>>>>> >>>>>>>>> On Tue, Jul 25, 2017 at 7:17 AM, Alexander Graf <ag...@suse.de> wrote: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On 25.07.17 13:10, Rob Clark wrote: >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On Tue, Jul 25, 2017 at 4:10 AM, Alexander Graf <ag...@suse.de> >>>>>>>>>>> wrote: >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> On 25.07.17 01:47, Rob Clark wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> To implement efi_load_image() for the case of loading an image >>>>>>>>>>>>> from a >>>>>>>>>>>>> device path rather than image already loaded into source_buffer, >>>>>>>>>>>>> it is >>>>>>>>>>>>> convenient to be able to re-use simple-file-system and efi-file >>>>>>>>>>>>> interfaces. But these are already using EFI_ENTER()/EFI_EXIT(). >>>>>>>>>>>>> Allow >>>>>>>>>>>>> entry into EFI interfaces to be recursive, since this greatly >>>>>>>>>>>>> simplifies >>>>>>>>>>>>> things. >>>>>>>>>>>>> >>>>>>>>>>>>> (And hypothetically this would be needed anyways to allow grub >>>>>>>>>>>>> to call >>>>>>>>>>>>> into interfaces installed by something outside of grub.) >>>>>>>>>>>>> >>>>>>>>>>>>> Signed-off-by: Rob Clark <robdcl...@gmail.com> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> So there are 2 ways to do this: >>>>>>>>>>>> >>>>>>>>>>>> 1) Keep a refcount, only transition when passing the 0 level >>>>>>>>>>>> 2) Explicitly split functions with ENTRY/EXIT from their core >>>>>>>>>>>> functions >>>>>>>>>>>> >>>>>>>>>>>> So far we used approach 2, splitting functions that are used by >>>>>>>>>>>> both >>>>>>>>>>>> internal and external code into _ext (for externally called) and >>>>>>>>>>>> normal >>>>>>>>>>>> functions. You can see this pattern quite a few times throughout >>>>>>>>>>>> efi_loader. >>>>>>>>>>>> >>>>>>>>>>>> I definitely did try the refcount approach back when I decided for >>>>>>>>>>>> approach >>>>>>>>>>>> 2 and it failed on me in some case, but I can't remember where. >>>>>>>>>>>> >>>>>>>>>>>> Either way, we should definitely be consistent. Either we always >>>>>>>>>>>> use >>>>>>>>>>>> refcounting or we shouldn't bother with it. So if you can make a >>>>>>>>>>>> version >>>>>>>>>>>> work where all _ext variants disappear and we're magically >>>>>>>>>>>> reentrant, >>>>>>>>>>>> I'll >>>>>>>>>>>> be happy to take that. I'm fairly sure it'll break somewhere >>>>>>>>>>>> though :). >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> for load_image via file-path, we end up needing a *bunch* of >>>>>>>>>>> functions >>>>>>>>>>> normally called via EFI.. so it is going to be a lot more _ext >>>>>>>>>>> variants. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Yes, but imagine a case like this: >>>>>>>>>> >>>>>>>>>> open_protocol >>>>>>>>>> -> open() >>>>>>>>>> -> random_junk() >>>>>>>>>> -> efi_timer_check() >>>>>>>>>> -> timer notifier >>>>>>>>>> -> console print() >>>>>>>>>> >>>>>>>>>> Here the refcounting will simply fail on us, as the timer notifier >>>>>>>>>> needs >>>>>>>>>> to >>>>>>>>>> be run in UEFI context while efi_timer_check() as well as console >>>>>>>>>> print() >>>>>>>>>> need to be run in U-Boot context. >>>>>>>>>> >>>>>>>>>> And if we start to mix and mesh the 2 approaches, I can promise you >>>>>>>>>> that >>>>>>>>>> 2 >>>>>>>>>> weeks down some corner case nobody thought of falls apart because >>>>>>>>>> people >>>>>>>>>> don't manage to fully grasp the code flow anymore. >>>>>>>>>> >>>>>>>>> >>>>>>>>> ugg.. gd is a gd pita ;-) >>>>>>>>> >>>>>>>>> how many places do we have callbacks into UEFI context like this? If >>>>>>>>> just one or two maybe we can suppress them while in u-boot context and >>>>>>>>> handle in efi_exit_func() when entry_count drops to zero? >>>>>>>> >>>>>>>> >>>>>>>> What do you mean by suppressing? We need to call those helpers >>>>>>>> synchronously. And yes, we can probably hand massage the refcount on >>>>>>>> the >>>>>>>> ones that we remember, but I'm just afraid that the whole thing will >>>>>>>> be so >>>>>>>> complicated eventually that nobody understands what's going on. >>>>>>> >>>>>>> Maybe suppress isn't the right word.. I was thinking of delaying the >>>>>>> callback until EFI_EXIT() that transitions back to the UEFI world. So >>>>>>> from the PoV of the UEFI world, it is still synchronous. >>>>>>> >>>>>>> I haven't looked at the efi_event stuff until just now, but from a >>>>>>> 30sec look, maybe efi_signal_event() could just put the event on a >>>>>>> list of signalled events and not immediately call >>>>>>> event->notify_function(), and handle the calling of notify_function() >>>>>>> in the last EFI_EXIT()?? >>>>>> >>>>>> Maybe, I'll leave Heinrich to comment on that. >>>>> >>>>> Let the application call WaitForEvent for a keyboard event. >>>>> Rob suggests that neither timer events nor any other events are served >>>>> until the user possibly presses a key. >>>>> >>>>> No, we have to call notification functions immediately to serve time >>>>> critical communication like network traffic. >>>>> >>>> >>>> Does WaitForEvent or anything else that dispatches events dispatch >>>> more than one event, or is it just one event and then return to UEFI >>>> world to poll again? We aren't really multi-threaded so there can't >>>> really be async callbacks. >>>> >>>> Either way, I think we can make this work by changing >>>> EFI_EXIT()/callback/EFI_ENTER() to EFI_CALLBACK(callback).. for the >>>> non-nested EFI_ENTER() case we don't have to do anything special (and >>>> the nested case probably should never happen in places where we need >>>> EFI_CALLBACK()) >>> >>> WaitForEvent and CheckEvent calls will serve all notify_functions of >>> timer events this includes a notify_function checking the keyboard and >>> in future I want to check the watchdog in a notify_function. >>> >>> Please, consider that in the notify functions any number of UEFI API >>> functions will be called by the EFI application which themselves may >>> signal events which in turn call notify_functions in which again UEFI >>> API functions may be called. >> >> sure, one way or another I don't want to call a notifier from nested >> EFI_ENTER()'s.. > > We call EFI_EXIT before calling the notifier function and > call EFI_ENTER after returning form the notifier function. > > So currently we always switch gd correctly. An nested EFI_ENTER simply > does not occur.
yes, I understand how it currently works.. although I think an EFI_CALLBACK() macro that did the equiv (with extra error checking if we allow re-entrant EFI_ENTER()) would be clearer. It would make the callbacks to UEFI world easy to spot and make the code a bit more concise. > I still do not understand why you need to change anything here. > >> >> so either EFI_CALLBACK() should detect the condition and yell loudly, >> or it should detect it and queue up callbacks until we are not >> nested.. > > Your suggestions are getting more and more complicated. > > I would always go for easily maintainable code and not for > sophistication. Please, stick to the KISS principle. > > If you want to call functions internally as you described in the first > mail in this thread, do not call the UEFI functions but supply separate > entry points. The issue that I am trying to address at the moment is that efi_load_image(), to load an image specified by file_path (as opposed to already loaded into source_buffer, ie. the source_buffer==NULL case, which UEFI spec allows) requires using a handful of APIs from the EFI file interface, and also requires the simple-file-system-protocol, to actually read the image specified by file_path into a buffer. We could go down the path of continuing to add _ext functions.. but it is a bit ugly, and seems like it would keep growing worse. And it requires exposing functions from different modules which could otherwise remain static. All for a case that won't happen (for these particular APIs at least) or at least is easy to detect. Keeping the _ext functions for only the special cases that could lead to a callback into UEFI world, and avoiding them in the normal case, seems cleaner. It isn't terribly complicated to implement, and it is easy to detect programming errors. At a *minimum* I'd say that the current EFI_ENTRY()/EFI_EXIT() should detect nesting (since the result if you screw that up is a less than obvious crash). And an EFI_CALLBACK() macro would make the callback into UEFI world more obvious via grep. Once you have that, it is trivial to allow nesting and detect problematic EFI_CALLBACK(). BR, -R _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
