Re: [PATCH V13 10/10] arm/arm64: KVM: add guest SEA support
sea_status) {
> kvm_err("Unsupported FSC: EC=%#x xFSC=%#lx ESR_EL2=%#lx\n",
> kvm_vcpu_trap_get_class(vcpu),
> (unsigned long)kvm_vcpu_trap_get_fault(vcpu),
> diff --git a/arch/arm64/include/asm/kvm_arm.h
> b/arch/arm64/include/asm/kvm_arm.h
> index 6e99978..61d694c 100644
> --- a/arch/arm64/include/asm/kvm_arm.h
> +++ b/arch/arm64/include/asm/kvm_arm.h
> @@ -204,6 +204,16 @@
> #define FSC_FAULTESR_ELx_FSC_FAULT
> #define FSC_ACCESS ESR_ELx_FSC_ACCESS
> #define FSC_PERM ESR_ELx_FSC_PERM
> +#define FSC_SEA ESR_ELx_FSC_EXTABT
> +#define FSC_SEA_TTW0 (0x14)
> +#define FSC_SEA_TTW1 (0x15)
> +#define FSC_SEA_TTW2 (0x16)
> +#define FSC_SEA_TTW3 (0x17)
> +#define FSC_SECC (0x18)
> +#define FSC_SECC_TTW0(0x1c)
> +#define FSC_SECC_TTW1(0x1d)
> +#define FSC_SECC_TTW2(0x1e)
> +#define FSC_SECC_TTW3(0x1f)
>
> /* Hyp Prefetch Fault Address Register (HPFAR/HDFAR) */
> #define HPFAR_MASK (~UL(0xf))
> diff --git a/arch/arm64/include/asm/system_misc.h
> b/arch/arm64/include/asm/system_misc.h
> index bc81243..5b2cecd 100644
> --- a/arch/arm64/include/asm/system_misc.h
> +++ b/arch/arm64/include/asm/system_misc.h
> @@ -58,4 +58,6 @@ void hook_debug_fault_code(int nr, int (*fn)(unsigned long,
> unsigned int,
>
> #endif /* __ASSEMBLY__ */
>
> +int handle_guest_sea(unsigned long addr, unsigned int esr);
Same remark here.
> +
> #endif /* __ASM_SYSTEM_MISC_H */
> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index f7372ce..ee96307 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c
> @@ -497,6 +497,7 @@ static int do_bad(unsigned long addr, unsigned int esr,
> struct pt_regs *regs)
> static int do_sea(unsigned long addr, unsigned int esr, struct pt_regs *regs)
> {
> struct siginfo info;
> + int ret = 0;
>
> pr_err("Synchronous External Abort: %s (0x%08x) at 0x%016lx\n",
>fault_name(esr), esr, addr);
> @@ -508,7 +509,7 @@ static int do_sea(unsigned long addr, unsigned int esr,
> struct pt_regs *regs)
>*/
> if (IS_ENABLED(CONFIG_ACPI_APEI_SEA)) {
> nmi_enter();
> - ghes_notify_sea();
> + ret = ghes_notify_sea();
> nmi_exit();
> }
>
> @@ -521,7 +522,7 @@ static int do_sea(unsigned long addr, unsigned int esr,
> struct pt_regs *regs)
> info.si_addr = (void __user *)addr;
> arm64_notify_die("", regs, , esr);
>
> - return 0;
> + return ret;
> }
>
> static const struct fault_info {
> @@ -603,6 +604,20 @@ static const char *fault_name(unsigned int esr)
> }
>
> /*
> + * Handle Synchronous External Aborts that occur in a guest kernel.
> + */
> +int handle_guest_sea(unsigned long addr, unsigned int esr)
> +{
> + int ret = -ENOENT;
> +
> + if(IS_ENABLED(CONFIG_ACPI_APEI_SEA)) {
> + ret = ghes_notify_sea();
> + }
> +
> + return ret;
> +}
> +
> +/*
> * Dispatch a data abort to the relevant handler.
> */
> asmlinkage void __exception do_mem_abort(unsigned long addr, unsigned int
> esr,
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 230b095..81eabc6 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -810,17 +810,18 @@ static int ghes_notify_sci(struct notifier_block *this,
> #ifdef CONFIG_ACPI_APEI_SEA
> static LIST_HEAD(ghes_sea);
>
> -void ghes_notify_sea(void)
> +int ghes_notify_sea(void)
> {
> struct ghes *ghes;
> + int ret = -ENOENT;
>
> - /*
> - * synchronize_rcu() will wait for nmi_exit(), so no need to
> - * rcu_read_lock().
> - */
> + rcu_read_lock();
> list_for_each_entry_rcu(ghes, _sea, list) {
> - ghes_proc(ghes);
> + if(!ghes_proc(ghes))
> + ret = 0;
> }
> + rcu_read_unlock();
> + return ret;
> }
>
> static void ghes_sea_add(struct ghes *ghes)
> diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h
> index 18bc935..2a727dc 100644
> --- a/include/acpi/ghes.h
> +++ b/include/acpi/ghes.h
> @@ -99,6 +99,6 @@ static inline void *acpi_hest_generic_data_payload(struct
> acpi_hest_generic_data
> gdata + 1;
> }
>
> -void ghes_notify_sea(void);
> +int ghes_notify_sea(void);
>
> #endif /* GHES_H */
>
Otherwise, the KVM changes look good to me. Assuming you fix the two
nits above:
Acked-by: Marc Zyngier <marc.zyng...@arm.com>
Thanks,
M.
--
Jazz is not dead. It just smells funny...
--
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 0/4] arm/efi: fix memblock reallocation crash due to persistent reservations
Hi Ard, On 06/11/18 11:37, Ard Biesheuvel wrote: > This series addresses the kexec/kdump crash on arm64 system with many CPUs > that was reported by Bhupesh. > > Patches #1 and #2 fix the actual crash. Patches #3 and #4 optimize the > EFI persistent memreserve infrastructure so that fewer memblock reservations > are required. > > Ard Biesheuvel (4): > arm64: memblock: don't permit memblock resizing until linear mapping > is up > efi/arm: defer persistent reservations until after paging_init() > efi: permit multiple entries in persistent memreserve data structure > efi: reduce the amount of memblock reservations for persistent > allocations > > arch/arm/kernel/setup.c | 1 + > arch/arm64/kernel/setup.c | 1 + > arch/arm64/mm/init.c| 2 - > arch/arm64/mm/mmu.c | 2 + > drivers/firmware/efi/efi.c | 59 ++-- > drivers/firmware/efi/libstub/arm-stub.c | 2 +- > include/linux/efi.h | 23 +++- > 7 files changed, 68 insertions(+), 22 deletions(-) I've just given these patches a go a a TX2 box (one of the 224 CPU ones...), and kexec worked just fine (v4.20-rc1 vanilla didn't manage to kexec on this box). There seem to be some additional userspace patches that are required for the ACPI tables not to be corrupted in the secondary kernel, but that's an orthogonal issue. Feel free to add Tested-by: Marc Zyngier Thanks, M. -- Jazz is not dead. It just smells funny...
Re: [PATCH 2/4] efi/arm: defer persistent reservations until after paging_init()
On 06/11/18 23:49, Russell King - ARM Linux wrote:
> On Tue, Nov 06, 2018 at 09:06:56PM +0100, Ard Biesheuvel wrote:
>> On 6 November 2018 at 20:08, Russell King - ARM Linux
>> wrote:
>>> On Tue, Nov 06, 2018 at 08:02:58PM +0100, Ard Biesheuvel wrote:
On 6 November 2018 at 12:37, Ard Biesheuvel
wrote:
> The new memory EFI reservation feature we introduced to allow memory
> reservations to persist across kexec may trigger an unbounded number
> of calls to memblock_reserve(). The memblock subsystem can deal with
> this fine, but not before memblock resizing is enabled, which we can
> only do after paging_init(), when the memory we reallocate the array
> into is actually mapped.
>
> So break out the memreserve table processing into a separate function
> and call if after paging_init() on both arm64 and ARM.
>
> Cc: Russell King
> Signed-off-by: Ard Biesheuvel
Russell,
If you are ok with this patch, may I have your ack please? I would
like to send it out before the end of the week.
>>>
>>> You're not going to get a quick answer to this, because it needs me to
>>> investigate what the effect of this change actually is by code review.
>>> I can't guarantee when I'll get around to that.
>>>
>>
>> Fair enough.
>>
>> I will drop the ARM hunk for now then, and we'll fix ARM when you have
>> more time.
>
> I don't see how you can do that - you're dropping the processing of
> reserved areas out of the efi_config_parse_tables() path, so that
> won't happen any more on ARM if you don't apply the ARM hunk.
The only reason for the whole persistent region thing is to support
GICv3 redistributors memory tables across kexec, for those GIC
implementations where LPIs cannot be disabled once they have been
enabled.
There is no HW platform that combines 32bit cores and GICv3 (not to
mention using EFI). The only existing "platform" is KVM/arm64, which
doesn't suffer from the above problem (LPIs can be freely disabled in
emulation). So the whole persistent region feature serves no purpose on
32bit ARM.
The only issue with dropping this hunk is that the memory reservation
feature is enabled on 32bit ARM the first place, which can easily be
dealt with something along those lines:
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 787d7850e064..8bb263c9b99a 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1290,6 +1290,7 @@ config EFI
select EFI_RUNTIME_WRAPPERS
select EFI_STUB
select EFI_ARMSTUB
+ select EFI_PERSISTENT_RESERVATIONS
default y
help
This option provides support for runtime services provided
diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index 89110dfc7127..b728dab9e901 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -198,3 +198,6 @@ config EFI_DEV_PATH_PARSER
bool
depends on ACPI
default n
+
+config EFI_PERSISTENT_RESERVATIONS
+ bool
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 249eb70691b0..2a1903ab6d21 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -962,6 +962,7 @@ bool efi_is_table_address(unsigned long phys_addr)
return false;
}
+#ifdef CONFIG_EFI_PERSISTENT_RESERVATIONS
static DEFINE_SPINLOCK(efi_mem_reserve_persistent_lock);
int efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
@@ -993,6 +994,12 @@ int efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
return 0;
}
+#else
+int efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
+{
+ return 0;
+}
+#endif
#ifdef CONFIG_KEXEC
static int update_efi_random_seed(struct notifier_block *nb,
Thanks,
M.
--
Jazz is not dead. It just smells funny...
Re: [PATCH 0/4] arm/efi: fix memblock reallocation crash due to persistent reservations
On Tue, 06 Nov 2018 19:01:51 +, Ard Biesheuvel wrote: > > On 6 November 2018 at 19:27, Marc Zyngier wrote: > > Hi Ard, > > > > On 06/11/18 11:37, Ard Biesheuvel wrote: > >> This series addresses the kexec/kdump crash on arm64 system with many CPUs > >> that was reported by Bhupesh. > >> > >> Patches #1 and #2 fix the actual crash. Patches #3 and #4 optimize the > >> EFI persistent memreserve infrastructure so that fewer memblock > >> reservations > >> are required. > >> > >> Ard Biesheuvel (4): > >> arm64: memblock: don't permit memblock resizing until linear mapping > >> is up > >> efi/arm: defer persistent reservations until after paging_init() > >> efi: permit multiple entries in persistent memreserve data structure > >> efi: reduce the amount of memblock reservations for persistent > >> allocations > >> > >> arch/arm/kernel/setup.c | 1 + > >> arch/arm64/kernel/setup.c | 1 + > >> arch/arm64/mm/init.c| 2 - > >> arch/arm64/mm/mmu.c | 2 + > >> drivers/firmware/efi/efi.c | 59 ++-- > >> drivers/firmware/efi/libstub/arm-stub.c | 2 +- > >> include/linux/efi.h | 23 +++- > >> 7 files changed, 68 insertions(+), 22 deletions(-) > > > > I've just given these patches a go a a TX2 box (one of the 224 CPU > > ones...), and kexec worked just fine (v4.20-rc1 vanilla didn't manage to > > kexec on this box). > > > > There seem to be some additional userspace patches that are required for > > the ACPI tables not to be corrupted in the secondary kernel, but that's > > an orthogonal issue. > > > > Feel free to add > > > > Tested-by: Marc Zyngier > > > > Thanks Marc. > > Any thoughts on whether patches #3 and #4 should be included as fixes? > I'm leaning towards yes. They certainly massively reduce the number of list entries, and probably help reducing the overhead. I'd be inclined to merge them as well. Thanks, M. -- Jazz is not dead, it just smell funny.
Re: [PATCH 0/3] efi: add support for persistent memory reservations
On Fri, 21 Sep 2018 18:51:08 +0100, Ard Biesheuvel wrote: > > On 21 September 2018 at 10:46, Marc Zyngier wrote: > > On Fri, 21 Sep 2018 18:14:43 +0100, > > Jeffrey Hugo wrote: > >> > >> On 9/21/2018 10:32 AM, Ard Biesheuvel wrote: > >> > Add support for persistent memory reservations across kexec reboot on EFI > >> > systems by introducing a Linux-specific UEFI configuration table that > >> > points > >> > to a linked list in memory that can be augmented by each successive kexec > >> > kernel to describe regions in memory that the subsequent kernel should > >> > treat > >> > as reserved. > >> > > >> > The specific use case for this feature is GICv3 ARM systems that are not > >> > able to disable DMA access to LPI tables, meaning we have to reserve them > >> > and make the next kernel reuse the existing tables rather than allocating > >> > them from scratch. > >> > >> As far as I recall, Shanker Donthineni attempted to address a similar > >> (the same?) issue. I think we would be very interested, if that is > >> the case. > > > > Shanker's patches didn't solve the story of preserving the allocated > > memory across kexec, and were just expecting to get similar mappings. > > > > Ard's patches allow us to track the allocation, and to reuse those if > > it is safe to do so. > > > >> Is there another series that depends on these changes? If not, can > >> you describe the plan for this? > > > > I have a series which does exactly that[1], and that rely on Ard's > > patches. I'm about to post these patches now the dependency is public. > > > > FYI the efi_reserve_mem_region() function has been renamed to > efi_mem_reserve_persistent() Ah, indeed. Now fixed. Thanks, M. -- Jazz is not dead, it just smell funny.
Re: [PATCH 0/3] efi: add support for persistent memory reservations
On Fri, 21 Sep 2018 18:14:43 +0100, Jeffrey Hugo wrote: > > On 9/21/2018 10:32 AM, Ard Biesheuvel wrote: > > Add support for persistent memory reservations across kexec reboot on EFI > > systems by introducing a Linux-specific UEFI configuration table that points > > to a linked list in memory that can be augmented by each successive kexec > > kernel to describe regions in memory that the subsequent kernel should treat > > as reserved. > > > > The specific use case for this feature is GICv3 ARM systems that are not > > able to disable DMA access to LPI tables, meaning we have to reserve them > > and make the next kernel reuse the existing tables rather than allocating > > them from scratch. > > As far as I recall, Shanker Donthineni attempted to address a similar > (the same?) issue. I think we would be very interested, if that is > the case. Shanker's patches didn't solve the story of preserving the allocated memory across kexec, and were just expecting to get similar mappings. Ard's patches allow us to track the allocation, and to reuse those if it is safe to do so. > Is there another series that depends on these changes? If not, can > you describe the plan for this? I have a series which does exactly that[1], and that rely on Ard's patches. I'm about to post these patches now the dependency is public. Thanks, M. [1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/gicv3-kdump -- Jazz is not dead, it just smell funny.
Re: [GIT PULL 00/11] EFI updates for v4.20
Hi all, On 27/09/18 09:50, Ard Biesheuvel wrote: Thomas, Ingo, Please pull/cherry-pick the below. Note that the first three patches will be depended upon by an irqchip series that Marc Zyngier has sent out last week, and that targets the next release as well. So please advise how to proceed with that: we could simply apply those patches first and use the resulting commit in tip.git as a stable branch, I suppose, but I'll let Marc chime in in case he has any other ideas. A stable branch with these three patches would be great. The irqchip code will also end-up in tip, so it should all be quite easy to manage anyway. Thanks, M. -- Jazz is not dead. It just smells funny...
Re: [GIT PULL 00/11] EFI updates for v4.20
On Tue, 02 Oct 2018 09:38:17 +0100, Ingo Molnar wrote: > > > * Ingo Molnar wrote: > > > > > * Marc Zyngier wrote: > > > > > Hi all, > > > > > > On 27/09/18 09:50, Ard Biesheuvel wrote: > > > > Thomas, Ingo, > > > > > > > > Please pull/cherry-pick the below. Note that the first three patches > > > > will > > > > be depended upon by an irqchip series that Marc Zyngier has sent out > > > > last > > > > week, and that targets the next release as well. So please advise how to > > > > proceed with that: we could simply apply those patches first and use the > > > > resulting commit in tip.git as a stable branch, I suppose, but I'll let > > > > Marc chime in in case he has any other ideas. > > > > > > A stable branch with these three patches would be great. The irqchip code > > > will also end-up in tip, so it should all be quite easy to manage anyway. > > > > Yeah - nevertheless, to keep it all separated nicely, I've created a new > > topic tree for this: tip:efi/irqchip (2e3ac98133a3), with just these three > > patches on top of latest -git. > > > > That way it can then all be iterated separately. > > > > Does this approach work for you? > > It's a23d3bb05ccb - i.e. the commit ID of Ard's tree, so if you > already based your work on top of that then you won't even need to > rebase anything. No, it was already based on the tip/efi/core. Never mind, I'll rebase it. Thanks, M. -- Jazz is not dead, it just smell funny.
Re: [Bug Report] kdump crashes after latest EFI memblock changes on arm64 machines with large number of CPUs
On 05/11/18 11:11, Ard Biesheuvel wrote: > (+ Marc) > > On 1 November 2018 at 22:14, Bhupesh Sharma wrote: >> Hi, >> >> With the latest EFI changes for memblock reservation across kdump >> kernel from Ard (Commit 71e0940d52e107748b270213a01d3b1546657d74 >> ["efi: honour memory reservations passed via a linux specific config >> table"]), we hit a panic while trying to boot the kdump kernel on >> machines which have large number of CPUs. >> > > Just for my understanding: why do you boot all 224 CPus when running > the crash kernel? FWIW, I've used these patches to kexec a kernel on a 256 CPUs system, without any issue. Why am I not seeing this problem? Thanks, M. -- Jazz is not dead. It just smells funny...
Re: [PATCH] efi: permit calling efi_mem_reserve_persistent from atomic context
On Mon, 12 Nov 2018 02:45:48 +,
Qian Cai wrote:
>
>
>
> > On Nov 9, 2018, at 9:45 PM, Qian Cai wrote:
> >
> >
> > On 11/8/18 at 1:05 PM, Ard Biesheuvel wrote:
> >
> >> Currently, efi_mem_reserve_persistent() may not be called from atomic
> >> context, since both the kmalloc() call and the memremap() call may
> >> sleep.
> >>
> >> The kmalloc() call is easy enough to fix, but the memremap() call
> >> needs to be moved into an init hook since we cannot control the
> >> memory allocation behavior of memremap() at the call site.
> >>
> >> Signed-off-by: Ard Biesheuvel
> >> ---
> >> drivers/firmware/efi/efi.c | 31 +++
> >> 1 file changed, 19 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> >> index 249eb70691b0..cfc876e0b67b 100644
> >> --- a/drivers/firmware/efi/efi.c
> >> +++ b/drivers/firmware/efi/efi.c
> >> @@ -963,36 +963,43 @@ bool efi_is_table_address(unsigned long phys_addr)
> >> }
> >>
> >> static DEFINE_SPINLOCK(efi_mem_reserve_persistent_lock);
> >> +static struct linux_efi_memreserve *efi_memreserve_root __ro_after_init;
> >>
> >> int efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
> >> {
> >> - struct linux_efi_memreserve *rsv, *parent;
> >> + struct linux_efi_memreserve *rsv;
> >>
> >> - if (efi.mem_reserve == EFI_INVALID_TABLE_ADDR)
> >> + if (!efi_memreserve_root)
> >>return -ENODEV;
> >>
> >> - rsv = kmalloc(sizeof(*rsv), GFP_KERNEL);
> >> + rsv = kmalloc(sizeof(*rsv), GFP_ATOMIC);
> >>if (!rsv)
> >>return -ENOMEM;
> >>
> >> - parent = memremap(efi.mem_reserve, sizeof(*rsv), MEMREMAP_WB);
> >> - if (!parent) {
> >> - kfree(rsv);
> >> - return -ENOMEM;
> >> - }
> >> -
> >>rsv->base = addr;
> >>rsv->size = size;
> >>
> >>spin_lock(_mem_reserve_persistent_lock);
> >> - rsv->next = parent->next;
> >> - parent->next = __pa(rsv);
> >> + rsv->next = efi_memreserve_root->next;
> >> + efi_memreserve_root->next = __pa(rsv);
> >>spin_unlock(_mem_reserve_persistent_lock);
> >>
> >> - memunmap(parent);
> >> + return 0;
> >> +}
> >>
> >> +static int __init efi_memreserve_root_init(void)
> >> +{
> >> + if (efi.mem_reserve == EFI_INVALID_TABLE_ADDR)
> >> + return -ENODEV;
> >> +
> >> + efi_memreserve_root = memremap(efi.mem_reserve,
> >> + sizeof(*efi_memreserve_root),
> >> + MEMREMAP_WB);
> >> + if (!efi_memreserve_root)
> >> + return -ENOMEM;
> >>return 0;
> >> }
> >> +early_initcall(efi_memreserve_root_init);
> >>
> >> #ifdef CONFIG_KEXEC
> >> static int update_efi_random_seed(struct notifier_block *nb,
> >> --
> >> 2.19.1
> > BTW, I won’t be able to apply this patch on top of this series [1]. After
> > applied that series, the original BUG sleep from atomic is gone as well as
> > two other GIC warnings. Do you think a new patch is needed here?
> >
> > [1] https://www.spinics.net/lists/arm-kernel/msg685751.html
> OK, I was able to apply this patch on top of latest mainline (ccda4af0f4b9)
> which also include one patch (1/6) from the above series,
>
> However, the efi-related patches from the series (4/6, 5/6, and 6/6) are no
> longer able to be cleanly applied.
>
> As the results, the above patch did fix the original BUG: sleep from atomic,
> but it introduces 2 new warnings.
[...]
These are the warnings you've already reported, aren't they? And we've
established that if you apply the whole series, everything work as
intended at least on the GIC side (the timer issue is a different
story altogether).
Or am I missing something?
Thanks,
M.
--
Jazz is not dead, it just smell funny.
Re: [RFT PATCH] efi: map memreserve table before first use
On 20/11/2018 18:35, Ard Biesheuvel wrote:
> On Tue, 20 Nov 2018 at 19:29, Marc Zyngier wrote:
>>
>> Hi Ard,
>>
>> On 20/11/2018 17:35, Ard Biesheuvel wrote:
>>> Mapping the MEMRESERVE EFI configuration table from an early initcall
>>> is too late: the GICv3 ITS code that creates persistent reservations
>>> for the boot CPU's LPI tables is invoked from init_IRQ(), which runs
>>> much earlier than the handling of the initcalls.
>>>
>>> So instead, move the initialization performed by the initcall into
>>> efi_mem_reserve_persistent() itself.
>>>
>>> Signed-off-by: Ard Biesheuvel
>>
>> I've just given it a go on one of my TX2s, and it boots just fine. So
>> for that:
>>
>> Tested-by: Marc Zyngier
>>
>> A comment below though:
>>
>>> ---
>>> drivers/firmware/efi/efi.c | 26 ++
>>> 1 file changed, 10 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
>>> index fad7c62cfc0e..40de2f6734cc 100644
>>> --- a/drivers/firmware/efi/efi.c
>>> +++ b/drivers/firmware/efi/efi.c
>>> @@ -967,15 +967,23 @@ bool efi_is_table_address(unsigned long phys_addr)
>>> }
>>>
>>> static DEFINE_SPINLOCK(efi_mem_reserve_persistent_lock);
>>> -static struct linux_efi_memreserve *efi_memreserve_root __ro_after_init;
>>> +static struct linux_efi_memreserve *efi_memreserve_root;
>>>
>>> int efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
>>> {
>>> struct linux_efi_memreserve *rsv;
>>>
>>> - if (!efi_memreserve_root)
>>> + if (efi.mem_reserve == EFI_INVALID_TABLE_ADDR)
>>> return -ENODEV;
>>>
>>> + if (!efi_memreserve_root) {
>>> + efi_memreserve_root = memremap(efi.mem_reserve,
>>> +sizeof(*efi_memreserve_root),
>>> +MEMREMAP_WB);
>>> + if (WARN_ON_ONCE(!efi_memreserve_root))
>>> + return -ENOMEM;
>>
>> This is now a bit racy if there is more than a single online CPU.
>>
>>> + }
>>> +
>>> rsv = kmalloc(sizeof(*rsv), GFP_ATOMIC);
>>> if (!rsv)
>>> return -ENOMEM;
>>> @@ -991,20 +999,6 @@ int efi_mem_reserve_persistent(phys_addr_t addr, u64
>>> size)
>>> return 0;
>>> }
>>>
>>> -static int __init efi_memreserve_root_init(void)
>>> -{
>>> - if (efi.mem_reserve == EFI_INVALID_TABLE_ADDR)
>>> - return -ENODEV;
>>> -
>>> - efi_memreserve_root = memremap(efi.mem_reserve,
>>> -sizeof(*efi_memreserve_root),
>>> -MEMREMAP_WB);
>>> - if (!efi_memreserve_root)
>>> - return -ENOMEM;
>>> - return 0;
>>> -}
>>> -early_initcall(efi_memreserve_root_init);
>>
>> But if we keep this (+ a check that the root is indeed NULL), we should
>> be able to make sure efi_memreserve_root is set before we enable a
>> secondary CPU. Still fragile though.
>>
>
> Why is that stil fragile? It sure isn't pretty, but early initcalls
> run before SMP boot so it should always work as expected no?
It is just me being paranoid about things happening without locking, but
it should indeed work as expected.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
Re: [RFT PATCH] efi: map memreserve table before first use
Hi Ard,
On 20/11/2018 17:35, Ard Biesheuvel wrote:
> Mapping the MEMRESERVE EFI configuration table from an early initcall
> is too late: the GICv3 ITS code that creates persistent reservations
> for the boot CPU's LPI tables is invoked from init_IRQ(), which runs
> much earlier than the handling of the initcalls.
>
> So instead, move the initialization performed by the initcall into
> efi_mem_reserve_persistent() itself.
>
> Signed-off-by: Ard Biesheuvel
I've just given it a go on one of my TX2s, and it boots just fine. So
for that:
Tested-by: Marc Zyngier
A comment below though:
> ---
> drivers/firmware/efi/efi.c | 26 ++
> 1 file changed, 10 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index fad7c62cfc0e..40de2f6734cc 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -967,15 +967,23 @@ bool efi_is_table_address(unsigned long phys_addr)
> }
>
> static DEFINE_SPINLOCK(efi_mem_reserve_persistent_lock);
> -static struct linux_efi_memreserve *efi_memreserve_root __ro_after_init;
> +static struct linux_efi_memreserve *efi_memreserve_root;
>
> int efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
> {
> struct linux_efi_memreserve *rsv;
>
> - if (!efi_memreserve_root)
> + if (efi.mem_reserve == EFI_INVALID_TABLE_ADDR)
> return -ENODEV;
>
> + if (!efi_memreserve_root) {
> + efi_memreserve_root = memremap(efi.mem_reserve,
> +sizeof(*efi_memreserve_root),
> +MEMREMAP_WB);
> + if (WARN_ON_ONCE(!efi_memreserve_root))
> + return -ENOMEM;
This is now a bit racy if there is more than a single online CPU.
> + }
> +
> rsv = kmalloc(sizeof(*rsv), GFP_ATOMIC);
> if (!rsv)
> return -ENOMEM;
> @@ -991,20 +999,6 @@ int efi_mem_reserve_persistent(phys_addr_t addr, u64
> size)
> return 0;
> }
>
> -static int __init efi_memreserve_root_init(void)
> -{
> - if (efi.mem_reserve == EFI_INVALID_TABLE_ADDR)
> - return -ENODEV;
> -
> - efi_memreserve_root = memremap(efi.mem_reserve,
> -sizeof(*efi_memreserve_root),
> -MEMREMAP_WB);
> - if (!efi_memreserve_root)
> - return -ENOMEM;
> - return 0;
> -}
> -early_initcall(efi_memreserve_root_init);
But if we keep this (+ a check that the root is indeed NULL), we should
be able to make sure efi_memreserve_root is set before we enable a
secondary CPU. Still fragile though.
Thoughts?
M.
--
Jazz is not dead. It just smells funny...
Re: [PATCH] efi/arm: enable CP15 DMB instructions before cleaning the cache
Hi Ard, On Fri, 29 Mar 2019 18:24:18 +, Ard Biesheuvel wrote: > > The EFI stub is entered with the caches and MMU enabled by the > firmware, and once the stub is ready to hand over to the decompressor, > we clean and disable the caches. > > The cache clean routines use CP15 barrier instructions, which can be > disabled via SCTLR. Normally, when using the provided cache handling > routines to enable the caches and MMU, this bit is enabled as well. > However, but since we entered the stub with the caches already enabled, > this routine is not executed before we call the cache clean routines, > resulting in undefined instruction exceptions if the firmware never > enabled this bit. > > So set the bit explicitly in the EFI entry code. > > Cc: Marc Zyngier > Signed-off-by: Ard Biesheuvel > --- > arch/arm/boot/compressed/head.S | 10 ++ > 1 file changed, 10 insertions(+) > > diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S > index 6c7ccb428c07..62a49356fca3 100644 > --- a/arch/arm/boot/compressed/head.S > +++ b/arch/arm/boot/compressed/head.S > @@ -1438,6 +1438,16 @@ ENTRY(efi_stub_entry) > > @ Preserve return value of efi_entry() in r4 > mov r4, r0 > + > + @ our cache maintenance code relies on CP15 barrier instructions > + @ but since we arrived here with the MMU and caches configured > + @ by UEFI, we must ensure that the use of those instructions is > + @ enabled in the SCTLR register, since we never executed our own > + @ cache enable routine, which is normally in charge of this. > + mrc p15, 0, r1, c1, c0, 0 @ read SCTLR > + orr r1, r1, #(1 << 5) @ CP15 barrier instructions > + mcr p15, 0, r1, c1, c0, 0 @ write SCTLR > + To be on the safe side, you could add an isb here. I'm pretty sure it is immaterial on any ARMv7 core, but hey, I'm paranoid. With that: Acked-by: Marc Zyngier Thanks, M. -- Jazz is not dead, it just smell funny.
Re: [PATCH] efi/arm: enable CP15 DMB instructions before cleaning the cache
On Sat, 30 Mar 2019 13:10:58 +, Ard Biesheuvel wrote: > > On Sat, 30 Mar 2019 at 10:50, Marc Zyngier wrote: > > > > Hi Ard, > > > > On Fri, 29 Mar 2019 18:24:18 +, > > Ard Biesheuvel wrote: > > > > > > The EFI stub is entered with the caches and MMU enabled by the > > > firmware, and once the stub is ready to hand over to the decompressor, > > > we clean and disable the caches. > > > > > > The cache clean routines use CP15 barrier instructions, which can be > > > disabled via SCTLR. Normally, when using the provided cache handling > > > routines to enable the caches and MMU, this bit is enabled as well. > > > However, but since we entered the stub with the caches already enabled, > > > this routine is not executed before we call the cache clean routines, > > > resulting in undefined instruction exceptions if the firmware never > > > enabled this bit. > > > > > > So set the bit explicitly in the EFI entry code. > > > > > > Cc: Marc Zyngier > > > Signed-off-by: Ard Biesheuvel > > > --- > > > arch/arm/boot/compressed/head.S | 10 ++ > > > 1 file changed, 10 insertions(+) > > > > > > diff --git a/arch/arm/boot/compressed/head.S > > > b/arch/arm/boot/compressed/head.S > > > index 6c7ccb428c07..62a49356fca3 100644 > > > --- a/arch/arm/boot/compressed/head.S > > > +++ b/arch/arm/boot/compressed/head.S > > > @@ -1438,6 +1438,16 @@ ENTRY(efi_stub_entry) > > > > > > @ Preserve return value of efi_entry() in r4 > > > mov r4, r0 > > > + > > > + @ our cache maintenance code relies on CP15 barrier > > > instructions > > > + @ but since we arrived here with the MMU and caches > > > configured > > > + @ by UEFI, we must ensure that the use of those > > > instructions is > > > + @ enabled in the SCTLR register, since we never executed > > > our own > > > + @ cache enable routine, which is normally in charge of this. > > > + mrc p15, 0, r1, c1, c0, 0 @ read SCTLR > > > + orr r1, r1, #(1 << 5) @ CP15 barrier instructions > > > + mcr p15, 0, r1, c1, c0, 0 @ write SCTLR > > > + > > > > To be on the safe side, you could add an isb here. I'm pretty sure it > > is immaterial on any ARMv7 core, but hey, I'm paranoid. > > > > Well, this is actually triggering now under KVM, so I wonder if this > is a regression of some kind on the host side. But the EFI stub > shouldn't use CP15 barriers without enabling them, so we need this > patch in any case. My remark was only about the lack of an ISB instruction after the write to SCTLR, and you're perfectly right to enable CP15BEN. But there is something fishy. Looking at the KVM/arm64 code, SCTLR_EL1.CP15BEN resets to 1. If the barrier undefs in the stubs, something must have cleared it (or the reset code is busted...). I've dumped SCTLR from a guest running EFI (using GDB), and I do see SCTLR.CP15EN being set... Thanks, M. -- Jazz is not dead, it just smell funny.
Re: [PATCH] efi/arm: enable CP15 DMB instructions before cleaning the cache
On 07/04/2019 19:19, Ard Biesheuvel wrote: > On Sun, 31 Mar 2019 at 10:47, Marc Zyngier wrote: >> >> On Sat, 30 Mar 2019 13:10:58 +, >> Ard Biesheuvel wrote: >>> >>> On Sat, 30 Mar 2019 at 10:50, Marc Zyngier wrote: >>>> >>>> Hi Ard, >>>> >>>> On Fri, 29 Mar 2019 18:24:18 +, >>>> Ard Biesheuvel wrote: >>>>> >>>>> The EFI stub is entered with the caches and MMU enabled by the >>>>> firmware, and once the stub is ready to hand over to the decompressor, >>>>> we clean and disable the caches. >>>>> >>>>> The cache clean routines use CP15 barrier instructions, which can be >>>>> disabled via SCTLR. Normally, when using the provided cache handling >>>>> routines to enable the caches and MMU, this bit is enabled as well. >>>>> However, but since we entered the stub with the caches already enabled, >>>>> this routine is not executed before we call the cache clean routines, >>>>> resulting in undefined instruction exceptions if the firmware never >>>>> enabled this bit. >>>>> >>>>> So set the bit explicitly in the EFI entry code. >>>>> >>>>> Cc: Marc Zyngier >>>>> Signed-off-by: Ard Biesheuvel >>>>> --- >>>>> arch/arm/boot/compressed/head.S | 10 ++ >>>>> 1 file changed, 10 insertions(+) >>>>> >>>>> diff --git a/arch/arm/boot/compressed/head.S >>>>> b/arch/arm/boot/compressed/head.S >>>>> index 6c7ccb428c07..62a49356fca3 100644 >>>>> --- a/arch/arm/boot/compressed/head.S >>>>> +++ b/arch/arm/boot/compressed/head.S >>>>> @@ -1438,6 +1438,16 @@ ENTRY(efi_stub_entry) >>>>> >>>>> @ Preserve return value of efi_entry() in r4 >>>>> mov r4, r0 >>>>> + >>>>> + @ our cache maintenance code relies on CP15 barrier >>>>> instructions >>>>> + @ but since we arrived here with the MMU and caches >>>>> configured >>>>> + @ by UEFI, we must ensure that the use of those >>>>> instructions is >>>>> + @ enabled in the SCTLR register, since we never executed >>>>> our own >>>>> + @ cache enable routine, which is normally in charge of this. >>>>> + mrc p15, 0, r1, c1, c0, 0 @ read SCTLR >>>>> + orr r1, r1, #(1 << 5) @ CP15 barrier instructions >>>>> + mcr p15, 0, r1, c1, c0, 0 @ write SCTLR >>>>> + >>>> >>>> To be on the safe side, you could add an isb here. I'm pretty sure it >>>> is immaterial on any ARMv7 core, but hey, I'm paranoid. >>>> >>> >>> Well, this is actually triggering now under KVM, so I wonder if this >>> is a regression of some kind on the host side. But the EFI stub >>> shouldn't use CP15 barriers without enabling them, so we need this >>> patch in any case. >> >> My remark was only about the lack of an ISB instruction after the >> write to SCTLR, and you're perfectly right to enable CP15BEN. >> > > Actually, the CP15 ISB is not usable here, and using the v7 ISB breaks > v6. Would reading back SCTLR suffice? I don't think it would, at least from an architectural perspective. But any CPU that has a non RAO/WI SCTLR.CP15BEN must also support the architected barriers, otherwise it wouldn't be able to flip them back on. I believe this is the case on all ARMv6 CPUs. We could write something like: mrc p15, 0, r1, c1, c0, 0 @ read SCTLR tst r1, r1, #(1 << 5) bne 1f orr r1, r1, #(1 << 5) @ CP15 barrier instructions mcr p15, 0, r1, c1, c0, 0 @ write SCTLR isb 1: What do you think? > >> But there is something fishy. Looking at the KVM/arm64 code, >> SCTLR_EL1.CP15BEN resets to 1. If the barrier undefs in the stubs, >> something must have cleared it (or the reset code is busted...). >> >> I've dumped SCTLR from a guest running EFI (using GDB), and I do see >> SCTLR.CP15EN being set... >> > > Yeah. I won't be able to dig into this for a while, but in the mean > time, I'd like to get a guest fix in in any case. Absolutely. These are two distinct issues, and the first one definitely needs addressing. Thanks, M. -- Jazz is not dead. It just smells funny...
Re: [PATCH 1/2] arm64: account for GICv3 LPI tables in static memblock reserve table
On Thu, 14 Feb 2019 16:55:28 +,
Ard Biesheuvel wrote:
>
> On Thu, 14 Feb 2019 at 16:48, Marc Zyngier wrote:
> >
> > Hi Ard,
> >
> > On 13/02/2019 13:27, Ard Biesheuvel wrote:
> > > In the irqchip and EFI code, we have what basically amounts to a quirk
> > > to work around a peculiarity in the GICv3 architecture, which permits
> > > the system memory address of LPI tables to be programmable only once
> > > after a CPU reset. This means kexec kernels must use the same memory
> > > as the first kernel, and thus ensure that this memory has not been
> > > given out for other purposes by the time the ITS init code runs, which
> > > is not very early for secondary CPUs.
> > >
> > > On systems with many CPUs, these reservations could overflow the
> > > memblock reservation table, and this was addressed in commit
> > > eff896288872 ("efi/arm: Defer persistent reservations until after
> > > paging_init()"). However, this turns out to have made things worse,
> > > since the allocation of page tables and heap space for the resized
> > > memblock reservation table itself may overwrite the regions we are
> > > attempting to reserve, which may cause all kinds of corruption,
> > > also considering that the ITS will still be poking bits into that
> > > memory in response to incoming MSIs.
> > >
> > > So instead, let's grow the static memblock reservation table on such
> > > systems so it can accommodate these reservations at an earlier time.
> > > This will permit us to revert the above commit in a subsequent patch.
> > >
> > > Signed-off-by: Ard Biesheuvel
> > > ---
> > > arch/arm64/include/asm/memory.h | 11 +++
> > > include/linux/memblock.h| 3 ---
> > > mm/memblock.c | 10 --
> > > 3 files changed, 19 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/arch/arm64/include/asm/memory.h
> > > b/arch/arm64/include/asm/memory.h
> > > index e1ec947e7c0c..7e2b13cdd970 100644
> > > --- a/arch/arm64/include/asm/memory.h
> > > +++ b/arch/arm64/include/asm/memory.h
> > > @@ -332,6 +332,17 @@ static inline void *phys_to_virt(phys_addr_t x)
> > > #define virt_addr_valid(kaddr) \
> > > (_virt_addr_is_linear(kaddr) && _virt_addr_valid(kaddr))
> > >
> > > +/*
> > > + * Given that the GIC architecture permits ITS implementations that can
> > > only be
> > > + * configured with a LPI table address once, GICv3 systems with many
> > > CPUs may
> > > + * end up reserving a lot of different regions after a kexec for their
> > > LPI
> > > + * tables, as we are forced to reuse the same memory after kexec (and
> > > thus
> > > + * reserve it persistently with EFI beforehand)
> > > + */
> > > +#if defined(CONFIG_EFI) && defined(CONFIG_ARM_GIC_V3_ITS)
> > > +#define INIT_MEMBLOCK_RESERVED_REGIONS (INIT_MEMBLOCK_REGIONS + 2
> > > * NR_CPUS)
> >
> > Since GICv3 has 1 pending table per CPU, plus one global property table,
> > can we make this 2 * NR_CPUS + 1? Or is that enough already?
> >
>
> Ah, I misread the code then. That would mean we'll only need 1 extra
> slot per CPU.
>
> So I will change this to
>
> > > +#define INIT_MEMBLOCK_RESERVED_REGIONS (INIT_MEMBLOCK_REGIONS +
> > > NR_CPUS)
>
> considering that INIT_MEMBLOCK_REGIONS defaults to 128, so that one
> global table is already accounted for.
Look good to me.
Thanks,
M.
--
Jazz is not dead, it just smell funny.
Re: [PATCH 1/2] arm64: account for GICv3 LPI tables in static memblock reserve table
Hi Ard,
On 13/02/2019 13:27, Ard Biesheuvel wrote:
> In the irqchip and EFI code, we have what basically amounts to a quirk
> to work around a peculiarity in the GICv3 architecture, which permits
> the system memory address of LPI tables to be programmable only once
> after a CPU reset. This means kexec kernels must use the same memory
> as the first kernel, and thus ensure that this memory has not been
> given out for other purposes by the time the ITS init code runs, which
> is not very early for secondary CPUs.
>
> On systems with many CPUs, these reservations could overflow the
> memblock reservation table, and this was addressed in commit
> eff896288872 ("efi/arm: Defer persistent reservations until after
> paging_init()"). However, this turns out to have made things worse,
> since the allocation of page tables and heap space for the resized
> memblock reservation table itself may overwrite the regions we are
> attempting to reserve, which may cause all kinds of corruption,
> also considering that the ITS will still be poking bits into that
> memory in response to incoming MSIs.
>
> So instead, let's grow the static memblock reservation table on such
> systems so it can accommodate these reservations at an earlier time.
> This will permit us to revert the above commit in a subsequent patch.
>
> Signed-off-by: Ard Biesheuvel
> ---
> arch/arm64/include/asm/memory.h | 11 +++
> include/linux/memblock.h| 3 ---
> mm/memblock.c | 10 --
> 3 files changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> index e1ec947e7c0c..7e2b13cdd970 100644
> --- a/arch/arm64/include/asm/memory.h
> +++ b/arch/arm64/include/asm/memory.h
> @@ -332,6 +332,17 @@ static inline void *phys_to_virt(phys_addr_t x)
> #define virt_addr_valid(kaddr) \
> (_virt_addr_is_linear(kaddr) && _virt_addr_valid(kaddr))
>
> +/*
> + * Given that the GIC architecture permits ITS implementations that can only
> be
> + * configured with a LPI table address once, GICv3 systems with many CPUs may
> + * end up reserving a lot of different regions after a kexec for their LPI
> + * tables, as we are forced to reuse the same memory after kexec (and thus
> + * reserve it persistently with EFI beforehand)
> + */
> +#if defined(CONFIG_EFI) && defined(CONFIG_ARM_GIC_V3_ITS)
> +#define INIT_MEMBLOCK_RESERVED_REGIONS (INIT_MEMBLOCK_REGIONS + 2 *
> NR_CPUS)
Since GICv3 has 1 pending table per CPU, plus one global property table,
can we make this 2 * NR_CPUS + 1? Or is that enough already?
> +#endif
> +
> #include
>
> #endif
> diff --git a/include/linux/memblock.h b/include/linux/memblock.h
> index 64c41cf45590..859b55b66db2 100644
> --- a/include/linux/memblock.h
> +++ b/include/linux/memblock.h
> @@ -29,9 +29,6 @@ extern unsigned long max_pfn;
> */
> extern unsigned long long max_possible_pfn;
>
> -#define INIT_MEMBLOCK_REGIONS128
> -#define INIT_PHYSMEM_REGIONS 4
> -
> /**
> * enum memblock_flags - definition of memory region attributes
> * @MEMBLOCK_NONE: no special request
> diff --git a/mm/memblock.c b/mm/memblock.c
> index 022d4cbb3618..a526c3ab8390 100644
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -26,6 +26,12 @@
>
> #include "internal.h"
>
> +#define INIT_MEMBLOCK_REGIONS128
> +#define INIT_PHYSMEM_REGIONS 4
> +#ifndef INIT_MEMBLOCK_RESERVED_REGIONS
> +#define INIT_MEMBLOCK_RESERVED_REGIONS INIT_MEMBLOCK_REGIONS
> +#endif
> +
> /**
> * DOC: memblock overview
> *
> @@ -92,7 +98,7 @@ unsigned long max_pfn;
> unsigned long long max_possible_pfn;
>
> static struct memblock_region
> memblock_memory_init_regions[INIT_MEMBLOCK_REGIONS] __initdata_memblock;
> -static struct memblock_region
> memblock_reserved_init_regions[INIT_MEMBLOCK_REGIONS] __initdata_memblock;
> +static struct memblock_region
> memblock_reserved_init_regions[INIT_MEMBLOCK_RESERVED_REGIONS]
> __initdata_memblock;
> #ifdef CONFIG_HAVE_MEMBLOCK_PHYS_MAP
> static struct memblock_region
> memblock_physmem_init_regions[INIT_PHYSMEM_REGIONS] __initdata_memblock;
> #endif
> @@ -105,7 +111,7 @@ struct memblock memblock __initdata_memblock = {
>
> .reserved.regions = memblock_reserved_init_regions,
> .reserved.cnt = 1,/* empty dummy entry */
> - .reserved.max = INIT_MEMBLOCK_REGIONS,
> + .reserved.max = INIT_MEMBLOCK_RESERVED_REGIONS,
> .reserved.name = "reserved",
>
> #ifdef CONFIG_HAVE_MEMBLOCK_PHYS_MAP
>
Otherwise:
Acked-by: Marc Zyngier
M.
--
Jazz is not dead. It just smells funny...
Re: [PATCH v9 11/26] efi: Let architectures decide the flags that should be saved/restored
On Mon, 21 Jan 2019 15:33:30 +, Julien Thierry wrote: > > Currently, irqflags are saved before calling runtime services and > checked for mismatch on return. > > Provide a pair of overridable macros to save and restore (if needed) the > state that need to be preserved on return from a runtime service. > This allows to check for flags that are not necesarly related to > irqflags. > > Signed-off-by: Julien Thierry > Acked-by: Catalin Marinas > Cc: Ard Biesheuvel > Cc: linux-efi@vger.kernel.org With the changes requested by Ard, Acked-by: Marc Zyngier M. -- Jazz is not dead, it just smell funny.
Re: gicv3-its driver crashes in crash dump kernel
Hi Jonathan,
On 05/06/2019 23:14, Jonathan Richardson wrote:
> Hi,
>
> As of the 5.0 kernel we're seeing the crash dump kernel crash when the
> gicv3-its driver calls gic_reserve_range():
[...]
> On crash dump boot, gic calls the same function, efi_mem_reserve_persistent,
> finds the entry that was on initial boot (0xc3836000), converts it to a va,
> and then crashes when it's used on this line:
> atomic_fetch_add_unless(>count
>
> In the previous revision of this file, kmalloc was called and this worked
> fine.
>
> [0.00] GICv3: GIC: Using split EOI/Deactivate mode
> [0.00] GICv3: Distributor has no Range Selector support
> [0.00] GICv3: no VLPI support, no direct LPI support
> [0.00] GICv3: CPU0: found redistributor 1 region 0:0x63e2
> [0.00] ITS [mem 0x63c2-0x63c2]
> [0.00] ITS@0x63c2: allocated 32768 Devices @fd48
> (flat, esz 8, psz 64K, shr 0)
> [0.00] ITS: using cache flushing for cmd queue
> [0.00] iter: prsv = 0xc3836000
> [0.00] rsv = 0x43836000
> [0.00] Unable to handle kernel paging request at virtual address
> 80a343836004
> [0.00] Mem abort info:
> [0.00] ESR = 0x9604
> [0.00] Exception class = DABT (current EL), IL = 32 bits
>
> int __ref efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
> {
>
> for (prsv = efi_memreserve_root->next; prsv; prsv = rsv->next) {
> printk("iter: prsv = 0x%x\n", prsv);
> rsv = __va(prsv);
> printk("rsv = 0x%x\n", rsv);
> index = atomic_fetch_add_unless(>count, 1, rsv->size);
> if (index < rsv->size) {
> rsv->entry[index].base = addr;
> rsv->entry[index].size = size;
>
>
> It looks like the change has broken crash dump kernel, but I'm not
> sure what it should be doing instead. Has anyone used gicv3-its with
> crash dump kernel after this change?
I've definitely used kexec/kdump since, both in VMs and bare-metal.
Other than __va() going horribly wrong, I have no idea.
Ard, do you have any suggestion?
Thanks,
M.
--
Jazz is not dead. It just smells funny...
