more prefetch/vga issues

Thanks to Asit's patch a while back, we've seen a lot less MCA's
from accesses to the VGA range. There still seems to be one lurking though. I've traced it down to the prefetching in free_one_pgd(). This function prefetches farther than it needs to, and can easily try to prefetch from the VGA MMIO region at 0xa0000. On an HP zx1 system, this causes an MCA if the VGA card doesn't respond.

There seem to be (at least) two solutions to this. One is to
modify mm/memory.c such that it only prefetches to the extent that it uses. This might have some performance implications, but they're likely minimal. The other alternative, is that efi_memmap_walk() could detect this situation, and ignore a page of memory. This can be a generic test, just checking for usable memory directly adjacent to MMIO. I've included diffs to illustrate each solution. I'd be interested to know which people think is the more viable alternative or if there are other potential solutions. Thanks,

Alex

--
Alex Williamson Linux Development Lab
[email protected] Hewlett Packard
970-898-9173 Fort Collins, CO

@@ -118,8 +118,11 @@ static inline void free_one_pgd(pgd_t * 
 	}
 	pmd = pmd_offset(dir, 0);
 	pgd_clear(dir);
-	for (j = 0; j < PTRS_PER_PMD ; j++) {
+	for (j = 0; j < (PTRS_PER_PMD - (PREFETCH_STRIDE/sizeof(*pmd))) ; j++) {
 		prefetchw(pmd + j + PREFETCH_STRIDE/sizeof(*pmd));
+		free_one_pmd(pmd+j);
+	}
+	for (; j < PTRS_PER_PMD ; j++) {
 		free_one_pmd(pmd+j);
 	}
 	pmd_free(pmd);

@@ -137,7 +137,7 @@ efi_memmap_walk (efi_freemem_callback_t 
 		u64 start;
 		u64 end;
 	} prev, curr;
-	void *efi_map_start, *efi_map_end, *p;
+	void *efi_map_start, *efi_map_end, *p, *p_next;
 	efi_memory_desc_t *md;
 	u64 efi_desc_size, start, end;
 

@@ -164,6 +164,19 @@ efi_memmap_walk (efi_freemem_callback_t 
 				printk("efi_memmap_walk: ignoring empty region at 0x%lx",
 				       md->phys_addr);
 				continue;
+			}
+
+			p_next = (p + efi_desc_size);
+
+			if (p_next < efi_map_end) {
+				efi_memory_desc_t *md_next = p_next;
+
+				if ((md_next->type == EFI_MEMORY_MAPPED_IO) &&
+				    (md_next->phys_addr == (md->phys_addr +
+				    (md->num_pages << 12)))) {
+
+					md->num_pages--;
+				}
 			}
 
 			curr.start = PAGE_OFFSET + md->phys_addr;

Alex> Thanks to Asit's patch a while back, we've seen a lot less
Alex> MCA's from accesses to the VGA range. There still seems to be Alex> one lurking though. I've traced it down to the prefetching in Alex> free_one_pgd(). This function prefetches farther than it Alex> needs to, and can easily try to prefetch from the VGA MMIO Alex> region at 0xa0000. On an HP zx1 system, this causes an MCA if Alex> the VGA card doesn't respond.

Again, prefetching arbitrary addresses is always valid. So, by definition, it's not the lfetch that's broken (though it may be a bad idea to do it anyhow, for performance reasons, but that's a different issue).

Alex> There seem to be (at least) two solutions to this. One is
Alex> to modify mm/memory.c such that it only prefetches to the Alex> extent that it uses. This might have some performance Alex> implications, but they're likely minimal.

We would have to show that. At the moment, we use an "asm" to do the lfetch, so it can't be predicated (we need to look into using __builtin_prefetch() instead; hopefully that one can be predicated). And on other archiectures, the performance effect of adding the check may be more noticable.

On the other hand, the major reason for doing the prefetch is to get good execve() performance on SMP, and for SMP, extraneous prefetching can be quite hurtful. So you might have a pretty good argument for not prefetching past the end of the page on all architectures. But I we'd still have to back it up with numbers.

Alex> The other
Alex> alternative, is that efi_memmap_walk() could detect this Alex> situation, and ignore a page of memory. This can be a generic Alex> test, just checking for usable memory directly adjacent to Alex> MMIO.

Yes, I think we need to put in such a test. However, it should check for any overlap of a UC/WC-only area with a "granule" that contains WB areas, where a "granule" is a 16 or 64MB page. Actually, if we put in this test, there should be less need for the 16MB granule size anymore. Of course, we may end up throwing a lot of memory that way. We should print a warning at least, so that users can see how much memory is being ignored (and why).

I think this is a reasonable workaround until we get around to fixing the <64MB memory attribute aliasing issues in a better way.

	--david