From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753047AbcFBTpw (ORCPT ); Thu, 2 Jun 2016 15:45:52 -0400 Received: from mail-wm0-f42.google.com ([74.125.82.42]:38159 "EHLO mail-wm0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751584AbcFBTpv (ORCPT ); Thu, 2 Jun 2016 15:45:51 -0400 Date: Thu, 2 Jun 2016 20:45:47 +0100 From: Matt Fleming To: Alex Thorlton Cc: linux-kernel@vger.kernel.org, Borislav Petkov , Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , Mike Travis , Russ Anderson , Dimitri Sivanich , x86@kernel.org, linux-efi@vger.kernel.org Subject: Re: [PATCH 2/3] Update uv_bios_call to use efi_call_virt_generic Message-ID: <20160602194547.GK2658@codeblueprint.co.uk> References: <1463598701-178201-1-git-send-email-athorlton@sgi.com> <1463598701-178201-3-git-send-email-athorlton@sgi.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1463598701-178201-3-git-send-email-athorlton@sgi.com> User-Agent: Mutt/1.5.24+41 (02bc14ed1569) (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 18 May, at 02:11:40PM, Alex Thorlton wrote: > Now that the efi_call_virt macro has been generalized to be able to > use EFI system tables besides efi.systab, we are able to convert our > uv_bios_call wrapper to use this standard EFI callback mechanism. > > This simple change is part of a much larger effort to recover from some > issues with the way we were mapping in some of our MMRs, and the way > that we were doing our BIOS callbacks, which were uncovered by commit > 67a9108ed431 ("x86/efi: Build our own page table structures"). > > The first issue that this uncovered was that we were relying on the EFI > memory mapping mechanism to map in our MMR space for us, which, while > reliable, was technically a bug, as it relied on "undefined" behavior in > the mapping code. > > The reason we were able to piggyback on the EFI memory mapping code to > map in our MMRs was because, previously, EFI code used the > trampoline_pgd, which shares a few entries with the main kernel pgd. It > just so happened, that the memory range containing our MMRs was inside > one of those shared regions, which kept our code working without issue > for quite a while. > > Anyways, once we discovered this problem, we brought back our original > code to map in the MMRs with commit 08914f436bdd ("x86/platform/UV: > Bring back the call to map_low_mmrs in uv_system_init"). This got our > systems a little further along, but we were still running into trouble > with our EFI callbacks, which prevented us from booting all the way up. > > Our first step towards fixing the BIOS callbacks was to get our > uv_bios_call wrapper updated to use efi_call_virt instead of the plain > efi_call. The previous patch took care of the effort needed to make > that possible. Along the way, we hit a major issue with some confusion > about how to properly pull arguments higher than number 6 off the stack > in the efi_call code, which resulted in Linus's commit 683ad8092cd2 > ("x86/efi: Fix 7-parameter efi_call()s"). > > Now that all of those issues are out of the way, we're able to make this > simple change to use the new efi_call_virt_generic in uv_bios_call which > gets our machines booting, running properly, and able to execute our > callbacks with 6+ arguments. > > Signed-off-by: Alex Thorlton > Cc: Matt Fleming > Cc: Borislav Petkov > Cc: Thomas Gleixner > Cc: Ingo Molnar > Cc: "H. Peter Anvin" > Cc: Mike Travis > Cc: Russ Anderson > Cc: Dimitri Sivanich > Cc: x86@kernel.org > Cc: linux-efi@vger.kernel.org > --- > arch/x86/platform/uv/bios_uv.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/arch/x86/platform/uv/bios_uv.c b/arch/x86/platform/uv/bios_uv.c > index 815fec6..0ae0826 100644 > --- a/arch/x86/platform/uv/bios_uv.c > +++ b/arch/x86/platform/uv/bios_uv.c > @@ -40,8 +40,7 @@ s64 uv_bios_call(enum uv_bios_cmd which, u64 a1, u64 a2, u64 a3, u64 a4, u64 a5) > */ > return BIOS_STATUS_UNIMPLEMENTED; > > - ret = efi_call((void *)__va(tab->function), (u64)which, > - a1, a2, a3, a4, a5); > + ret = efi_call_virt_generic(tab, function, (u64)which, a1, a2, a3, a4, a5); > return ret; > } > EXPORT_SYMBOL_GPL(uv_bios_call); Unless I've missed it, I didn't see an explanation in the changelog of why it's OK to switch from using __va(tab->function) to tab->function directly, which presumably is a physical address. Was that intended?