From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932983AbcFBVZT (ORCPT ); Thu, 2 Jun 2016 17:25:19 -0400 Received: from relay3.sgi.com ([192.48.152.1]:50050 "EHLO relay.sgi.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S932681AbcFBVZS (ORCPT ); Thu, 2 Jun 2016 17:25:18 -0400 Date: Thu, 2 Jun 2016 16:25:15 -0500 From: Alex Thorlton To: Matt Fleming Cc: Alex Thorlton , 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 3/3] Update efi_thunk to use efi_call_virt_generic Message-ID: <20160602212515.GE242721@stormcage.americas.sgi.com> References: <1463598701-178201-1-git-send-email-athorlton@sgi.com> <1463598701-178201-4-git-send-email-athorlton@sgi.com> <20160602201949.GL2658@codeblueprint.co.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160602201949.GL2658@codeblueprint.co.uk> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jun 02, 2016 at 09:19:49PM +0100, Matt Fleming wrote: > On Wed, 18 May, at 02:11:41PM, Alex Thorlton wrote: > > +#define arch_efi_call_virt(p, f, ...) \ > > +({ \ > > + u32 func = runtime_service32(f); \ > > + efi64_thunk(func, __VA_ARGS__); \ > > +}) > > + > > This isn't correct because you're turning the runtime decision of > whether we're executing the thunking code into a build time one. Ahh, yep, you're absolutely correct. That's not what I intended to do, but that's definitely the effect that this change has. > Would something like this work instead? It's not as neat as your > suggestion but it's a damn sight better than what we have today. > > --- > > diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c > index 6e7242be1c87..b976084e56ef 100644 > --- a/arch/x86/platform/efi/efi_64.c > +++ b/arch/x86/platform/efi/efi_64.c > @@ -469,18 +469,13 @@ extern efi_status_t efi64_thunk(u32, ...); > unsigned long flags; \ > u32 func; \ > \ > - efi_sync_low_kernel_mappings(); \ > local_irq_save(flags); \ > - \ > - efi_scratch.prev_cr3 = read_cr3(); \ > - write_cr3((unsigned long)efi_scratch.efi_pgt); \ > - __flush_tlb_all(); \ > + arch_efi_call_virt_setup(); \ > \ > func = runtime_service32(f); \ > __s = efi64_thunk(func, __VA_ARGS__); \ > \ > - write_cr3(efi_scratch.prev_cr3); \ > - __flush_tlb_all(); \ > + arch_efi_call_virt_teardown(); \ > local_irq_restore(flags); \ > \ > __s; \ This looks good to me. We're at least making use of the arch_efi_call_virt_* stuff where possible, and only using the special thunk code where necessary. I think it's a good middle ground between the two approaches (especially considering the fact that mine won't work :) I will re-work that last patch to include this change instead of my original, broken one. Thanks, Matt! - Alex