Re: [PATCH v3 0/6] efi/x86: add support for generic EFI mixed mode boot

[Arvind Sankar <nivedita@alum.mit.edu>]

On Mon, Mar 02, 2020 at 01:14:50PM +0100, Ard Biesheuvel wrote:
> On Mon, 2 Mar 2020 at 00:03, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> >
> > On Sun, Mar 01, 2020 at 11:56:44PM +0100, Ard Biesheuvel wrote:
> > > On Sun, 1 Mar 2020 at 23:01, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > > >
> > > > On Sun, Mar 01, 2020 at 10:36:05PM +0100, Ard Biesheuvel wrote:
> > > > > On Sun, 1 Mar 2020 at 21:54, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > > > > > > I see this in the memory map
> > > > > > >
> > > > > > > [    0.000000] efi: mem47: [Conventional Memory|   |  |  |  |  |  |  |
> > > > > > >  |   |WB|WT|WC|UC] range=[0x0000000100000000-0x000000013fffffff]
> > > > > > > (1024MB)
> > > > > > >
> > > > > > > so it looks like qemu-system-x86_64 puts the memory in a weird place?
> > > > > > > Or is this expected?
> > > > > >
> > > > > > Mine ended here:
> > > > > > [    0.000000] efi: mem45: [Memory Mapped I/O  |RUN|  |  |  |  |  |  |  |   |  |  |  |UC] range=[0x00000000ffc00000-0x00000000ffffffff] (4MB)
> > > > > > are you running with -m 3072 or more?
> > > > > >
> > > > >
> > > > > That is not memory, it's some mmio region
> > > > >
> > > >
> > > > Right, but it's the last (highest) range in my memory map. It was just
> > > > to illustrate that I have no addresses above 4Gb, unlike the mapping you
> > > > got, although I now see that the MMIO range is the last one printed
> > > > regardless of where RAM ends, so that wasn't quite enough. I only get
> > > > the 4g-5g mapping range if I run it with -m 4096.
> > > >
> > > > This is the tail end of the mapping I got.
> > > >
> > > > [    0.000000] efi: mem38: [Conventional Memory|   |  |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x00000000bfe00000-0x00000000bfe89fff] (0MB)
> > > > [    0.000000] efi: mem39: [Boot Data          |   |  |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x00000000bfe8a000-0x00000000bfea9fff] (0MB)
> > > > [    0.000000] efi: mem40: [Boot Code          |   |  |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x00000000bfeaa000-0x00000000bfeccfff] (0MB)
> > > > [    0.000000] efi: mem41: [Boot Data          |   |  |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x00000000bfecd000-0x00000000bfed5fff] (0MB)
> > > > [    0.000000] efi: mem42: [Boot Code          |   |  |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x00000000bfed6000-0x00000000bfef3fff] (0MB)
> > > > [    0.000000] efi: mem43: [Runtime Data       |RUN|  |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x00000000bfef4000-0x00000000bff77fff] (0MB)
> > > > [    0.000000] efi: mem44: [ACPI Memory NVS    |   |  |  |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x00000000bff78000-0x00000000bfffffff] (0MB)
> > > > [    0.000000] efi: mem45: [Memory Mapped I/O  |RUN|  |  |  |  |  |  |  |   |  |  |  |UC] range=[0x00000000ffc00000-0x00000000ffffffff] (4MB)
> > >
> > > Looks like it's the difference in machine type - I was using q35, and
> > > when I switch to the default, I can reproduce the early boot crash you
> > > sent the patch for. I don't see the other issue though.
> >
> > So you can boot successfully without hanging in SetVirtualAddressMap?
> > Weird. I'm using QEMU 4.2.0 in case that matters.
> 
> Mine is 2.11, as shipped with my Ubuntu Bionic installation (company laptop)

I think I've located the problem. kernel_map_pages_in_pgd has a bug,
when gb pages are not enabled, it can sometimes not actually map the
pages it claims to have mapped. The below fixes it, I'll post some
patches tomorrow to clean it up.

diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
index c4aedd00c1ba..a249260e71e7 100644
--- a/arch/x86/mm/pat/set_memory.c
+++ b/arch/x86/mm/pat/set_memory.c
@@ -1370,12 +1370,21 @@ static int populate_pud(struct cpa_data *cpa, unsigned long start, p4d_t *p4d,
 	/*
 	 * Map everything starting from the Gb boundary, possibly with 1G pages
 	 */
-	while (boot_cpu_has(X86_FEATURE_GBPAGES) && end - start >= PUD_SIZE) {
-		set_pud(pud, pud_mkhuge(pfn_pud(cpa->pfn,
-				   canon_pgprot(pud_pgprot))));
+	while (end - start >= PUD_SIZE) {
+		if (boot_cpu_has(X86_FEATURE_GBPAGES)) {
+			set_pud(pud, pud_mkhuge(pfn_pud(cpa->pfn,
+					   canon_pgprot(pud_pgprot))));
+			cpa->pfn  += PUD_SIZE >> PAGE_SHIFT;
+		} else {
+			if (pud_none(*pud))
+				if (alloc_pmd_page(pud))
+					return -1;
+			if (populate_pmd(cpa, start, start + PUD_SIZE,
+					 PUD_SIZE >> PAGE_SHIFT, pud, pgprot) < 0)
+				return cur_pages;
+		}
 
 		start	  += PUD_SIZE;
-		cpa->pfn  += PUD_SIZE >> PAGE_SHIFT;
 		cur_pages += PUD_SIZE >> PAGE_SHIFT;
 		pud++;
 	}