r336921 broke booting on MBP 2017, EFIRT related
Yuri Pankov
yuripv at yuripv.net
Thu Aug 30 09:22:42 UTC 2018
Rainer Hurling wrote:
> Am 29.08.18 um 16:12 schrieb Kyle Evans:
>> On Wed, Aug 29, 2018 at 6:53 AM Yuri Pankov <yuripv at yuripv.net> wrote:
>>>
>>> Yuri Pankov wrote:
>>>> Konstantin Belousov wrote:
>>>>> On Wed, Aug 29, 2018 at 12:37:52PM +0300, Yuri Pankov wrote:
>>>>>> Hi,
>>>>>>
>>>>>> I've noticed that all recent snapshots (ALPHA3, ALPHA2, ALPHA1,
>>>>>> 20180802) fail to boot on MBP 2017:
>>>>>>
>>>>>> kbd0 at kbdmux0
>>>>>> netmap: loaded module
>>>>>> nexus0
>>>>>>
>>>>>> Fatal trap 12: page fault while in kernel mode
>>>>>> cpuid = 2: apic id = 02
>>>>>> fault virtual address = 0x74c64a50
>>>>>> fault code = supervisor read data, page not present
>>>>>> instruction pointer = 0x20: 0x7abece31
>>>>>> stack pointer = 0x28: 0xffffffff82b2f7c0
>>>>>> frame pointer = 0x28: 0xffffffff82b2f810
>>>>>> code segment = base 0x0, limit 0xfffff, type 0x1b
>>>>>> = DPL 0, pres 1, long 1, def32 0, gran 1
>>>>>> processor eflags = interrupt enabled, resume, IOPL = 0
>>>>>> current process = 0 (swapper)
>>>>>> [ thread pid 0 tid 100000 ]
>>>>>> Stopped at 0x7abece31: calll *0x18(%rax)
>>>>>> db>
>>>>>>
>>>>>> Sadly, there's no support for internal keyboard yet (it's connected via
>>>>>> SPI), and external USB one stops working.
>>>>>>
>>>>>> A (not so quick) bisect is pointing at r336921, which enabled EFIRT.
>>>>>>
>>>>>> Some questions here:
>>>>>> - is this something that can/should be fixed?
>>>>>> - can we print some "enabling EFIRT" message to the console to make
>>>>>> guesses about the problem source a bit easier?
>>>>>
>>>>> It is not in 'enabling'. Looking at the faulting VA, I believe that
>>>>> it occurs inside the BIOS code.
>>>>>
>>>>> Disable efirt by removing the kernel option, then try to load the module
>>>>> at runtime. Does it still fault ? Also, get the efi mem map for the
>>>>> machine and look at which region the faulting address and the faulting
>>>>> instruction belong.
>>>>
>>>> kldload'ing the efirt module gets the same fault. Several top lines of
>>>> backtrace:
>>>>
>>>> kernphys() at 0x7abece31
>>>> efi_get_time() at efi_get_time+0xd9
>>>> efirtc_probe() at efirtc_probe+0x17
>>>
>>> For the efi mem map, if I'm understanding it correctly, there's the
>>> following:
>>>
>>> ...
>>> BootServicesData 00007421d000 000000000000 00000a8b UC WC WT WB
>>> ...
>>> RuntimeServicesCode 00007ab9f000 000000000000 00000070 UC WC WT WB
>>> ...
>>>
>>
>> Hi,
>>
>> I guess this patch might do it:
>> https://people.freebsd.org/~kevans/efi-bootmap.diff
>>
>> Linux commit messages depict a tale in which they used to also only
>> map RUNTIME entries, but they were effectively forced to back down on
>> that because of buggy firmware that does exactly what you've described
>> and they later reintroduced the restrictive mapping for i386-only
>> where they'd not found such bugs.
>>
>> Thanks,
>>
>> Kyle Evans
>
> Hi Kyle,
>
> After Yuri had no success with the patches of kib, I tried your patch on
> a DELL Latitude E6520 with BIOS version A21.
Sorry, I accidentally took the discussion off-list, where Konstantin
provided some more patches. I'm attaching the one that finally worked
for me. It also adds some diagnostic prints which require bootverbose
to be enabled.
-------------- next part --------------
diff --git a/sys/amd64/amd64/efirt_machdep.c b/sys/amd64/amd64/efirt_machdep.c
index da7783043a2..80ffa66f5ec 100644
--- a/sys/amd64/amd64/efirt_machdep.c
+++ b/sys/amd64/amd64/efirt_machdep.c
@@ -55,6 +55,7 @@ __FBSDID("$FreeBSD$");
#include <machine/vmparam.h>
#include <vm/vm.h>
#include <vm/pmap.h>
+#include <vm/vm_extern.h>
#include <vm/vm_map.h>
#include <vm/vm_object.h>
#include <vm/vm_page.h>
@@ -266,6 +267,7 @@ efi_arch_enter(void)
curpmap = PCPU_GET(curpmap);
PMAP_LOCK_ASSERT(curpmap, MA_OWNED);
+ curthread->td_md.md_efirt_dis_pf = vm_fault_disable_pagefaults();
/*
* IPI TLB shootdown handler invltlb_pcid_handler() reloads
@@ -300,6 +302,7 @@ efi_arch_leave(void)
curpmap->pm_pcids[PCPU_GET(cpuid)].pm_pcid : 0));
if (!pmap_pcid_enabled)
invltlb();
+ vm_fault_enable_pagefaults(curthread->td_md.md_efirt_dis_pf);
}
/* XXX debug stuff */
diff --git a/sys/amd64/amd64/efirt_support.S b/sys/amd64/amd64/efirt_support.S
new file mode 100644
index 00000000000..82e5646e645
--- /dev/null
+++ b/sys/amd64/amd64/efirt_support.S
@@ -0,0 +1,105 @@
+/*-
+ * SPDX-License-Identifier: BSD-2-Clause-FreeBSD
+ *
+ * Copyright (c) 2018 The FreeBSD Foundation
+ * All rights reserved.
+ *
+ * This software was developed by Konstantin Belousov <kib at FreeBSD.org>
+ * under sponsorship from the FreeBSD Foundation.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
+ * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
+ * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
+ * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
+ * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
+ * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ *
+ * $FreeBSD$
+ */
+
+#include <machine/asmacros.h>
+
+#include "assym.inc"
+
+ .text
+ENTRY(efirt_arch_call)
+ pushq %rbp
+ movq %rsp, %rbp
+
+ movq %rbx, EC_STATE+TF_RBX(%rdi)
+ movq %rsp, EC_STATE+TF_RSP(%rdi)
+ movq %rbp, EC_STATE+TF_RBP(%rdi)
+ movq %r12, EC_STATE+TF_R12(%rdi)
+ movq %r13, EC_STATE+TF_R13(%rdi)
+ movq %r14, EC_STATE+TF_R14(%rdi)
+ movq %r15, EC_STATE+TF_R15(%rdi)
+ movq PCPU(CURTHREAD), %rax
+ movq %rdi, TD_MD+MD_EFIRT_TMP(%rax)
+
+ movq PCPU(CURPCB), %rsi
+ movq $efirt_fault, PCB_ONFAULT(%rsi)
+
+ movl EC_ARGCNT(%rdi), %ecx
+ movl %ecx, %ebx
+ shll $3, %ecx
+ subq %rcx, %rsp
+
+ cmpl $0, %ebx
+ jz 1f
+ movq EC_ARG1(%rdi), %rcx
+ decl %ebx
+ jz 1f
+ movq EC_ARG2(%rdi), %rdx
+ decl %ebx
+ jz 1f
+ movq EC_ARG3(%rdi), %r8
+ decl %ebx
+ jz 1f
+ movq EC_ARG4(%rdi), %r9
+ /* XXX on-stack regs */
+1: movq EC_FPTR(%rdi), %rax
+ callq *%rax
+
+ movq PCPU(CURTHREAD), %rbx
+ movq TD_MD+MD_EFIRT_TMP(%rbx), %rdi
+ movq %rax, EC_STATE(%rdi)
+ movq PCPU(CURPCB), %rsi
+ xorl %eax, %eax
+ movq %rax, PCB_ONFAULT(%rsi)
+
+efirt_arch_call_tail:
+ movq EC_STATE+TF_R15(%rdi), %r15
+ movq EC_STATE+TF_R14(%rdi), %r14
+ movq EC_STATE+TF_R13(%rdi), %r13
+ movq EC_STATE+TF_R12(%rdi), %r12
+ movq EC_STATE+TF_RBP(%rdi), %rbp
+ movq EC_STATE+TF_RSP(%rdi), %rsp
+ movq EC_STATE+TF_RBX(%rdi), %rbx
+
+ popq %rbp
+ ret
+END(efirt_arch_call)
+
+ENTRY(efirt_fault)
+ xorl %eax, %eax
+ movq PCPU(CURPCB), %rsi
+ movq %rax, PCB_ONFAULT(%rsi)
+ movl $EFAULT, %eax
+ movq PCPU(CURTHREAD), %rbx
+ movq TD_MD+MD_EFIRT_TMP(%rbx), %rdi
+ jmp efirt_arch_call_tail
+END(efirt_fault)
diff --git a/sys/amd64/amd64/genassym.c b/sys/amd64/amd64/genassym.c
index d61b5c7bb6d..c38a596089a 100644
--- a/sys/amd64/amd64/genassym.c
+++ b/sys/amd64/amd64/genassym.c
@@ -77,12 +77,15 @@ ASSYM(P_MD, offsetof(struct proc, p_md));
ASSYM(MD_LDT, offsetof(struct mdproc, md_ldt));
ASSYM(MD_LDT_SD, offsetof(struct mdproc, md_ldt_sd));
+ASSYM(MD_EFIRT_TMP, offsetof(struct mdthread, md_efirt_tmp));
+
ASSYM(TD_LOCK, offsetof(struct thread, td_lock));
ASSYM(TD_FLAGS, offsetof(struct thread, td_flags));
ASSYM(TD_PCB, offsetof(struct thread, td_pcb));
ASSYM(TD_PFLAGS, offsetof(struct thread, td_pflags));
ASSYM(TD_PROC, offsetof(struct thread, td_proc));
ASSYM(TD_FRAME, offsetof(struct thread, td_frame));
+ASSYM(TD_MD, offsetof(struct thread, td_md));
ASSYM(TDF_ASTPENDING, TDF_ASTPENDING);
ASSYM(TDF_NEEDRESCHED, TDF_NEEDRESCHED);
@@ -249,3 +252,12 @@ ASSYM(__FreeBSD_version, __FreeBSD_version);
#ifdef HWPMC_HOOKS
ASSYM(PMC_FN_USER_CALLCHAIN, PMC_FN_USER_CALLCHAIN);
#endif
+
+ASSYM(EC_EFI_STATUS, offsetof(struct efirt_callinfo, ec_efi_status));
+ASSYM(EC_FPTR, offsetof(struct efirt_callinfo, ec_fptr));
+ASSYM(EC_ARGCNT, offsetof(struct efirt_callinfo, ec_argcnt));
+ASSYM(EC_ARG1, offsetof(struct efirt_callinfo, ec_arg1));
+ASSYM(EC_ARG2, offsetof(struct efirt_callinfo, ec_arg2));
+ASSYM(EC_ARG3, offsetof(struct efirt_callinfo, ec_arg3));
+ASSYM(EC_ARG4, offsetof(struct efirt_callinfo, ec_arg4));
+ASSYM(EC_STATE, offsetof(struct efirt_callinfo, ec_state));
diff --git a/sys/amd64/amd64/trap.c b/sys/amd64/amd64/trap.c
index 019decb837a..4d03da234f1 100644
--- a/sys/amd64/amd64/trap.c
+++ b/sys/amd64/amd64/trap.c
@@ -806,7 +806,7 @@ trap_pfault(struct trapframe *frame, int usermode)
* If nx protection of the usermode portion of kernel page
* tables caused trap, panic.
*/
- if (PCPU_GET(curpmap)->pm_ucr3 != PMAP_NO_CR3 && usermode &&
+ if (usermode && PCPU_GET(curpmap)->pm_ucr3 != PMAP_NO_CR3 &&
pg_nx != 0 && (frame->tf_err & (PGEX_P | PGEX_W |
PGEX_U | PGEX_I)) == (PGEX_P | PGEX_U | PGEX_I) &&
(curpcb->pcb_saved_ucr3 & ~CR3_PCID_MASK)==
diff --git a/sys/amd64/include/frame.h b/sys/amd64/include/frame.h
index f0a6fcf5bc9..c84ee2a2064 100644
--- a/sys/amd64/include/frame.h
+++ b/sys/amd64/include/frame.h
@@ -47,4 +47,15 @@ struct pti_frame {
register_t pti_ss;
};
+struct efirt_callinfo {
+ register_t ec_efi_status;
+ register_t ec_fptr;
+ register_t ec_argcnt;
+ register_t ec_arg1;
+ register_t ec_arg2;
+ register_t ec_arg3;
+ register_t ec_arg4;
+ struct trapframe ec_state;
+};
+
#endif
diff --git a/sys/amd64/include/proc.h b/sys/amd64/include/proc.h
index f52c71208e6..54f5b416a7d 100644
--- a/sys/amd64/include/proc.h
+++ b/sys/amd64/include/proc.h
@@ -62,6 +62,8 @@ struct mdthread {
register_t md_saved_flags; /* (k) */
register_t md_spurflt_addr; /* (k) Spurious page fault address. */
struct pmap_invl_gen md_invl_gen;
+ register_t md_efirt_tmp; /* (k) */
+ int md_efirt_dis_pf; /* (k) */
};
struct mdproc {
diff --git a/sys/dev/efidev/efirt.c b/sys/dev/efidev/efirt.c
index d48074d72d6..088c80cfb71 100644
--- a/sys/dev/efidev/efirt.c
+++ b/sys/dev/efidev/efirt.c
@@ -96,6 +96,8 @@ static int efi_status2err[25] = {
static int efi_enter(void);
static void efi_leave(void);
+int efirt_arch_call(struct efirt_callinfo *);
+
static int
efi_status_to_errno(efi_status status)
{
@@ -296,16 +298,22 @@ efi_get_table(struct uuid *uuid, void **ptr)
static int
efi_get_time_locked(struct efi_tm *tm, struct efi_tmcap *tmcap)
{
- efi_status status;
+ struct efirt_callinfo ec;
int error;
EFI_TIME_OWNED()
+ bzero(&ec, sizeof(ec));
+ ec.ec_argcnt = 2;
+ ec.ec_arg1 = (uintptr_t)tm;
+ ec.ec_arg2 = (uintptr_t)tmcap;
error = efi_enter();
if (error != 0)
return (error);
- status = efi_runtime->rt_gettime(tm, tmcap);
+ ec.ec_fptr = (uintptr_t)(efi_runtime->rt_gettime);
+ error = efirt_arch_call(&ec);
efi_leave();
- error = efi_status_to_errno(status);
+ if (error == 0)
+ error = efi_status_to_errno(ec.ec_efi_status);
return (error);
}
@@ -359,16 +367,21 @@ efi_reset_system(void)
static int
efi_set_time_locked(struct efi_tm *tm)
{
- efi_status status;
+ struct efirt_callinfo ec;
int error;
EFI_TIME_OWNED();
+ bzero(&ec, sizeof(ec));
+ ec.ec_argcnt = 1;
+ ec.ec_arg1 = (uintptr_t)tm;
error = efi_enter();
if (error != 0)
return (error);
- status = efi_runtime->rt_settime(tm);
+ ec.ec_fptr = (uintptr_t)(efi_runtime->rt_settime);
+ error = efirt_arch_call(&ec);
efi_leave();
- error = efi_status_to_errno(status);
+ if (error == 0)
+ error = efi_status_to_errno(ec.ec_efi_status);
return (error);
}
diff --git a/sys/dev/efidev/efirtc.c b/sys/dev/efidev/efirtc.c
index b9e06bcc362..e5a2ec262bf 100644
--- a/sys/dev/efidev/efirtc.c
+++ b/sys/dev/efidev/efirtc.c
@@ -74,7 +74,8 @@ efirtc_probe(device_t dev)
*/
if ((error = efi_get_time(&tm)) != 0) {
if (bootverbose)
- device_printf(dev, "cannot read EFI realtime clock\n");
+ device_printf(dev, "cannot read EFI realtime clock, "
+ "error %d\n", error);
return (error);
}
device_set_desc(dev, "EFI Realtime Clock");
diff --git a/sys/kern/kern_thread.c b/sys/kern/kern_thread.c
index 79b34dc7649..b60c2d5b40e 100644
--- a/sys/kern/kern_thread.c
+++ b/sys/kern/kern_thread.c
@@ -83,7 +83,7 @@ _Static_assert(offsetof(struct thread, td_pflags) == 0x104,
"struct thread KBI td_pflags");
_Static_assert(offsetof(struct thread, td_frame) == 0x470,
"struct thread KBI td_frame");
-_Static_assert(offsetof(struct thread, td_emuldata) == 0x518,
+_Static_assert(offsetof(struct thread, td_emuldata) == 0x528,
"struct thread KBI td_emuldata");
_Static_assert(offsetof(struct proc, p_flag) == 0xb0,
"struct proc KBI p_flag");
diff --git a/sys/kern/subr_rtc.c b/sys/kern/subr_rtc.c
index 82c276f210a..1c3a9804694 100644
--- a/sys/kern/subr_rtc.c
+++ b/sys/kern/subr_rtc.c
@@ -138,6 +138,7 @@ settime_task_func(void *arg, int pending)
{
struct timespec ts;
struct rtc_instance *rtc;
+ int error;
rtc = arg;
if (!(rtc->flags & CLOCKF_SETTIME_NO_TS)) {
@@ -150,7 +151,9 @@ settime_task_func(void *arg, int pending)
ts.tv_sec = 0;
ts.tv_nsec = 0;
}
- CLOCK_SETTIME(rtc->clockdev, &ts);
+ error = CLOCK_SETTIME(rtc->clockdev, &ts);
+ if (error != 0 && bootverbose)
+ device_printf(rtc->clockdev, "CLOCK_SETTIME error %d\n", error);
}
static void
diff --git a/sys/modules/efirt/Makefile b/sys/modules/efirt/Makefile
index 2613150db48..524d93ead93 100644
--- a/sys/modules/efirt/Makefile
+++ b/sys/modules/efirt/Makefile
@@ -5,7 +5,11 @@
KMOD= efirt
SRCS= efirt.c efirt_machdep.c efidev.c
-SRCS+= efirtc.c
+SRCS+= efirtc.c efirt_support.S
SRCS+= device_if.h bus_if.h clock_if.h
+efirt_support.o: efirt_support.S assym.inc
+ ${CC} -c -x assembler-with-cpp -DLOCORE ${CFLAGS} \
+ ${.IMPSRC} -o ${.TARGET}
+
.include <bsd.kmod.mk>
More information about the freebsd-current
mailing list