Re: [PATCH 2/3] Update uv_bios_call to use efi_call_virt_generic
On Thu, Jun 02, 2016 at 04:14:03PM -0500, Alex Thorlton wrote: > On Thu, Jun 02, 2016 at 08:45:47PM +0100, Matt Fleming wrote: > > 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? > > It was intended. The motivation is so that we can use the same > "dereference the pointer inside the macro" stuff that we do with the > efi.systab->runtime pointer. IINM, the reason it works is because we do > > /* > * Make sure the 1:1 mappings are present as a catch-all for b0rked > * firmware which doesn't update all internal pointers after switching > * to virtual mode and would otherwise crap on us. > */ > __map_region(md, md->phys_addr); > > Inside of efi_map_region, so we know we'll have that physical address > mapped into the EFI page table. > > Upon review, I'm wondering if the correct thing to do here is to > update that pointer during the switch to virtual mode, to avoid the > b0rkage mentioned in the above comment. Realizing after I typed this that the b0rkage in question is firmware b0rkage, so the comment there doesn't really apply to this situation. > Either way, the straigh tab->function dereference should work while > using the EFI page table, but I do wonder if that tab->function address > should have been updated to the __va() version of itself before we reach > this point. I played with this some and it's not working the way I expected. In thinking about it, I believe the reason is fairly obvious. We have already switched to the EFI page table when we try to dereference the pointer, so if we have some 88 address, it won't be mapped (this is somewhat related to the situation that got us here in the first place). I think, in order for this to work, tab->function has to be either the physical address, or the address in the ffef range that also gets mapped in during efi_map_region. My brain is getting close to fried for the day, so this might be utter nonsense, but it's sounding pretty reasonable to me right now. We'll see how I feel about it tomorrow :) Let me know what you think! - Alex
Re: [PATCH 2/3] Update uv_bios_call to use efi_call_virt_generic
On Thu, Jun 02, 2016 at 08:45:47PM +0100, Matt Fleming wrote: > 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? It was intended. The motivation is so that we can use the same "dereference the pointer inside the macro" stuff that we do with the efi.systab->runtime pointer. IINM, the reason it works is because we do /* * Make sure the 1:1 mappings are present as a catch-all for b0rked * firmware which doesn't update all internal pointers after switching * to virtual mode and would otherwise crap on us. */ __map_region(md, md->phys_addr); Inside of efi_map_region, so we know we'll have that physical address mapped into the EFI page table. Upon review, I'm wondering if the correct thing to do here is to update that pointer during the switch to virtual mode, to avoid the b0rkage mentioned in the above comment. Either way, the straigh tab->function dereference should work while using the EFI page table, but I do wonder if that tab->function address should have been updated to the __va() version of itself before we reach this point. Maybe you can comment on that? - Alex
Re: [PATCH 2/3] Update uv_bios_call to use efi_call_virt_generic
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: x...@kernel.org
> Cc: linux-...@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?
[PATCH 2/3] Update uv_bios_call to use efi_call_virt_generic
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: x...@kernel.org
Cc: linux-...@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);
--
1.8.5.6
