Re: [PATCH] efi/cper: Fix endianness of PCI class code
On Thu, May 25, 2017 at 06:07:35AM -0700, Ard Biesheuvel wrote:
> On 25 May 2017 at 05:56, Lukas Wunner wrote:
> > On Thu, May 25, 2017 at 05:47:59AM -0700, Ard Biesheuvel wrote:
> >> On 25 May 2017 at 05:44, Lukas Wunner wrote:
> >> > On Thu, May 25, 2017 at 05:36:01AM -0700, Ard Biesheuvel wrote:
> >> >> On 25 May 2017 at 05:30, Lukas Wunner wrote:
> >> >> > On Thu, May 11, 2017 at 03:06:42PM +0100, Ard Biesheuvel wrote:
> >> >> >> On 10 May 2017 at 09:41, Lukas Wunner wrote:
> >> >> >> > On Wed, May 10, 2017 at 09:03:11AM +0100, Ard Biesheuvel wrote:
> >> >> >> >> On 6 May 2017 at 10:07, Lukas Wunner wrote:
> >> >> >> >> > On Sat, May 06, 2017 at 08:46:07AM +0100, Ard Biesheuvel wrote:
> >> >> >> >> >> On 5 May 2017 at 19:38, Lukas Wunner wrote:
> >> >> >> >> >> > The CPER parser assumes that the class code is big endian,
> >> >> >> >> >> > but at least
> >> >> >> >> >> > on this edk2-derived Intel Purley platform it's little
> >> >> >> >> >> > endian:
> >> >> >> >> > [snip]
> >> >> >> >> >> > --- a/include/linux/cper.h
> >> >> >> >> >> > +++ b/include/linux/cper.h
> >> >> >> >> >> > @@ -416,7 +416,7 @@ struct cper_sec_pcie {
> >> >> >> >> >> > struct {
> >> >> >> >> >> > __u16 vendor_id;
> >> >> >> >> >> > __u16 device_id;
> >> >> >> >> >> > - __u8class_code[3];
> >> >> >> >> >> > + __u32 class_code:24;
> >> >> >> >> >>
> >> >> >> >> >> I'd like to avoid this change if we can. Couldn't we simply
> >> >> >> >> >> invert the
> >> >> >> >> >> order of p[] above?
> >> >> >> >> >
> >> >> >> >> > Hm, why would you like to avoid it?
> >> >> >> >>
> >> >> >> >> Because we shouldn't use bitfields in structs in code that should
> >> >> >> >> be
> >> >> >> >> portable across archs with different endiannesses.
> >> >> >> >
> >> >> >> > The CPER header is defined in the UEFI spec and UEFI mandates that
> >> >> >> > the
> >> >> >> > arch is little endian (UEFI r2.6, sec. 2.3.5, 2.3.6).
> >> >> >> >
> >> >> >>
> >> >> >> No it does not mandate that at all. It mandates how the core should
> >> >> >> be
> >> >> >> configured when running in UEFI, but the OS can do anything it likes.
> >> >> >>
> >> >> >> We are still interested in adding limited UEFI support to big endian
> >> >> >> arm64 in the future (i.e., access to a limited set of firmware tables
> >> >> >> but no runtime services), and I am not going to merge anything that
> >> >> >> moves us away from that goal.
> >> >> >>
> >> >> >> > So your argument seems moot to me. Am I missing something? Do you
> >> >> >> > have another argument?
> >> >> >> >
> >> >> >> > Moreover, the vendor_id and device_id fields are little endian as
> >> >> >> > well
> >> >> >> > (PCI r3.0, sec. 6.1), yet there are no provisions in our CPER
> >> >> >> > parser in
> >> >> >> > drivers/firmware/efi/cper.c to convert them to the endianness of
> >> >> >> > the host.
> >> >> >> >
> >> >> >>
> >> >> >> Indeed. I am aware we will need to add various endian-neutral
> >> >> >> accessors in the future.
> >> >> >>
> >> >> >> >> > The class_code element isn't
> >> >> >> >> > referenced anywhere else in the kernel and this isn't a uapi
> >> >> >> >> > header,
> >> >> >> >> > so the change would only impact out-of-tree drivers. Not sure
> >> >> >> >> > if
> >> >> >> >> > any exist which might be interested in CPER parsing.
> >> >> >> >> >
> >> >> >> >>
> >> >> >> >> The point is that the change in the struct definition is simply
> >> >> >> >> not
> >> >> >> >> necessary, given that inverting the order of p[] already achieves
> >> >> >> >> exactly what we want.
> >> >> >> >
> >> >> >> > It seems clumsy and unnecessary to me so I'd prefer the bitfield.
> >> >> >> > Please excuse my stubbornness.
> >> >> >> >
> >> >> >>
> >> >> >> Stubbornness alone is not going to convince me. What *could* convince
> >> >> >> me (although unlikely) is a quote from the C spec which explains why
> >> >> >> it is 100% legal to make assumptions about how bitfields are
> >> >> >> projected
> >> >> >> onto byte locations in memory.
> >> >> >
> >> >> > All structs in cper.h are declared "packed", so what you're asking for
> >> >> > isn't defined in the C spec but in the GCC documentation:
> >> >> >
> >> >> >"The packed attribute specifies that a variable or structure field
> >> >> > should have the smallest possible alignment -- one byte for a
> >> >> > variable,
> >> >> > and one bit for a field, unless you specify a larger value with
> >> >> > the
> >> >> > aligned attribute."
> >> >> >
> >> >> > So I maintain that the patch is fine, but you'll need to use
> >> >> > le32_to_cpu(),
> >> >> > le16_to_cpu() etc both for the class_code changed by the patch as
> >> >> > well as
> >> >> > all the other members of the struct not touched by the patch when
> >> >> > adding
> >> >> > "endianness mixed mode" for aarch64.
> >> >>
> >> >> I'm not talking about the 'packed' attribute but about the fact that
> >> >> t
Re: [PATCH v5 31/32] x86: Add sysfs support for Secure Memory Encryption
On 05/26/2017 at 10:49 AM, Dave Young wrote: > Ccing Xunlei he is reading the patches see what need to be done for > kdump. There should still be several places to handle to make kdump work. > > On 05/18/17 at 07:01pm, Borislav Petkov wrote: >> On Tue, Apr 18, 2017 at 04:22:12PM -0500, Tom Lendacky wrote: >>> Add sysfs support for SME so that user-space utilities (kdump, etc.) can >>> determine if SME is active. >> But why do user-space tools need to know that? >> >> I mean, when we load the kdump kernel, we do it with the first kernel, >> with the kexec_load() syscall, AFAICT. And that code does a lot of >> things during that init, like machine_kexec_prepare()->init_pgtable() to >> prepare the ident mapping of the second kernel, for example. >> >> What I'm aiming at is that the first kernel knows *exactly* whether SME >> is enabled or not and doesn't need to tell the second one through some >> sysfs entries - it can do that during loading. >> >> So I don't think we need any userspace things at all... > If kdump kernel can get the SME status from hardware register then this > should be not necessary and this patch can be dropped. Yes, I also agree with dropping this one. Regards, Xunlei -- 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 v5 28/32] x86/mm, kexec: Allow kexec to be used with SME
On 04/19/2017 at 05:21 AM, Tom Lendacky wrote:
> Provide support so that kexec can be used to boot a kernel when SME is
> enabled.
>
> Support is needed to allocate pages for kexec without encryption. This
> is needed in order to be able to reboot in the kernel in the same manner
> as originally booted.
Hi Tom,
Looks like kdump will break, I didn't see the similar handling for kdump cases,
see kernel:
kimage_alloc_crash_control_pages(), kimage_load_crash_segment(), etc.
We need to support kdump with SME, kdump
kernel/initramfs/purgatory/elfcorehdr/etc
are all loaded into the reserved memory(see crashkernel=X) by userspace
kexec-tools.
I think a straightforward way would be to mark the whole reserved memory range
without
encryption before loading all the kexec segments for kdump, I guess we can
handle this
easily in arch_kexec_unprotect_crashkres().
Moreover, now that "elfcorehdr=X" is left as decrypted, it needs to be remapped
to the
encrypted data.
Regards,
Xunlei
>
> Additionally, when shutting down all of the CPUs we need to be sure to
> flush the caches and then halt. This is needed when booting from a state
> where SME was not active into a state where SME is active (or vice-versa).
> Without these steps, it is possible for cache lines to exist for the same
> physical location but tagged both with and without the encryption bit. This
> can cause random memory corruption when caches are flushed depending on
> which cacheline is written last.
>
> Signed-off-by: Tom Lendacky
> ---
> arch/x86/include/asm/init.h |1 +
> arch/x86/include/asm/irqflags.h |5 +
> arch/x86/include/asm/kexec.h |8
> arch/x86/include/asm/pgtable_types.h |1 +
> arch/x86/kernel/machine_kexec_64.c | 35
> +-
> arch/x86/kernel/process.c| 26 +++--
> arch/x86/mm/ident_map.c | 11 +++
> include/linux/kexec.h| 14 ++
> kernel/kexec_core.c |7 +++
> 9 files changed, 101 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/include/asm/init.h b/arch/x86/include/asm/init.h
> index 737da62..b2ec511 100644
> --- a/arch/x86/include/asm/init.h
> +++ b/arch/x86/include/asm/init.h
> @@ -6,6 +6,7 @@ struct x86_mapping_info {
> void *context; /* context for alloc_pgt_page */
> unsigned long pmd_flag; /* page flag for PMD entry */
> unsigned long offset;/* ident mapping offset */
> + unsigned long kernpg_flag; /* kernel pagetable flag override */
> };
>
> int kernel_ident_mapping_init(struct x86_mapping_info *info, pgd_t *pgd_page,
> diff --git a/arch/x86/include/asm/irqflags.h b/arch/x86/include/asm/irqflags.h
> index ac7692d..38b5920 100644
> --- a/arch/x86/include/asm/irqflags.h
> +++ b/arch/x86/include/asm/irqflags.h
> @@ -58,6 +58,11 @@ static inline __cpuidle void native_halt(void)
> asm volatile("hlt": : :"memory");
> }
>
> +static inline __cpuidle void native_wbinvd_halt(void)
> +{
> + asm volatile("wbinvd; hlt" : : : "memory");
> +}
> +
> #endif
>
> #ifdef CONFIG_PARAVIRT
> diff --git a/arch/x86/include/asm/kexec.h b/arch/x86/include/asm/kexec.h
> index 70ef205..e8183ac 100644
> --- a/arch/x86/include/asm/kexec.h
> +++ b/arch/x86/include/asm/kexec.h
> @@ -207,6 +207,14 @@ struct kexec_entry64_regs {
> uint64_t r15;
> uint64_t rip;
> };
> +
> +extern int arch_kexec_post_alloc_pages(void *vaddr, unsigned int pages,
> +gfp_t gfp);
> +#define arch_kexec_post_alloc_pages arch_kexec_post_alloc_pages
> +
> +extern void arch_kexec_pre_free_pages(void *vaddr, unsigned int pages);
> +#define arch_kexec_pre_free_pages arch_kexec_pre_free_pages
> +
> #endif
>
> typedef void crash_vmclear_fn(void);
> diff --git a/arch/x86/include/asm/pgtable_types.h
> b/arch/x86/include/asm/pgtable_types.h
> index ce8cb1c..0f326f4 100644
> --- a/arch/x86/include/asm/pgtable_types.h
> +++ b/arch/x86/include/asm/pgtable_types.h
> @@ -213,6 +213,7 @@ enum page_cache_mode {
> #define PAGE_KERNEL __pgprot(__PAGE_KERNEL | _PAGE_ENC)
> #define PAGE_KERNEL_RO __pgprot(__PAGE_KERNEL_RO | _PAGE_ENC)
> #define PAGE_KERNEL_EXEC __pgprot(__PAGE_KERNEL_EXEC | _PAGE_ENC)
> +#define PAGE_KERNEL_EXEC_NOENC __pgprot(__PAGE_KERNEL_EXEC)
> #define PAGE_KERNEL_RX __pgprot(__PAGE_KERNEL_RX | _PAGE_ENC)
> #define PAGE_KERNEL_NOCACHE __pgprot(__PAGE_KERNEL_NOCACHE | _PAGE_ENC)
> #define PAGE_KERNEL_LARGE__pgprot(__PAGE_KERNEL_LARGE | _PAGE_ENC)
> diff --git a/arch/x86/kernel/machine_kexec_64.c
> b/arch/x86/kernel/machine_kexec_64.c
> index 085c3b3..11c0ca9 100644
> --- a/arch/x86/kernel/machine_kexec_64.c
> +++ b/arch/x86/kernel/machine_kexec_64.c
> @@ -86,7 +86,7 @@ static int init_transition_pgtable(struct kimage *image,
> pgd_t *pgd)
> set_pmd(pmd, __pmd(__pa(
Re: [PATCH v5 31/32] x86: Add sysfs support for Secure Memory Encryption
Ccing Xunlei he is reading the patches see what need to be done for kdump. There should still be several places to handle to make kdump work. On 05/18/17 at 07:01pm, Borislav Petkov wrote: > On Tue, Apr 18, 2017 at 04:22:12PM -0500, Tom Lendacky wrote: > > Add sysfs support for SME so that user-space utilities (kdump, etc.) can > > determine if SME is active. > > But why do user-space tools need to know that? > > I mean, when we load the kdump kernel, we do it with the first kernel, > with the kexec_load() syscall, AFAICT. And that code does a lot of > things during that init, like machine_kexec_prepare()->init_pgtable() to > prepare the ident mapping of the second kernel, for example. > > What I'm aiming at is that the first kernel knows *exactly* whether SME > is enabled or not and doesn't need to tell the second one through some > sysfs entries - it can do that during loading. > > So I don't think we need any userspace things at all... If kdump kernel can get the SME status from hardware register then this should be not necessary and this patch can be dropped. Thanks Dave -- 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: PROBLEM: Kernel panic on EFI BGRT early init code (kernel 4.11/4.12rc2) [regression]
Hello, On 05/26/17 at 12:49am, Maniaxx wrote: > PROBLEM: Kernel panic on EFI BGRT early init code (kernel 4.11/4.12rc2) > [regression] > > Commit: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7b0a911478c74ca02581d496f732c10e811e894f > > Kernel 4.10 and below are ok. 4.11 and 4.12rc2.r62.g2426125ab4eb freeze. > Reverting the commit on 4.11 fixes the problem. Hardware report and > earlyprintk output on the bugtracker. > https://bugzilla.kernel.org/show_bug.cgi?id=195633 > > earlyprintk (text version): > https://bbs.archlinux.org/viewtopic.php?pid=1713322#p1713322 > > Discussions on Arch Linux Forum: > https://bbs.archlinux.org/viewtopic.php?pid=1713344 > https://bbs.archlinux.org/viewtopic.php?id=226490 > Can you try below patch see if it works for you? https://lkml.org/lkml/2017/5/15/940 Thanks Dave -- 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 v5 29/32] x86/mm: Add support to encrypt the kernel in-place
On 5/18/2017 7:46 AM, Borislav Petkov wrote:
On Tue, Apr 18, 2017 at 04:21:49PM -0500, Tom Lendacky wrote:
Add the support to encrypt the kernel in-place. This is done by creating
new page mappings for the kernel - a decrypted write-protected mapping
and an encrypted mapping. The kernel is encrypted by copying it through
a temporary buffer.
Signed-off-by: Tom Lendacky
---
arch/x86/include/asm/mem_encrypt.h |6 +
arch/x86/mm/Makefile |2
arch/x86/mm/mem_encrypt.c | 262
arch/x86/mm/mem_encrypt_boot.S | 151 +
4 files changed, 421 insertions(+)
create mode 100644 arch/x86/mm/mem_encrypt_boot.S
diff --git a/arch/x86/include/asm/mem_encrypt.h
b/arch/x86/include/asm/mem_encrypt.h
index b406df2..8f6f9b4 100644
--- a/arch/x86/include/asm/mem_encrypt.h
+++ b/arch/x86/include/asm/mem_encrypt.h
@@ -31,6 +31,12 @@ static inline u64 sme_dma_mask(void)
return ((u64)sme_me_mask << 1) - 1;
}
+void sme_encrypt_execute(unsigned long encrypted_kernel_vaddr,
+unsigned long decrypted_kernel_vaddr,
+unsigned long kernel_len,
+unsigned long encryption_wa,
+unsigned long encryption_pgd);
+
void __init sme_early_encrypt(resource_size_t paddr,
unsigned long size);
void __init sme_early_decrypt(resource_size_t paddr,
diff --git a/arch/x86/mm/Makefile b/arch/x86/mm/Makefile
index 9e13841..0633142 100644
--- a/arch/x86/mm/Makefile
+++ b/arch/x86/mm/Makefile
@@ -38,3 +38,5 @@ obj-$(CONFIG_NUMA_EMU)+= numa_emulation.o
obj-$(CONFIG_X86_INTEL_MPX)+= mpx.o
obj-$(CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS) += pkeys.o
obj-$(CONFIG_RANDOMIZE_MEMORY) += kaslr.o
+
+obj-$(CONFIG_AMD_MEM_ENCRYPT) += mem_encrypt_boot.o
diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
index 30b07a3..0ff41a4 100644
--- a/arch/x86/mm/mem_encrypt.c
+++ b/arch/x86/mm/mem_encrypt.c
@@ -24,6 +24,7 @@
#include
#include
#include
+#include
/*
* Since SME related variables are set early in the boot process they must
@@ -216,8 +217,269 @@ void swiotlb_set_mem_attributes(void *vaddr, unsigned
long size)
set_memory_decrypted((unsigned long)vaddr, size >> PAGE_SHIFT);
}
+void __init sme_clear_pgd(pgd_t *pgd_base, unsigned long start,
static
Yup.
+ unsigned long end)
+{
+ unsigned long addr = start;
+ pgdval_t *pgd_p;
+
+ while (addr < end) {
+ unsigned long pgd_end;
+
+ pgd_end = (addr & PGDIR_MASK) + PGDIR_SIZE;
+ if (pgd_end > end)
+ pgd_end = end;
+
+ pgd_p = (pgdval_t *)pgd_base + pgd_index(addr);
+ *pgd_p = 0;
Hmm, so this is a contiguous range from [start:end] which translates to
8-byte PGD pointers in the PGD page so you can simply memset that range,
no?
Instead of iterating over each one?
I guess I could do that, but this will probably only end up clearing a
single PGD entry anyway since it's highly doubtful the address range
would cross a 512GB boundary.
+
+ addr = pgd_end;
+ }
+}
+
+#define PGD_FLAGS _KERNPG_TABLE_NOENC
+#define PUD_FLAGS _KERNPG_TABLE_NOENC
+#define PMD_FLAGS (__PAGE_KERNEL_LARGE_EXEC & ~_PAGE_GLOBAL)
+
+static void __init *sme_populate_pgd(pgd_t *pgd_base, void *pgtable_area,
+unsigned long vaddr, pmdval_t pmd_val)
+{
+ pgdval_t pgd, *pgd_p;
+ pudval_t pud, *pud_p;
+ pmdval_t pmd, *pmd_p;
You should use the enclosing type, not the underlying one. I.e.,
pgd_t *pgd;
pud_t *pud;
...
and then the macros native_p*d_val(), p*d_offset() and so on. I say
native_* because we don't want to have any paravirt nastyness here.
I believe your previous version was using the proper interfaces.
I won't be able to use the p*d_offset() macros since they use __va()
and we're identity mapped during this time (which is why I would guess
the proposed changes for the 5-level pagetables in
arch/x86/kernel/head64.c, __startup_64, don't use these macros
either). I should be able to use the native_set_p*d() and others though,
I'll look into that.
And the kernel has gotten 5-level pagetables support in
the meantime, so this'll need to start at p4d AFAICT.
arch/x86/mm/fault.c::dump_pagetable() looks like a good example to stare
at.
Yeah, I accounted for that in the other parts of the code but I need
to do that here also.
+ pgd_p = (pgdval_t *)pgd_base + pgd_index(vaddr);
+ pgd = *pgd_p;
+ if (pgd) {
+ pud_p = (pudval_t *)(pgd & ~PTE_FLAGS_MASK);
+ } else {
+ pud_p = pgtable_area;
+ memset(pud_p, 0, sizeof(*pud_p) * PTRS_PER_PUD);
+ pgtable_area += sizeof(*pud_p) * PTRS_PER_PUD;
+
+ *pgd_p = (pgdval_t)pud_p + PGD_FLAGS
Re: [PATCH v4] x86/efi: Correct ident mapping of efi old_map when kalsr enabled
On Thu, 18 May, at 02:39:30PM, Baoquan He wrote: > For EFI with 'efi=old_map' kernel option specified, Kernel will panic > when kaslr is enabled. > > The back trace is: > > BUG: unable to handle kernel paging request at 7febd57e > IP: 0x7febd57e > PGD 1025a067 > PUD 0 > > Oops: 0010 [#1] SMP > [ ... ] > Call Trace: > ? efi_call+0x58/0x90 > ? printk+0x58/0x6f > efi_enter_virtual_mode+0x3c5/0x50d > start_kernel+0x40f/0x4b8 > ? set_init_arg+0x55/0x55 > ? early_idt_handler_array+0x120/0x120 > x86_64_start_reservations+0x24/0x26 > x86_64_start_kernel+0x14c/0x16f > start_cpu+0x14/0x14 > > The root cause is the ident mapping is not built correctly in old_map case. > > For nokaslr kernel, PAGE_OFFSET is 0x8800 which is PGDIR_SIZE > aligned. We can borrow the pud table from direct mapping safely. Given a > physical address X, we have pud_index(X) == pud_index(__va(X)). However, > for kaslr kernel, PAGE_OFFSET is PUD_SIZE aligned. For a given physical > address X, pud_index(X) != pud_index(__va(X)). We can't only copy pgd entry > from direct mapping to build ident mapping, instead need copy pud entry > one by one from direct mapping. > > Fix it. > > Signed-off-by: Baoquan He > Signed-off-by: Dave Young > Cc: Matt Fleming > Cc: Ard Biesheuvel > Cc: Thomas Gleixner > Cc: Ingo Molnar > Cc: "H. Peter Anvin" > Cc: Thomas Garnier > Cc: Kees Cook > Cc: Russ Anderson > Cc: Frank Ramsay > Cc: Borislav Petkov > Cc: Bhupesh Sharma > Cc: x...@kernel.org > Cc: linux-efi@vger.kernel.org > --- > v3->v4: > 1. Forget running scripts/checkpatch.pl to check patch, there are several > code stype issue. Correct them in this version. > > v2->v3: > 1. Rewrite code to copy pud entry one by one so that code can be > understood > better. Usually we only have less than 1TB or several TB memory, pud entry > copy one by one won't impact efficiency. > > 2. Adding p4d page table handling. > > v1->v2: > Change code and add description according to Thomas's suggestion as below: > > 1. Add checking if pud table is allocated successfully. If not just break > the for loop. > > 2. Add code comment to explain how the 1:1 mapping is built in > efi_call_phys_prolog > > 3. Other minor change > > arch/x86/platform/efi/efi_64.c | 70 > +- > 1 file changed, 62 insertions(+), 8 deletions(-) Thanks, applied. -- 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 4.11 00/28] 4.11.1-stable review
On 25 May 2017 at 04:39, Matt Fleming wrote:
> On Mon, 15 May, at 11:28:16AM, Shuah Khan wrote:
>> Hi Matt,
>>
>> On 05/15/2017 08:36 AM, Matt Fleming wrote:
>> > On Fri, 12 May, at 10:01:41AM, Shuah Khan wrote:
>> >>
>> >> Greg/Matt,
>> >>
>> >> I started seeing this maybe since 4.11, so it isn't really a 4.11.1
>> >> issue, however sounds like this shouldn't be an error message, since
>> >> it is showing up on an older platform.
>> >>
>> >> efi: EFI_MEMMAP is not enabled.
>> >> efi: Failed to lookup EFI memory descriptor for 0xd9e0f018
>> >>
>> >>
>> >> From 816e76129ed5fadd28e526c43397c79775194b5c Mon Sep 17 00:00:00 2001
>> >> From: Matt Fleming
>> >> Date: Mon, 29 Feb 2016 21:22:52 +
>> >> Subject: [PATCH] efi: Allow drivers to reserve boot services forever
>> >>
>> >> This change was introduced in Commit:
>> >> 816e76129ed5fadd28e526c43397c79775194b5c
>> >>
>> >> Matt!
>> >>
>> >> Shouldn't this be a debug message?? Is this a really an error??
>> >
>> > Yes, it's most likely an error. We shouldn't be trying to reserve
>> > EFI regions if we can't find them in the memory map.
>> >
>> > Can you provide the full dmesg, please?
>> >
>>
>> Please see attached dmesg and dmidecode output. Please let me know if you
>> need
>> any other information.
>
> Did you use git bisect to find the above commit that introduced this
> error message? Because while the error message itself was merged in
> the above commit there were no users until commit 8e80632fb23f
> ("efi/esrt: Use efi_mem_reserve() and avoid a kmalloc()").
>
> Can you apply the attached patch, boot your machine with the efi=debug
> command line option and capture the dmesg output? It doesn't look like
> your machine is booting using EFI.
>
> ---
>
> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
> index 30031d5293c4..7f942df48f42 100644
> --- a/arch/x86/platform/efi/quirks.c
> +++ b/arch/x86/platform/efi/quirks.c
> @@ -193,6 +193,7 @@ void __init efi_arch_mem_reserve(phys_addr_t addr, u64
> size)
>
> if (efi_mem_desc_lookup(addr, &md)) {
> pr_err("Failed to lookup EFI memory descriptor for %pa\n",
> &addr);
> + WARN_ON(1);
> return;
> }
>
Could this be related to the BGRT patch we have in efi/next?
--
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 3/5] Add the ability to lock down access to the running kernel image
On 5/24/2017 11:53 PM, David Howells wrote: > Casey Schaufler wrote: > >>> +#ifdef CONFIG_LOCK_DOWN_KERNEL >>> +extern bool kernel_is_locked_down(void); >>> +#else >>> +static inline bool kernel_is_locked_down(void) >> Should this be a bool or an int? I can imagine that someone is going to want >> various different degrees of lock down for kernels. As an int you could >> return a bitmap indicating which features were locked. This would allow >> additional things to be locked down without changing the interface. > At the moment it makes no difference, since the return value is only ever > passed directly to an if-statement. > > Also, do you have an idea as to how is should be divided up? You called out five distinct features in 0/5, so how about a bit for each of those? Actually, I don't care which way you go. The current code works for me. I am just concerned that the granularity fiends might come around later. > > There aren't so many cases, at least not yet, that they can't be fixed up, > perhaps with a coccinelle script. > > David > -- > To unsubscribe from this list: send the line "unsubscribe > linux-security-module" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- 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] efi/cper: Fix endianness of PCI class code
On 25 May 2017 at 05:56, Lukas Wunner wrote:
> On Thu, May 25, 2017 at 05:47:59AM -0700, Ard Biesheuvel wrote:
>> On 25 May 2017 at 05:44, Lukas Wunner wrote:
>> > On Thu, May 25, 2017 at 05:36:01AM -0700, Ard Biesheuvel wrote:
>> >> On 25 May 2017 at 05:30, Lukas Wunner wrote:
>> >> > On Thu, May 11, 2017 at 03:06:42PM +0100, Ard Biesheuvel wrote:
>> >> >> On 10 May 2017 at 09:41, Lukas Wunner wrote:
>> >> >> > On Wed, May 10, 2017 at 09:03:11AM +0100, Ard Biesheuvel wrote:
>> >> >> >> On 6 May 2017 at 10:07, Lukas Wunner wrote:
>> >> >> >> > On Sat, May 06, 2017 at 08:46:07AM +0100, Ard Biesheuvel wrote:
>> >> >> >> >> On 5 May 2017 at 19:38, Lukas Wunner wrote:
>> >> >> >> >> > The CPER parser assumes that the class code is big endian, but
>> >> >> >> >> > at least
>> >> >> >> >> > on this edk2-derived Intel Purley platform it's little endian:
>> >> >> >> > [snip]
>> >> >> >> >> > --- a/include/linux/cper.h
>> >> >> >> >> > +++ b/include/linux/cper.h
>> >> >> >> >> > @@ -416,7 +416,7 @@ struct cper_sec_pcie {
>> >> >> >> >> > struct {
>> >> >> >> >> > __u16 vendor_id;
>> >> >> >> >> > __u16 device_id;
>> >> >> >> >> > - __u8class_code[3];
>> >> >> >> >> > + __u32 class_code:24;
>> >> >> >> >>
>> >> >> >> >> I'd like to avoid this change if we can. Couldn't we simply
>> >> >> >> >> invert the
>> >> >> >> >> order of p[] above?
>> >> >> >> >
>> >> >> >> > Hm, why would you like to avoid it?
>> >> >> >>
>> >> >> >> Because we shouldn't use bitfields in structs in code that should be
>> >> >> >> portable across archs with different endiannesses.
>> >> >> >
>> >> >> > The CPER header is defined in the UEFI spec and UEFI mandates that
>> >> >> > the
>> >> >> > arch is little endian (UEFI r2.6, sec. 2.3.5, 2.3.6).
>> >> >> >
>> >> >>
>> >> >> No it does not mandate that at all. It mandates how the core should be
>> >> >> configured when running in UEFI, but the OS can do anything it likes.
>> >> >>
>> >> >> We are still interested in adding limited UEFI support to big endian
>> >> >> arm64 in the future (i.e., access to a limited set of firmware tables
>> >> >> but no runtime services), and I am not going to merge anything that
>> >> >> moves us away from that goal.
>> >> >>
>> >> >> > So your argument seems moot to me. Am I missing something? Do you
>> >> >> > have another argument?
>> >> >> >
>> >> >> > Moreover, the vendor_id and device_id fields are little endian as
>> >> >> > well
>> >> >> > (PCI r3.0, sec. 6.1), yet there are no provisions in our CPER parser
>> >> >> > in
>> >> >> > drivers/firmware/efi/cper.c to convert them to the endianness of the
>> >> >> > host.
>> >> >> >
>> >> >>
>> >> >> Indeed. I am aware we will need to add various endian-neutral
>> >> >> accessors in the future.
>> >> >>
>> >> >> >> > The class_code element isn't
>> >> >> >> > referenced anywhere else in the kernel and this isn't a uapi
>> >> >> >> > header,
>> >> >> >> > so the change would only impact out-of-tree drivers. Not sure if
>> >> >> >> > any exist which might be interested in CPER parsing.
>> >> >> >> >
>> >> >> >>
>> >> >> >> The point is that the change in the struct definition is simply not
>> >> >> >> necessary, given that inverting the order of p[] already achieves
>> >> >> >> exactly what we want.
>> >> >> >
>> >> >> > It seems clumsy and unnecessary to me so I'd prefer the bitfield.
>> >> >> > Please excuse my stubbornness.
>> >> >> >
>> >> >>
>> >> >> Stubbornness alone is not going to convince me. What *could* convince
>> >> >> me (although unlikely) is a quote from the C spec which explains why
>> >> >> it is 100% legal to make assumptions about how bitfields are projected
>> >> >> onto byte locations in memory.
>> >> >
>> >> > All structs in cper.h are declared "packed", so what you're asking for
>> >> > isn't defined in the C spec but in the GCC documentation:
>> >> >
>> >> >"The packed attribute specifies that a variable or structure field
>> >> > should have the smallest possible alignment -- one byte for a
>> >> > variable,
>> >> > and one bit for a field, unless you specify a larger value with the
>> >> > aligned attribute."
>> >> >
>> >> > So I maintain that the patch is fine, but you'll need to use
>> >> > le32_to_cpu(),
>> >> > le16_to_cpu() etc both for the class_code changed by the patch as well
>> >> > as
>> >> > all the other members of the struct not touched by the patch when adding
>> >> > "endianness mixed mode" for aarch64.
>> >>
>> >> I'm not talking about the 'packed' attribute but about the fact that
>> >> the C spec does not guarantee that bitfields are projected onto byte
>> >> locations in memory in the way you expect.
>> >
>> > What relevance does that have as long as the header file uses a pragma
>> > specific to gcc (or other compilers that are compatible to gcc with
>> > respect to that pragma (such as clang)), and gcc guarantees the
>> > correct layout regardless of
Re: [PATCH] efi/cper: Fix endianness of PCI class code
On Thu, May 25, 2017 at 05:47:59AM -0700, Ard Biesheuvel wrote:
> On 25 May 2017 at 05:44, Lukas Wunner wrote:
> > On Thu, May 25, 2017 at 05:36:01AM -0700, Ard Biesheuvel wrote:
> >> On 25 May 2017 at 05:30, Lukas Wunner wrote:
> >> > On Thu, May 11, 2017 at 03:06:42PM +0100, Ard Biesheuvel wrote:
> >> >> On 10 May 2017 at 09:41, Lukas Wunner wrote:
> >> >> > On Wed, May 10, 2017 at 09:03:11AM +0100, Ard Biesheuvel wrote:
> >> >> >> On 6 May 2017 at 10:07, Lukas Wunner wrote:
> >> >> >> > On Sat, May 06, 2017 at 08:46:07AM +0100, Ard Biesheuvel wrote:
> >> >> >> >> On 5 May 2017 at 19:38, Lukas Wunner wrote:
> >> >> >> >> > The CPER parser assumes that the class code is big endian, but
> >> >> >> >> > at least
> >> >> >> >> > on this edk2-derived Intel Purley platform it's little endian:
> >> >> >> > [snip]
> >> >> >> >> > --- a/include/linux/cper.h
> >> >> >> >> > +++ b/include/linux/cper.h
> >> >> >> >> > @@ -416,7 +416,7 @@ struct cper_sec_pcie {
> >> >> >> >> > struct {
> >> >> >> >> > __u16 vendor_id;
> >> >> >> >> > __u16 device_id;
> >> >> >> >> > - __u8class_code[3];
> >> >> >> >> > + __u32 class_code:24;
> >> >> >> >>
> >> >> >> >> I'd like to avoid this change if we can. Couldn't we simply
> >> >> >> >> invert the
> >> >> >> >> order of p[] above?
> >> >> >> >
> >> >> >> > Hm, why would you like to avoid it?
> >> >> >>
> >> >> >> Because we shouldn't use bitfields in structs in code that should be
> >> >> >> portable across archs with different endiannesses.
> >> >> >
> >> >> > The CPER header is defined in the UEFI spec and UEFI mandates that the
> >> >> > arch is little endian (UEFI r2.6, sec. 2.3.5, 2.3.6).
> >> >> >
> >> >>
> >> >> No it does not mandate that at all. It mandates how the core should be
> >> >> configured when running in UEFI, but the OS can do anything it likes.
> >> >>
> >> >> We are still interested in adding limited UEFI support to big endian
> >> >> arm64 in the future (i.e., access to a limited set of firmware tables
> >> >> but no runtime services), and I am not going to merge anything that
> >> >> moves us away from that goal.
> >> >>
> >> >> > So your argument seems moot to me. Am I missing something? Do you
> >> >> > have another argument?
> >> >> >
> >> >> > Moreover, the vendor_id and device_id fields are little endian as well
> >> >> > (PCI r3.0, sec. 6.1), yet there are no provisions in our CPER parser
> >> >> > in
> >> >> > drivers/firmware/efi/cper.c to convert them to the endianness of the
> >> >> > host.
> >> >> >
> >> >>
> >> >> Indeed. I am aware we will need to add various endian-neutral
> >> >> accessors in the future.
> >> >>
> >> >> >> > The class_code element isn't
> >> >> >> > referenced anywhere else in the kernel and this isn't a uapi
> >> >> >> > header,
> >> >> >> > so the change would only impact out-of-tree drivers. Not sure if
> >> >> >> > any exist which might be interested in CPER parsing.
> >> >> >> >
> >> >> >>
> >> >> >> The point is that the change in the struct definition is simply not
> >> >> >> necessary, given that inverting the order of p[] already achieves
> >> >> >> exactly what we want.
> >> >> >
> >> >> > It seems clumsy and unnecessary to me so I'd prefer the bitfield.
> >> >> > Please excuse my stubbornness.
> >> >> >
> >> >>
> >> >> Stubbornness alone is not going to convince me. What *could* convince
> >> >> me (although unlikely) is a quote from the C spec which explains why
> >> >> it is 100% legal to make assumptions about how bitfields are projected
> >> >> onto byte locations in memory.
> >> >
> >> > All structs in cper.h are declared "packed", so what you're asking for
> >> > isn't defined in the C spec but in the GCC documentation:
> >> >
> >> >"The packed attribute specifies that a variable or structure field
> >> > should have the smallest possible alignment -- one byte for a
> >> > variable,
> >> > and one bit for a field, unless you specify a larger value with the
> >> > aligned attribute."
> >> >
> >> > So I maintain that the patch is fine, but you'll need to use
> >> > le32_to_cpu(),
> >> > le16_to_cpu() etc both for the class_code changed by the patch as well as
> >> > all the other members of the struct not touched by the patch when adding
> >> > "endianness mixed mode" for aarch64.
> >>
> >> I'm not talking about the 'packed' attribute but about the fact that
> >> the C spec does not guarantee that bitfields are projected onto byte
> >> locations in memory in the way you expect.
> >
> > What relevance does that have as long as the header file uses a pragma
> > specific to gcc (or other compilers that are compatible to gcc with
> > respect to that pragma (such as clang)), and gcc guarantees the
> > correct layout regardless of endianness?
>
> The relevance is that we should not add GCC specific code because you
> think it looks prettier.
The code already *is* gcc-specific.
> And where does GCC guara
Re: [PATCH] efi/cper: Fix endianness of PCI class code
On 25 May 2017 at 05:44, Lukas Wunner wrote:
> On Thu, May 25, 2017 at 05:36:01AM -0700, Ard Biesheuvel wrote:
>> On 25 May 2017 at 05:30, Lukas Wunner wrote:
>> > On Thu, May 11, 2017 at 03:06:42PM +0100, Ard Biesheuvel wrote:
>> >> On 10 May 2017 at 09:41, Lukas Wunner wrote:
>> >> > On Wed, May 10, 2017 at 09:03:11AM +0100, Ard Biesheuvel wrote:
>> >> >> On 6 May 2017 at 10:07, Lukas Wunner wrote:
>> >> >> > On Sat, May 06, 2017 at 08:46:07AM +0100, Ard Biesheuvel wrote:
>> >> >> >> On 5 May 2017 at 19:38, Lukas Wunner wrote:
>> >> >> >> > The CPER parser assumes that the class code is big endian, but at
>> >> >> >> > least
>> >> >> >> > on this edk2-derived Intel Purley platform it's little endian:
>> >> >> > [snip]
>> >> >> >> > --- a/include/linux/cper.h
>> >> >> >> > +++ b/include/linux/cper.h
>> >> >> >> > @@ -416,7 +416,7 @@ struct cper_sec_pcie {
>> >> >> >> > struct {
>> >> >> >> > __u16 vendor_id;
>> >> >> >> > __u16 device_id;
>> >> >> >> > - __u8class_code[3];
>> >> >> >> > + __u32 class_code:24;
>> >> >> >>
>> >> >> >> I'd like to avoid this change if we can. Couldn't we simply invert
>> >> >> >> the
>> >> >> >> order of p[] above?
>> >> >> >
>> >> >> > Hm, why would you like to avoid it?
>> >> >>
>> >> >> Because we shouldn't use bitfields in structs in code that should be
>> >> >> portable across archs with different endiannesses.
>> >> >
>> >> > The CPER header is defined in the UEFI spec and UEFI mandates that the
>> >> > arch is little endian (UEFI r2.6, sec. 2.3.5, 2.3.6).
>> >> >
>> >>
>> >> No it does not mandate that at all. It mandates how the core should be
>> >> configured when running in UEFI, but the OS can do anything it likes.
>> >>
>> >> We are still interested in adding limited UEFI support to big endian
>> >> arm64 in the future (i.e., access to a limited set of firmware tables
>> >> but no runtime services), and I am not going to merge anything that
>> >> moves us away from that goal.
>> >>
>> >> > So your argument seems moot to me. Am I missing something? Do you
>> >> > have another argument?
>> >> >
>> >> > Moreover, the vendor_id and device_id fields are little endian as well
>> >> > (PCI r3.0, sec. 6.1), yet there are no provisions in our CPER parser in
>> >> > drivers/firmware/efi/cper.c to convert them to the endianness of the
>> >> > host.
>> >> >
>> >>
>> >> Indeed. I am aware we will need to add various endian-neutral
>> >> accessors in the future.
>> >>
>> >> >> > The class_code element isn't
>> >> >> > referenced anywhere else in the kernel and this isn't a uapi header,
>> >> >> > so the change would only impact out-of-tree drivers. Not sure if
>> >> >> > any exist which might be interested in CPER parsing.
>> >> >> >
>> >> >>
>> >> >> The point is that the change in the struct definition is simply not
>> >> >> necessary, given that inverting the order of p[] already achieves
>> >> >> exactly what we want.
>> >> >
>> >> > It seems clumsy and unnecessary to me so I'd prefer the bitfield.
>> >> > Please excuse my stubbornness.
>> >> >
>> >>
>> >> Stubbornness alone is not going to convince me. What *could* convince
>> >> me (although unlikely) is a quote from the C spec which explains why
>> >> it is 100% legal to make assumptions about how bitfields are projected
>> >> onto byte locations in memory.
>> >
>> > All structs in cper.h are declared "packed", so what you're asking for
>> > isn't defined in the C spec but in the GCC documentation:
>> >
>> >"The packed attribute specifies that a variable or structure field
>> > should have the smallest possible alignment -- one byte for a variable,
>> > and one bit for a field, unless you specify a larger value with the
>> > aligned attribute."
>> >
>> > So I maintain that the patch is fine, but you'll need to use le32_to_cpu(),
>> > le16_to_cpu() etc both for the class_code changed by the patch as well as
>> > all the other members of the struct not touched by the patch when adding
>> > "endianness mixed mode" for aarch64.
>>
>> I'm not talking about the 'packed' attribute but about the fact that
>> the C spec does not guarantee that bitfields are projected onto byte
>> locations in memory in the way you expect.
>
> What relevance does that have as long as the header file uses a pragma
> specific to gcc (or other compilers that are compatible to gcc with
> respect to that pragma (such as clang)), and gcc guarantees the
> correct layout regardless of endianness?
The relevance is that we should not add GCC specific code because you
think it looks prettier. And where does GCC guarantee the correct
layout? Did you find an unambiguous GCC documentation reference that
explains how bitfields are mapped onto byte locations? Or does
'guarantee' mean 'I tested it and it works'?
--
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
Re: [PATCH] efi/cper: Fix endianness of PCI class code
On Thu, May 25, 2017 at 05:36:01AM -0700, Ard Biesheuvel wrote:
> On 25 May 2017 at 05:30, Lukas Wunner wrote:
> > On Thu, May 11, 2017 at 03:06:42PM +0100, Ard Biesheuvel wrote:
> >> On 10 May 2017 at 09:41, Lukas Wunner wrote:
> >> > On Wed, May 10, 2017 at 09:03:11AM +0100, Ard Biesheuvel wrote:
> >> >> On 6 May 2017 at 10:07, Lukas Wunner wrote:
> >> >> > On Sat, May 06, 2017 at 08:46:07AM +0100, Ard Biesheuvel wrote:
> >> >> >> On 5 May 2017 at 19:38, Lukas Wunner wrote:
> >> >> >> > The CPER parser assumes that the class code is big endian, but at
> >> >> >> > least
> >> >> >> > on this edk2-derived Intel Purley platform it's little endian:
> >> >> > [snip]
> >> >> >> > --- a/include/linux/cper.h
> >> >> >> > +++ b/include/linux/cper.h
> >> >> >> > @@ -416,7 +416,7 @@ struct cper_sec_pcie {
> >> >> >> > struct {
> >> >> >> > __u16 vendor_id;
> >> >> >> > __u16 device_id;
> >> >> >> > - __u8class_code[3];
> >> >> >> > + __u32 class_code:24;
> >> >> >>
> >> >> >> I'd like to avoid this change if we can. Couldn't we simply invert
> >> >> >> the
> >> >> >> order of p[] above?
> >> >> >
> >> >> > Hm, why would you like to avoid it?
> >> >>
> >> >> Because we shouldn't use bitfields in structs in code that should be
> >> >> portable across archs with different endiannesses.
> >> >
> >> > The CPER header is defined in the UEFI spec and UEFI mandates that the
> >> > arch is little endian (UEFI r2.6, sec. 2.3.5, 2.3.6).
> >> >
> >>
> >> No it does not mandate that at all. It mandates how the core should be
> >> configured when running in UEFI, but the OS can do anything it likes.
> >>
> >> We are still interested in adding limited UEFI support to big endian
> >> arm64 in the future (i.e., access to a limited set of firmware tables
> >> but no runtime services), and I am not going to merge anything that
> >> moves us away from that goal.
> >>
> >> > So your argument seems moot to me. Am I missing something? Do you
> >> > have another argument?
> >> >
> >> > Moreover, the vendor_id and device_id fields are little endian as well
> >> > (PCI r3.0, sec. 6.1), yet there are no provisions in our CPER parser in
> >> > drivers/firmware/efi/cper.c to convert them to the endianness of the
> >> > host.
> >> >
> >>
> >> Indeed. I am aware we will need to add various endian-neutral
> >> accessors in the future.
> >>
> >> >> > The class_code element isn't
> >> >> > referenced anywhere else in the kernel and this isn't a uapi header,
> >> >> > so the change would only impact out-of-tree drivers. Not sure if
> >> >> > any exist which might be interested in CPER parsing.
> >> >> >
> >> >>
> >> >> The point is that the change in the struct definition is simply not
> >> >> necessary, given that inverting the order of p[] already achieves
> >> >> exactly what we want.
> >> >
> >> > It seems clumsy and unnecessary to me so I'd prefer the bitfield.
> >> > Please excuse my stubbornness.
> >> >
> >>
> >> Stubbornness alone is not going to convince me. What *could* convince
> >> me (although unlikely) is a quote from the C spec which explains why
> >> it is 100% legal to make assumptions about how bitfields are projected
> >> onto byte locations in memory.
> >
> > All structs in cper.h are declared "packed", so what you're asking for
> > isn't defined in the C spec but in the GCC documentation:
> >
> >"The packed attribute specifies that a variable or structure field
> > should have the smallest possible alignment -- one byte for a variable,
> > and one bit for a field, unless you specify a larger value with the
> > aligned attribute."
> >
> > So I maintain that the patch is fine, but you'll need to use le32_to_cpu(),
> > le16_to_cpu() etc both for the class_code changed by the patch as well as
> > all the other members of the struct not touched by the patch when adding
> > "endianness mixed mode" for aarch64.
>
> I'm not talking about the 'packed' attribute but about the fact that
> the C spec does not guarantee that bitfields are projected onto byte
> locations in memory in the way you expect.
What relevance does that have as long as the header file uses a pragma
specific to gcc (or other compilers that are compatible to gcc with
respect to that pragma (such as clang)), and gcc guarantees the
correct layout regardless of endianness?
--
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] efi/cper: Fix endianness of PCI class code
On 25 May 2017 at 05:30, Lukas Wunner wrote:
> On Thu, May 11, 2017 at 03:06:42PM +0100, Ard Biesheuvel wrote:
>> On 10 May 2017 at 09:41, Lukas Wunner wrote:
>> > On Wed, May 10, 2017 at 09:03:11AM +0100, Ard Biesheuvel wrote:
>> >> On 6 May 2017 at 10:07, Lukas Wunner wrote:
>> >> > On Sat, May 06, 2017 at 08:46:07AM +0100, Ard Biesheuvel wrote:
>> >> >> On 5 May 2017 at 19:38, Lukas Wunner wrote:
>> >> >> > The CPER parser assumes that the class code is big endian, but at
>> >> >> > least
>> >> >> > on this edk2-derived Intel Purley platform it's little endian:
>> >> > [snip]
>> >> >> > --- a/include/linux/cper.h
>> >> >> > +++ b/include/linux/cper.h
>> >> >> > @@ -416,7 +416,7 @@ struct cper_sec_pcie {
>> >> >> > struct {
>> >> >> > __u16 vendor_id;
>> >> >> > __u16 device_id;
>> >> >> > - __u8class_code[3];
>> >> >> > + __u32 class_code:24;
>> >> >>
>> >> >> I'd like to avoid this change if we can. Couldn't we simply invert the
>> >> >> order of p[] above?
>> >> >
>> >> > Hm, why would you like to avoid it?
>> >>
>> >> Because we shouldn't use bitfields in structs in code that should be
>> >> portable across archs with different endiannesses.
>> >
>> > The CPER header is defined in the UEFI spec and UEFI mandates that the
>> > arch is little endian (UEFI r2.6, sec. 2.3.5, 2.3.6).
>> >
>>
>> No it does not mandate that at all. It mandates how the core should be
>> configured when running in UEFI, but the OS can do anything it likes.
>>
>> We are still interested in adding limited UEFI support to big endian
>> arm64 in the future (i.e., access to a limited set of firmware tables
>> but no runtime services), and I am not going to merge anything that
>> moves us away from that goal.
>>
>> > So your argument seems moot to me. Am I missing something? Do you
>> > have another argument?
>> >
>> > Moreover, the vendor_id and device_id fields are little endian as well
>> > (PCI r3.0, sec. 6.1), yet there are no provisions in our CPER parser in
>> > drivers/firmware/efi/cper.c to convert them to the endianness of the host.
>> >
>>
>> Indeed. I am aware we will need to add various endian-neutral
>> accessors in the future.
>>
>> >> > The class_code element isn't
>> >> > referenced anywhere else in the kernel and this isn't a uapi header,
>> >> > so the change would only impact out-of-tree drivers. Not sure if
>> >> > any exist which might be interested in CPER parsing.
>> >> >
>> >>
>> >> The point is that the change in the struct definition is simply not
>> >> necessary, given that inverting the order of p[] already achieves
>> >> exactly what we want.
>> >
>> > It seems clumsy and unnecessary to me so I'd prefer the bitfield.
>> > Please excuse my stubbornness.
>> >
>>
>> Stubbornness alone is not going to convince me. What *could* convince
>> me (although unlikely) is a quote from the C spec which explains why
>> it is 100% legal to make assumptions about how bitfields are projected
>> onto byte locations in memory.
>
> All structs in cper.h are declared "packed", so what you're asking for
> isn't defined in the C spec but in the GCC documentation:
>
>"The packed attribute specifies that a variable or structure field
> should have the smallest possible alignment -- one byte for a variable,
> and one bit for a field, unless you specify a larger value with the
> aligned attribute."
>
> So I maintain that the patch is fine, but you'll need to use le32_to_cpu(),
> le16_to_cpu() etc both for the class_code changed by the patch as well as
> all the other members of the struct not touched by the patch when adding
> "endianness mixed mode" for aarch64.
>
I'm not talking about the 'packed' attribute but about the fact that
the C spec does not guarantee that bitfields are projected onto byte
locations in memory in the way you expect.
--
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] efi/cper: Fix endianness of PCI class code
On Thu, May 11, 2017 at 03:06:42PM +0100, Ard Biesheuvel wrote:
> On 10 May 2017 at 09:41, Lukas Wunner wrote:
> > On Wed, May 10, 2017 at 09:03:11AM +0100, Ard Biesheuvel wrote:
> >> On 6 May 2017 at 10:07, Lukas Wunner wrote:
> >> > On Sat, May 06, 2017 at 08:46:07AM +0100, Ard Biesheuvel wrote:
> >> >> On 5 May 2017 at 19:38, Lukas Wunner wrote:
> >> >> > The CPER parser assumes that the class code is big endian, but at
> >> >> > least
> >> >> > on this edk2-derived Intel Purley platform it's little endian:
> >> > [snip]
> >> >> > --- a/include/linux/cper.h
> >> >> > +++ b/include/linux/cper.h
> >> >> > @@ -416,7 +416,7 @@ struct cper_sec_pcie {
> >> >> > struct {
> >> >> > __u16 vendor_id;
> >> >> > __u16 device_id;
> >> >> > - __u8class_code[3];
> >> >> > + __u32 class_code:24;
> >> >>
> >> >> I'd like to avoid this change if we can. Couldn't we simply invert the
> >> >> order of p[] above?
> >> >
> >> > Hm, why would you like to avoid it?
> >>
> >> Because we shouldn't use bitfields in structs in code that should be
> >> portable across archs with different endiannesses.
> >
> > The CPER header is defined in the UEFI spec and UEFI mandates that the
> > arch is little endian (UEFI r2.6, sec. 2.3.5, 2.3.6).
> >
>
> No it does not mandate that at all. It mandates how the core should be
> configured when running in UEFI, but the OS can do anything it likes.
>
> We are still interested in adding limited UEFI support to big endian
> arm64 in the future (i.e., access to a limited set of firmware tables
> but no runtime services), and I am not going to merge anything that
> moves us away from that goal.
>
> > So your argument seems moot to me. Am I missing something? Do you
> > have another argument?
> >
> > Moreover, the vendor_id and device_id fields are little endian as well
> > (PCI r3.0, sec. 6.1), yet there are no provisions in our CPER parser in
> > drivers/firmware/efi/cper.c to convert them to the endianness of the host.
> >
>
> Indeed. I am aware we will need to add various endian-neutral
> accessors in the future.
>
> >> > The class_code element isn't
> >> > referenced anywhere else in the kernel and this isn't a uapi header,
> >> > so the change would only impact out-of-tree drivers. Not sure if
> >> > any exist which might be interested in CPER parsing.
> >> >
> >>
> >> The point is that the change in the struct definition is simply not
> >> necessary, given that inverting the order of p[] already achieves
> >> exactly what we want.
> >
> > It seems clumsy and unnecessary to me so I'd prefer the bitfield.
> > Please excuse my stubbornness.
> >
>
> Stubbornness alone is not going to convince me. What *could* convince
> me (although unlikely) is a quote from the C spec which explains why
> it is 100% legal to make assumptions about how bitfields are projected
> onto byte locations in memory.
All structs in cper.h are declared "packed", so what you're asking for
isn't defined in the C spec but in the GCC documentation:
"The packed attribute specifies that a variable or structure field
should have the smallest possible alignment -- one byte for a variable,
and one bit for a field, unless you specify a larger value with the
aligned attribute."
So I maintain that the patch is fine, but you'll need to use le32_to_cpu(),
le16_to_cpu() etc both for the class_code changed by the patch as well as
all the other members of the struct not touched by the patch when adding
"endianness mixed mode" for aarch64.
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 v2] x86/efi: Disable runtime services on kexec kernel if booted with efi=old_map
On Tue, 16 May, at 06:14:23PM, Sai Praneeth Prakhya wrote: > From: Sai Praneeth > > Booting kexec kernel with "efi=old_map" in kernel command line hits > kernel panic as shown below. > > [0.001000] BUG: unable to handle kernel paging request at 88007fe78070 > [0.001000] IP: virt_efi_set_variable.part.7+0x63/0x1b0 > [0.001000] PGD 7ea28067 > [0.001000] PUD 7ea2b067 > [0.001000] PMD 7ea2d067 > [0.001000] PTE 0 > [0.001000] > [0.001000] Oops: [#1] SMP > [0.001000] Modules linked in: > [0.001000] CPU: 0 PID: 0 Comm: swapper/0 Not tainted > 4.11.0-rc2-yocto-standard+ #229 > [0.001000] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 0.0.0 > 02/06/2015 > [0.001000] task: 82022500 task.stack: 8200 > [0.001000] RIP: 0010:virt_efi_set_variable.part.7+0x63/0x1b0 > [0.001000] RSP: :82003dc0 EFLAGS: 00010246 > [0.001000] RAX: 88007fe78018 RBX: 82050300 RCX: 0007 > [0.001000] RDX: 82003e50 RSI: 82050300 RDI: 82050300 > [0.001000] RBP: 82003e08 R08: R09: > [0.001000] R10: R11: R12: 82003e50 > [0.001000] R13: 0007 R14: 0246 R15: > [0.001000] FS: () GS:88007fa0() > knlGS: > [0.001000] CS: 0010 DS: ES: CR0: 80050033 > [0.001000] CR2: 88007fe78070 CR3: 7da1d000 CR4: 06b0 > [0.001000] DR0: DR1: DR2: > [0.001000] DR3: DR6: fffe0ff0 DR7: 0400 > [0.001000] Call Trace: > [0.001000] virt_efi_set_variable+0x5d/0x70 > [0.001000] efi_delete_dummy_variable+0x7a/0x80 > [0.001000] efi_enter_virtual_mode+0x3f6/0x4a7 > [0.001000] start_kernel+0x375/0x400 > [0.001000] x86_64_start_reservations+0x2a/0x2c > [0.001000] x86_64_start_kernel+0x168/0x176 > [0.001000] start_cpu+0x14/0x14 > [0.001000] Code: 04 b0 84 ff 80 3d c5 56 b3 00 00 4c 8b 44 24 08 75 > 6b 9c 41 5e 48 8b 05 9c 78 99 00 4d 89 c1 48 89 de 4d 89 f8 44 89 e9 4c > 89 e2 <48> 8b 40 58 48 8b 78 58 e8 b0 2d 88 ff 48 c7 c6 b6 1d f4 81 4c > [0.001000] RIP: virt_efi_set_variable.part.7+0x63/0x1b0 RSP: 82003dc0 > [0.001000] CR2: 88007fe78070 > [0.001000] ---[ end trace ]--- > [0.001000] Kernel panic - not syncing: Attempted to kill the idle task! > [0.001000] ---[ end Kernel panic - not syncing: Attempted to kill the idle > task! > > [efi=old_map was never intended to work with kexec. The problem with > using efi=old_map is that the virtual addresses are assigned from the > memory region used by other kernel mappings; vmalloc() space. > Potentially there could be collisions when booting kexec if something > else is mapped at the virtual address we allocated for runtime service > regions in the initial boot] - Matt Fleming > > Since kexec was never intended to work with efi=old_map, disable > runtime services in kexec if booted with efi=old_map, so that we don't > panic. > > Signed-off-by: Sai Praneeth Prakhya > Cc: Borislav Petkov > Cc: Ricardo Neri > Cc: Matt Fleming > Cc: Ard Biesheuvel > Cc: Ravi Shankar > Cc: Lee Chun-Yi > Cc: Dave Young > > Changes since v1: > Don't fix the panic, because this was never intended to work. > --- > arch/x86/platform/efi/efi.c | 6 -- > 1 file changed, 4 insertions(+), 2 deletions(-) Thanks Sai, applied to the urgent branch. -- 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 4.11 00/28] 4.11.1-stable review
On Mon, 15 May, at 11:28:16AM, Shuah Khan wrote:
> Hi Matt,
>
> On 05/15/2017 08:36 AM, Matt Fleming wrote:
> > On Fri, 12 May, at 10:01:41AM, Shuah Khan wrote:
> >>
> >> Greg/Matt,
> >>
> >> I started seeing this maybe since 4.11, so it isn't really a 4.11.1
> >> issue, however sounds like this shouldn't be an error message, since
> >> it is showing up on an older platform.
> >>
> >> efi: EFI_MEMMAP is not enabled.
> >> efi: Failed to lookup EFI memory descriptor for 0xd9e0f018
> >>
> >>
> >> From 816e76129ed5fadd28e526c43397c79775194b5c Mon Sep 17 00:00:00 2001
> >> From: Matt Fleming
> >> Date: Mon, 29 Feb 2016 21:22:52 +
> >> Subject: [PATCH] efi: Allow drivers to reserve boot services forever
> >>
> >> This change was introduced in Commit:
> >> 816e76129ed5fadd28e526c43397c79775194b5c
> >>
> >> Matt!
> >>
> >> Shouldn't this be a debug message?? Is this a really an error??
> >
> > Yes, it's most likely an error. We shouldn't be trying to reserve
> > EFI regions if we can't find them in the memory map.
> >
> > Can you provide the full dmesg, please?
> >
>
> Please see attached dmesg and dmidecode output. Please let me know if you need
> any other information.
Did you use git bisect to find the above commit that introduced this
error message? Because while the error message itself was merged in
the above commit there were no users until commit 8e80632fb23f
("efi/esrt: Use efi_mem_reserve() and avoid a kmalloc()").
Can you apply the attached patch, boot your machine with the efi=debug
command line option and capture the dmesg output? It doesn't look like
your machine is booting using EFI.
---
diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
index 30031d5293c4..7f942df48f42 100644
--- a/arch/x86/platform/efi/quirks.c
+++ b/arch/x86/platform/efi/quirks.c
@@ -193,6 +193,7 @@ void __init efi_arch_mem_reserve(phys_addr_t addr, u64 size)
if (efi_mem_desc_lookup(addr, &md)) {
pr_err("Failed to lookup EFI memory descriptor for %pa\n",
&addr);
+ WARN_ON(1);
return;
}
--
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] efi/reboot: Fall back to original power-off method if EFI_RESET_SHUTDOWN returns
On Fri, 19 May, at 04:45:49PM, Ard Biesheuvel wrote: > > This does not look unreasonable to me, but this is more Matt's turf so > I will let him handle this one. I was hoping that either Len or Rafael would have chimed in by now, but they were probably waiting for me... Doesn't ACPI reduced require that EFI power off be supported? I can't find anything in the spec that makes that connection. Unless the ACPI folks provide a reason that falling back to ACPI poweroff doesn't make sense for ACPI reduced hardware, I think we should apply this patch. -- 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
