On Thu, 2 Jan 2020 at 15:06, Arvind Sankar <nivedita@xxxxxxxxxxxx> wrote: > > On Thu, Jan 02, 2020 at 08:33:54AM +0100, Ard Biesheuvel wrote: > > On Wed, 1 Jan 2020 at 20:08, Arvind Sankar <nivedita@xxxxxxxxxxxx> wrote: > > > > > > On Wed, Jan 01, 2020 at 07:13:45PM +0100, Ard Biesheuvel wrote: > > > > The GCC documentation mentions that it does not make sense for a > > > > function annotated as const not to take any arguments, so I'd rather > > > > avoid it here. > > > > > > Where does it say that? I only see it saying it doesn't make sense for > > > it to return void. > > > > > > > You're right. I looked into this in the past, and I misremembered, and > > paraphrased it incorrectly. > > > > The documentation does mention that const functions are not permitted > > to read global memory. > > > > What it says is that "[the const attribute] tells GCC that subsequent > calls to function square with the same argument value can be replaced by > the result of the first call regardless of the statements in between" > and "The const attribute prohibits a function from reading objects that > affect its return value between successive invocations. However, > functions declared with the attribute can safely read objects that do > not change their return value, such as non-volatile constants." > > In this case since the globals in question never change after being set, > it should be safe -- every call to these functions does return the same > value. > I agree that it should be safe. However, the document I looked at literally says """ Many functions do not examine any values except their arguments, and have no effects except the return value. Basically this is just slightly more strict class than the pure attribute below, since function is not allowed to read global memory. """ However, this is actually a fairly old version of the docs [0], and the most recent version seems to permit it. [0] https://gcc.gnu.org/onlinedocs/gcc-4.7.2/gcc/Function-Attributes.html > > > Currently if we call 5 EFI services in the same function, it has to > > > re-evaluate systemtable and is64 for each call, which seems wasteful, > > > though of course this is not exactly performance-critical code. > > > > The alternative would be to use globals with external linkage in a way > > that is guaranteed not to rely on GOT entries, since we'll end up with > > absolute addresses that need to be fixed up first. This has caused > > breakage in the past, and is the reason we use this scheme with > > globals with static linkage and __pure getters. > > > > However, hidden visibility should yield the same results so we should > > be able to make it work with that instead. However, given the breakage > > in the past, I don't think it's worth it since the performance gain > > will be negligible. > > This only saves the overhead of the function call, but the compiler > would still have to re-read the variables and re-test them, since so far > as it knows the firmware call could change them. With the const function > attribute, it will only load them once. To make it work with just the > variables, they'd have to be declared const and initialized in the > assembly entry code. Yeah, good point. I pushed as branch here; https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=efi-libstub-attr-const Could you please check if that fixes the issue for efi_is_64bit() ?
