From mboxrd@z Thu Jan 1 00:00:00 1970 From: Doug Goldstein Subject: Re: [PATCH v11 07/13] x86: add multiboot2 protocol support for EFI platforms Date: Wed, 11 Jan 2017 14:20:15 -0600 Message-ID: <66b5b99e-14fc-5125-492e-83a692f816a9@cardoe.com> References: <1480976718-12198-1-git-send-email-daniel.kiper@oracle.com> <1480976718-12198-8-git-send-email-daniel.kiper@oracle.com> <96e86b22-8940-dce2-d1b9-92d1c132ad0e@cardoe.com> <20170111194714.GZ32675@olila.local.net-space.pl> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1340091286629032131==" Return-path: Received: from mail6.bemta6.messagelabs.com ([193.109.254.103]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1cRPNg-00075v-Bn for xen-devel@lists.xenproject.org; Wed, 11 Jan 2017 20:20:24 +0000 Received: by mail-io0-f193.google.com with SMTP id m98so296100iod.2 for ; Wed, 11 Jan 2017 12:20:22 -0800 (PST) In-Reply-To: <20170111194714.GZ32675@olila.local.net-space.pl> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Daniel Kiper Cc: jgross@suse.com, sstabellini@kernel.org, andrew.cooper3@citrix.com, pgnet.dev@gmail.com, ning.sun@intel.com, julien.grall@arm.com, jbeulich@suse.com, xen-devel@lists.xenproject.org, qiaowei.ren@intel.com, gang.wei@intel.com, fu.wei@linaro.org List-Id: xen-devel@lists.xenproject.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --===============1340091286629032131== Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="vhFkEphsIlNr0bG5fDVtnXRP1eC7VjM9d" This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --vhFkEphsIlNr0bG5fDVtnXRP1eC7VjM9d Content-Type: multipart/mixed; boundary="fXk17USFOffSxc4ptrl7FpsrSUARvhIUE"; protected-headers="v1" From: Doug Goldstein To: Daniel Kiper Cc: xen-devel@lists.xenproject.org, andrew.cooper3@citrix.com, fu.wei@linaro.org, gang.wei@intel.com, jbeulich@suse.com, jgross@suse.com, julien.grall@arm.com, konrad.wilk@oracle.com, ning.sun@intel.com, pgnet.dev@gmail.com, qiaowei.ren@intel.com, sstabellini@kernel.org Message-ID: <66b5b99e-14fc-5125-492e-83a692f816a9@cardoe.com> Subject: Re: [PATCH v11 07/13] x86: add multiboot2 protocol support for EFI platforms References: <1480976718-12198-1-git-send-email-daniel.kiper@oracle.com> <1480976718-12198-8-git-send-email-daniel.kiper@oracle.com> <96e86b22-8940-dce2-d1b9-92d1c132ad0e@cardoe.com> <20170111194714.GZ32675@olila.local.net-space.pl> In-Reply-To: <20170111194714.GZ32675@olila.local.net-space.pl> --fXk17USFOffSxc4ptrl7FpsrSUARvhIUE Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable On 1/11/17 1:47 PM, Daniel Kiper wrote: > On Tue, Jan 10, 2017 at 02:51:27PM -0600, Doug Goldstein wrote: >> On 1/9/17 7:37 PM, Doug Goldstein wrote: >>> On 12/5/16 4:25 PM, Daniel Kiper wrote: >> >>>> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot= =2Eh >>>> index 62c010e..dc857d8 100644 >>>> --- a/xen/arch/x86/efi/efi-boot.h >>>> +++ b/xen/arch/x86/efi/efi-boot.h >>>> @@ -146,6 +146,8 @@ static void __init efi_arch_process_memory_map(E= FI_SYSTEM_TABLE *SystemTable, >>>> { >>>> struct e820entry *e; >>>> unsigned int i; >>>> + /* Check for extra mem for mbi data if Xen is loaded via multib= oot2 protocol. */ >>>> + UINTN extra_mem =3D efi_enabled(EFI_LOADER) ? 0 : (64 << 10); >>> >>> Just wondering where the constant came from? And if there should be a= >>> little bit of information about it. To me its just weird to shift 64.= >> >> Its the size of the stack used in the assembly code. >=20 > No, it is trampoline region size. trampoline + stack in head.S We take the address where we're going to copy the trampoline and set the stack to 0x10000 past it. >=20 >>>> /* Populate E820 table and check trampoline area availability. = */ >>>> e =3D e820map - 1; >>>> @@ -168,7 +170,8 @@ static void __init efi_arch_process_memory_map(E= FI_SYSTEM_TABLE *SystemTable, >>>> /* fall through */ >>>> case EfiConventionalMemory: >>>> if ( !trampoline_phys && desc->PhysicalStart + len <=3D= 0x100000 && >>>> - len >=3D cfg.size && desc->PhysicalStart + len > c= fg.addr ) >>>> + len >=3D cfg.size + extra_mem && >>>> + desc->PhysicalStart + len > cfg.addr ) >>>> cfg.addr =3D (desc->PhysicalStart + len - cfg.size)= & PAGE_MASK; >>> >>> So this is where the current series blows up and fails on real hardwa= re. >> >> Honestly this was my misunderstanding and this shouldn't ever be used = to >> get memory for the trampoline. This also has the bug in it that it nee= ds >> to be: >> >> ASSERT(cfg.size > 0); >> cfg.addr =3D (desc->PhysicalStart + len - (cfg.size + extra_mem) & PAG= E_MASK; >=20 > As I said earlier. This extra_mem stuff is (maybe) wrong and should be = fixed > in one way or another. Hmmm... It looks OK. I will double check it beca= use > I do not looked at this code long time and maybe I am missing something= =2E cfg.size needs to be the size of the trampolines + stack. >>> No where in the EFI + MB2 code path is cfg.size ever initialized. Its= >>> only initialized in the straight EFI case. The result is that cfg.add= r >>> is set to the section immediately following this. Took a bit to >>> trackdown because I checked for memory overlaps with where Xen was >>> loaded and where it was relocated to but forgot to check for overlaps= >>> with the trampoline code. This is the address where the trampoline ju= mps >>> are copied. >>> >>> Personally I'd like to see an ASSERT added or the code swizzled aroun= d >>> in such a way that its not possible to get into a bad state. But this= is >>> probably another patch series. >>> >>>> /* fall through */ >>>> case EfiLoaderCode: >>>> @@ -210,12 +213,14 @@ static void *__init efi_arch_allocate_mmap_buf= fer(UINTN map_size) >>>> >>>> static void __init efi_arch_pre_exit_boot(void) >>>> { >>>> - if ( !trampoline_phys ) >>>> - { >>>> - if ( !cfg.addr ) >>>> - blexit(L"No memory for trampoline"); >>>> + if ( trampoline_phys ) >>>> + return; >>>> + >>>> + if ( !cfg.addr ) >>>> + blexit(L"No memory for trampoline"); >>>> + >>>> + if ( efi_enabled(EFI_LOADER) ) >>>> relocate_trampoline(cfg.addr); >> >> Why is this call even here anymore? Its called in >> efi_arch_memory_setup() already. If it was unable to allocate memory >> under the 1mb region its just going to trample over ANY conventional >> memory region that might be in use. >=20 > Trampoline is relocated in xen/arch/x86/boot/head.S. For the MB2/MB case. But for the straight EFI case its called in efi_arch_memory_setup() and then you've added the wrapper of efi_enabled(EFI_LOADER) which in theory would have it called again in the straight EFI case if trampoline_phys isn't set and cfg.addr is set. >=20 >>>> - } >>>> } >>>> >>>> static void __init noreturn efi_arch_post_exit_boot(void) >>>> @@ -653,6 +658,43 @@ static bool_t __init efi_arch_use_config_file(E= FI_SYSTEM_TABLE *SystemTable) >>>> >>>> static void efi_arch_flush_dcache_area(const void *vaddr, UINTN siz= e) { } >>>> >>>> +paddr_t __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TA= BLE *SystemTable) >>>> +{ >>>> + EFI_GRAPHICS_OUTPUT_PROTOCOL *gop; >>>> + UINTN cols, gop_mode =3D ~0, rows; >>>> + >>>> + __set_bit(EFI_BOOT, &efi_flags); >>>> + __set_bit(EFI_RS, &efi_flags); >>>> + >>>> + efi_init(ImageHandle, SystemTable); >>>> + >>>> + efi_console_set_mode(); >>>> + >>>> + if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode, >>>> + &cols, &rows) =3D=3D EFI_SUCCESS ) >>>> + efi_arch_console_init(cols, rows); >>>> + >>>> + gop =3D efi_get_gop(); >>>> + >>>> + if ( gop ) >>>> + gop_mode =3D efi_find_gop_mode(gop, 0, 0, 0); >>>> + >>>> + efi_arch_edd(); >>>> + efi_arch_cpu(); >>>> + >>>> + efi_tables(); >>>> + setup_efi_pci(); >>>> + efi_variables(); >>> >>> This is probably where you missed the call to "efi_arch_memory_setup(= );" >>> that gave me hell above. >> >> Well it turns out that calling "efi_arch_memory_setup()" isn't correct= >> because it also messes with the page tables AND also does the trampoli= ne >=20 > Yep. >=20 >> relocation. Which after this call finishes then it does the trampoline= >> relocations in assembly. The code currently makes the assumption it ca= n >=20 > I am not sure what do you mean here. >=20 >> use any conventional memory range below 1mb (and unfortunately does th= e >> math incorrectly and instead uses the region following the conventiona= l >=20 > Which code? Which math? The code where cfg.size =3D 0 and the extra_mem was missing. >=20 >> memory region). You need to use AllocatePages() otherwise you are >> trampling memory that might have been allocated by the bootloader or a= ny >=20 > Bootloader code/data should be dead here. Correct. Unfortunately on my Lenovo laptop and my Intel NUCs I can't currently call ExitBootServices and a timer that iPXE has wired up has some memory reserved down there and it was getting trampled. The real answer is that we need to fix up stock Xen to be able to always call EBS.= >=20 >> multiboot modules (e.g. tboot) prior to Xen. >=20 > How come? >=20 >> I'm just going to write a patch to fix the issues that I've found thus= >> far. I believe at this point there is something wrong with the page >> tables because setting cr0 for the non-0 CPU in >> trampoline_protmode_entry causes the machine to reboot. >=20 > I have a feeling that problem lays somewhere else. Probably far before > trampoline_protmode_entry. >=20 >> I've also found where we have the possibility to call >> relocate_trampoline() twice in the EFI case. Its protected by a check = to >> trampoline_phys but I'm not sure why we even have the code there to be= >> able to do so. >=20 > Consider case when boot services use memory below 1 MiB. >=20 > Thank you for testing my patch series. >=20 > Daniel >=20 So do we really need to go down below 1MiB? We're never going into 16-bit mode. Unless the other CPUs are starting up in 16-bit mode. I'll keep whacking away at this until it lands on any piece of hardware I've got. --=20 Doug Goldstein --fXk17USFOffSxc4ptrl7FpsrSUARvhIUE-- --vhFkEphsIlNr0bG5fDVtnXRP1eC7VjM9d Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG/MacGPG2 v2.0 iQJ8BAEBCgBmBQJYdpOCXxSAAAAAAC4AKGlzc3Vlci1mcHJAbm90YXRpb25zLm9w ZW5wZ3AuZmlmdGhob3JzZW1hbi5uZXRBNTM5MEQ2RTNFMTkyNzlCNzVDMzIwOTVB MkJDMDNEQzg3RUQxQkQ0AAoJEKK8A9yH7RvUpvoP/2lHRzPXzzmofJ+NLtv76X1X QY9n9r3Y6Evi30Y7FbyoEp8+v5qCSmZfko/JxogTZm8LKj2M81Mnwhy4zN0shDzu 3iRPsbxkJlo1EQM72pP7DgVstf7tuU7NFHZD9j40tvAssWGMqspmy2V1NBVTiYte j/IVGzwPvNTNiWlZpeKs8MOmmhdQWlziSvuV9duX7NBeoLfzpF0/+8IvhpARoYnS 323qr4JSd56BTwdH/D75ujBKWzSGH6RVv5WNLiRvjSV+PHvJ3d1qP0E6+PScIt2A unOMiCOKaKvMExei9D8CTviXqxCI6SFDzYAw8lq42EkOjGQq0N5itPe6szz+GrAA RDqvvslnLwcoJAPtLIl/j3WJNM+/FysUok7I/1c4MxWf61aFrBlvLmGR8ZToD330 o8Z8E4hHexeZwi+x2PcCjxQd8fXvTFpciLNplpZTW9QZSr1OtVN3kJEbFAhGvBoS Q324p3vNehgSxSh6CHUbRQaglNQSJhc8A673/GfwJZ5N7MZ83HAcRONe7i0doRFN 2RbFsQEXOxVBkBlLQMeRmd5VGzuhQ6gxPAdkE/Txf3585Sgw4VhrKfGOKKYShdRV GCNxRlpyqtItx/h1Qm0RjoxMfCwycGzlLdcKhMY2ESe2DkhnkLoXTfFEYt/a01qP m6aXSfAZ4eJ2srwiNgLw =rP2y -----END PGP SIGNATURE----- --vhFkEphsIlNr0bG5fDVtnXRP1eC7VjM9d-- --===============1340091286629032131== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v cmcveGVuLWRldmVsCg== --===============1340091286629032131==--