RE: [PATCH V2 2/3] efi: Introduce efi_rts_workqueue and some infrastructure to invoke all efi_runtime_services()

[Prakhya, Sai Praneeth]

> > That's true! AFAIK, we don't have any issues handling NMI while in efi_pgd.
> > We might have issues only when, we are already in efi_pgd, NMI comes
> > along
> 
> Can you trigger this? Or is it something hypothetical?
> 

AFAIK, it's hypothetical. I did try to trigger the issue, but failed [1].
Maybe, I need to have some more constraints [2].

[1] https://lkml.org/lkml/2017/8/23/715
[2] https://lkml.org/lkml/2017/8/25/469

> > and NMI handler tries to touch the regions that are not mapped in
> > efi_pgd
> 
> If it is not hypothetical, the NMI handler should learn to look at CR3 first 
> and
> return if CR3 has the efi pgd.

This solution and it's variants were discussed here [1], [2] and for varied 
reasons 
the community had decided to go with "Everything EFI as kthread" approach [3] 
[4].

Although the discussions were off my understanding, the present issue I see is, 
(and also the motivation for me to do the patch is)
when a thread tries to execute any  efi_runtime_service() we switch to efi_pgd 
(which doesn't have user space mappings) and all other subsystems in kernel 
aren't aware of this switch. This looks like a perfect case for kthread.

Kthread by definition doesn’t have user space mappings and if we run 
efi_runtime_services()
in a kthread context and if any other subsystem tries to access user space 
mappings 
while in efi_kthread, it's terminally broken [5].

There were several issues Andy, Peter and Mark raised.
One such (hypothetical) case is accessing user space from the back of an 
interrupt (NMI).
Others include
1. Issue specific to ARM because it runs efi_runtime_services() with interrupts 
enabled [6]
2. Interrupt taken while mmap_sem() is held for write that tries to access user 
memory [7]
3. If EFI were to have IO memory mapped at a "user" address, perf could end up 
reading it [8]

[1] https://lkml.org/lkml/2017/8/15/757
[2] https://lkml.org/lkml/2017/8/16/487
[3] https://lkml.org/lkml/2017/8/21/573
[4] https://lkml.org/lkml/2017/8/16/540

[5] https://lkml.org/lkml/2017/8/17/667
[6] https://lkml.org/lkml/2017/8/16/176
[7] https://lkml.org/lkml/2017/8/17/667
[8] https://lkml.org/lkml/2017/8/21/427

Regards,
Sai

Re: Regression from efi: call get_event_log before ExitBootServices

[James Bottomley]

On Fri, 2018-03-09 at 10:29 +0100, Hans de Goede wrote:
> Hi,
> 
> On 08-03-18 18:26, Jeremy Cline wrote:
> > 
> > On 03/08/2018 11:50 AM, Hans de Goede wrote:
[...]
> > > > The UEFI firmware does some measurements and so does shim. So
> > > > you should have some event logs. What version of shim are you
> > > > using? And also would be good to know if it's the same shim
> > > > version that Jeremy is using.
> > > 
> > > That is a very good question, I'm using: shim-ia32-13-0.7.x86_64, 
> > > which is the last version for F27 AFAICT.
> > 
> > All my tablet has installed is shim-0.8-10.x86_64, no shim-ia32.
> 
> Yes my bad, although if the kernel changes break booting on systems
> without the shim that is still good to know and something which
> we probably ought to fix.

My laptop is set up with secure boot but without shim using a shim
protocol thin layer to check the kernel signature against db variables:

https://git.kernel.org/pub/scm/linux/kernel/git/jejb/efitools.git/tree/ShimReplace.c

and I haven't seen any breakage, so not having a shim that does
measurements works for me all the way up to -rc4.

James

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Regression from efi: call get_event_log before ExitBootServices

[Jeremy Cline]

On Fri, Mar 09, 2018 at 10:43:50AM +, Thiebaud Weksteen wrote:
> Thanks a lot for trying out the patch!
> 
> Please don't modify your install at this stage, I think we are hitting a
> firmware bug and that would be awesome if we can fix how we are handling it.
> So, if we reach that stage in the function it could either be that:
> * The allocation did not succeed, somehow, but the firmware still returned
> EFI_SUCCEED.
> * The size requested is incorrect (I'm thinking something like a 1G of
> log). This would be due to either a miscalculation of log_size (possible)
> or; the returned values of GetEventLog are not correct.
> I'm sending a patch to add checks for these. Could you please apply and
> retest?
> Again, thanks for helping debugging this.

No problem, thanks for the help :)

With the new patch:

Locating the TCG2Protocol
Calling GetEventLog on TCG2Protocol
Log returned
log_location is not empty
log_size != 0
log_size < 1M
Allocating memory for storing the logs
Returned from memory allocation
Copying log to new location

And then it hangs. I added a couple more print statements:

diff --git a/drivers/firmware/efi/libstub/tpm.c 
b/drivers/firmware/efi/libstub/tpm.c
index ee3fac109078..1ab5638bc50e 100644
--- a/drivers/firmware/efi/libstub/tpm.c
+++ b/drivers/firmware/efi/libstub/tpm.c
@@ -148,8 +148,11 @@ void efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t 
*sys_table_arg)
efi_printk(sys_table_arg, "Copying log to new location\n");
 
memset(log_tbl, 0, sizeof(*log_tbl) + log_size);
+   efi_printk(sys_table_arg, "Successfully memset log_tbl to 0\n");
log_tbl->size = log_size;
+   efi_printk(sys_table_arg, "Set log_tbl->size\n");
log_tbl->version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
+   efi_printk(sys_table_arg, "Set log_tbl-version\n");
memcpy(log_tbl->log, (void *) first_entry_addr, log_size);
 
efi_printk(sys_table_arg, "Installing the log into the configuration 
table\n");

and it's hanging at "memset(log_tbl, 0, sizeof(*log_tbl) + log_size);"

Regards,
Jeremy
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 2/3] efi: Introduce efi_rts_workqueue and some infrastructure to invoke all efi_runtime_services()

[Borislav Petkov]

On Fri, Mar 09, 2018 at 02:37:59AM +, Prakhya, Sai Praneeth wrote:
> But, I guess, we have some downsides with this design:
> 1. We are doing this to have "no exceptions to use efi_rts_wq", but we will 
> be making
> the common case complicated. i.e. When a user requests to write some efi 
> variable,

So if the pstore use case is so important and special, I think we should
make the EFI path as fast as possible as getting that data to the pstore
is a priority.

> Alternatively, instead of playing around with in_atomic(), we could have 
> wrapper
> functions like efi_write_var_non_wq() which will only be used by pstore. This 
> function
> will not use efi_rts_wq and directly invoke efi_runtime_service. Just an 
> attempt to
> make the code not look messy.

I guess.

If the write-to-pstore case is such a critical one, I guess the
exception is justified.

> That's true! AFAIK, we don't have any issues handling NMI while in efi_pgd.
> We might have issues only when, we are already in efi_pgd, NMI comes along

Can you trigger this? Or is it something hypothetical?

> and NMI handler tries to touch the regions that are not mapped in efi_pgd

If it is not hypothetical, the NMI handler should learn to look at CR3
first and return if CR3 has the efi pgd.

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 2/3] efi: Introduce efi_rts_workqueue and some infrastructure to invoke all efi_runtime_services()

[Borislav Petkov]

On Thu, Mar 08, 2018 at 05:05:44PM +, Luck, Tony wrote:
> > "Hence, pstore calls efi_runtime_services() without using efi_rts_wq" -
> > that doesn't sound like optimal design to me. I would try to shove them
> > all through the workqueue - not have exceptions.
> 
> But pstore is trying to save the last gasp dying words from a kernel that
> has paniced. There isn't any guarantee that work queue will run. I think
> it is reasonable to have some special case to make sure that we do save
> the information.  But perhaps that special case should be to have pstore
> call directly into the guts of the EFI code and not worry about all these
> fancy switches of "mm" etc.

I guess...

> This is true for the machine check logging case too, but the mitigation is
> that the details of the error persist in the machine check banks across the
> reset ... so all is not lost if the work queue isn't run here.

Well, I'm still hoping to have this wonderful and reliable
logging facility one day where we can dump bytes into a
persistent-across-reboots buffer and analyze them later. And yap, in
that case, I don't mind if the code simply bypasses all the dancing in
the OS and writes those bytes. Even switching pagetables would be too
much for that case - just fire'n'forget writing from ring 0.

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Regression from efi: call get_event_log before ExitBootServices

[Thiebaud Weksteen]

---
 drivers/firmware/efi/libstub/tpm.c | 21 +
 1 file changed, 21 insertions(+)

diff --git a/drivers/firmware/efi/libstub/tpm.c 
b/drivers/firmware/efi/libstub/tpm.c
index 773afcd6a37c..ee3fac109078 100644
--- a/drivers/firmware/efi/libstub/tpm.c
+++ b/drivers/firmware/efi/libstub/tpm.c
@@ -112,6 +112,22 @@ void efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t 
*sys_table_arg)
log_size = log_last_entry - log_location + last_entry_size;
}
 
+   if (log_size == 0) {
+   efi_printk(sys_table_arg, "log_size == 0\n");
+   }
+   else if (log_size < 1 * 1024 * 1024) {
+   efi_printk(sys_table_arg, "log_size < 1M\n");
+   }
+   else if (log_size < 500 * 1024 * 1024) {
+   efi_printk(sys_table_arg, "log_size < 500M\n");
+   }
+   else if (log_size < 1000 * 1024 * 1024) {
+   efi_printk(sys_table_arg, "log_size < 1G\n");
+   }
+   else {
+   efi_printk(sys_table_arg, "log_size > 1G\n");
+   }
+
efi_printk(sys_table_arg, "Allocating memory for storing the logs\n");
/* Allocate space for the logs and copy them. */
status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
@@ -124,6 +140,11 @@ void efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t 
*sys_table_arg)
   "Unable to allocate memory for event log\n");
return;
}
+   if (!log_tbl) {
+   efi_printk(sys_table_arg, "Pointer returned from allocation is 
NULL!\n");
+   return;
+   }
+
efi_printk(sys_table_arg, "Copying log to new location\n");
 
memset(log_tbl, 0, sizeof(*log_tbl) + log_size);
-- 
2.16.2.395.g2e18187dfd-goog

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Regression from efi: call get_event_log before ExitBootServices

[Thiebaud Weksteen]

On Fri, Mar 9, 2018 at 10:29 AM Hans de Goede  wrote:

> Hi,

> On 08-03-18 18:26, Jeremy Cline wrote:
> > On 03/08/2018 11:50 AM, Hans de Goede wrote:
> >>  >> added these now>
> >>
> >> Hi,
> >>
> >> On 07-03-18 12:34, Javier Martinez Canillas wrote:
> >
> > 
> >
> >>> Are you also able to read the TPM event logs?
> >>>
> >>> $ hexdump /sys/kernel/security/tpm0/binary_bios_measurements
> >>
> >> Yes for me that outputs a lot of hex :)
> >
> > For me, /sys/kernel/security/tmp0 doesn't exist on 4.15.6 or 4.16 with
> > the patch reverted.

> Hmm, have you re-enabled the TPM in the BIOS?

> >>> The UEFI firmware does some measurements and so does shim. So you
should
> >>> have some event logs. What version of shim are you using? And also
would
> >>> be good to know if it's the same shim version that Jeremy is using.
> >>
> >> That is a very good question, I'm using: shim-ia32-13-0.7.x86_64,
which is
> >> the last version for F27 AFAICT.
> >
> > All my tablet has installed is shim-0.8-10.x86_64, no shim-ia32.

> Yes my bad, although if the kernel changes break booting on systems
> without the shim that is still good to know and something which
> we probably ought to fix.

> >> But Jeremy's tablet might very well be not using the shim at all, as
> >> I manually installed Fedora 25 on the tablet he now has, before Fedora
> >> supported
> >> machines with 32 bit EFI. I then later did a "dnf distro-sync" to
> >> Fedora-27.
> >>
> >> Jeremy might also very well still be booting using a grub binary I
build
> >> manually back then, without any shim being involved.
> >>
> >> Jeremy what does efibootmgr -v output on your device ?
> >
> > # efibootmgr -v
> > BootCurrent: 0003
> > Timeout: 4 seconds
> > BootOrder: 0003,,0001,2001,2002,2003
> > Boot* Android X64 OS
> >
HD(1,GPT,215e6cf3-e97d-4735-9c4e-7338c8f5a645,0x800,0x32000)/File(\EFI\BOOT\bootx64.efi)RC
> > Boot0001* Internal EFI Shell
> >
FvVol(a881d567-6cb0-4eee-8435-2e72d33e45b5)/FvFile(c57ad6b7-0515-40a8-9d21-551652854e37)RCM&".
> > Boot0003* Fedora
> >
HD(1,GPT,215e6cf3-e97d-4735-9c4e-7338c8f5a645,0x800,0x32000)/File(\EFI\fedora\grubx64.efi)
> > Boot2001* EFI USB Device  RC
> > Boot2002* EFI DVD/CDROM   RC
> > Boot2003* EFI Network RC
> > Boot8087* Udm
> >
FvVol(a881d567-6cb0-4eee-8435-2e72d33e45b5)/FvFile(9a9ab4c1-ee1b-488b-b300-24544a7bd418)
> >
> > I think you're right about it using the old grub binary. I'm
> > embarrassingly unfamiliar with both UEFI and grub, but I'm guessing you
> > set the location of grub.cfg at compile time? When I boot
> > \EFI\fedora\grubx64.efi, it's pulling the grub.cfg from
> > \EFI\redhat\grub.cfg.

> Ah yes, so I did not build my own grub I took one from RHEL as that had
> 32 bit UEFI support before Fedora got it and as I was lazy I copied the
> 32 bit binary over the 64 bit one, so don't let the filename fool you.

> What you could do is install grub2-efi-ia32 from the Fedora 27 repos
> and then use efibootmgr to add an entry pointing to
\EFI\fedora\grubia32.efi
> note that one will look at \EFI\fedora\grub.cfg .

> Then see if the problem persists. A second step would be to also install
> shim-ia32 and point to that...

Thanks a lot for trying out the patch!

Please don't modify your install at this stage, I think we are hitting a
firmware bug and that would be awesome if we can fix how we are handling it.
So, if we reach that stage in the function it could either be that:
* The allocation did not succeed, somehow, but the firmware still returned
EFI_SUCCEED.
* The size requested is incorrect (I'm thinking something like a 1G of
log). This would be due to either a miscalculation of log_size (possible)
or; the returned values of GetEventLog are not correct.
I'm sending a patch to add checks for these. Could you please apply and
retest?
Again, thanks for helping debugging this.


> Regards,

> Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 07/12] efi: Use efi_mm in x86 as well as ARM

[Ard Biesheuvel]

On 9 March 2018 at 08:37, Prakhya, Sai Praneeth
 wrote:
>> > diff --git a/include/linux/efi.h b/include/linux/efi.h index
>> > f5083aa72eae..f1b7d68ac460 100644
>> > --- a/include/linux/efi.h
>> > +++ b/include/linux/efi.h
>> > @@ -966,6 +966,8 @@ extern struct efi {
>> > unsigned long flags;
>> >  } efi;
>> >
>> > +extern struct mm_struct efi_mm;
>> > +
>> >  static inline int
>> >  efi_guidcmp (efi_guid_t left, efi_guid_t right)  {
>>
>> Ugh, I can see three problems with this patch:
>>
>> 1)
>>
>> Why is the low level asm/efi.h header polluted with two of the biggest header
>> files in existence, to add a type to _another_ header (efi.h)?
>>
>> 2)
>>
>> Why is  included if what is being relied on is mm_struct?
>>
>> 3)
>>
>> But even  looks unnecessary in efi.h, a simple forward
>> declaration of mm_struct would do ...
>>
>> The high level MM and sched headers should be added to the actual .c files 
>> that
>> make use of them.
>
> Ok, makes sense.
> Sorry! for that. I will fix the issues.
>

I have some other fixups to do, so if this is as easy as it seems
(remove the #includes and add the forward declaration), I can fix it
up and resend it for you.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 12/12] efi: make const array 'apple' static

[Joe Perches]

On Fri, 2018-03-09 at 07:44 +, Ard Biesheuvel wrote:
> On 9 March 2018 at 07:43, Ard Biesheuvel  wrote:
> > On 8 March 2018 at 11:05, Joe Perches  wrote:
> > > On Thu, 2018-03-08 at 08:00 +, Ard Biesheuvel wrote:
> > > > From: Colin Ian King 
> > > > 
> > > > Don't populate the const read-only array 'buf' on the stack but instead
> > > > make it static. Makes the object code smaller by 64 bytes:
> > > > 
> > > > Before:
> > > >text  data bss dec hex filename
> > > >9264 1  1692812441 
> > > > arch/x86/boot/compressed/eboot.o
> > > > 
> > > > After:
> > > >text  data bss dec hex filename
> > > >9200 1  1692172401 
> > > > arch/x86/boot/compressed/eboot.o
> > > > 
> > > > (gcc version 7.2.0 x86_64)
> > > > 
> > > > Signed-off-by: Colin Ian King 
> > > > Signed-off-by: Ard Biesheuvel 
> > > > ---
> > > >  arch/x86/boot/compressed/eboot.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/arch/x86/boot/compressed/eboot.c 
> > > > b/arch/x86/boot/compressed/eboot.c
> > > > index 886a9115af62..f2251c1c9853 100644
> > > > --- a/arch/x86/boot/compressed/eboot.c
> > > > +++ b/arch/x86/boot/compressed/eboot.c
> > > > @@ -423,7 +423,7 @@ static void retrieve_apple_device_properties(struct 
> > > > boot_params *boot_params)
> > > > 
> > > >  static void setup_quirks(struct boot_params *boot_params)
> > > >  {
> > > > - efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 };
> > > > + static efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 
> > > > };
> > > 
> > > Perhaps
> > > 
> > > static const efi_char16_t apple[] ...
> > > 
> > > is better.
> > > 
> > 
> > Why would that be any better?

It'd be more like the the style used
in the rest of the kernel.

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Regression from efi: call get_event_log before ExitBootServices

[Hans de Goede]

Hi,

On 08-03-18 18:26, Jeremy Cline wrote:

On 03/08/2018 11:50 AM, Hans de Goede wrote:



Hi,

On 07-03-18 12:34, Javier Martinez Canillas wrote:





Are you also able to read the TPM event logs?

$ hexdump /sys/kernel/security/tpm0/binary_bios_measurements


Yes for me that outputs a lot of hex :)


For me, /sys/kernel/security/tmp0 doesn't exist on 4.15.6 or 4.16 with
the patch reverted.


Hmm, have you re-enabled the TPM in the BIOS?


The UEFI firmware does some measurements and so does shim. So you should
have some event logs. What version of shim are you using? And also would
be good to know if it's the same shim version that Jeremy is using.


That is a very good question, I'm using: shim-ia32-13-0.7.x86_64, which is
the last version for F27 AFAICT.


All my tablet has installed is shim-0.8-10.x86_64, no shim-ia32.


Yes my bad, although if the kernel changes break booting on systems
without the shim that is still good to know and something which
we probably ought to fix.


But Jeremy's tablet might very well be not using the shim at all, as
I manually installed Fedora 25 on the tablet he now has, before Fedora
supported
machines with 32 bit EFI. I then later did a "dnf distro-sync" to
Fedora-27.

Jeremy might also very well still be booting using a grub binary I build
manually back then, without any shim being involved.

Jeremy what does efibootmgr -v output on your device ?


# efibootmgr -v
BootCurrent: 0003
Timeout: 4 seconds
BootOrder: 0003,,0001,2001,2002,2003
Boot* Android X64 OS
HD(1,GPT,215e6cf3-e97d-4735-9c4e-7338c8f5a645,0x800,0x32000)/File(\EFI\BOOT\bootx64.efi)RC
Boot0001* Internal EFI Shell
FvVol(a881d567-6cb0-4eee-8435-2e72d33e45b5)/FvFile(c57ad6b7-0515-40a8-9d21-551652854e37)RCM&".
Boot0003* Fedora
HD(1,GPT,215e6cf3-e97d-4735-9c4e-7338c8f5a645,0x800,0x32000)/File(\EFI\fedora\grubx64.efi)
Boot2001* EFI USB DeviceRC
Boot2002* EFI DVD/CDROM RC
Boot2003* EFI Network   RC
Boot8087* Udm
FvVol(a881d567-6cb0-4eee-8435-2e72d33e45b5)/FvFile(9a9ab4c1-ee1b-488b-b300-24544a7bd418)

I think you're right about it using the old grub binary. I'm
embarrassingly unfamiliar with both UEFI and grub, but I'm guessing you
set the location of grub.cfg at compile time? When I boot
\EFI\fedora\grubx64.efi, it's pulling the grub.cfg from
\EFI\redhat\grub.cfg.


Ah yes, so I did not build my own grub I took one from RHEL as that had
32 bit UEFI support before Fedora got it and as I was lazy I copied the
32 bit binary over the 64 bit one, so don't let the filename fool you.

What you could do is install grub2-efi-ia32 from the Fedora 27 repos
and then use efibootmgr to add an entry pointing to \EFI\fedora\grubia32.efi
note that one will look at \EFI\fedora\grub.cfg .

Then see if the problem persists. A second step would be to also install
shim-ia32 and point to that...

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 07/12] efi: Use efi_mm in x86 as well as ARM

[Prakhya, Sai Praneeth]

> > diff --git a/include/linux/efi.h b/include/linux/efi.h index
> > f5083aa72eae..f1b7d68ac460 100644
> > --- a/include/linux/efi.h
> > +++ b/include/linux/efi.h
> > @@ -966,6 +966,8 @@ extern struct efi {
> > unsigned long flags;
> >  } efi;
> >
> > +extern struct mm_struct efi_mm;
> > +
> >  static inline int
> >  efi_guidcmp (efi_guid_t left, efi_guid_t right)  {
> 
> Ugh, I can see three problems with this patch:
> 
> 1)
> 
> Why is the low level asm/efi.h header polluted with two of the biggest header
> files in existence, to add a type to _another_ header (efi.h)?
> 
> 2)
> 
> Why is  included if what is being relied on is mm_struct?
> 
> 3)
> 
> But even  looks unnecessary in efi.h, a simple forward
> declaration of mm_struct would do ...
> 
> The high level MM and sched headers should be added to the actual .c files 
> that
> make use of them.

Ok, makes sense.
Sorry! for that. I will fix the issues.

Regards,
Sai
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 12/12] efi: make const array 'apple' static

[Ard Biesheuvel]

Hi Lukas,

On 9 March 2018 at 08:29, Lukas Wunner  wrote:
> On Fri, Mar 09, 2018 at 08:47:19AM +0100, Ingo Molnar wrote:
>> * Ard Biesheuvel  wrote:
>> > From: Colin Ian King 
>> >
>> > Don't populate the const read-only array 'buf' on the stack but instead
>> > make it static. Makes the object code smaller by 64 bytes:
>> >
>> > Before:
>> >textdata bss dec hex filename
>> >9264   1  1692812441 
>> > arch/x86/boot/compressed/eboot.o
>> >
>> > After:
>> >textdata bss dec hex filename
>> >9200   1  1692172401 
>> > arch/x86/boot/compressed/eboot.o
>> >
>> > (gcc version 7.2.0 x86_64)
>> >
>> > Signed-off-by: Colin Ian King 
>> > Signed-off-by: Ard Biesheuvel 
>> > ---
>> >  arch/x86/boot/compressed/eboot.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/arch/x86/boot/compressed/eboot.c 
>> > b/arch/x86/boot/compressed/eboot.c
>> > index 886a9115af62..f2251c1c9853 100644
>> > --- a/arch/x86/boot/compressed/eboot.c
>> > +++ b/arch/x86/boot/compressed/eboot.c
>> > @@ -423,7 +423,7 @@ static void retrieve_apple_device_properties(struct 
>> > boot_params *boot_params)
>> >
>> >  static void setup_quirks(struct boot_params *boot_params)
>> >  {
>> > -   efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 };
>> > +   static efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 };
>> > efi_char16_t *fw_vendor = (efi_char16_t *)(unsigned long)
>> > efi_table_attr(efi_system_table, fw_vendor, sys_table);
>>
>> As a general policy, please don't put 'static' variables into the local
>> scope, use file scope instead - right before setup_quirks() would be fine.
>
> Well, I believe the end result is the same and the closer the declaration
> is to where it's used, the easier the code is to read and understand.
>
> I object to patches like this because they paper over a missing
> compiler optimization:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68725
>
> I have told Colin before that it would be more useful to look into
> fixing the underlying compiler issue rather than polluting the kernel
> with "static" keywords, but he keeps sending these patches so I've
> given up responding:
> https://lkml.org/lkml/2017/8/25/636
>
>
>> Plus an unicode string literal initializer would be pretty descriptive
>> as well,  instead of the weird looking character array, i.e. something
>> like:
>>
>>   static efi_char16_t const apple_unicode_str[] = u"Apple";
>>
>> ... or so?
>
> Last time I checked this didn't work, I believe it's because it's C11
> and the kernel is compiled with -std=gnu89.  And I'm also not sure if
> the oldest gcc version that we support understands u"".
>

Indeed

arch/x86/platform/efi/quirks.c:78:46: error: 'u' undeclared here (not
in a function); did you mean 'up'?

 static const efi_char16_t efi_dummy_name[] = u"DUMMY";
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 12/12] efi: make const array 'apple' static

[Ingo Molnar]

* Ard Biesheuvel  wrote:

> On 9 March 2018 at 08:04, Ingo Molnar  wrote:
> >
> > * Ard Biesheuvel  wrote:
> >
> >> > Also, would it make sense to rename it to something more descriptive like
> >> > "apple_unicode_str[]" or so?
> >> >
> >> > Plus an unicode string literal initializer would be pretty descriptive 
> >> > as well,
> >> > instead of the weird looking character array, i.e. something like:
> >> >
> >> >   static efi_char16_t const apple_unicode_str[] = u"Apple";
> >> >
> >> > ... or so?
> >> >
> >>
> >> is u"xxx" the same as L"xxx"?
> >
> > So "L" literals map to wchar_t, which wide character type is implementation
> > specific IIRC, could be 16-bit or 32-bit wide.
> >
> > u"" literals OTOH are specified by the C11 spec to be char16_t, i.e. 16-bit 
> > wide
> > characters - which I assume is the EFI type as well?
> >
> >> In any case, this is for historical reasons: at some point (and I
> >> don't remember the exact details) we had a conflict at link time with
> >> objects using 4 byte wchar_t, so we started using this notation to be
> >> independent of the size of wchar_t. That issue no longer exists so we
> >> should be able to get rid of this.
> >
> > Yes, my guess is that those problems were due to L"xyz" mapping to wchar_t 
> > and
> > having a different type in the kernel build and the host build side - but 
> > u"xyz"
> > should solve that.
> >
> 
> Excellent!

Please double check the generated code though, all of this is from memory.

> Do you mind taking this patch as is? I will follow up with a patch
> that updates all occurrences of this pattern (we have a few of them),
> i.e., use u"" notation and move them to file scope.

Sure, done!

Thanks,

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 12/12] efi: make const array 'apple' static

[Lukas Wunner]

On Fri, Mar 09, 2018 at 08:47:19AM +0100, Ingo Molnar wrote:
> * Ard Biesheuvel  wrote:
> > From: Colin Ian King 
> > 
> > Don't populate the const read-only array 'buf' on the stack but instead
> > make it static. Makes the object code smaller by 64 bytes:
> > 
> > Before:
> >textdata bss dec hex filename
> >9264   1  1692812441 arch/x86/boot/compressed/eboot.o
> > 
> > After:
> >textdata bss dec hex filename
> >9200   1  1692172401 arch/x86/boot/compressed/eboot.o
> > 
> > (gcc version 7.2.0 x86_64)
> > 
> > Signed-off-by: Colin Ian King 
> > Signed-off-by: Ard Biesheuvel 
> > ---
> >  arch/x86/boot/compressed/eboot.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/boot/compressed/eboot.c 
> > b/arch/x86/boot/compressed/eboot.c
> > index 886a9115af62..f2251c1c9853 100644
> > --- a/arch/x86/boot/compressed/eboot.c
> > +++ b/arch/x86/boot/compressed/eboot.c
> > @@ -423,7 +423,7 @@ static void retrieve_apple_device_properties(struct 
> > boot_params *boot_params)
> >  
> >  static void setup_quirks(struct boot_params *boot_params)
> >  {
> > -   efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 };
> > +   static efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 };
> > efi_char16_t *fw_vendor = (efi_char16_t *)(unsigned long)
> > efi_table_attr(efi_system_table, fw_vendor, sys_table);
> 
> As a general policy, please don't put 'static' variables into the local
> scope, use file scope instead - right before setup_quirks() would be fine.

Well, I believe the end result is the same and the closer the declaration
is to where it's used, the easier the code is to read and understand.

I object to patches like this because they paper over a missing
compiler optimization:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68725

I have told Colin before that it would be more useful to look into
fixing the underlying compiler issue rather than polluting the kernel
with "static" keywords, but he keeps sending these patches so I've
given up responding:
https://lkml.org/lkml/2017/8/25/636


> Plus an unicode string literal initializer would be pretty descriptive
> as well,  instead of the weird looking character array, i.e. something
> like:
> 
>   static efi_char16_t const apple_unicode_str[] = u"Apple";
> 
> ... or so?

Last time I checked this didn't work, I believe it's because it's C11
and the kernel is compiled with -std=gnu89.  And I'm also not sure if
the oldest gcc version that we support understands u"".

Thanks,

Lukas
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 12/12] efi: make const array 'apple' static

[Ard Biesheuvel]

On 9 March 2018 at 08:07, Ard Biesheuvel  wrote:
> On 9 March 2018 at 08:04, Ingo Molnar  wrote:
>>
>> * Ard Biesheuvel  wrote:
>>
>>> > Also, would it make sense to rename it to something more descriptive like
>>> > "apple_unicode_str[]" or so?
>>> >
>>> > Plus an unicode string literal initializer would be pretty descriptive as 
>>> > well,
>>> > instead of the weird looking character array, i.e. something like:
>>> >
>>> >   static efi_char16_t const apple_unicode_str[] = u"Apple";
>>> >
>>> > ... or so?
>>> >
>>>
>>> is u"xxx" the same as L"xxx"?
>>
>> So "L" literals map to wchar_t, which wide character type is implementation
>> specific IIRC, could be 16-bit or 32-bit wide.
>>
>> u"" literals OTOH are specified by the C11 spec to be char16_t, i.e. 16-bit 
>> wide
>> characters - which I assume is the EFI type as well?
>>
>>> In any case, this is for historical reasons: at some point (and I
>>> don't remember the exact details) we had a conflict at link time with
>>> objects using 4 byte wchar_t, so we started using this notation to be
>>> independent of the size of wchar_t. That issue no longer exists so we
>>> should be able to get rid of this.
>>
>> Yes, my guess is that those problems were due to L"xyz" mapping to wchar_t 
>> and
>> having a different type in the kernel build and the host build side - but 
>> u"xyz"
>> should solve that.
>>
>
> Excellent!
>
> Do you mind taking this patch as is? I will follow up with a patch
> that updates all occurrences of this pattern (we have a few of them),
> i.e., use u"" notation and move them to file scope.

OK, I misremembered: the other occurrences have already been moved to
file scope a while ago.

I will follow up with a patch that switches to u"" notation, please
let me know if I should respin this or not
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 12/12] efi: make const array 'apple' static

[Ard Biesheuvel]

On 9 March 2018 at 08:04, Ingo Molnar  wrote:
>
> * Ard Biesheuvel  wrote:
>
>> > Also, would it make sense to rename it to something more descriptive like
>> > "apple_unicode_str[]" or so?
>> >
>> > Plus an unicode string literal initializer would be pretty descriptive as 
>> > well,
>> > instead of the weird looking character array, i.e. something like:
>> >
>> >   static efi_char16_t const apple_unicode_str[] = u"Apple";
>> >
>> > ... or so?
>> >
>>
>> is u"xxx" the same as L"xxx"?
>
> So "L" literals map to wchar_t, which wide character type is implementation
> specific IIRC, could be 16-bit or 32-bit wide.
>
> u"" literals OTOH are specified by the C11 spec to be char16_t, i.e. 16-bit 
> wide
> characters - which I assume is the EFI type as well?
>
>> In any case, this is for historical reasons: at some point (and I
>> don't remember the exact details) we had a conflict at link time with
>> objects using 4 byte wchar_t, so we started using this notation to be
>> independent of the size of wchar_t. That issue no longer exists so we
>> should be able to get rid of this.
>
> Yes, my guess is that those problems were due to L"xyz" mapping to wchar_t and
> having a different type in the kernel build and the host build side - but 
> u"xyz"
> should solve that.
>

Excellent!

Do you mind taking this patch as is? I will follow up with a patch
that updates all occurrences of this pattern (we have a few of them),
i.e., use u"" notation and move them to file scope.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 12/12] efi: make const array 'apple' static

[Ingo Molnar]

* Ard Biesheuvel  wrote:

> > Also, would it make sense to rename it to something more descriptive like
> > "apple_unicode_str[]" or so?
> >
> > Plus an unicode string literal initializer would be pretty descriptive as 
> > well,
> > instead of the weird looking character array, i.e. something like:
> >
> >   static efi_char16_t const apple_unicode_str[] = u"Apple";
> >
> > ... or so?
> >
> 
> is u"xxx" the same as L"xxx"?

So "L" literals map to wchar_t, which wide character type is implementation 
specific IIRC, could be 16-bit or 32-bit wide.

u"" literals OTOH are specified by the C11 spec to be char16_t, i.e. 16-bit 
wide 
characters - which I assume is the EFI type as well?

> In any case, this is for historical reasons: at some point (and I
> don't remember the exact details) we had a conflict at link time with
> objects using 4 byte wchar_t, so we started using this notation to be
> independent of the size of wchar_t. That issue no longer exists so we
> should be able to get rid of this.

Yes, my guess is that those problems were due to L"xyz" mapping to wchar_t and 
having a different type in the kernel build and the host build side - but 
u"xyz" 
should solve that.

Thanks,

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html