Re: [PATCH] arm64/efi: fix variable 'si' set but not used

[Will Deacon]

On Tue, Jul 30, 2019 at 05:23:48PM -0400, Qian Cai wrote:
> GCC throws out this warning on arm64.
> 
> drivers/firmware/efi/libstub/arm-stub.c: In function 'efi_entry':
> drivers/firmware/efi/libstub/arm-stub.c:132:22: warning: variable 'si'
> set but not used [-Wunused-but-set-variable]
> 
> Fix it by making free_screen_info() a static inline function.
> 
> Signed-off-by: Qian Cai 
> ---
>  arch/arm64/include/asm/efi.h | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
> index 8e79ce9c3f5c..76a144702586 100644
> --- a/arch/arm64/include/asm/efi.h
> +++ b/arch/arm64/include/asm/efi.h
> @@ -105,7 +105,11 @@ static inline unsigned long 
> efi_get_max_initrd_addr(unsigned long dram_base,
>   ((protocol##_t *)instance)->f(instance, ##__VA_ARGS__)
>  
>  #define alloc_screen_info(x...)  _info
> -#define free_screen_info(x...)
> +
> +static inline void free_screen_info(efi_system_table_t *sys_table_arg,
> + struct screen_info *si)
> +{
> +}
>  
>  /* redeclare as 'hidden' so the compiler will generate relative references */
>  extern struct screen_info screen_info 
> __attribute__((__visibility__("hidden")));
> -- 
> 1.8.3.1

Acked-by: Will Deacon 

Will

Re: "arm64: Silence gcc warnings about arch ABI drift" breaks clang

[Will Deacon]

On Fri, Jun 07, 2019 at 08:40:10AM -0700, Nathan Chancellor wrote:
> On Fri, Jun 07, 2019 at 11:26:11AM -0400, Qian Cai wrote:
> > On Fri, 2019-06-07 at 16:25 +0100, Will Deacon wrote:
> > > On Fri, Jun 07, 2019 at 11:22:45AM -0400, Qian Cai wrote:
> > > > The linux-next commit "arm64: Silence gcc warnings about arch ABI 
> > > > drift" [1]
> > > > breaks clang build where it screams that unknown option "-Wno-psabi" and
> > > > generates errors below,
> > > 
> > > So that can be easily fixed with cc-option...
> > > 
> > > > [1] 
> > > > https://lore.kernel.org/linux-arm-kernel/1559817223-32585-1-git-send-ema
> > > > il-D
> > > > ave.mar...@arm.com/
> > > > 
> > > > ./drivers/firmware/efi/libstub/arm-stub.stub.o: In function
> > > > `install_memreserve_table':
> > > > ./linux/drivers/firmware/efi/libstub/arm-stub.c:73: undefined reference 
> > > > to
> > > > `__efistub___stack_chk_guard'
> > > > ./linux/drivers/firmware/efi/libstub/arm-stub.c:73: undefined reference 
> > > > to
> > > > `__efistub___stack_chk_guard'
> > > > ./linux/drivers/firmware/efi/libstub/arm-stub.c:93: undefined reference 
> > > > to
> > > > `__efistub___stack_chk_guard'
> > > > ./linux/drivers/firmware/efi/libstub/arm-stub.c:93: undefined reference 
> > > > to
> > > > `__efistub___stack_chk_guard'
> > > > ./linux/drivers/firmware/efi/libstub/arm-stub.c:94: undefined reference 
> > > > to
> > > > `__efistub___stack_chk_fail
> > > 
> > > ... but this looks unrelated. Are you saying you don't see these errors if
> > > you revert Dave's patch?
> > 
> > Yes.
> 
> I suspect the reason for this is -Wunknown-warning-option causes
> cc-option to fail. I see some disabled warnings like
> -Waddress-of-packed-member and -Wunused-const-variable when -Wno-psabi
> is unconditionally added.
> 
> I'll do some further triage but I think the obvious fix as Will
> suggested is to use cc-disable-warning. I'll send a patch.

Cheers, Nathan. I've already sent the pull for -rc4, but I can send your
fix for -rc5 next week.

Will

Re: "arm64: Silence gcc warnings about arch ABI drift" breaks clang

[Will Deacon]

On Fri, Jun 07, 2019 at 11:22:45AM -0400, Qian Cai wrote:
> The linux-next commit "arm64: Silence gcc warnings about arch ABI drift" [1]
> breaks clang build where it screams that unknown option "-Wno-psabi" and
> generates errors below,

So that can be easily fixed with cc-option...

> [1] 
> https://lore.kernel.org/linux-arm-kernel/1559817223-32585-1-git-send-email-D
> ave.mar...@arm.com/
> 
> ./drivers/firmware/efi/libstub/arm-stub.stub.o: In function
> `install_memreserve_table':
> ./linux/drivers/firmware/efi/libstub/arm-stub.c:73: undefined reference to
> `__efistub___stack_chk_guard'
> ./linux/drivers/firmware/efi/libstub/arm-stub.c:73: undefined reference to
> `__efistub___stack_chk_guard'
> ./linux/drivers/firmware/efi/libstub/arm-stub.c:93: undefined reference to
> `__efistub___stack_chk_guard'
> ./linux/drivers/firmware/efi/libstub/arm-stub.c:93: undefined reference to
> `__efistub___stack_chk_guard'
> ./linux/drivers/firmware/efi/libstub/arm-stub.c:94: undefined reference to
> `__efistub___stack_chk_fail

... but this looks unrelated. Are you saying you don't see these errors if
you revert Dave's patch?

Will

Re: [PATCH 1/2] arm64: account for GICv3 LPI tables in static memblock reserve table

[Will Deacon]

On Wed, Feb 13, 2019 at 02:27:37PM +0100, 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)
> +#endif

Assuming this "ought to be enough for anybody", then:

Acked-by: Will Deacon 

Will

Re: [PATCH] efi/arm: Don't expect a return value of ptdump_debugfs_register

[Will Deacon]

On Fri, Feb 01, 2019 at 11:18:22PM +0100, Ard Biesheuvel wrote:
> On Fri, 1 Feb 2019 at 20:21, Nathan Chancellor  
> wrote:
> >
> > As of commit e2a2e56e4082 ("arm64: dump: no need to check return value
> > of debugfs_create functions") in the arm64 for-next/core branch,
> > ptdump_debugfs_register does not have a return value, which causes a
> > build error here:
> >
> > drivers/firmware/efi/arm-runtime.c:51:9: error: returning 'void' from a
> > function with incompatible result type 'int'
> > return ptdump_debugfs_register(_ptdump_info, "efi_page_tables");
> >^~~~
> > 1 error generated.
> >
> > The arm version is still awaiting acceptance [1] but in anticipation
> > of that patch being merged, restructure this function to call
> > ptdump_debugfs_register without expecting a return value.
> >
> > [1]: 
> > https://lore.kernel.org/lkml/20190122144114.9816-3-gre...@linuxfoundation.org/
> >
> > Signed-off-by: Nathan Chancellor 
> 
> Acked-by: Ard Biesheuvel 
> 
> 
> Catalin, Will,
> 
> Could you please apply this directly?

Sure, we'll pick it up.

Will

Re: [PATCH] efi: arm/arm64: allow SetVirtualAddressMap() to be omitted

[Will Deacon]

Hi Ard,

On Sat, Jan 26, 2019 at 11:22:07AM +0100, Ard Biesheuvel wrote:
> The UEFI spec revision 2.7 errata A section 8.4 has the following to
> say about the virtual memory runtime services:
> 
>   "This section contains function definitions for the virtual memory
>   support that may be optionally used by an operating system at runtime.
>   If an operating system chooses to make EFI runtime service calls in a
>   virtual addressing mode instead of the flat physical mode, then the
>   operating system must use the services in this section to switch the
>   EFI runtime services from flat physical addressing to virtual
>   addressing."

I should probably go RTFM, but what does UEFI say about the attributes of
"flat physical addressing"? The wording above implies to me that it should
act as though the stage-1 MMU is disabled because it's described as an
alternative to virtual addressing.

If we move in a direction where we avoid calling SetVirtualAddressMap() by
default on arm64, isn't there a real threat that this firmware call will no
longer be validated? Do we need to worry about that?

Finally, Bjorn said that SDM850 is unbootable without this change. Why is
that?

Will

Re: [PATCH 0/4] arm/efi: fix memblock reallocation crash due to persistent reservations

[Will Deacon]

On Tue, Nov 06, 2018 at 10:39:47PM +0100, Ard Biesheuvel wrote:
> On 6 November 2018 at 22:34, Will Deacon  wrote:
> > On Tue, Nov 06, 2018 at 12:37:28PM +0100, 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.
> >
> > I acked the arm64 part and patches 3 and 4 look good afaict, so:
> >
> > Acked-by: Will Deacon 
> >
> > for those as well.
> >
> > The only thing I was wondering is whether or not exhausting the page-sized
> > array in the first list entry is rare enough that we could just realloc the
> > thing and copy instead of chaining together new pages. That said, without
> > seeing the code it's hard to tell whether you save much complexity with such
> > a scheme so I'll leave it up to you.
> >
> 
> Well, the problem is that the page-sized array may have been allocated
> by a previous kernel, and so it may be memblock_reserve()d already, in
> which case reallocating does not actually free up the memory in a
> useful way.
> 
> Also, in the unlikely event that we race and allocate two new pages at
> the same time, the current scheme will not break (and we will
> ultimately fill up all the slots in both pages before allocating even
> more pages). This will become a lot trickier if we do reallocation.

Ah crap, yeah, the concurrency angle makes that really awful. Thanks.

> So if the current approach looks correct to you, then I'd prefer to
> stick with it.
> 
> Do you agree with applying #3 and #4 as fixes?

If Patch 1 alone is sufficient to solve the issue for Bhupesh, then I don't
think we have a need to treat 3 and 4 as fixes (and therefore we can avoid
having to backport them to stable kernels).

Will

Re: [PATCH 0/4] arm/efi: fix memblock reallocation crash due to persistent reservations

[Will Deacon]

Hi Ard,

On Tue, Nov 06, 2018 at 12:37:28PM +0100, 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.

I acked the arm64 part and patches 3 and 4 look good afaict, so:

Acked-by: Will Deacon 

for those as well.

The only thing I was wondering is whether or not exhausting the page-sized
array in the first list entry is rare enough that we could just realloc the
thing and copy instead of chaining together new pages. That said, without
seeing the code it's hard to tell whether you save much complexity with such
a scheme so I'll leave it up to you.

Will

Re: [PATCH 1/4] arm64: memblock: don't permit memblock resizing until linear mapping is up

[Will Deacon]

On Tue, Nov 06, 2018 at 12:37:29PM +0100, Ard Biesheuvel wrote:
> Bhupesh reports that having numerous memblock reservations at early
> boot may result in the following crash:
> 
>   Unable to handle kernel paging request at virtual address 80003ffe
>   ...
>   Call trace:
>__memcpy+0x110/0x180
>memblock_add_range+0x134/0x2e8
>memblock_reserve+0x70/0xb8
>memblock_alloc_base_nid+0x6c/0x88
>__memblock_alloc_base+0x3c/0x4c
>memblock_alloc_base+0x28/0x4c
>memblock_alloc+0x2c/0x38
>early_pgtable_alloc+0x20/0xb0
>paging_init+0x28/0x7f8
> 
> This is caused by the fact that we permit memblock resizing before the
> linear mapping is up, and so the memblock_reserved() array is moved
> into memory that is not mapped yet.
> 
> So let's ensure that this crash can no longer occur, by deferring to
> call to memblock_allow_resize() to after the linear mapping has been
> created.
> 
> Reported-by: Bhupesh Sharma 
> Signed-off-by: Ard Biesheuvel 
> ---
>  arch/arm64/mm/init.c | 2 --
>  arch/arm64/mm/mmu.c  | 2 ++
>  2 files changed, 2 insertions(+), 2 deletions(-)

Thanks for posting this so quickly.

Acked-by: Will Deacon 

Bhupesh -- please can you give this series a spin and confirm that it fixes
the problem you were seeing?

Thanks,

Will

Re: [Bug Report] kdump crashes after latest EFI memblock changes on arm64 machines with large number of CPUs

[Will Deacon]

On Fri, Nov 02, 2018 at 02:44:10AM +0530, Bhupesh Sharma wrote:
> 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.
> 
> I have a arm64 board which has 224 CPUS:
> # lscpu
> <..snip..>
> CPU(s):  224
> On-line CPU(s) list: 0-223
> <..snip..>
> 
> Here are the crash logs in the kdump kernel on this machine:
> 
> [0.00] Unable to handle kernel paging request at virtual
> address 80003ffe
> val)nt EL), IL ata abort info:
> [0.or: Oops: 96inted 4.18.0+ #3
> [0.00] pstate: 20400089 (nzCv daIf +PAN -UAO)
> [0.00] pc : __memcpy+0x110/0x180
> [0.00] lr : memblock_double_array+0x240/0x348
> [0.00] sp : 092efc80 x28: bffe
> [0.00] x27: 1800 x26: 09d59000
> [0.00] x25: 80003ffe x24: 
> [0.00] x23: 0001 x22: 09d594e8
> [0.00] x21: 09d594f4 x20: 093c7268
> [0.00] x19: 0c00 x18: 0010
> [0.00] x17:  x16: 
> [0.00] x15: 3: 000fc18d x12: 0008
> [0.00] x11: 0018 x10: ddab9e18
> [0.00] x9 : 0008 x8 : 02c1
> [0.00] x7 : 91b9 x6 : 80003ffe
> [0.00] x5 : 0001 x4 : 
> [0.00] x3 :  x2 : 0b80
> [0.00] x1 : 09d59540 x0 : 80003ffe
> [0.00] Process swapper)
> [0.00] Call trace:
> [0.00]  __memcpy+0x110/0x180
> [0.00]  memblock_add_range+0x134/0x2e8
> [0.00]  memblock_reserve+0x70/0xb8
> [0.00]  memblock_alloc_base_nid+0x6c/0x88
> [0.00]  __memblock_alloc_base+0x3c/0x4c
> [0.00]  memblock_alloc_base+0x28/0x4c
> [0.00]  memblock_alloc+0x2c/0x38
> [0.00]  early_pgtable_alloc+0x20/0xb0

Hmm, so this seems to be the crux of the issue: early_pgtable_alloc() relies
on memblock to allocate page-table memory, but this can be called before the
linear mapping is up and running (or even as part of creating the linear
mapping itself!) so the use of __va in memblock_double_array() actually
returns an unmapped address.

So I guess we either need to implement early_pgtable_alloc() some other way
(how?) or get memblock_double_array() to use a fixmap if it's called too
early (yuck). Alternatively, would it be possible to postpone processing of
the EFI mem_reserve entries until after we've created the linear mapping?

Will

Re: [PATCH 0/8] add generic builtin command line

[Will Deacon]

On Wed, Oct 24, 2018 at 10:07:32AM +0100, Russell King - ARM Linux wrote:
> On Wed, Oct 24, 2018 at 11:57:44AM +0300, Maksym Kokhan wrote:
> > Do you mean, that you haven't seen patch for ARM, which I sent on
> > September 27 along with cover and patch 1? It is strange, because
> > you was the one from recipients. If so, you can see this patch here:
> > https://lore.kernel.org/patchwork/patch/992779/
> 
> It seems that I have received patch 5, _but_ it's not threaded with
> the cover message and patch 1.  With 50k messages in my inbox, and 3k
> messages since you sent the series, it's virtually impossible to find
> it (I only found it by looking at my mail server logs from September
> to find the subject, and then searching my mailbox for that subject.)
> 
> This is unnecessarily difficult.

This comes up surprisingly often, and I think part of the issue is that
different maintainers have different preferences. I also prefer to receive
the entire series and cover-letter, but I've seen people object to being
CC'd on the whole series as well (how they manage to review things in
isolation is another question...!)

I wonder if we could have an entry in MAINTAINERS for this sort of
preference?

Maksym: in the short term, please just stick me and Russell on CC for the
entire thing.

Will

Re: [RFC PATCH 0/2] efi: add contents of LinuxExtraArgs EFI var to command line

[Will Deacon]

On Fri, Jul 13, 2018 at 08:15:26AM +0200, Ard Biesheuvel wrote:
> On 13 July 2018 at 00:22, Geoff Levand  wrote:
> > Hi Ard,
> >
> > On 07/04/2018 08:49 AM, Ard Biesheuvel wrote:
> >>   efi/libstub: taken contents of LinuxExtraArgs UEFI variable into
> >> account
> >
> > To me this seems an overly complicated interface to the kernel, and
> > still doesn't in itself solve the problem at hand -- make the
> > generic distro kernel with APEI support run on m400 systems.
> >
> > As for me, I'd prefer something like my original patch.  It fixes
> > that problem, it is simple and self contained, and is very clear
> > in what it does.  Also, there is a limited life.  When the time
> > comes an announcement is made to the mailing lists 'Linux-XYZ will
> > no longer support the m400 Moonshot'.  Then, when the Linux-XYZ-rc1
> > merge window opens a patch goes in that removes the
> > acpi_fixup_m400_quirks() routine.
> >
> 
> Actually, that is a very good point. One of the issues I have with
> these quirks is that the burden is on the maintainers to keep them
> around forever, unless they can prove that it is no longer needed.
> 
> This is not my call to make, but I would be much less averse to this
> being merged if we could agree upfront on an expiration time of, say,
> 2 years (or more?), after which it will be removed (unless anyone
> makes a very good case for why it needs to be retained). This should
> be mentioned in the kernel log as well when the quirk is triggered.

The problem with that is it all falls apart when somebody who wasn't
involved in the initial discussion crops up after we remove the quirk to
complain that their machine broke. In such a situation, we don't have a
leg to stand on in my opinion.

Will
--
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: [RFC PATCH 0/2] efi: add contents of LinuxExtraArgs EFI var to command line

[Will Deacon]

Hi Ard,

On Wed, Jul 04, 2018 at 05:49:17PM +0200, Ard Biesheuvel wrote:
> As noted by Ian, many DMI based quirks for x86 ACPI machines disable
> features that can also be disabled via the kernel command line. Similarly,
> a quirk is under discussion for a arm64 ACPI machine [0] that explodes
> when enabling support for hardware error reporting via the ACPI HEST table.
> 
> When support for DMI tables was introduced to arm64 and ARM, the agreement
> was that they are informational only, i.e., provided to userland to describe
> the platform, not for keying off of to enable quirks in the kernel.
> 
> There are a couple of reasons for this:
> - Unlike the x86 architecture, where virtually all platforms are PC variants,
>   and the presence of ACPI and DMI tables may be assumed, the arm64 
> architecture
>   is much more heterogeneous, and none of UEFI, ACPI or DMI or actually 
> mandated
>   or especially common across arm64 platforms; using DMI only makes sense for
>   working around a limited subset of platform issues that have to do with
>   firmware running on platforms that bother to implement it in the first 
> place.
> - DMI is not initialized as early as it is on x86, and doing so is not 
> trivial.
>   This means that even on ACPI/DMI machines, some issues may require quirks 
> that
>   are enabled in a different way, or we need to refactor the DMI support so 
> that
>   we can enable it as early as x86 does.
> - Using DMI tables fundamentally means quirking *after* the fact, which makes 
> it
>   a moving target. Some quirks may require the DMI match table to be updated 
> if
>   someone happens to change a string in the DMI tables when shipping a mostly
>   identical platform.
> 
> So instead, let's provide these platforms with the facilities required to 
> enable
> such quirks at the platform level. Especially for UEFI systems, if upgrading
> firmware is a reasonable prerequisite for being able to upgrade to the latest
> kernel, having to run a script that sets a UEFI variable (either via the Linux
> command line of from the UEFI shell) should not be an unreasonable requirement
> either, and so we can solve part of this issue by configuring extra command 
> line
> arguments persistenly from the firmware environment. This solves all the above
> issues in an unintrusive manner, given that the kernel command line is already
> made available extremely early in the boot, and the fact that it already 
> permits
> a wide range of configuration options and overrides to be set, including the
> 'hest_disable=1' option that works around the issue addressed by [0].

I'm torn on this one. Whilst I strongly agree that keying off DMI tables
to detect firmware quirks is a bad idea on arm64, silently extending the
kernel command-line also has its downsides. The command-line provides ways
to override kernel defaults, so if a user has forced a feature on/off,
then I think this should take precedence over quirks and we should taint
instead, rather than silently override the option.

I'd be interested in other opinions on this.

Will
--
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/4] efi/arm64: unmap the kernel while executing UEFI services

[Will Deacon]

On Fri, Jan 26, 2018 at 05:06:42PM +, Ard Biesheuvel wrote:
> On 26 January 2018 at 17:05, Will Deacon <will.dea...@arm.com> wrote:
> > On Thu, Jan 25, 2018 at 10:31:31AM +, Ard Biesheuvel wrote:
> >> Now that all UEFI runtime service wrappers ensure that byref
> >> arguments are moved into the UEFI marshalling buffer (which
> >> is not part of the kernel mapping), we can proceed and unmap
> >> the kernel while UEFI runtime service calls are in progress.
> >>
> >> This is done by setting the EPD1 bit and flushing the TLB of
> >> the local CPU. This makes it independent of KPTI or whether
> >> non-global mappings are being used.
> >
> > One snag with this is that it will break SPE, so I'd prefer this behaviour
> > to be predicated on kpti so that the arm64_kernel_unmapped_at_el0() check
> > in drivers/perf/arm_spe_pmu.c remains valid.
> >
> 
> The problem with that is that they serve two different purposes: kpti
> protects against meltdown, this protects against Spectre variant 1.

Fair enough, but we should do something because it renders SPE unusable
and it can be a really handy profiling feature. Having the new EFI behaviour
optional in some way would be my preference.

Will
--
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 2/4] efi/arm64: map the stack and entry wrapper into the UEFI page tables

[Will Deacon]

On Fri, Jan 26, 2018 at 05:03:29PM +, Ard Biesheuvel wrote:
> On 26 January 2018 at 16:57, Will Deacon <will.dea...@arm.com> wrote:
> > On Thu, Jan 25, 2018 at 10:31:29AM +, Ard Biesheuvel wrote:
> >> As a preparatory step towards unmapping the kernel entirely while
> >> executing UEFI runtime services, move the stack and the entry
> >> wrapper routine mappings into the EFI page tables. Also, create a
> >> vector table that overrides the main one while executing in the
> >> firmware so we will be able to remap/unmap the kernel while taking
> >> interrupts.
> >
> > [...]
> >
> >> + .macro  ventry
> >> + .align  7
> >> +.Lv\@ :  stp x29, x30, [sp, #-16]!   // preserve x29 and 
> >> x30
> >> + mrs x29, elr_el1// preserve ELR
> >> + adr x30, .Lret  // take return address
> >> + msr elr_el1, x30// set ELR to return address
> >
> > Oh man, are you really doing two ERETs for a single exception, or am I
> > missing something?
> >
> 
> Yes. I told you it was poetry.

We should organise a recital.

> >> + ldr x30, 2b // take address of 'vectors'
> >> + msr vbar_el1, x30   // set VBAR to 'vectors'
> >> + isb
> >> + add x30, x30, #.Lv\@ - __efi_rt_vectors
> >> + br  x30
> >> + .endm
> >> +
> >> +.Lret:   msr elr_el1, x29
> >
> > If you take an IRQ here, aren't you toast?
> >
> 
> Yep. So we need to switch this with setting the VBAR below then.

Hmm, but the ELR will still be clobbered by an IRQ, so I don't see how you
can make this safe unless you hack SPSR before entering the kernel vectors
on the entry side.

> >> +__efi_rt_vectors:
> >> + .rept   8
> >> + ventry
> >> + .endr
> >
> > Have you thought about SDEI at all? I can't see any code here to handle
> > that.
> >
> 
> Nope

Add JamesM to cc ;)

> >> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> >> index b34e717d7597..3bab6c60a12b 100644
> >> --- a/arch/arm64/kernel/entry.S
> >> +++ b/arch/arm64/kernel/entry.S
> >> @@ -204,6 +204,7 @@ alternative_if ARM64_HAS_PAN
> >>  alternative_else_nop_endif
> >>
> >>   .if \el != 0
> >> + tbz x21, #63, 1f// skip if TTBR0 covers the 
> >> stack
> >
> > So this is really a "detect EFI" check, right? Maybe comment it as such.
> > Also, probably want to check bit 55 just in case tagging ever takes off.
> >
> 
> Right. So just bit #55 should be sufficient then, right?

Yes, I think so.

Will
--
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/4] efi/arm64: unmap the kernel while executing UEFI services

[Will Deacon]

On Thu, Jan 25, 2018 at 10:31:31AM +, Ard Biesheuvel wrote:
> Now that all UEFI runtime service wrappers ensure that byref
> arguments are moved into the UEFI marshalling buffer (which
> is not part of the kernel mapping), we can proceed and unmap
> the kernel while UEFI runtime service calls are in progress.
> 
> This is done by setting the EPD1 bit and flushing the TLB of
> the local CPU. This makes it independent of KPTI or whether
> non-global mappings are being used.

One snag with this is that it will break SPE, so I'd prefer this behaviour
to be predicated on kpti so that the arm64_kernel_unmapped_at_el0() check
in drivers/perf/arm_spe_pmu.c remains valid.

Will
--
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 2/4] efi/arm64: map the stack and entry wrapper into the UEFI page tables

[Will Deacon]

Hi Ard,

On Thu, Jan 25, 2018 at 10:31:29AM +, Ard Biesheuvel wrote:
> As a preparatory step towards unmapping the kernel entirely while
> executing UEFI runtime services, move the stack and the entry
> wrapper routine mappings into the EFI page tables. Also, create a
> vector table that overrides the main one while executing in the
> firmware so we will be able to remap/unmap the kernel while taking
> interrupts.

[...]

> + .macro  ventry
> + .align  7
> +.Lv\@ :  stp x29, x30, [sp, #-16]!   // preserve x29 and x30
> + mrs x29, elr_el1// preserve ELR
> + adr x30, .Lret  // take return address
> + msr elr_el1, x30// set ELR to return address

Oh man, are you really doing two ERETs for a single exception, or am I
missing something?

> + ldr x30, 2b // take address of 'vectors'
> + msr vbar_el1, x30   // set VBAR to 'vectors'
> + isb
> + add x30, x30, #.Lv\@ - __efi_rt_vectors
> + br  x30
> + .endm
> +
> +.Lret:   msr elr_el1, x29

If you take an IRQ here, aren't you toast?

> + adr x30, __efi_rt_vectors
> + msr vbar_el1, x30
> + isb
> + ldp x29, x30, [sp], #16
> + eret
> +
> + .align  11
> +__efi_rt_vectors:
> + .rept   8
> + ventry
> + .endr

Have you thought about SDEI at all? I can't see any code here to handle
that.

> + /*
> +  * EFI runtime services never drop to EL0, so the
> +  * remaining vector table entries are not needed.
> +  */
> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> index af4f943cffac..68c920b2f4f0 100644
> --- a/arch/arm64/kernel/efi.c
> +++ b/arch/arm64/kernel/efi.c
> @@ -130,3 +130,27 @@ asmlinkage efi_status_t 
> efi_handle_corrupted_x18(efi_status_t s, const char *f)
>   pr_err_ratelimited(FW_BUG "register x18 corrupted by EFI %s\n", f);
>   return s;
>  }
> +
> +bool on_efi_stack(unsigned long sp)
> +{
> + return sp >= EFI_STACK_BASE && sp < (EFI_STACK_BASE + EFI_STACK_SIZE);
> +}
> +
> +int __init efi_allocate_runtime_regions(struct mm_struct *mm)
> +{
> + static u8 stack[EFI_STACK_SIZE] __page_aligned_bss;

Probably just me, but the use of a function static variable in a function
annotated with __init just makes me feel uneasy. Could we move it out into
wider scope?

> +
> + /* map the stack */
> + create_pgd_mapping(mm, __pa_symbol(stack),
> +EFI_STACK_BASE, EFI_STACK_SIZE,
> +__pgprot(pgprot_val(PAGE_KERNEL) | PTE_NG),
> +false);
> +
> + /* map the runtime wrapper pivot function */
> + create_pgd_mapping(mm, __pa_symbol(__efi_rt_asm_wrapper),
> +EFI_CODE_BASE, EFI_CODE_SIZE,
> +__pgprot(pgprot_val(PAGE_KERNEL_ROX) | PTE_NG),
> +false);
> +
> + return 0;
> +}
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index b34e717d7597..3bab6c60a12b 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -204,6 +204,7 @@ alternative_if ARM64_HAS_PAN
>  alternative_else_nop_endif
>  
>   .if \el != 0
> + tbz x21, #63, 1f// skip if TTBR0 covers the 
> stack

So this is really a "detect EFI" check, right? Maybe comment it as such.
Also, probably want to check bit 55 just in case tagging ever takes off.

Will
--
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 1/4] efi: arm64: Check whether x18 is preserved by runtime services calls

[Will Deacon]

On Thu, Jan 25, 2018 at 10:31:28AM +, Ard Biesheuvel wrote:
> Whether or not we will ever decide to start using x18 as a platform
> register in Linux is uncertain, but by that time, we will need to
> ensure that UEFI runtime services calls don't corrupt it. So let's
> start issuing warnings now for this, and increase the likelihood that
> these firmware images have all been replaced by that time.
> 
> This has been fixed on the EDK2 side in commit 6d73863b5464
> ("BaseTools/tools_def AARCH64: mark register x18 as reserved").,
> dated July 13, 2017.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
> ---
>  arch/arm64/include/asm/efi.h   |  4 +-
>  arch/arm64/kernel/Makefile |  3 +-
>  arch/arm64/kernel/efi-rt-wrapper.S | 41 
>  arch/arm64/kernel/efi.c|  6 +++
>  4 files changed, 52 insertions(+), 2 deletions(-)

Looks good to me:

Acked-by: Will Deacon <will.dea...@arm.com>

Will

> diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
> index 8389050328bb..192d791f1103 100644
> --- a/arch/arm64/include/asm/efi.h
> +++ b/arch/arm64/include/asm/efi.h
> @@ -31,7 +31,7 @@ int efi_set_mapping_permissions(struct mm_struct *mm, 
> efi_memory_desc_t *md);
>  ({   \
>   efi_##f##_t *__f;   \
>   __f = p->f; \
> - __f(args);  \
> + __efi_rt_asm_wrapper(__f, #f, args);\
>  })
>  
>  #define arch_efi_call_virt_teardown()
> \
> @@ -40,6 +40,8 @@ int efi_set_mapping_permissions(struct mm_struct *mm, 
> efi_memory_desc_t *md);
>   efi_virtmap_unload();   \
>  })
>  
> +efi_status_t __efi_rt_asm_wrapper(void *, const char *, ...);
> +
>  #define ARCH_EFI_IRQ_FLAGS_MASK (PSR_D_BIT | PSR_A_BIT | PSR_I_BIT | 
> PSR_F_BIT)
>  
>  /* arch specific definitions used by the stub code */
> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
> index b87541360f43..6a4bd80c75bd 100644
> --- a/arch/arm64/kernel/Makefile
> +++ b/arch/arm64/kernel/Makefile
> @@ -38,7 +38,8 @@ arm64-obj-$(CONFIG_CPU_PM)  += sleep.o suspend.o
>  arm64-obj-$(CONFIG_CPU_IDLE) += cpuidle.o
>  arm64-obj-$(CONFIG_JUMP_LABEL)   += jump_label.o
>  arm64-obj-$(CONFIG_KGDB) += kgdb.o
> -arm64-obj-$(CONFIG_EFI)  += efi.o efi-entry.stub.o
> +arm64-obj-$(CONFIG_EFI)  += efi.o efi-entry.stub.o   
> \
> +efi-rt-wrapper.o
>  arm64-obj-$(CONFIG_PCI)  += pci.o
>  arm64-obj-$(CONFIG_ARMV8_DEPRECATED) += armv8_deprecated.o
>  arm64-obj-$(CONFIG_ACPI) += acpi.o
> diff --git a/arch/arm64/kernel/efi-rt-wrapper.S 
> b/arch/arm64/kernel/efi-rt-wrapper.S
> new file mode 100644
> index ..05235ebb336d
> --- /dev/null
> +++ b/arch/arm64/kernel/efi-rt-wrapper.S
> @@ -0,0 +1,41 @@
> +/*
> + * Copyright (C) 2018 Linaro Ltd <ard.biesheu...@linaro.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include 
> +
> +ENTRY(__efi_rt_asm_wrapper)
> + stp x29, x30, [sp, #-32]!
> + mov x29, sp
> +
> + /*
> +  * Register x18 is designated as the 'platform' register by the AAPCS,
> +  * which means firmware running at the same exception level as the OS
> +  * (such as UEFI) should never touch it.
> +  */
> + stp x1, x18, [sp, #16]
> +
> + /*
> +  * We are lucky enough that no EFI runtime services take more than
> +  * 5 arguments, so all are passed in registers rather than via the
> +  * stack.
> +  */
> + mov x8, x0
> + mov x0, x2
> + mov x1, x3
> + mov x2, x4
> + mov x3, x5
> + mov x4, x6
> + blr x8
> +
> + ldp x1, x2, [sp, #16]
> + cmp x2, x18
> + ldp x29, x30, [sp], #32
> + b.ne0f
> + ret
> +0:   b   efi_handle_corrupted_x18// tail call
> +ENDPROC(__efi_rt_asm_wrapper)
> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> index 82cd07592519..af4f943cffac 100644
> --- a/arch/arm64/kernel/efi.c
> +++ b/arch/arm64/kernel/efi.c
> @@ -124,3 +124,9 @@ bool efi_poweroff_required(void)
>

Re: [PATCH] efi/arm*: Only register page tables when they exist

[Will Deacon]

On Tue, Jan 16, 2018 at 02:08:32PM +, Mark Rutland wrote:
> Currently the arm/arm64 runtime code registers the runtime servies
> pagetables with ptdump regardless of whether runtime services page
> tables have been created.
> 
> As efi_mm.pgd is NULL in these cases, attempting to dump the efi page
> tables results in a NULL pointer dereference in the ptdump code:

Acked-by: Will Deacon <will.dea...@arm.com>

Ard -- please can you pick this one up?

Cheers,

Will
--
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 v4 2/2] arm64: Add software workaround for Falkor erratum 1041

[Will Deacon]

On Mon, Nov 27, 2017 at 05:18:00PM -0600, Shanker Donthineni wrote:
> The ARM architecture defines the memory locations that are permitted
> to be accessed as the result of a speculative instruction fetch from
> an exception level for which all stages of translation are disabled.
> Specifically, the core is permitted to speculatively fetch from the
> 4KB region containing the current program counter 4K and next 4K.
> 
> When translation is changed from enabled to disabled for the running
> exception level (SCTLR_ELn[M] changed from a value of 1 to 0), the
> Falkor core may errantly speculatively access memory locations outside
> of the 4KB region permitted by the architecture. The errant memory
> access may lead to one of the following unexpected behaviors.
> 
> 1) A System Error Interrupt (SEI) being raised by the Falkor core due
>to the errant memory access attempting to access a region of memory
>that is protected by a slave-side memory protection unit.
> 2) Unpredictable device behavior due to a speculative read from device
>memory. This behavior may only occur if the instruction cache is
>disabled prior to or coincident with translation being changed from
>enabled to disabled.
> 
> The conditions leading to this erratum will not occur when either of the
> following occur:
>  1) A higher exception level disables translation of a lower exception level
>(e.g. EL2 changing SCTLR_EL1[M] from a value of 1 to 0).
>  2) An exception level disabling its stage-1 translation if its stage-2
> translation is enabled (e.g. EL1 changing SCTLR_EL1[M] from a value of 1
> to 0 when HCR_EL2[VM] has a value of 1).
> 
> To avoid the errant behavior, software must execute an ISB immediately
> prior to executing the MSR that will change SCTLR_ELn[M] from 1 to 0.
> 
> Signed-off-by: Shanker Donthineni 
> ---
> Changes since v3:
>   Rebased to kernel v4.15-rc1.
> Changes since v2:
>   Repost the corrected patches.
> Changes since v1:
>   Apply the workaround where it's required.
> 
>  Documentation/arm64/silicon-errata.txt |  1 +
>  arch/arm64/Kconfig | 12 +++-
>  arch/arm64/include/asm/assembler.h | 19 +++
>  arch/arm64/include/asm/cpucaps.h   |  3 ++-
>  arch/arm64/kernel/cpu-reset.S  |  1 +
>  arch/arm64/kernel/cpu_errata.c | 16 
>  arch/arm64/kernel/efi-entry.S  |  2 ++
>  arch/arm64/kernel/head.S   |  1 +
>  arch/arm64/kernel/relocate_kernel.S|  1 +
>  arch/arm64/kvm/hyp-init.S  |  1 +

This is an awful lot of code just to add an ISB instruction prior to
disabling the MMU. Why do you need to go through the alternatives framework
for this? Just do it with an #ifdef; this isn't a fastpath.

Will
--
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/libstub: arm: omit sorting of the UEFI memory map

[Will Deacon]

On Tue, Oct 24, 2017 at 12:09:59PM +0100, Ard Biesheuvel wrote:
> On 24 October 2017 at 12:05, Russell King - ARM Linux
> <li...@armlinux.org.uk> wrote:
> > On Sun, Oct 22, 2017 at 03:14:57PM +0100, Ard Biesheuvel wrote:
> >> ARM shares its EFI stub implementation with arm64, which has some
> >> special handling in the virtual remapping code to
> >> a) make sure that we can map everything even if the OS executes
> >>with 64k page size, and
> >> b) make sure that adjacent regions with the same attributes are not
> >>reordered or moved apart in memory.
> >>
> >> The latter is a workaround for a 'feature' that was shortly recommended
> >> by UEFI spec v2.5, but deprecated shortly after, due to the fact that
> >> it broke many OS installers, including non-Linux ones, and it was never
> >> widely implemented for ARM systems. Before implementing b), the arm64
> >> code simply rounded up all regions to 64 KB granularity, but given that
> >> that results in moving adjacent regions apart, it had to be refined when
> >> b) was implemented.
> >>
> >> The adjacency check requires a sort() pass, due to the fact that the
> >> UEFI spec does not mandate any ordering, and the inclusion of the
> >> lib/sort.c code into the ARM EFI stub is causing some trouble with
> >> the decompressor build due to the fact that its EXPORT_SYMBOL() call
> >> triggers the creation of ksymtab/kcrctab sections.
> >>
> >> So let's simply do away with the adjacency check for ARM, and simply put
> >> all UEFI runtime regions together if they have the same memory attributes.
> >> This is guaranteed to work, given that ARM only supports 4 KB pages,
> >> and allows us to remove the sort() call entirely.
> >>
> >> Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
> >
> > I guess as this is shared between ARM and ARM64, ARM64 folk should ack
> > this before I merge it - can I have an ack from that side please?
> >
> 
> Note that the patch does not touch arch/arm64, nor does it affect
> arm64, given that the change is a no-op if CONFIG_ARM64=y. That said,
> I welcome any acks from the arm64 maintainers, but I don't think they
> are actually required here.

Looks fine to me:

Acked-by: Will Deacon <will.dea...@arm.com>

Thanks,

Will
--
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/3] x86/efi: Use efi_switch_mm() rather than manually twiddling with cr3

[Will Deacon]

On Wed, Aug 16, 2017 at 10:53:38AM +0100, Mark Rutland wrote:
> On Wed, Aug 16, 2017 at 10:31:12AM +0100, Ard Biesheuvel wrote:
> > (+ Mark, Will)
> > 
> > On 15 August 2017 at 22:46, Andy Lutomirski  wrote:
> > > On Tue, Aug 15, 2017 at 12:18 PM, Sai Praneeth Prakhya
> > >  wrote:
> > >> +/*
> > >> + * Makes the calling kernel thread switch to/from efi_mm context
> > >> + * Can be used from SetVirtualAddressMap() or during efi runtime calls
> > >> + * (Note: This routine is heavily inspired from use_mm)
> > >> + */
> > >> +void efi_switch_mm(struct mm_struct *mm)
> > >> +{
> > >> +   struct task_struct *tsk = current;
> > >> +
> > >> +   task_lock(tsk);
> > >> +   efi_scratch.prev_mm = tsk->active_mm;
> > >> +   if (efi_scratch.prev_mm != mm) {
> > >> +   mmgrab(mm);
> > >> +   tsk->active_mm = mm;
> > >> +   }
> > >> +   switch_mm(efi_scratch.prev_mm, mm, NULL);
> > >> +   task_unlock(tsk);
> > >> +
> > >> +   if (efi_scratch.prev_mm != mm)
> > >> +   mmdrop(efi_scratch.prev_mm);
> > >
> > > I'm confused.  You're mmdropping an mm that you are still keeping a
> > > pointer to.  This is also a bit confusing in the case where you do
> > > efi_switch_mm(efi_scratch.prev_mm).
> > >
> > > This whole manipulation seems fairly dangerous to me for another
> > > reason -- you're taking a user thread (I think) and swapping out its
> > > mm to something that the user in question should *not* have access to.
> > > What if a perf interrupt happens while you're in the alternate mm?
> > > What if you segfault and dump core?  Should we maybe just have a flag
> > > that says "this cpu is using a funny mm", assert that the flag is
> > > clear when scheduling, and teach perf, coredumps, etc not to touch
> > > user memory when the flag is set?
> > 
> > It appears we may have introduced this exact issue on arm64 and ARM by
> > starting to run the UEFI runtime services with interrupts enabled.
> > (perf does not use NMI on ARM, so the issue did not exist beforehand)
> > 
> > Mark, Will, any thoughts?
> 
> Yup, I can cause perf to take samples from the EFI FW code, so that's
> less than ideal.

But that should only happen if you're profiling EL1, right, which needs
root privileges? (assuming the skid issue is solved -- not sure what
happened to those patches after they broke criu).

> The "funny mm" flag sounds like a good idea to me, though given recent
> pain with sampling in the case of skid, I don't know exactly what we
> should do if/when we take an overflow interrupt while in EFI.

I don't think special-casing perf interrupts is the right thing to do here.
If we're concerned about user-accesses being made off the back of interrupts
taken whilst in EFI, then we should probably either swizzle back in the
user page table on the IRQ path or postpone handling it until we're done
with the firmware. Having a flag feels a bit weird: would the uaccess
routines return -EFAULT if it's set?

Will
--
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 V17 00/11] Add UEFI 2.6 and ACPI 6.1 updates for RAS on ARM64

[Will Deacon]

Hi Robert,

On Tue, Jun 20, 2017 at 08:34:39AM +0200, Robert Richter wrote:
> On 07.06.17 12:50:12, Will Deacon wrote:
> 
> > Thanks, I've pushed this out as:
> >
> >   git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git 
> > for-next/ras-apei
> >
> > which I'll merge into for-next/core (and therefore linux-next) either the
> > end of this week or the beginning of next week. Please take a look if you
> > get a chance.
> 
> any reason why there was a roll back of for-next/core?
> 
>  + 0870f692c2ed...e27c7fa015d6 for-next/core -> arm64/for-next/core  (forced 
> update)
> 
> Is it because for-next/ras-apei goes through tip?

No, it's because the RAS stuff ran into horrible conflicts with the UUID
tree:

https://lkml.org/lkml/2017/6/16/22

Tyler should be rebasing that soon, so hopefully I can requeue that stuff
this week.

Will
--
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 V17 00/11] Add UEFI 2.6 and ACPI 6.1 updates for RAS on ARM64

[Will Deacon]

On Fri, May 19, 2017 at 02:32:02PM -0600, Tyler Baicar wrote:
> When a memory error, CPU error, PCIe error, or other type of hardware error
> that's covered by RAS occurs, firmware should populate the shared GHES memory
> location with the proper GHES structures to notify the OS of the error.
> For example, platforms that implement firmware first handling may implement
> separate GHES sources for corrected errors and uncorrected errors. If the
> error is an uncorrectable error, then the firmware will notify the OS
> immediately since the error needs to be handled ASAP. The OS will then be able
> to take the appropriate action needed such as offlining a page. If the error
> is a corrected error, then the firmware will not interrupt the OS immediately.
> Instead, the OS will see and report the error the next time it's GHES timer
> expires. The kernel will first parse the GHES structures and report the errors
> through the kernel logs and then notify the user space through RAS trace
> events. This allows user space applications such as RAS Daemon to see the
> errors and report them however the user desires. This patchset extends the
> kernel functionality for RAS errors based on updates in the UEFI 2.6 and
> ACPI 6.1 specifications.

Thanks, I've pushed this out as:

  git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git 
for-next/ras-apei

which I'll merge into for-next/core (and therefore linux-next) either the
end of this week or the beginning of next week. Please take a look if you
get a chance.

Will
--
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 V17 02/11] ras: acpi/apei: cper: add support for generic data v3 structure

[Will Deacon]

On Fri, May 19, 2017 at 02:32:04PM -0600, Tyler Baicar wrote:
> The ACPI 6.1 spec adds a new revision of the generic error data
> entry structure. Add support to handle the new structure as well
> as properly verify and iterate through the generic data entries.
> 
> Signed-off-by: Tyler Baicar 
> CC: Jonathan (Zhixiong) Zhang 
> ---
>  drivers/acpi/apei/ghes.c| 11 +--
>  drivers/firmware/efi/cper.c | 37 ++---
>  include/acpi/ghes.h | 36 
>  3 files changed, 63 insertions(+), 21 deletions(-)

Given that Boris and Rafael are ok with this series, it makes sense to
take this via arm64, but I need an ack from Ard or Matt on the EFI changes
in this patch and the subsequent one.

Will
--
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: stub: define DISABLE_BRANCH_PROFILING for all architectures

[Will Deacon]

On Tue, Jan 05, 2016 at 03:39:18PM +, Matt Fleming wrote:
> On Tue, 05 Jan, at 03:56:39PM, Ard Biesheuvel wrote:
> > (+ Arnd)
> > 
> > On 4 January 2016 at 13:31, Will Deacon <will.dea...@arm.com> wrote:
> > > On Wed, Dec 23, 2015 at 10:29:28AM +0100, Ard Biesheuvel wrote:
> > >> This moves the DISABLE_BRANCH_PROFILING define from the x86 specific
> > >> to the general CFLAGS definition for the stub. This fixes build errors
> > >> when building for arm64 with CONFIG_PROFILE_ALL_BRANCHES_ENABLED.
> > >>
> > >> Reported-by: Will Deacon <will.dea...@arm.com>
> > >> Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
> > >> ---
> > >>  drivers/firmware/efi/libstub/Makefile | 4 ++--
> > >>  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > Cheers, Ard. The kernel now builds and boots as an EFI application for
> > > me when the dreaded #define if config option is enabled:
> > >
> > >   Tested-by: Will Deacon <will.dea...@arm.com>
> > >
> > > Will
> > >
> > 
> > @Matt: this is causing randconfig build errors for ARM in -next. Mind
> > if we take this through some other tree?
> 
> Go ahead.
> 
> Reviewed-by: Matt Fleming <m...@codeblueprint.co.uk>

I can take this via the arm64 tree, unless Arnd wants to queue it in
arm-soc. Arnd?

Will
--
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: stub: define DISABLE_BRANCH_PROFILING for all architectures

[Will Deacon]

On Wed, Dec 23, 2015 at 10:29:28AM +0100, Ard Biesheuvel wrote:
> This moves the DISABLE_BRANCH_PROFILING define from the x86 specific
> to the general CFLAGS definition for the stub. This fixes build errors
> when building for arm64 with CONFIG_PROFILE_ALL_BRANCHES_ENABLED.
> 
> Reported-by: Will Deacon <will.dea...@arm.com>
> Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
> ---
>  drivers/firmware/efi/libstub/Makefile | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Cheers, Ard. The kernel now builds and boots as an EFI application for
me when the dreaded #define if config option is enabled:

  Tested-by: Will Deacon <will.dea...@arm.com>

Will

> diff --git a/drivers/firmware/efi/libstub/Makefile 
> b/drivers/firmware/efi/libstub/Makefile
> index 3c0467d3688c..c0ddd1b8dca3 100644
> --- a/drivers/firmware/efi/libstub/Makefile
> +++ b/drivers/firmware/efi/libstub/Makefile
> @@ -8,7 +8,7 @@ cflags-$(CONFIG_X86_32)   := -march=i386
>  cflags-$(CONFIG_X86_64)  := -mcmodel=small
>  cflags-$(CONFIG_X86) += -m$(BITS) -D__KERNEL__ $(LINUX_INCLUDE) -O2 \
>  -fPIC -fno-strict-aliasing -mno-red-zone \
> --mno-mmx -mno-sse -DDISABLE_BRANCH_PROFILING
> +-mno-mmx -mno-sse
>  
>  cflags-$(CONFIG_ARM64)   := $(subst -pg,,$(KBUILD_CFLAGS))
>  cflags-$(CONFIG_ARM) := $(subst -pg,,$(KBUILD_CFLAGS)) \
> @@ -16,7 +16,7 @@ cflags-$(CONFIG_ARM):= $(subst 
> -pg,,$(KBUILD_CFLAGS)) \
>  
>  cflags-$(CONFIG_EFI_ARMSTUB) += -I$(srctree)/scripts/dtc/libfdt
>  
> -KBUILD_CFLAGS:= $(cflags-y) \
> +KBUILD_CFLAGS:= $(cflags-y) 
> -DDISABLE_BRANCH_PROFILING \
>  $(call cc-option,-ffreestanding) \
>  $(call cc-option,-fno-stack-protector)
>  
> -- 
> 2.5.0
> 
--
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 v4 01/13] mm/memblock: add MEMBLOCK_NOMAP attribute to memblock memory table

[Will Deacon]

On Tue, Dec 08, 2015 at 02:23:41PM -0800, Andrew Morton wrote:
> On Tue, 8 Dec 2015 12:07:44 + Will Deacon <will.dea...@arm.com> wrote:
> 
> > > I should note that this change should not affect any memblock users
> > > that never set the MEMBLOCK_NOMAP flag, but please, if you see any
> > > issues beyond 'this may conflict with other stuff we have queued for
> > > 4.5', please do let me know.
> > 
> > Indeed, I can't see that this would cause any issues, but I would really
> > like an Ack from one of the MM maintainers before taking this.
> > 
> > Please could somebody take a look?
> 
> It looks OK to me.  Please go ahead and merge it via an arm tree.

Will do, thanks Andrew.

Will
--
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 v4 01/13] mm/memblock: add MEMBLOCK_NOMAP attribute to memblock memory table

[Will Deacon]

Hi Ard,

On Thu, Dec 03, 2015 at 11:55:53AM +0100, Ard Biesheuvel wrote:
> On 30 November 2015 at 13:28, Ard Biesheuvel  
> wrote:
> > This introduces the MEMBLOCK_NOMAP attribute and the required plumbing
> > to make it usable as an indicator that some parts of normal memory
> > should not be covered by the kernel direct mapping. It is up to the
> > arch to actually honor the attribute when laying out this mapping,
> > but the memblock code itself is modified to disregard these regions
> > for allocations and other general use.
> >
> > Cc: linux...@kvack.org
> > Cc: Alexander Kuleshov 
> > Cc: Andrew Morton 
> > Reviewed-by: Matt Fleming 
> > Signed-off-by: Ard Biesheuvel 
> > ---
> >  include/linux/memblock.h |  8 ++
> >  mm/memblock.c| 28 
> >  2 files changed, 36 insertions(+)

[...]

> May I kindly ask team-mm/Andrew/Alexander to chime in here, and
> indicate whether you are ok with this patch going in for 4.5? If so,
> could you please provide your ack so the patch can be kept together
> with the rest of the series, which depends on it?

I'm keen to queue this in the arm64 tree, since it's a prerequisite for
cleaning up a bunch of our EFI code and sharing it with 32-bit ARM.

> I should note that this change should not affect any memblock users
> that never set the MEMBLOCK_NOMAP flag, but please, if you see any
> issues beyond 'this may conflict with other stuff we have queued for
> 4.5', please do let me know.

Indeed, I can't see that this would cause any issues, but I would really
like an Ack from one of the MM maintainers before taking this.

Please could somebody take a look?

Will
--
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 v4 00/13] UEFI boot and runtime services support for 32-bit ARM

[Will Deacon]

Hi Ard,

On Mon, Nov 30, 2015 at 01:28:14PM +0100, Ard Biesheuvel wrote:
> This series adds support for booting the 32-bit ARM kernel directly from
> UEFI firmware using a builtin UEFI stub. It mostly reuses refactored arm64
> code, and the differences (primarily the PE/COFF header and entry point and
> the efi_create_mapping() implementation) are split out into arm64 and ARM
> versions.
> 
> Since I did not receive any further comments in reply to v3 from the people 
> who
> commented on v2, I think this series in now in sufficient shape to be pulled.
> Note that patch #1 touches mm/memblock.c and include/linux/memblock.h, for 
> which
> get_maintainer.pl does not provide a maintainer, so it has been cc'ed to 
> various
> past editors of those files, and to the linux-mm mailing list.
> 
> Since the series affects both arm64 and ARM, it is up to the maintainers to 
> let
> me know how and when they wish to proceed with this. My suggestion would be to
> send out pull request for patches #1 - #5 to the arm64 maintainer, and for the
> whole series to the ARM maintainer. This should keep any conflicts on either
> side confined to the respective maintainer tree, rather then propagating all 
> the
> way to -next.

For the arm64 bits (patches 2-5):

  Acked-by: Will Deacon <will.dea...@arm.com>

I'd really like an ack from the mm crowd on patch 1 before I queue it.

Will
--
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 v3 0/6] arm64 UEFI early FDT handling

[Will Deacon]

Hi Ard,

On Tue, Sep 29, 2015 at 08:38:57AM +0100, Ard Biesheuvel wrote:
> (+ Grant)
> 
> On 22 September 2015 at 02:21, Ard Biesheuvel  
> wrote:
> > This is a followup to the "arm64: update/clarify/relax Image and FDT 
> > placement
> > rules" series I sent a while ago:
> > (http://article.gmane.org/gmane.linux.ports.arm.kernel/407148)
> >
> > This has now been split in two series: this first series deals with the
> > early FDT handling, primarily in the context of UEFI, but not exclusively.
> >
> > A number of minor issues exist in the early UEFI/FDT handling path, such as:
> > - when booting via UEFI, memreserve entries are removed from the device 
> > tree but
> >   the /reserved-memory node is not
> 
> After reading 
> Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> again, I think simply ignoring the reserved-memory node is not the way
> to go. The reason is that it may contain dynamic allocations that are
> referenced by other nodes in the DT, and there is no good technical
> reason IMO to disallow those. OTOH, static allocations may conflict
> with the UEFI memory map, so those need to be dropped or at least
> checked against the memory map. The problem here is that static nodes
> may also be referenced by phandle, so we need to handle the referring
> node in some way as well.
> 
> So I think we have a number of options:
> - ignore /memreserve/s and reject static allocations in
> /reserved-memory (*) but honor dynamic ones
> - ignore /memreserve/s and honor all of /reserved-memory after
> checking that static allocations don't conflict
> - honor all /memreserve/s and /reserved-memory nodes and check all for 
> conflicts
> - ...
> 
> (*) static allocations for regions that the UEFI memory map does not
> describe should be OK, though
> 
> I personally prefer the first one, since a dynamic allocation
> implicitly conveys that the region does not contain anything special
> when coming out of boot, and there is very little we need to do other
> than perform the actual reservation. Static allocations are ambiguous
> in the sense that there is no annotation that explains the choice of
> address.
> 
> Thoughts, please?

What's the status of this series? It was on my "list of patches to watch"
that I'm just refreshing for 4.5, but I can't see any comments on-list
about it.

Cheers,

Will
--
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] arm64: efi: make sure vmlinux load address aligned on 2MBytes

[Will Deacon]

On Wed, Oct 28, 2015 at 12:11:57PM -0500, Timur Tabi wrote:
> On 10/27/2015 09:06 PM, Ard Biesheuvel wrote:
> >diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c
> >index 816120ece6bc..a60ce249cfc0 100644
> >--- a/arch/arm64/kernel/efi-stub.c
> >+++ b/arch/arm64/kernel/efi-stub.c
> >@@ -42,7 +42,8 @@
> >  * Mustang), we can still place the kernel at the address
> >  * 'dram_base + TEXT_OFFSET'.
> >  */
> >-   *image_addr = *reserve_addr = dram_base + TEXT_OFFSET;
> >+   *image_addr = *reserve_addr = round_up(dram_base, SZ_2M) +
> >+  TEXT_OFFSET);
> > nr_pages = round_up(kernel_memsize, EFI_ALLOC_ALIGN) /
> >EFI_PAGE_SIZE;
> > status = efi_call_early(allocate_pages, 
> > EFI_ALLOCATE_ADDRESS,
> 
> Tested-by: Timur Tabi 
> Tested-by: Shanker Donthineni 
> 
> However, I think this formatting is easier to read:
> 
>   *image_addr = *reserve_addr =
>   round_up(dram_base, SZ_2M) + TEXT_OFFSET;
> 
> This does make the kernel boot, but we suspect that there may be another
> problem.  We need to investigate it, but we have a suspicion that the EFI
> stub is trying to allocate from the Runtime Data block, and the alignment
> adjustment "fixes" the problem by moving the pointer to Conventional Memory.
> 
> Anyway, is there any chance we can get this fix into 4.3?  I'd hate to have
> 4.3 released knowing that it's broken on our hardware.

If you want something in for 4.3, you'll need to post a new patch in a
separate thread and I'd like to see at least Ard's ack on it.

Will
--
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 v4 3/3] arm64/efi: isolate EFI stub from the kernel proper

[Will Deacon]

On Fri, Oct 09, 2015 at 12:40:21PM +0300, Andrey Ryabinin wrote:
> 2015-10-09 12:10 GMT+03:00 Will Deacon <will.dea...@arm.com>:
> > On Fri, Oct 09, 2015 at 11:12:24AM +0300, Andrey Ryabinin wrote:
> >> 2015-10-08 22:02 GMT+03:00 Ard Biesheuvel <ard.biesheu...@linaro.org>:
> >> > --- a/arch/arm64/kernel/image.h
> >> > +++ b/arch/arm64/kernel/image.h
> >> > @@ -59,4 +59,31 @@
> >> > _kernel_offset_le   = DATA_LE64(TEXT_OFFSET);   \
> >> > _kernel_flags_le= DATA_LE64(__HEAD_FLAGS);
> >> >
> >> > +#ifdef CONFIG_EFI
> >> > +
> >> > +/*
> >> > + * The EFI stub has its own symbol namespace prefixed by __efistub_, to
> >> > + * isolate it from the kernel proper. The following symbols are legally
> >> > + * accessed by the stub, so provide some aliases to make them 
> >> > accessible.
> >> > + * Only include data symbols here, or text symbols of functions that are
> >> > + * guaranteed to be safe when executed at another offset than they were
> >> > + * linked at. The routines below are all implemented in assembler in a
> >> > + * position independent manner
> >> > + */
> >> > +__efistub_memcmp   = __pi_memcmp;
> >> > +__efistub_memchr   = __pi_memchr;
> >> > +__efistub_memcpy   = __pi_memcpy;
> >> > +__efistub_memmove  = __pi_memmove;
> >> > +__efistub_memset   = __pi_memset;
> >> > +__efistub_strlen   = __pi_strlen;
> >> > +__efistub_strcmp   = __pi_strcmp;
> >> > +__efistub_strncmp  = __pi_strncmp;
> >> > +__efistub___flush_dcache_area  = __pi___flush_dcache_area;
> >>
> >> So why we need these __pi_* aliases?
> >> We could just do __efistub_memcmp = memcmp; Right?
> >
> > We *could*, but that defeats the whole purpose of tagging
> > position-independent functions explicitly in the kernel text.
> >
> 
> I just don't get that "the whole purpose of tagging".
> 
> Yes, the EFI stub is allowed to call only PI kernel functions.
> But the EFI stub already protected by __efistub_ namespace.
> If we want to use some new PI function in the stub, we would need add
> it into this list at first.
> So that __pi_ namespace doesn't bring any protection or isolation.

What it does it force people making future changes to the __pi_* functions
to think about the stub, otherwise if the function happens to be
position-independent when efistub starts using it, it could easily break
due to some future patch where the author didn't realise that it was
being used in that manner.

Will
--
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: [RFC PATCH] arm64/efi: isolate EFI stub from the kernel proper

[Will Deacon]

Hi Ard,

On Thu, Sep 17, 2015 at 11:51:07AM +0100, Ard Biesheuvel wrote:
> On 15 September 2015 at 17:24, Ard Biesheuvel <ard.biesheu...@linaro.org> 
> wrote:
> > On 15 September 2015 at 16:46, Will Deacon <will.dea...@arm.com> wrote:
> >> On Tue, Sep 15, 2015 at 11:11:43AM +0100, Ard Biesheuvel wrote:
> >>> Since arm64 does not use a builtin decompressor, the EFI stub is built
> >>> into the kernel proper. So far, this has been working fine, but actually,
> >>> since the stub is in fact a PE/COFF relocatable binary that is executed
> >>> at an unknown offset in the 1:1 mapping provided by the UEFI firmware, we
> >>> should not be seamlessly sharing code with the kernel proper, which is a
> >>> position dependent executable linked at a high virtual offset.
> >>>
> >>> So instead, separate the contents of libstub and its dependencies, by
> >>> putting them into their own namespace by prefixing all of its symbols
> >>> with __efistub. This way, we have tight control over what parts of the
> >>> kernel proper are referenced by the stub.
> >>
> >> Could we add an __efistub annotation to spit out warnings if the stub
> >> calls into unexpected kernel code, like we do for __init/__ref?
> >>
> >
> > Currently, it will break the build rather than warn if you use a
> > disallowed symbol, which I think is not unreasonable.
> >
> > But I suppose that the objcopy step in this patch could rename the
> > sections to .efistub.xxx sections, which would allow us to reuse some
> > of the section mismatch code.
> > However, this would involve marking things like the generic string and
> > memcpy routines __efistub as well, which I think may be too much.
> > Also, note that the logic is inverted here: with __init, we disallow
> > normal code to call __init functions, but with __efistub, it will be
> > the other way around, which may be more difficult to accomplish
> > (Rutland and I did discuss this option when we talked about this over
> > IRC)
> >
> 
> OK, I have given this a go, and as it turns out, it implies that we go
> and mark generic pieces of lib/ as __section(.text.efistubok) in order
> for modpost.c to accept it. Tweaking modpost.c itself seems quite
> doable, since the logic is fairly flexible and can easily be augmented
> to complain about unauthorized references from the stub to the kernel
> proper.
> 
> So what we could do is fold libfdt and lib/sort.c (which are the
> primary generic dependencies) into the stub, but we would still need
> to retain the symbol prefixing bit to prevent the stub's symbols from
> clashing with the ones from the kernel proper. And with the symbol
> prefixing in place, we have something that is even stronger than
> section mismatch, which is to error out on undefined references rather
> than warn about section mismatches.
> 
> I think the current approach is better, but only if we agree that we
> should do something in the first place. (Currently, there are no known
> issues, just the awareness that things are not quite as tidy as they
> should be)

I agree, but thanks for looking into it. The only downside I still see
with symbol namespacing is that the error produced won't be as obvious
as something akin to a section mismatch, but then again, it's mainly you
hacking on the stub so it's not really an issue :)

Will
--
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] arm64/efi: base UEFI mapping permissions on region attributes

[Will Deacon]

Hi Ard,

On Wed, Aug 26, 2015 at 02:30:02PM +0100, Ard Biesheuvel wrote:
 Currently, we infer the UEFI memory region mapping permissions
 from the memory region type (i.e., runtime services code are
 mapped RWX and runtime services data mapped RW-). This appears to
 work fine but is not entirely UEFI spec compliant. So instead, use
 the designated permission attributes to decide how these regions
 should be mapped.
 
 Since UEFIv2.5 introduces a new EFI_MEMORY_RO permission attribute,
 and redefines EFI_MEMORY_WP as a cacheability attribute, use only
 the former as a read-only attribute. For setting the PXN bit, the
 corresponding EFI_MEMORY_XP attribute is used.
 
 Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org
 ---
 Changes since v1:
 - rewrote page size and alignment check to be more legible
 - use code that is STRICT_MM_TYPECHECKS compliant
 
 Example output of a recent Tianocore build on FVP Foundation model
 is attached below.
 
  arch/arm64/kernel/efi.c | 37 +---
  1 file changed, 24 insertions(+), 13 deletions(-)
 
 diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
 index ab21e0d58278..c8d587f46f3e 100644
 --- a/arch/arm64/kernel/efi.c
 +++ b/arch/arm64/kernel/efi.c
 @@ -235,7 +235,7 @@ static bool __init efi_virtmap_init(void)
  
   for_each_efi_memory_desc(memmap, md) {
   u64 paddr, npages, size;
 - pgprot_t prot;
 + pteval_t prot_val;
  
   if (!(md-attribute  EFI_MEMORY_RUNTIME))
   continue;
 @@ -247,22 +247,33 @@ static bool __init efi_virtmap_init(void)
   memrange_efi_to_native(paddr, npages);
   size = npages  PAGE_SHIFT;
  
 - pr_info(  EFI remap 0x%016llx = %p\n,
 - md-phys_addr, (void *)md-virt_addr);
 + if (!is_normal_ram(md))
 + prot_val = PROT_DEVICE_nGnRE;
 + else
 + prot_val = pgprot_val(PAGE_KERNEL_EXEC);
  
   /*
 -  * Only regions of type EFI_RUNTIME_SERVICES_CODE need to be
 -  * executable, everything else can be mapped with the XN bits
 -  * set.
 +  * On 64 KB granule kernels, only use strict permissions when
 +  * the region does not share a 64 KB page frame with another
 +  * region at either end.
*/
 - if (!is_normal_ram(md))
 - prot = __pgprot(PROT_DEVICE_nGnRE);
 - else if (md-type == EFI_RUNTIME_SERVICES_CODE)
 - prot = PAGE_KERNEL_EXEC;
 - else
 - prot = PAGE_KERNEL;
 + if (PAGE_SIZE == EFI_PAGE_SIZE ||
 + (PAGE_ALIGNED(md-virt_addr) 
 +  PAGE_ALIGNED(md-phys_addr + md-num_pages * 
 EFI_PAGE_SIZE))) {

Why do you use virt_addr instead of phys_addr for the base check?

Will
--
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 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory

[Will Deacon]

On Thu, Aug 13, 2015 at 10:24:32AM +0100, Matt Fleming wrote:
 On Thu, 13 Aug, at 10:19:17AM, Ingo Molnar wrote:
  * Matt Fleming m...@codeblueprint.co.uk wrote:
  
   From: Jonathan (Zhixiong) Zhang zjzh...@codeaurora.org
   
   With ACPI APEI firmware first handling, generic hardware error
   record is updated by firmware in GHES memory region. On an arm64
   platform, firmware updates GHES memory region with uncached
   access attribute, and then Linux reads stale data from cache.
  
  So this paragraph does not parse for me ...
  
  If it tries to explain a bug it falls very short of doing a proper job of 
  that.
 
 I'll let Jonathan provide more details but I understood the problem to
 be a cache (in)coherency issue.
 
 The kernel currently maps the the GHES memory region as cacheable
 (PAGE_KERNEL) for all architectures. This memory region is used as a
 communication buffer for reporting hardware errors from the firmware to
 kernel. Essentially the firmware writes hardware error records there,
 trigger an NMI/interrupt, and the GHES driver goes off and grabs the
 error record from the GHES region.
 
 Since the firmware gets first crack at inspecting the error this
 mechanism is referred to as firmware first in the ACPI spec.
 
 Now, there's a mismatch on arm64 platforms between how the kernel maps
 the GHES region (PAGE_KERNEL) and how the firmware maps it
 (EFI_MEMORY_UC, i.e. uncacheable), leading to the possibility of the
 kernel GHES driver reading stale data from the cache when it receives
 the NMI/interrupt.
 
 As for exactly why the arm64 firmware uses an uncached mapping, I
 presume it's to avoid relying on the kernel to get the necessary cache
 flushing correct.
 
 The proposed solution is to query the EFI memory map to ensure the
 kernel uses a compatible mapping.
 
 None of this should affect x86, it still uses PAGE_KERNEL because we're
 yet to see any hardware that has an EFI memory map entry for the GHES
 region that's incompatible with PAGE_KERNEL.
 
 Jonathan, would you like to provide more details?

FWIW, that matches my understanding of the problem too. The ARM architecture
refers to this situation as mismatched memory attributes and typically
requires some explicit cache maintenance to achieve portable behaviour in
this scenario.

It's much better to avoid the mismatch in the first place, if you can,
which is what this is all about.

Will
--
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 V9 0/5] map GHES memory region according to EFI memory map

[Will Deacon]

Hi Jonathan,

On Thu, Jul 30, 2015 at 10:35:04PM +0100, Jonathan (Zhixiong) Zhang wrote:
 From: Jonathan (Zhixiong) Zhang zjzh...@codeaurora.org
 
 On a platform with APEI (ACPI Platform Error Interface) enabled, firmware
 updates a memory region with hardware error record using nocache
 attribute. When OS reads the region, since it maps the region with
 cacahed attribute even though EFI memory map defines this region as
 uncached, OS gets stale data and errorneously reports there is no new
 HW error.
 
 When ghes driver maps the memory region, it uses the cache attribute
 according to EFI memory map, if EFI memory map feature is enabled
 at runtime.
 
 Since both arch/x86 and arch/ia64 implemented architecture agnostic EFI
 memory map attribue lookup function efi_memattributes(), the code is
 moved from arch/x86 into EFI subsystem and is declared as __weak; archs
 other than ia64 should not override the default implementation.
 
 V9:
 1. Rebased to arm64-upstream-14543 of arm64/master.
 2. Match strict MM type in arch_apei_get_mem_attribute().

I guess this is all going via Matt's tree? I'm happy to take the new
memory type in arch/arm64/ if there's nothing currently queued, but I
suspect it makes more sense for it to stay together.

Will
--
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] arm64/efi: prefer AllocatePages() over efi_low_alloc() for vmlinux

[Will Deacon]

On Tue, Jul 28, 2015 at 10:24:23PM +0100, Ard Biesheuvel wrote:
 On 28 July 2015 at 23:17, Matt Fleming m...@codeblueprint.co.uk wrote:
  On Fri, 24 Jul, at 01:38:27PM, Ard Biesheuvel wrote:
  When allocating memory for the kernel image, try the AllocatePages()
  boot service to obtain memory at the preferred offset of
  'dram_base + TEXT_OFFSET', and only revert to efi_low_alloc() if that
  fails. This is the only way to allocate at the base of DRAM if DRAM
  starts at 0x0, since efi_low_alloc() refuses to allocate at 0x0.
 
  Tested-by: Haojian Zhuang haojian.zhu...@linaro.org
  Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org
  ---
  v2:
  - reshuffle code flow to make it more logical, and have only a single
memcpy() invocation at the end of the function
  ---
   arch/arm64/kernel/efi-stub.c | 41 
  -
   1 file changed, 32 insertions(+), 9 deletions(-)
 
  Would it be easier if we allow efi_low_alloc() to return 0x0 for some
  uses? If you don't need the preference for low allocations, probably
  not, but I don't want to see us working around limitations in
  efi_low_alloc() instead of just fixing it.
 
 
 This workaround fixes another issue as well: the arm64 kernel needs to
 be loaded 512 KB above a 2MB aligned boundary, and using
 efi_low_alloc() as we do loses (2 MB - 512 KB) at the bottom if part
 of that 512 KB is occupied, since efi_low_alloc() is not aware of the
 fact that the first 512 KB will remain unused.
 
 What would be most helpful is if efi_low_alloc() could take an offset
 param in addition to the alignment, i.e., alignment == 2MB and offset
 == 512 KB. The offset would default to 0, reverting to the original
 behavior.
 
 If you'd be ok with such a change, I can propose it instead, and wire
 it up into this function.

I already merged the original patch, so if you propose anything extra,
please do it on top of that!

Will
--
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 1/8] arm64/efi: use UEFI memory map unconditionally if available

[Will Deacon]

On Mon, Dec 22, 2014 at 07:08:35PM +, Ard Biesheuvel wrote:
 On systems that boot via UEFI, all memory nodes are deleted from the
 device tree, and instead, the size and location of system RAM is derived
 from the UEFI memory map. This is handled by reserve_regions, which not only
 reserves parts of memory that UEFI declares as reserved, but also installs
 the memblocks that cover the remaining usable memory.
 
 Currently, reserve_regions() is only called if uefi_init() succeeds.
 However, it does not actually depend on anything that uefi_init() does,
 and not calling reserve_regions() results in a broken boot, so it is
 better to just call it unconditionally.
 
 Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org
 ---
  arch/arm64/kernel/efi.c | 11 ---
  1 file changed, 4 insertions(+), 7 deletions(-)

Acked-by: Will Deacon will.dea...@arm.com

Will

 diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
 index d7d2e818c856..d2f483a7cffe 100644
 --- a/arch/arm64/kernel/efi.c
 +++ b/arch/arm64/kernel/efi.c
 @@ -208,8 +208,7 @@ void __init efi_init(void)
   memmap.desc_size = params.desc_size;
   memmap.desc_version = params.desc_ver;
  
 - if (uefi_init()  0)
 - return;
 + WARN_ON(uefi_init()  0);
  
   reserve_regions();
  }
 @@ -218,15 +217,13 @@ static int __init arm64_enter_virtual_mode(void)
  {
   u64 mapsize;
  
 - if (!efi_enabled(EFI_BOOT)) {
 - pr_info(EFI services will not be available.\n);
 - return -1;
 - }
 + if (!efi_enabled(EFI_MEMMAP))
 + return 0;
  
   mapsize = memmap.map_end - memmap.map;
   early_memunmap(memmap.map, mapsize);
  
 - if (efi_runtime_disabled()) {
 + if (!efi_enabled(EFI_BOOT) || efi_runtime_disabled()) {
   pr_info(EFI runtime services will be disabled.\n);
   return -1;
   }
 -- 
 1.8.3.2
 
 
--
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 7/8] arm64: use 'physmem' memblock to improve CONFIG_STRICT_DEVMEM handling

[Will Deacon]

On Mon, Dec 22, 2014 at 07:08:41PM +, Ard Biesheuvel wrote:
 The 'physmem' memblock table allows us to classify memory as RAM even
 if it is not covered by the ordinary 'memory' memblock table.
 
 Under CONFIG_STRICT_DEVMEM, we can use this to:
 - allow read-only access to parts of RAM that are not considered memory
   by the kernel, this is mainly intended for exposing UEFI configuration
   tables such as SMBIOS to userland;
 - avoid using non-cached mappings for those parts of RAM, as it may
   result in mismatched attributes.

Looks fine to me assuming that the rest of the series is ok:

  Acked-by: Will Deacon will.dea...@arm.com

Will

 Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org
 ---
  arch/arm64/Kconfig   |  1 +
  arch/arm64/mm/mmap.c |  2 +-
  arch/arm64/mm/mmu.c  | 15 ++-
  3 files changed, 16 insertions(+), 2 deletions(-)
 
 diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
 index b1f9a20a3677..86902f8d8e36 100644
 --- a/arch/arm64/Kconfig
 +++ b/arch/arm64/Kconfig
 @@ -60,6 +60,7 @@ config ARM64
   select HAVE_GENERIC_DMA_COHERENT
   select HAVE_HW_BREAKPOINT if PERF_EVENTS
   select HAVE_MEMBLOCK
 + select HAVE_MEMBLOCK_PHYS_MAP
   select HAVE_PATA_PLATFORM
   select HAVE_PERF_EVENTS
   select HAVE_PERF_REGS
 diff --git a/arch/arm64/mm/mmap.c b/arch/arm64/mm/mmap.c
 index 54922d1275b8..9f558ab41f39 100644
 --- a/arch/arm64/mm/mmap.c
 +++ b/arch/arm64/mm/mmap.c
 @@ -126,7 +126,7 @@ int devmem_is_allowed(unsigned long pfn)
  {
   if (iomem_is_exclusive(pfn  PAGE_SHIFT))
   return 0;
 - if (!page_is_ram(pfn))
 + if (!pfn_valid(pfn))
   return 1;
   return 0;
  }
 diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
 index 328638548871..083be3de1a87 100644
 --- a/arch/arm64/mm/mmu.c
 +++ b/arch/arm64/mm/mmu.c
 @@ -122,7 +122,7 @@ early_param(cachepolicy, early_cachepolicy);
  pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn,
 unsigned long size, pgprot_t vma_prot)
  {
 - if (!pfn_valid(pfn))
 + if (!memblock_is_physmem(PFN_PHYS(pfn)))
   return pgprot_noncached(vma_prot);
   else if (file-f_flags  O_SYNC)
   return pgprot_writecombine(vma_prot);
 @@ -130,6 +130,19 @@ pgprot_t phys_mem_access_prot(struct file *file, 
 unsigned long pfn,
  }
  EXPORT_SYMBOL(phys_mem_access_prot);
  
 +/*
 + * This definition of phys_mem_access_prot_allowed() overrides
 + * the __weak definition in drivers/char/mem.c
 + */
 +int phys_mem_access_prot_allowed(struct file *file, unsigned long pfn,
 +  unsigned long size, pgprot_t *prot)
 +{
 + /* Disallow read-write access to system RAM */
 + if (memblock_is_physmem(PFN_PHYS(pfn))  pgprot_val(*prot)  PTE_WRITE)
 + return 0;
 + return 1;
 +}
 +
  static void __init *early_alloc(unsigned long sz)
  {
   void *ptr = __va(memblock_alloc(sz, sz));
 -- 
 1.8.3.2
 
 
--
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 8/8] arm64/efi: remove idmap manipulations from UEFI code

[Will Deacon]

On Thu, Jan 08, 2015 at 06:48:34PM +, Ard Biesheuvel wrote:
 Now that we have moved the call to SetVirtualAddressMap() to the stub,
 UEFI has no use for the ID map, so we can drop the code that installs
 ID mappings for UEFI memory regions.
 
 Acked-by: Leif Lindholm leif.lindh...@linaro.org
 Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org
 ---
  arch/arm64/include/asm/efi.h |  2 --
  arch/arm64/include/asm/mmu.h |  2 --
  arch/arm64/kernel/efi.c  | 32 +---
  arch/arm64/kernel/setup.c|  1 -
  arch/arm64/mm/mmu.c  | 12 
  5 files changed, 1 insertion(+), 48 deletions(-)

Looks like a good cleanup!

Acked-by: Will Deacon will.dea...@arm.com

Will

 diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
 index effef3713c5a..7baf2cc04e1e 100644
 --- a/arch/arm64/include/asm/efi.h
 +++ b/arch/arm64/include/asm/efi.h
 @@ -6,11 +6,9 @@
  
  #ifdef CONFIG_EFI
  extern void efi_init(void);
 -extern void efi_idmap_init(void);
  extern void efi_virtmap_init(void);
  #else
  #define efi_init()
 -#define efi_idmap_init()
  #define efi_virtmap_init()
  #endif
  
 diff --git a/arch/arm64/include/asm/mmu.h b/arch/arm64/include/asm/mmu.h
 index 5fd40c43be80..3d311761e3c2 100644
 --- a/arch/arm64/include/asm/mmu.h
 +++ b/arch/arm64/include/asm/mmu.h
 @@ -31,8 +31,6 @@ extern void paging_init(void);
  extern void setup_mm_for_reboot(void);
  extern void __iomem *early_io_map(phys_addr_t phys, unsigned long virt);
  extern void init_mem_pgprot(void);
 -/* create an identity mapping for memory (or io if map_io is true) */
 -extern void create_id_mapping(phys_addr_t addr, phys_addr_t size, int 
 map_io);
  extern void create_pgd_mapping(struct mm_struct *mm, phys_addr_t phys,
  unsigned long virt, phys_addr_t size,
  pgprot_t prot);
 diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
 index 4a5d7343..a98415b5979c 100644
 --- a/arch/arm64/kernel/efi.c
 +++ b/arch/arm64/kernel/efi.c
 @@ -54,27 +54,6 @@ static int __init is_normal_ram(efi_memory_desc_t *md)
   return 0;
  }
  
 -static void __init efi_setup_idmap(void)
 -{
 - struct memblock_region *r;
 - efi_memory_desc_t *md;
 - u64 paddr, npages, size;
 -
 - for_each_memblock(memory, r)
 - create_id_mapping(r-base, r-size, 0);
 -
 - /* map runtime io spaces */
 - for_each_efi_memory_desc(memmap, md) {
 - if (!(md-attribute  EFI_MEMORY_RUNTIME) || is_normal_ram(md))
 - continue;
 - paddr = md-phys_addr;
 - npages = md-num_pages;
 - memrange_efi_to_native(paddr, npages);
 - size = npages  PAGE_SHIFT;
 - create_id_mapping(paddr, size, 1);
 - }
 -}
 -
  /*
   * Translate a EFI virtual address into a physical address: this is 
 necessary,
   * as some data members of the EFI system table are virtually remapped after
 @@ -236,16 +215,6 @@ void __init efi_init(void)
   reserve_regions();
  }
  
 -void __init efi_idmap_init(void)
 -{
 - if (!efi_enabled(EFI_BOOT))
 - return;
 -
 - /* boot time idmap_pg_dir is incomplete, so fill in missing parts */
 - efi_setup_idmap();
 - early_memunmap(memmap.map, memmap.map_end - memmap.map);
 -}
 -
  /*
   * Enable the UEFI Runtime Services if all prerequisites are in place, i.e.,
   * non-early mapping of the UEFI system table and virtual mappings for all
 @@ -386,4 +355,5 @@ void __init efi_virtmap_init(void)
   create_pgd_mapping(efi_mm, paddr, md-virt_addr, size, prot);
   }
   set_bit(EFI_VIRTMAP, efi.flags);
 + early_memunmap(memmap.map, memmap.map_end - memmap.map);
  }
 diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
 index beac8188fdbd..199d1b7809d7 100644
 --- a/arch/arm64/kernel/setup.c
 +++ b/arch/arm64/kernel/setup.c
 @@ -402,7 +402,6 @@ void __init setup_arch(char **cmdline_p)
   request_standard_resources();
  
   efi_virtmap_init();
 - efi_idmap_init();
   early_ioremap_reset();
  
   unflatten_device_tree();
 diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
 index 3f3d5aa4a8b1..328638548871 100644
 --- a/arch/arm64/mm/mmu.c
 +++ b/arch/arm64/mm/mmu.c
 @@ -271,18 +271,6 @@ static void __init create_mapping(phys_addr_t phys, 
 unsigned long virt,
size, PAGE_KERNEL_EXEC);
  }
  
 -void __init create_id_mapping(phys_addr_t addr, phys_addr_t size, int map_io)
 -{
 - if ((addr  PGDIR_SHIFT) = ARRAY_SIZE(idmap_pg_dir)) {
 - pr_warn(BUG: not creating id mapping for %pa\n, addr);
 - return;
 - }
 - __create_mapping(init_mm, idmap_pg_dir[pgd_index(addr)],
 -  addr, addr, size,
 -  map_io ? __pgprot(PROT_DEVICE_nGnRE)
 - : PAGE_KERNEL_EXEC);
 -}
 -
  void __init create_pgd_mapping(struct mm_struct *mm, phys_addr_t phys

Re: [PATCH v5 7/8] arm64/efi: remove free_boot_services() and friends

[Will Deacon]

On Thu, Jan 08, 2015 at 06:48:33PM +, Ard Biesheuvel wrote:
 Now that we are calling SetVirtualAddressMap() from the stub, there is no
 need to reserve boot-only memory regions, which implies that there is also
 no reason to free them again later.
 
 Acked-by: Leif Lindholm leif.lindh...@linaro.org
 Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org
 ---
  arch/arm64/kernel/efi.c | 123 
 +---
  1 file changed, 1 insertion(+), 122 deletions(-)

[...]

 -static void __init free_boot_services(void)
 -{
 - u64 total_freed = 0;
 - u64 keep_end, free_start, free_end;
 - efi_memory_desc_t *md;
 -
 - /*
 -  * If kernel uses larger pages than UEFI, we have to be careful
 -  * not to inadvertantly free memory we want to keep if there is
 -  * overlap at the kernel page size alignment. We do not want to
 -  * free is_reserve_region() memory nor the UEFI memmap itself.
 -  *
 -  * The memory map is sorted, so we keep track of the end of
 -  * any previous region we want to keep, remember any region
 -  * we want to free and defer freeing it until we encounter
 -  * the next region we want to keep. This way, before freeing
 -  * it, we can clip it as needed to avoid freeing memory we
 -  * want to keep for UEFI.
 -  */
 -
 - keep_end = 0;
 - free_start = 0;
 -
 - for_each_efi_memory_desc(memmap, md) {
 - u64 paddr, npages, size;

I'm glad to see the back of this function, thanks.

Acked-by: Will Deacon will.dea...@arm.com

Will
--
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 1/2] arm64: don't make early_*map() calls post paging_init()

[Will Deacon]

On Tue, Jan 06, 2015 at 08:35:22PM +, Mark Salter wrote:
 On Tue, 2015-01-06 at 13:41 +, Leif Lindholm wrote:
  diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
  index 6fac253..e311066 100644
  --- a/arch/arm64/kernel/efi.c
  +++ b/arch/arm64/kernel/efi.c
  @@ -313,17 +313,26 @@ void __init efi_init(void)
  memmap.desc_size = params.desc_size;
  memmap.desc_version = params.desc_ver;
   
  -   if (uefi_init()  0)
  -   return;
  +   if (uefi_init() = 0)
  +   reserve_regions();
   
  -   reserve_regions();
  +   early_memunmap(memmap.map, params.mmap_size);
   }
   
  -void __init efi_idmap_init(void)
  +void __init efi_memmap_init(void)
   {
  +   u64 mapsize;
  +
  if (!efi_enabled(EFI_BOOT))
  return;
   
  +   /* replace early memmap mapping with permanent mapping */
  +   mapsize = memmap.map_end - memmap.map;
  +   memmap.map = (__force void *)ioremap_cache((phys_addr_t)memmap.phys_map,
  +  mapsize);
 
 ioremap_cache() could potententially fail here if the phys_map address
 doesn't have a valid pfn (not in the kernel linear ram mapping) because
 some of the underlying vm support hasn't been initialized yet.

Can you be more specific about the case you have in mind, please? pfn_valid
uses the memblocks on arm64, and that should all have been sorted out in
paging_init(). What's the issue that you're anticipating?

Will
--
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 0/2] arm64: don't call early_*map() post paging_init()

[Will Deacon]

On Tue, Jan 06, 2015 at 03:54:29PM +, Ard Biesheuvel wrote:
 On 6 January 2015 at 15:52, Leif Lindholm leif.lindh...@linaro.org wrote:
  On Tue, Jan 06, 2015 at 02:04:50PM +, Ard Biesheuvel wrote:
   Changes since v1:
   - Rebased to v3.19-rc3
   - Added 'Fixes:' tags
   - Reworked 1/2 to restore call to efi_setup_idmap() to the original
 location in the boot process.
  
   Looks reasonable to me; do they need a CC stable,
 
  Yes, but with a note that both patches should be taken and the order
  is retained, or we break bisect.
 
  Where would one add such a note?
 
 
 I guess the best way would be to not add a 'Cc: stable' tag, but send
 an email to Greg once the patches have been merged by Linus.

I thought Greg didn't take anything until it had hit mainline anyway? I
think it should be fine with just the Cc...

Will
--
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 v3 02/13] arm64/mm: add create_pgd_mapping() to create private page tables

[Will Deacon]

On Tue, Nov 18, 2014 at 12:57:01PM +, Ard Biesheuvel wrote:
 For UEFI, we need to install the memory mappings used for Runtime Services
 in a dedicated set of page tables. Add create_pgd_mapping(), which allows
 us to allocate and install those page table entries early.
 
 Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org
 ---
  arch/arm64/include/asm/mmu.h |  3 +++
  arch/arm64/include/asm/pgtable.h |  5 +
  arch/arm64/mm/mmu.c  | 43 
 
  3 files changed, 30 insertions(+), 21 deletions(-)

Looks fairly mechanical to me:

  Reviewed-by: Will Deacon will.dea...@arm.com

Will

 diff --git a/arch/arm64/include/asm/mmu.h b/arch/arm64/include/asm/mmu.h
 index c2f006c48bdb..5fd40c43be80 100644
 --- a/arch/arm64/include/asm/mmu.h
 +++ b/arch/arm64/include/asm/mmu.h
 @@ -33,5 +33,8 @@ extern void __iomem *early_io_map(phys_addr_t phys, 
 unsigned long virt);
  extern void init_mem_pgprot(void);
  /* create an identity mapping for memory (or io if map_io is true) */
  extern void create_id_mapping(phys_addr_t addr, phys_addr_t size, int 
 map_io);
 +extern void create_pgd_mapping(struct mm_struct *mm, phys_addr_t phys,
 +unsigned long virt, phys_addr_t size,
 +pgprot_t prot);
  
  #endif
 diff --git a/arch/arm64/include/asm/pgtable.h 
 b/arch/arm64/include/asm/pgtable.h
 index 41a43bf26492..1abe4d08725b 100644
 --- a/arch/arm64/include/asm/pgtable.h
 +++ b/arch/arm64/include/asm/pgtable.h
 @@ -264,6 +264,11 @@ static inline pmd_t pte_pmd(pte_t pte)
   return __pmd(pte_val(pte));
  }
  
 +static inline pgprot_t mk_sect_prot(pgprot_t prot)
 +{
 + return __pgprot(pgprot_val(prot)  ~PTE_TABLE_BIT);
 +}
 +
  /*
   * THP definitions.
   */
 diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
 index 83e6713143a3..4d583aa9ff4e 100644
 --- a/arch/arm64/mm/mmu.c
 +++ b/arch/arm64/mm/mmu.c
 @@ -157,20 +157,10 @@ static void __init alloc_init_pte(pmd_t *pmd, unsigned 
 long addr,
  
  static void __init alloc_init_pmd(struct mm_struct *mm, pud_t *pud,
 unsigned long addr, unsigned long end,
 -   phys_addr_t phys, int map_io)
 +   phys_addr_t phys, pgprot_t prot)
  {
   pmd_t *pmd;
   unsigned long next;
 - pmdval_t prot_sect;
 - pgprot_t prot_pte;
 -
 - if (map_io) {
 - prot_sect = PROT_SECT_DEVICE_nGnRE;
 - prot_pte = __pgprot(PROT_DEVICE_nGnRE);
 - } else {
 - prot_sect = PROT_SECT_NORMAL_EXEC;
 - prot_pte = PAGE_KERNEL_EXEC;
 - }
  
   /*
* Check for initial section mappings in the pgd/pud and remove them.
 @@ -186,7 +176,8 @@ static void __init alloc_init_pmd(struct mm_struct *mm, 
 pud_t *pud,
   /* try section mapping first */
   if (((addr | next | phys)  ~SECTION_MASK) == 0) {
   pmd_t old_pmd =*pmd;
 - set_pmd(pmd, __pmd(phys | prot_sect));
 + set_pmd(pmd, __pmd(phys |
 +pgprot_val(mk_sect_prot(prot;
   /*
* Check for previous table entries created during
* boot (__create_page_tables) and flush them.
 @@ -195,7 +186,7 @@ static void __init alloc_init_pmd(struct mm_struct *mm, 
 pud_t *pud,
   flush_tlb_all();
   } else {
   alloc_init_pte(pmd, addr, next, __phys_to_pfn(phys),
 -prot_pte);
 +prot);
   }
   phys += next - addr;
   } while (pmd++, addr = next, addr != end);
 @@ -203,7 +194,7 @@ static void __init alloc_init_pmd(struct mm_struct *mm, 
 pud_t *pud,
  
  static void __init alloc_init_pud(struct mm_struct *mm, pgd_t *pgd,
 unsigned long addr, unsigned long end,
 -   unsigned long phys, int map_io)
 +   unsigned long phys, pgprot_t prot)
  {
   pud_t *pud;
   unsigned long next;
 @@ -221,10 +212,11 @@ static void __init alloc_init_pud(struct mm_struct *mm, 
 pgd_t *pgd,
   /*
* For 4K granule only, attempt to put down a 1GB block
*/
 - if (!map_io  (PAGE_SHIFT == 12) 
 + if ((PAGE_SHIFT == 12) 
   ((addr | next | phys)  ~PUD_MASK) == 0) {
   pud_t old_pud = *pud;
 - set_pud(pud, __pud(phys | PROT_SECT_NORMAL_EXEC));
 + set_pud(pud, __pud(phys |
 +pgprot_val(mk_sect_prot(prot;
  
   /*
* If we have an old value for a pud, it will
 @@ -239,7 +231,7 @@ static void __init alloc_init_pud(struct mm_struct *mm, 
 pgd_t *pgd

Re: [PATCH v3 04/13] efi: split off remapping code from efi_config_init()

[Will Deacon]

On Tue, Nov 25, 2014 at 05:24:19PM +, Matt Fleming wrote:
 On Tue, 18 Nov, at 01:57:03PM, Ard Biesheuvel wrote:
  Split of the remapping code from efi_config_init() so that the caller
  can perform its own remapping. This is necessary to correctly handle
  virtually remapped UEFI memory regions under kexec, as efi.systab will
  have been updated to a virtual address.
  
  Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org
  ---
   drivers/firmware/efi/efi.c | 49 
  +-
   include/linux/efi.h|  2 ++
   2 files changed, 33 insertions(+), 18 deletions(-)
 
 Looks straight forward.
 
 What tree are these going through?

I think a bunch of the series is still under review, so we can probably
decide on that nearer 3.20 based on any dependencies that crop up.

Will
--
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 00/10] arm64 EFI patches for 3.19

[Will Deacon]

On Wed, Nov 05, 2014 at 07:53:39AM +, Ard Biesheuvel wrote:
 OK, it appears we're good to go with this series.
 
 @Will: would you like me to repost one final time? Or would you prefer
 a pull request instead? The only changes are added acks and a single
 code change where an open-coded constant 127 is replaced with its
 symbolic name DMI_ENTRY_END_OF_TABLE.

A pull request is fine, cheers.

Will
--
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 05/10] arm64/efi: drop redundant set_bit(EFI_CONFIG_TABLES)

[Will Deacon]

On Wed, Oct 22, 2014 at 03:21:48PM +0100, Ard Biesheuvel wrote:
 The EFI_CONFIG_TABLES bit already gets set by efi_config_init(),
 so there is no reason to set it again after this function returns
 successfully.
 
 Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org
 ---
  arch/arm64/kernel/efi.c | 2 --
  1 file changed, 2 deletions(-)

Since you mentioned it,

  Acked-by: Will Deacon will.dea...@arm.com

Will

 diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
 index 71ea4fc0aa8a..51522ab0c6da 100644
 --- a/arch/arm64/kernel/efi.c
 +++ b/arch/arm64/kernel/efi.c
 @@ -112,8 +112,6 @@ static int __init uefi_init(void)
   efi.systab-hdr.revision  0x, vendor);
  
   retval = efi_config_init(NULL);
 - if (retval == 0)
 - set_bit(EFI_CONFIG_TABLES, efi.flags);
  
  out:
   early_memunmap(efi.systab,  sizeof(efi_system_table_t));
 -- 
 1.8.3.2
 
 
--
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 10/10] arm64: dmi: set DMI string as dump stack arch description

[Will Deacon]

On Wed, Oct 22, 2014 at 03:21:53PM +0100, Ard Biesheuvel wrote:
 This sets the DMI string, containing system type, serial number,
 firmware version etc. as dump stack arch description, so that oopses
 and other kernel stack dumps automatically have this information
 included, if available.
 
 Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org
 ---
  arch/arm64/kernel/efi.c | 2 ++
  1 file changed, 2 insertions(+)
 
 diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
 index 0e9da0067ef2..baab9344a32b 100644
 --- a/arch/arm64/kernel/efi.c
 +++ b/arch/arm64/kernel/efi.c
 @@ -477,6 +477,8 @@ static int __init arm64_dmi_init(void)
* itself, depends on dmi_scan_machine() having been called already.
*/
   dmi_scan_machine();
 + if (dmi_available)
 + dmi_set_dump_stack_arch_desc();

This looks fine, but I don't understand why you would ever *not* want to
call this when DMI is available. In other words, why can't this be part
of dmi_scan_machine?

Will
--
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/arm64: store Runtime Services revision

[Will Deacon]

On Thu, Aug 14, 2014 at 03:55:22PM +0100, Semen Protsenko wrote:
 efi global data structure contains runtime_version field which must
 be assigned in order to use it later in Runtime Services virtual calls
 (virt_efi_* functions).
 
 Before this patch runtime_version was unassigned (0), so each
 Runtime Service virtual call that checks revision would fail.
 
 Runtime Services revision being passed in UEFI system table from UEFI
 to kernel (see efi_entry() function). In UEFI it's being stored at
 MdePkg/Include/Uefi/UefiSpec.h as EFI_RUNTIME_SERVICES_REVISION.
 
 Signed-off-by: Semen Protsenko semen.protse...@linaro.org
 ---
  arch/arm64/kernel/efi.c | 2 ++
  1 file changed, 2 insertions(+)
 
 diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
 index e72f310..5dbb7bd 100644
 --- a/arch/arm64/kernel/efi.c
 +++ b/arch/arm64/kernel/efi.c
 @@ -463,6 +463,8 @@ static int __init arm64_enter_virtual_mode(void)
   efi_native_runtime_setup();
   set_bit(EFI_RUNTIME_SERVICES, efi.flags);
  
 + efi.runtime_version = efi.systab-hdr.revision;
 +
   return 0;

I doubt it makes any difference, but can we assign this before we call
efi_native_runtime_setup please?

Will
--
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/arm64: store Runtime Services revision

[Will Deacon]

On Tue, Aug 26, 2014 at 07:24:50PM +0100, Matt Fleming wrote:
 On Tue, 26 Aug, at 10:05:33AM, Will Deacon wrote:
  
  I doubt it makes any difference, but can we assign this before we call
  efi_native_runtime_setup please?
 
 This would have to be a follow up patch, v2 of this patch is already in
 Linus' tree as commit 6a7519e81321.

Ah, ok, I wouldn't worry about it then. I was just trawling the list for
arm64 patches without me on CC in case I missed anything during the kernel
summit.

Thanks for picking it up!

Will
--
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 5/7 update] arm64/efi: do not enter virtual mode in case booting with efi=noruntime or noefi

[Will Deacon]

On Mon, Aug 18, 2014 at 02:30:07AM +0100, Dave Young wrote:
 In case efi runtime disabled via noefi kernel cmdline arm64_enter_virtual_mode
 should error out.
 
 At the same time move early_memunmap(memmap.map, mapsize) to the beginning of
 the function or it will leak early mem.
 
 Signed-off-by: Dave Young dyo...@redhat.com
 ---
  arch/arm64/kernel/efi.c | 11 ---
  1 file changed, 8 insertions(+), 3 deletions(-)
 
 diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
 index 6ed0362..8f5db4a 100644
 --- a/arch/arm64/kernel/efi.c
 +++ b/arch/arm64/kernel/efi.c
 @@ -392,11 +392,16 @@ static int __init arm64_enter_virtual_mode(void)
   return -1;
   }
  
 - pr_info(Remapping and enabling EFI services.\n);
 -
 - /* replace early memmap mapping with permanent mapping */
   mapsize = memmap.map_end - memmap.map;
   early_memunmap(memmap.map, mapsize);
 +
 + if (efi_runtime_disabled()) {
 + pr_info(EFI runtime services will be disabled.\n);
 + return -1;
 + }
 +
 + pr_info(Remapping and enabling EFI services.\n);
 + /* replace early memmap mapping with permanent mapping */
   memmap.map = (__force void *)ioremap_cache((phys_addr_t)memmap.phys_map,
  mapsize);
   memmap.map_end = memmap.map + mapsize;

This looks better, thanks:

  Reviewed-by: Will Deacon will.dea...@arm.com

Are you sending these via the efi tree as part of this series, or do you
need me to pick anything up into the arm64 tree?

Will
--
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 5/7 update] arm64/efi: do not enter virtual mode in case booting with efi=noruntime or noefi

[Will Deacon]

On Fri, Aug 22, 2014 at 04:47:19PM +0100, Matt Fleming wrote:
 On Fri, 22 Aug, at 04:10:07PM, Will Deacon wrote:
  
  Are you sending these via the efi tree as part of this series, or do you
  need me to pick anything up into the arm64 tree?
 
 I was planning on taking the entire series through the EFI tree (once
 I've had chance to review it).
 
 Does that work?

Fine by me; I think I've acked/reviewed all the arm64 bits. If you find an
arm64 patch with a missing ack, just ping me and I'll take a look.

Cheers,

Will
--
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/7] arm64/efi: uefi_init error handling fix

[Will Deacon]

On Thu, Aug 14, 2014 at 10:15:29AM +0100, Dave Young wrote:
 There's one early memmap leak in uefi_init error path, fix it and slightly 
 tune
 the error handling code.
 
 Signed-off-by: Dave Young dyo...@redhat.com

Reported-by: Will Deacon will.dea...@arm.com
Acked-by: Will Deacon will.dea...@arm.com

Thanks for fixing this.

Will

 ---
  arch/arm64/kernel/efi.c | 7 ---
  1 file changed, 4 insertions(+), 3 deletions(-)
 
 diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
 index e72f310..6ed0362 100644
 --- a/arch/arm64/kernel/efi.c
 +++ b/arch/arm64/kernel/efi.c
 @@ -89,7 +89,8 @@ static int __init uefi_init(void)
*/
   if (efi.systab-hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE) {
   pr_err(System table signature incorrect\n);
 - return -EINVAL;
 + retval = -EINVAL;
 + goto out;
   }
   if ((efi.systab-hdr.revision  16)  2)
   pr_warn(Warning: EFI system table version %d.%02d, expected 
 2.00 or greater\n,
 @@ -103,6 +104,7 @@ static int __init uefi_init(void)
   for (i = 0; i  (int) sizeof(vendor) - 1  *c16; ++i)
   vendor[i] = c16[i];
   vendor[i] = '\0';
 + early_memunmap(c16, sizeof(vendor));
   }
  
   pr_info(EFI v%u.%.02u by %s\n,
 @@ -113,9 +115,8 @@ static int __init uefi_init(void)
   if (retval == 0)
   set_bit(EFI_CONFIG_TABLES, efi.flags);
  
 - early_memunmap(c16, sizeof(vendor));
 +out:
   early_memunmap(efi.systab,  sizeof(efi_system_table_t));
 -
   return retval;
  }
  
 -- 
 1.8.3.1
 
 
--
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 5/7] arm64/efi: do not enter virtual mode in case booting with efi=noruntime or noefi

[Will Deacon]

On Thu, Aug 14, 2014 at 10:15:30AM +0100, Dave Young wrote:
 In case efi runtime disabled via noefi kernel cmdline arm64_enter_virtual_mode
 should error out.
 
 At the same time move early_memunmap(memmap.map, mapsize) to the beginning of
 the function or it will leak early mem.
 
 Signed-off-by: Dave Young dyo...@redhat.com
 ---
  arch/arm64/kernel/efi.c | 9 +++--
  1 file changed, 7 insertions(+), 2 deletions(-)
 
 diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
 index 6ed0362..309fab1 100644
 --- a/arch/arm64/kernel/efi.c
 +++ b/arch/arm64/kernel/efi.c
 @@ -392,11 +392,16 @@ static int __init arm64_enter_virtual_mode(void)
   return -1;
   }
  
 + mapsize = memmap.map_end - memmap.map;
 + if (efi_runtime_disabled()) {
 + early_memunmap(memmap.map, mapsize);

Should this early_memunmap really be conditional? With this change, we no
longer unmap it before setting up the permanent mapping below.

Will

 + pr_info(EFI runtime services will be disabled.\n);
 + return -1;
 + }
 +
   pr_info(Remapping and enabling EFI services.\n);
  
   /* replace early memmap mapping with permanent mapping */
 - mapsize = memmap.map_end - memmap.map;
 - early_memunmap(memmap.map, mapsize);
   memmap.map = (__force void *)ioremap_cache((phys_addr_t)memmap.phys_map,
  mapsize);
   memmap.map_end = memmap.map + mapsize;
 -- 
 1.8.3.1
 
 
--
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: [RFC PATCH] arm64/efi: use id mapping for Runtime Services

[Will Deacon]

On Wed, Aug 06, 2014 at 04:15:23PM +0100, Ard Biesheuvel wrote:
 On 6 August 2014 16:36, Will Deacon will.dea...@arm.com wrote:
  On Thu, Jul 31, 2014 at 03:11:49PM +0100, Ard Biesheuvel wrote:
   #define efi_call_virt(f, ...) 
 \
   ({   \
  - efi_##f##_t *__f = efi.systab-runtime-f;  \
  + efi_##f##_t *__f;   \
efi_status_t __s;   \
\
  - kernel_neon_begin();\
  + kernel_neon_begin(); /* disables preemption */  \
  + switch_pgd(idmap_pg_dir, init_mm); \
  + __f =  efi.systab-runtime-f;  \
__s = __f(__VA_ARGS__); \
  + switch_pgd(current-active_mm-pgd, current-active_mm);\
kernel_neon_end();  \
__s;\
   })
 
  This scares the bejesus out of me, but I can't put my finger on exactly why.
  I think it does what you intend and I can't break it myself, so it would be
  really good if the EFI folks could confirm that this looks good to them.
 
 
 There is something similar in the x86 code (arch/x86/platform/efi/efi.c)
 
  * The new method does a pagetable switch in a preemption-safe manner
  * so that we're in a different address space when calling a runtime
  * function. For function arguments passing we do copy the PGDs of the
  * kernel page table into -trampoline_pgd prior to each call.
 
 
 How exactly this will turn out for arm64 (and ARM) is still under
 discussion, though. My position is that if you are going to switch
 pgd's anyway, why not just use the id mapping? And even if you feel it
 is mandatory to install a virtual address mapping into UEFI (which I
 think is /not/ the case), you could install an id mapping as well,
 which means all the related machinery still gets invoked.
 The alternative to using a TTBR0 mapping would be to reserve a slice
 of kernel virtual memory so that the Runtime Services are guaranteed
 to live at the same virtual address after a kexec, ideally the same
 region on 4k and 64k pages ...
 
 We are planning to discuss this further at Linaro Connect next month.

Ok, thanks for the update. I'll hold off on this until you've discussed it
some more.

Cheers,

Will
--
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 1/3] arm64: spin-table: handle unmapped cpu-release-addrs

[Will Deacon]

On Wed, Jul 30, 2014 at 08:17:02PM +0100, Ard Biesheuvel wrote:
 ]On 30 July 2014 13:30, Will Deacon will.dea...@arm.com wrote:
  On Wed, Jul 30, 2014 at 11:59:02AM +0100, Ard Biesheuvel wrote:
  From: Mark Rutland mark.rutl...@arm.com
 
  In certain cases the cpu-release-addr of a CPU may not fall in the
  linear mapping (e.g. when the kernel is loaded above this address due to
  the presence of other images in memory). This is problematic for the
  spin-table code as it assumes that it can trivially convert a
  cpu-release-addr to a valid VA in the linear map.
 
  This patch modifies the spin-table code to use a temporary cached
  mapping to write to a given cpu-release-addr, enabling us to support
  addresses regardless of whether they are covered by the linear mapping.
 
  Signed-off-by: Mark Rutland mark.rutl...@arm.com
  Tested-by: Mark Salter msal...@redhat.com
  [ardb: added (__force void *) cast]
  Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org
  ---
   arch/arm64/kernel/smp_spin_table.c | 22 +-
   1 file changed, 17 insertions(+), 5 deletions(-)
 
  I'm nervous about this. What if the spin table sits in the same physical 64k
  frame as a read-sensitive device and we're running with 64k pages?
 
 
 Actually, booting.txt requires cpu-release-addr to point to a
 /memreserve/d part of memory, which implies DRAM (or you wouldn't have
 to memreserve it)
 That means it should always be covered by the linear mapping, unless
 it is located before Image in DRAM, which is the case addressed by
 this patch.

But if it's located before before the Image in DRAM and isn't covered by
the linear mapping, then surely the /memreserve/ is pointless too? In which
case, this looks like we're simply trying to cater for platforms that aren't
following booting.txt (which may need updating if we need to handle this).

Will
--
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 1/3] arm64: spin-table: handle unmapped cpu-release-addrs

[Will Deacon]

On Wed, Jul 30, 2014 at 11:59:02AM +0100, Ard Biesheuvel wrote:
 From: Mark Rutland mark.rutl...@arm.com
 
 In certain cases the cpu-release-addr of a CPU may not fall in the
 linear mapping (e.g. when the kernel is loaded above this address due to
 the presence of other images in memory). This is problematic for the
 spin-table code as it assumes that it can trivially convert a
 cpu-release-addr to a valid VA in the linear map.
 
 This patch modifies the spin-table code to use a temporary cached
 mapping to write to a given cpu-release-addr, enabling us to support
 addresses regardless of whether they are covered by the linear mapping.
 
 Signed-off-by: Mark Rutland mark.rutl...@arm.com
 Tested-by: Mark Salter msal...@redhat.com
 [ardb: added (__force void *) cast]
 Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org
 ---
  arch/arm64/kernel/smp_spin_table.c | 22 +-
  1 file changed, 17 insertions(+), 5 deletions(-)

I'm nervous about this. What if the spin table sits in the same physical 64k
frame as a read-sensitive device and we're running with 64k pages?

Will
--
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 1/3] arm64: spin-table: handle unmapped cpu-release-addrs

[Will Deacon]

On Wed, Jul 30, 2014 at 01:30:29PM +0100, Mark Rutland wrote:
 On Wed, Jul 30, 2014 at 01:00:40PM +0100, Ard Biesheuvel wrote:
  On 30 July 2014 13:30, Will Deacon will.dea...@arm.com wrote:
   On Wed, Jul 30, 2014 at 11:59:02AM +0100, Ard Biesheuvel wrote:
   From: Mark Rutland mark.rutl...@arm.com
  
   In certain cases the cpu-release-addr of a CPU may not fall in the
   linear mapping (e.g. when the kernel is loaded above this address due to
   the presence of other images in memory). This is problematic for the
   spin-table code as it assumes that it can trivially convert a
   cpu-release-addr to a valid VA in the linear map.
  
   This patch modifies the spin-table code to use a temporary cached
   mapping to write to a given cpu-release-addr, enabling us to support
   addresses regardless of whether they are covered by the linear mapping.
  
   Signed-off-by: Mark Rutland mark.rutl...@arm.com
   Tested-by: Mark Salter msal...@redhat.com
   [ardb: added (__force void *) cast]
   Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org
   ---
arch/arm64/kernel/smp_spin_table.c | 22 +-
1 file changed, 17 insertions(+), 5 deletions(-)
  
   I'm nervous about this. What if the spin table sits in the same physical 
   64k
   frame as a read-sensitive device and we're running with 64k pages?
  
  
  I see what you mean. This is potentially hairy, as EFI already
  ioremap_cache()s everything known to it as normal DRAM, so using plain
  ioremap() here if pfn_valid() returns false for cpu-release-addr's PFN
  may still result in mappings with different attributes for the same
  region. So how should we decide whether to call ioremap() or
  ioremap_cache() in this case?
 
 If we're careful about handling mismatched attributes we might be able
 to get away with always using a device mapping.

Even then, I think ioremap hits a WARN_ON if pfn_valid.

 I'll need to have a think about that, I'm not sure on the architected
 cache behaviour in such a case.

Of we just skip the cache flush if !pfn_valid.

Will
--
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: [RFC PATCH 04/10] arm64: add EFI little endian constants to linker script

[Will Deacon]

On Wed, Jul 30, 2014 at 03:18:05PM +0100, Matt Fleming wrote:
 On Mon, 21 Jul, at 05:16:19PM, Ard Biesheuvel wrote:
  Similar to how text offset and kernel size are mangled to produce little
  endian constants for the Image header regardless of the endianness of the
  kernel, this adds a number of constants used in the EFI PE/COFF header which
  can only be calculated (and byte swapped) by the linker.
  
  Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org
  ---
   arch/arm64/kernel/image.h | 16 +++-
   1 file changed, 15 insertions(+), 1 deletion(-)
 
 Where is this file? I can't find it in Linus' tree.

It's queued in the arm64 tree for 3.17 (should also appear in -next at the
moment).

See commit a2c1d73b94ed (arm64: Update the Image header).

Will
--
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: EFI_STUB fails to boot non-EFI on arm64

[Will Deacon]

On Fri, May 23, 2014 at 04:03:31PM +0100, Leif Lindholm wrote:
 On Fri, May 23, 2014 at 02:47:20PM +0100, Catalin Marinas wrote:
  Can we add another of detecting whether it's an EFI application and
  avoid calling efi_init()? I can see x86 sets some efi_loader_signature
  string in exit_boot() and checks against it later when calling
  efi_init().
 
 Well, I agree that we shouldn't be spewing error messages for expected
 operation, but efi_init() is the function we call to determine
 whether we _are_ booting via UEFI - and it sets flags accordingly for
 the efi_enabled() macro.
 
 My view is that this should be fixed in fdt_find_uefi_params(). A
 single info message that we can't find evidence of UEFI should be
 printed in the non-error case.
 
 Like below?

Why not move the efi_get_fdt_params call out of efi_init and into
setup_arch via a wrapper? Then efi_get_fdt_params and efi_init can have
useful return values, which allow us to distinguish between My DT doesn't
have the necessary UEFI properties and UEFI failed to initialise without
having to make some printks pr_info and others pr_err within efi_init
itself..

Will
--
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: EFI_STUB fails to boot non-EFI on arm64

[Will Deacon]

On Wed, May 28, 2014 at 07:05:26PM +0100, Leif Lindholm wrote:
 On Wed, May 28, 2014 at 04:59:31PM +0100, Will Deacon wrote:
Can we add another of detecting whether it's an EFI application and
avoid calling efi_init()? I can see x86 sets some efi_loader_signature
string in exit_boot() and checks against it later when calling
efi_init().
   
   Well, I agree that we shouldn't be spewing error messages for expected
   operation, but efi_init() is the function we call to determine
   whether we _are_ booting via UEFI - and it sets flags accordingly for
   the efi_enabled() macro.
   
   My view is that this should be fixed in fdt_find_uefi_params(). A
   single info message that we can't find evidence of UEFI should be
   printed in the non-error case.
   
   Like below?
  
  Why not move the efi_get_fdt_params call out of efi_init and into
  setup_arch via a wrapper? Then efi_get_fdt_params and efi_init can have
  useful return values, which allow us to distinguish between My DT doesn't
  have the necessary UEFI properties and UEFI failed to initialise without
  having to make some printks pr_info and others pr_err within efi_init
  itself..
 
 Well, but (for the output part) my patch already did that?
 If the Getting parameters from FDT:\n was too verbose, we could
 just drop it, and have the same effect on output.

It's the pr_err which is annoying, not the Getting parameters from FDT:\n
message. Why should I have an error logged to my console when I was never
intending to boot using EFI anyway?

 Thing is - there is not really any error case available anywhere
 during the execution of efi_init() and its branches other than:
 - Information required for UEFI boot cannot be found.
 - Information exists, but is invalid.
 - Failed to early_memremap some UEFI regions into the kernel.
 which all amounts to UEFI not available or something went wrong,
 rather than UEFI failed to initialise.

Fine, but in this case the DT had the relevant properties which is a good
indication that the user was at least *trying* to boot using EFI, no?

 If efi_init returns successfully, EFI_BOOT is set, and testable using
 the efi_enabled() macro.
 
 The proper UEFI failed to initialise bit does not come until the
 early_initcall arm64_enter_virtual_mode(), and is indicated not by
 a return value, but by setting the flag indicating that
 EFI_RUNTIME_SERVICES are available, which is checked later in core
 code using the efi_enabled() macro.

Sorry, I naively assumed that with a name like efi_init it might, you know,
initialise EFI? ;)

 So moving the call to efi_get_fdt_params() would have little effect
 other than adding a third call site for UEFI bits in setup_arch().

I don't mind having the extra call site if it allows us to distinguish
errors from information.

Will
--
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 02/22] arm: add new asm macro update_sctlr

[Will Deacon]

On Wed, Feb 05, 2014 at 05:03:53PM +, Leif Lindholm wrote:
 A new macro for setting/clearing bits in the SCTLR.
 
 Signed-off-by: Leif Lindholm leif.lindh...@linaro.org
 Suggested-by: Will Deacon will.dea...@arm.com
 Cc: Will Deacon will.dea...@arm.com
 ---
  arch/arm/include/asm/assembler.h |   14 ++
  1 file changed, 14 insertions(+)

Acked-by: Will Deacon will.dea...@arm.com

(although really minor comment below)

 diff --git a/arch/arm/include/asm/assembler.h 
 b/arch/arm/include/asm/assembler.h
 index 5c22851..e8ca24b 100644
 --- a/arch/arm/include/asm/assembler.h
 +++ b/arch/arm/include/asm/assembler.h
 @@ -383,4 +383,18 @@ THUMB(   orr \reg , \reg , #PSR_T_BIT)
  #endif
   .endm
  
 +#ifdef CONFIG_CPU_CP15
 +/* Macro for setting/clearing bits in sctlr */
 + .macro  update_sctlr, tmp:req, set=, clear=
 + mrc p15, 0, \tmp, c1, c0, 0
 + .ifnc   \set,
 + orr \tmp, \set

I'd prefer the 3-arg form here for consistency (with this macro and the
rest of the file).

Will
--
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 v4 2/5] arm: add new asm macro update_sctlr

[Will Deacon]

On Thu, Jan 30, 2014 at 01:12:47PM +, Leif Lindholm wrote:
 Oh, that's neat - thanks!
 
 Well, given that, I can think of two less horrible options:
 1)
   .macro  update_sctlr, tmp:req, set=, clear=
 mrc   p15, 0, \tmp, c1, c0, 0
   .ifnc   \set,
 orr   \tmp, \set
   .endif
   .ifnc   \clear,
   mvn \clear, \clear
   and \tmp, \tmp, \clear

Can't you use bic here?

   .endif
 mcr   p15, 0, \tmp, c1, c0, 0
   .endm
 
 With the two call sites in uefi_phys.S as:
 
   ldr r5, =(CR_M)
   update_sctlrr12, , r5
 and
   ldr r4, =(CR_I | CR_C | CR_M)
   update_sctlrr12, r4

These ldr= could be movs, right?

If so, I definitely prefer this to putting an ldr = into the macro itself
(option 2).

Cheers,

Will
--
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 v4 2/5] arm: add new asm macro update_sctlr

[Will Deacon]

On Mon, Feb 03, 2014 at 03:55:42PM +, Leif Lindholm wrote:
 On Mon, Feb 03, 2014 at 10:34:15AM +, Will Deacon wrote:
  On Thu, Jan 30, 2014 at 01:12:47PM +, Leif Lindholm wrote:
   Oh, that's neat - thanks!
   
   Well, given that, I can think of two less horrible options:
   1)
 .macro  update_sctlr, tmp:req, set=, clear=
   mrc   p15, 0, \tmp, c1, c0, 0
 .ifnc   \set,
   orr   \tmp, \set
 .endif
 .ifnc   \clear,
 mvn \clear, \clear
 and \tmp, \tmp, \clear
  
  Can't you use bic here?
 
 Yeah.
 
 .endif
   mcr   p15, 0, \tmp, c1, c0, 0
 .endm
   
   With the two call sites in uefi_phys.S as:
   
 ldr r5, =(CR_M)
 update_sctlrr12, , r5
   and
 ldr r4, =(CR_I | CR_C | CR_M)
 update_sctlrr12, r4
  
  These ldr= could be movs, right?
 
 The first one could.
 The second one could be movw on armv7+.
 
  If so, I definitely prefer this to putting an ldr = into the macro itself
  (option 2).
 
 And your preference between 1) and 2) is?

(1), using bic and mov[tw] where possible.

Will
--
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 v4 2/5] arm: add new asm macro update_sctlr

[Will Deacon]

On Mon, Feb 03, 2014 at 04:46:36PM +, Leif Lindholm wrote:
 On Mon, Feb 03, 2014 at 04:00:51PM +, Will Deacon wrote:
 With the two call sites in uefi_phys.S as:
 
   ldr r5, =(CR_M)
   update_sctlrr12, , r5
 and
   ldr r4, =(CR_I | CR_C | CR_M)
   update_sctlrr12, r4

These ldr= could be movs, right?
   
   The first one could.
   The second one could be movw on armv7+.
   
If so, I definitely prefer this to putting an ldr = into the macro 
itself
(option 2).
   
   And your preference between 1) and 2) is?
  
  (1), using bic and mov[tw] where possible.
 
 (1): ok, thanks.
 
 bic: sure, that was an oversight.
 
 mov[tw]: why?
 Then we end up battling different available immediate fields in A32/T32
 instruction sets and v5/v6/v7 architecture versions.

I was making the assumption that UEFI was going to be v7 only... is this not
true?

Will
--
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 v4 2/5] arm: add new asm macro update_sctlr

[Will Deacon]

Hi Leif,

On Wed, Jan 29, 2014 at 06:28:05PM +, Leif Lindholm wrote:
 On Wed, Jan 22, 2014 at 11:20:55AM +, Will Deacon wrote:
   +#ifdef CONFIG_CPU_CP15
   +/* Macro for setting/clearing bits in sctlr */
   + .macro update_sctlr, set:req, clear:req, tmp:req, tmp2:req
   + mrc p15, 0, \tmp, c1, c0, 0
   + ldr \tmp2, =\set
   + orr \tmp, \tmp, \tmp2
   + ldr \tmp2, =\clear
   + mvn \tmp2, \tmp2
   + and \tmp, \tmp, \tmp2
   + mcr p15, 0, \tmp, c1, c0, 0
  
  I think this would be cleaner if you force the caller to put set and clear
  into registers beforehand, rather than have to do the literal load every
  time. Also, I don't think set and clear should be required (and then you can
  lose tmp2 as well).
 
 I can't figure out how to make register-parameters non-required
 (i.e. conditionalise on whether an optional parameter was provided),
 so my attempt of refactoring actually ends up using an additional
 register:
 
 #ifdef CONFIG_CPU_CP15
 /* Macro for setting/clearing bits in sctlr */
   .macro  update_sctlr, set:req, clear:req, tmp:req
   mrc p15, 0, \tmp, c1, c0, 0
   orr \tmp, \set
   mvn \clear, \clear
   and \tmp, \tmp, \clear
   mcr p15, 0, \tmp, c1, c0, 0
   .endm
 #endif
 
 If you think that's an improvement I can do that, and I have (just)
 enough registers to spare.
 If I'm being daft with my macro issues, do point it out.

I was thinking along the lines of:

.macro  update_sctlr, tmp:req, set=0, clear=0
.if \set
orr ...
.endif
.if \clear
bic ...
.endif
.endm

however, that puts us back to the problem of having to pass immediates
instead of registers. Gas *does* seem to accept the following:

.macro  update_sctlr, tmp:req, set=0, clear=0
.if \set != 0
orr ...
.endif
.if \clear != 0
bic ...
.endif
.endm

which is filthy, so we'd need to see how beautiful the caller ends up being
to justify that!

Will
--
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 v4 1/5] arm: break part of __soft_restart out into separate function

[Will Deacon]

On Sat, Jan 11, 2014 at 01:05:20PM +, Leif Lindholm wrote:
 Certain operations can be considered mandatory for any piece of code
 preparing to switch off the MMU. Break this out into separate function
 dmap_prepare().

idmap_prepare. dmap_prepare sounds like something *completely* different!

With that:

  Acked-by: Will Deacon will.dea...@arm.com

Will
--
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 v4 2/5] arm: add new asm macro update_sctlr

[Will Deacon]

On Sat, Jan 11, 2014 at 01:05:21PM +, Leif Lindholm wrote:
 A new macro for setting/clearing bits in the SCTLR.
 
 Signed-off-by: Leif Lindholm leif.lindh...@linaro.org
 Suggested-by: Will Deacon will.dea...@arm.com
 ---
  arch/arm/include/asm/assembler.h |   13 +
  1 file changed, 13 insertions(+)
 
 diff --git a/arch/arm/include/asm/assembler.h 
 b/arch/arm/include/asm/assembler.h
 index 5c22851..aba6458 100644
 --- a/arch/arm/include/asm/assembler.h
 +++ b/arch/arm/include/asm/assembler.h
 @@ -383,4 +383,17 @@ THUMB(   orr \reg , \reg , #PSR_T_BIT)
  #endif
   .endm
  
 +#ifdef CONFIG_CPU_CP15
 +/* Macro for setting/clearing bits in sctlr */
 + .macro update_sctlr, set:req, clear:req, tmp:req, tmp2:req
 + mrc p15, 0, \tmp, c1, c0, 0
 + ldr \tmp2, =\set
 + orr \tmp, \tmp, \tmp2
 + ldr \tmp2, =\clear
 + mvn \tmp2, \tmp2
 + and \tmp, \tmp, \tmp2
 + mcr p15, 0, \tmp, c1, c0, 0

I think this would be cleaner if you force the caller to put set and clear
into registers beforehand, rather than have to do the literal load every
time. Also, I don't think set and clear should be required (and then you can
lose tmp2 as well).

Will
--
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 v3 2/3] arm: Add [U]EFI runtime services support

[Will Deacon]

Hi Leif,

Sorry it took so long to get back to you on this...

On Fri, Nov 29, 2013 at 05:43:04PM +, Leif Lindholm wrote:
   + */
   +static void __init phys_call_prologue(void)
   +{
   +   local_irq_disable();
   +
   +   /* Take out a flat memory mapping. */
   +   setup_mm_for_reboot();
   +
   +   /* Clean and invalidate caches */
   +   flush_cache_all();
   +
   +   /* Turn off caching */
   +   cpu_proc_fin();
   +
   +   /* Push out any further dirty data, and ensure cache is empty */
   +   flush_cache_all();
 
  Do we care about outer caches here?
 
 I think we do. We jump into UEFI and make it relocate itself to the
 new virtual addresses, with MMU disabled (so all accesses NC).

Ok, so that means you need some calls to the outer_* cache ops.

  This all looks suspiciously like
  copy-paste from __soft_restart;
 
 Yeah, 'cause you told me to :)
 
  perhaps some refactoring/reuse is in order?
 
 Could do. Turn this into a process.c:idcall_prepare(), or something less
 daftly named?

Sure, something like that (can't think of a good name either, right
now...).

   + * Restore original memory map and re-enable interrupts.
   + */
   +static void __init phys_call_epilogue(void)
   +{
   +   static struct mm_struct *mm = init_mm;
   +
   +   /* Restore original memory mapping */
   +   cpu_switch_mm(mm-pgd, mm);
   +
   +   /* Flush branch predictor and TLBs */
   +   local_flush_bp_all();
   +#ifdef CONFIG_CPU_HAS_ASID
   +   local_flush_tlb_all();
   +#endif
 
  ... and this looks like copy-paste from setup_mm_for_reboot.
 
 You told me that too!

Hehe, I don't recall the conversation but I was probably talking in terms of
`let's get something working, then tidy it up afterwards'.

 Only this goes the other way.
 
 Is the refactoring worth the extra code?

I think so. This code is pretty subtle, and I don't like it getting copied
around, particularly if we have to fix issues in it later on.

   +   local_irq_enable();
   +}
   +
   +static int __init remap_region(efi_memory_desc_t *md, efi_memory_desc_t 
   *entry)
   +{
   +   u64 va;
   +   u64 paddr;
   +   u64 size;
   +
   +   *entry = *md;
   +   paddr = entry-phys_addr;
   +   size = entry-num_pages  EFI_PAGE_SHIFT;
   +
   +   /*
   +* Map everything writeback-capable as coherent memory,
   +* anything else as device.
   +*/
   +   if (md-attribute  EFI_MEMORY_WB)
   +   va = (u64)((u32)uefi_remap(paddr, size)  0xUL);
   +   else
   +   va = (u64)((u32)uefi_ioremap(paddr, size)  0xUL);
 
  Do you really need all those casts/masking?
 
 I didn't find a better way to avoid warnings when building this code
 for both LPAE/non-LPAE.

Hmm, well phys_addr_t will be appropriately sized, if that helps you.

   + * This function switches the UEFI runtime services to virtual mode.
   + * This operation must be performed only once in the system's lifetime,
   + * including any kecec calls.
   + *
   + * This must be done with a 1:1 mapping. The current implementation
   + * resolves this by disabling the MMU.
   + */
   +efi_status_t  __init phys_set_virtual_address_map(u32 memory_map_size,
   + u32 descriptor_size,
   + u32 descriptor_version,
   + efi_memory_desc_t *dsc)
   +{
   +   uefi_phys_call_t *phys_set_map;
   +   efi_status_t status;
   +
   +   phys_call_prologue();
   +
   +   phys_set_map = (void *)(unsigned 
   long)virt_to_phys(uefi_phys_call);
   +
   +   /* Called with caches disabled, returns with caches enabled */
   +   status = phys_set_map(memory_map_size, descriptor_size,
   + descriptor_version, dsc,
   + efi.set_virtual_address_map)
   +;
 
  Guessing this relies on a physically-tagged cache? Wouldn't hurt to put
  something in the epilogue code to deal with vivt caches if they're not
  prohibited by construction (e.g. due to some build dependency magic).
 
 Sorry, I don't quite follow?
 How would it depend on a physically-tagged cache?

I was wondering if you could run into issues in the epilogue, since you've
changed page tables without cleaning the cache, but actually cpu_switch_mm
takes care of that.

   +   phys_call_epilogue();
   +
   +   return status;
   +}
 
 
   diff --git a/arch/arm/kernel/uefi_phys.S b/arch/arm/kernel/uefi_phys.S
   new file mode 100644
   index 000..e9a1542
   --- /dev/null
   +++ b/arch/arm/kernel/uefi_phys.S
   @@ -0,0 +1,59 @@
   +/*
   + * arch/arm/kernel/uefi_phys.S
   + *
   + * Copyright (C) 2013  Linaro Ltd.
   + *
   + * This program is free software; you can redistribute it and/or modify
   + * it under the terms of the GNU General Public License version 2 as
   + * published by the Free 

Re: [PATCH v3 2/3] arm: Add [U]EFI runtime services support

[Will Deacon]

Hi Leif,

On Thu, Nov 28, 2013 at 04:41:22PM +, Leif Lindholm wrote:
 This patch implements basic support for UEFI runtime services in the
 ARM architecture - a requirement for using efibootmgr to read and update
 the system boot configuration.
 
 It uses the generic configuration table scanning to populate ACPI and
 SMBIOS pointers.

I've got a bunch of implementation questions, so hopefully you can help me
to understand what this is doing!

 diff --git a/arch/arm/kernel/uefi.c b/arch/arm/kernel/uefi.c
 new file mode 100644
 index 000..f771026
 --- /dev/null
 +++ b/arch/arm/kernel/uefi.c
 @@ -0,0 +1,469 @@
 +/*
 + * Based on Unified Extensible Firmware Interface Specification version 2.3.1
 + *
 + * Copyright (C) 2013  Linaro Ltd.
 + *
 + * This program is free software; you can redistribute it and/or modify
 + * it under the terms of the GNU General Public License version 2 as
 + * published by the Free Software Foundation.
 + */
 +
 +#include linux/efi.h
 +#include linux/export.h
 +#include linux/memblock.h
 +#include linux/of.h
 +#include linux/of_fdt.h
 +#include linux/sched.h
 +#include linux/slab.h
 +
 +#include asm/cacheflush.h
 +#include asm/idmap.h
 +#include asm/tlbflush.h
 +#include asm/uefi.h
 +
 +struct efi_memory_map memmap;
 +
 +static efi_runtime_services_t *runtime;
 +
 +static phys_addr_t uefi_system_table;
 +static phys_addr_t uefi_boot_mmap;
 +static u32 uefi_boot_mmap_size;
 +static u32 uefi_mmap_desc_size;
 +static u32 uefi_mmap_desc_ver;
 +
 +static unsigned long arm_uefi_facility;
 +
 +/*
 + * If you're planning to wire up a debugger and debug the UEFI side ...
 + */
 +#undef KEEP_ALL_REGIONS
 +
 +/*
 + * If you need to (temporarily) support buggy firmware.
 + */
 +#define KEEP_BOOT_SERVICES_REGIONS
 +
 +/*
 + * Returns 1 if 'facility' is enabled, 0 otherwise.
 + */
 +int efi_enabled(int facility)
 +{
 +   return test_bit(facility, arm_uefi_facility) != 0;

!test_bit(...) ?

 +static int __init uefi_init(void)
 +{
 +   efi_char16_t *c16;
 +   char vendor[100] = unknown;
 +   int i, retval;
 +
 +   efi.systab = early_memremap(uefi_system_table,
 +   sizeof(efi_system_table_t));
 +
 +   /*
 +* Verify the UEFI System Table
 +*/
 +   if (efi.systab == NULL)
 +   panic(Whoa! Can't find UEFI system table.\n);
 +   if (efi.systab-hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
 +   panic(Whoa! UEFI system table signature incorrect\n);
 +   if ((efi.systab-hdr.revision  16) == 0)
 +   pr_warn(Warning: UEFI system table version %d.%02d, expected 
 1.00 or greater\n,
 +   efi.systab-hdr.revision  16,
 +   efi.systab-hdr.revision  0x);
 +
 +   /* Show what we know for posterity */
 +   c16 = (efi_char16_t *)early_memremap(efi.systab-fw_vendor,
 +   sizeof(vendor));
 +   if (c16) {
 +   for (i = 0; i  (int) sizeof(vendor) - 1  *c16; ++i)
 +   vendor[i] = c16[i];
 +   vendor[i] = '\0';
 +   }
 +
 +   pr_info(UEFI v%u.%.02u by %s\n,
 +   efi.systab-hdr.revision  16,
 +   efi.systab-hdr.revision  0x, vendor);
 +
 +   retval = efi_config_init(NULL);
 +   if (retval == 0)
 +   set_bit(EFI_CONFIG_TABLES, arm_uefi_facility);

Does this actually need to be atomic?

 +
 +   early_iounmap(c16, sizeof(vendor));
 +   early_iounmap(efi.systab,  sizeof(efi_system_table_t));
 +
 +   return retval;
 +}
 +
 +static __init int is_discardable_region(efi_memory_desc_t *md)
 +{
 +#ifdef KEEP_ALL_REGIONS
 +   return 0;
 +#endif

Maybe just have a dummy is_discardable_region in this case, like we usually
do instead of inlining the #ifdef?

 +   if (md-attribute  EFI_MEMORY_RUNTIME)
 +   return 0;
 +
 +   switch (md-type) {
 +#ifdef KEEP_BOOT_SERVICES_REGIONS
 +   case EFI_BOOT_SERVICES_CODE:
 +   case EFI_BOOT_SERVICES_DATA:
 +#endif
 +   /* Keep tables around for any future kexec operations */
 +   case EFI_ACPI_RECLAIM_MEMORY:
 +   return 0;
 +   /* Preserve */
 +   case EFI_RESERVED_TYPE:
 +   return 0;
 +   }
 +
 +   return 1;
 +}

[...]

 +int __init uefi_memblock_arm_reserve_range(void)
 +{
 +   if (!of_scan_flat_dt(fdt_find_uefi_params, NULL))
 +   return 0;
 +
 +   set_bit(EFI_BOOT, arm_uefi_facility);

Similar comment wrt atomicity here (and in the rest of this patch).

 +   uefi_init();
 +
 +   remove_regions();
 +
 +   return 0;
 +}
 +
 +/*
 + * Disable instrrupts, enable idmap and disable caches.

interrupts

 + */
 +static void __init phys_call_prologue(void)
 +{
 +   local_irq_disable();
 +
 +   /* Take out a flat memory mapping. */
 +   setup_mm_for_reboot();
 +
 +   /* Clean and invalidate caches */
 +   flush_cache_all();
 +
 +   /* Turn 

Re: arm/arm64 UEFI boot protocol

[Will Deacon]

Hi Leif,

On Mon, Nov 11, 2013 at 05:43:21PM +, Leif Lindholm wrote:
 We currently have a few sets of patches floating around, for UEFI
 runtime services, UEFI stub and GRUB support for Linux/UEFI on
 arm/arm64.
 
 The last version of Documentation/arm/uefi.txt I sent out raised a few
 questions with regards to the boot protocol between UEFI and Linux, and
 there is also upcoming support for ACPI which could use a few
 clarifications in this area.
 
 So, rather than sending out complete sets of patches for all these until
 consensus is reached, below is my proposed suggestion for a
 Documentation/arm/uefi.txt, to be shared for arm and arm64. If noone has
 strong objections to this, we can quickly send updated (and hopefully
 final-ish) versions of these sets.

In the absence of code, I'll play English teacher then :)

 UEFI, the Unified Extensible Firmware Interface is a specification

Interface, is

 governing the behaviours of compatible firmware interfaces.
 It is maintained by the UEFI Forum - http://www.uefi.org/.
 Support for the AArch32 (arm) architecture was added in version 2.3 of
 the specification, and support for AAaarch64 (arm64) was added in
 version 2.4.

AArch64

 UEFI is an evolution of its predecessor 'EFI', so the terms EFI and
 EFI are used somewhat interchangeably in this document and associated

UEFI

 source code. As a rule, anything new uses 'UEFI', whereas 'EFI' refers
 to legacy code or specifications.  
 
 UEFI support in Linux 
   
 = 
   
 Booting on a platform with firmware compliant with the UEFI
 specification makes it possible for the kernel to support additional
 features:
 - UEFI Runtime Services
 - Retrieving various configuration information through the standardised
   interface of UEFI configuration tables. (ACPI, SMBIOS, ...)
 
 For actually enabling [U]EFI support, enable:
 - CONFIG_EFI=y
 - CONFIG_EFI_VARS=y or m
 
 UEFI stub
 =
 The stub is a feature that turns the Image/zImage/bzImage into a valid

We don't support bzImage for arm or arm64.

 UEFI PE/COFF executable, including a loader application that makes it
 possible to load the kernel directly from the UEFI shell, boot menu, or
 one of the lightweight bootloaders like Gummiboot or rEFInd. 
 The kernel image built with stub support remains a valid kernel image
 for booting in the legacy fashion.

By legacy, you mean non-UEFI rather than EFI? I consider UEFI as a choice
rather than the future ;p

 UEFI kernel support on ARM
 ==

Wait, isn't this all under Documentation/arm/? You could probably combine
some of these sections together, since the scope is really tied to ARM here.

 The implementation depends on receiving information about the UEFI
 environment in a Flattened Device Tree (FDT) - so is only available with
 CONFIG_OF.

You could mention this in your earlier list of dependencies.

 UEFI support also depends on early_memremap().

Why is that worthy of note? Can a user even turn that off?

 UEFI kernel support on the ARM architectures (arm and arm64) is only
 available when boot is performed through the stub.
 
 When booting in UEFI mode, the kernel ignores any memory nodes in the
 DT, and instead reads the UEFI memory map. To prevent confusion, the
 stub deletes any memory nodes from a provided DT.
 
 The stub populates the FDT /chosen node with, and the kernel scans for
 the following parameters:

Sentence doesn't read very well. Maybe stick some brackets around (and the
kernel scans for)?

 __
 Name  | Size   | Description
 
 linux,uefi-system-table   | 64-bit | Physical address of the UEFI System 
 Table.
 
 linux,uefi-mmap   | 64-bit | Physical address of the UEFI memory map,
   || populated by the UEFI GetMemoryMap() 
 call.
 
 linux,uefi-mmap-size  | 32-bit | Size in bytes of the UEFI memory map
   || pointed to in previous entry.
 
 linux,uefi-mmap-desc-size | 32-bit | Size in bytes of each entry in the UEFI
   || memory map.

Do we actually need to define these sizes here, or can they be dealt with
using the usual #address-cells property? Also, I think you should describe
the binding in a separate document somewhere under Documentation/devicetree,
then cross-reference it from here.

 
 linux,uefi-stub-kern-ver  | string | Copy of