iounmap after ioremap in efi_enter_virtual_mode, file arch/ia64/kernel/efi.c

I am less certain about this one, so please review --
Iounmap when EFI won't switch to virtual mode

---

index 3f7ea13..af925ab 100644
@@ -585,6 +585,8 @@ efi_enter_virtual_mode (void)
 			       efi_desc_size, ia64_boot_param->efi_memdesc_version,
 			       ia64_boot_param->efi_memmap);
 	if (status != EFI_SUCCESS) {
+		if ((md->attribute & EFI_MEMORY_UC) || (md->attribute & EFI_MEMORY_WC))
+			iounmap(md->virt_addr);
 		printk(KERN_WARNING "warning: unable to switch EFI into virtual mode "
 		       "(status=%lu)\n", status);
 		return;

Hi Roel,

I'm not really sure what the intention of this patch is. But I'm pretty sure its not doing what you want it to do.

I guess that you wish to reverse all the calls to ioremap() that are made in efi_enter_virtual_mode(). ioremap() is called during iterating through efi_map_start, but you only seem to call iounmap on whatever md happens to be set to at the end of the iteration. Surely you need to run through efi_map_start again?

The next thing that I wonder about is weather calling iounmap() actually reverses ioremap() in this case. Though now that I look at it and see that basically iounmap() will do nothing in this case, which seems appropriate as in this case ioremap() ends up being:

return (void __iomem *) (__IA64_UNCACHED_OFFSET | phys_addr)

Its probably not to much of a bother that all the md->virt_addr have been mangled and stay mangled. Though if we are concerned about such things, perhaps it would be cleaner to zero them on error?

Some other issues that aren't part of your patch but are related.

1. Without the phys_efi patches that I posted to the linux-ia64 about a year ago, if EFI fails to switch to virtual mode then all SAL calls will fail. Which in turn means that the kernel will fail to boot (unless I am mistaken and some machines don't make SAL calls directly from the kernel).

This is because currently the kernel doesn't have any mechanism to make SAL calls in physical mode. My patches fixed this and introduced a switch, to force efi to stay in physical mode, for testing purpuses. But there was no interest in the code. Actually there were some suggestions that some machines simply couldn't perform some opperations with EFI in physical mode.

Basically this means that the will fail to boot. That is, unless I am mistaken and some machines don't make SAL calls directly from the kernel.

Given that the kernel can't function with EFI in physical mode (without the phys_efi patches), I really have to conclude that in fact EFI never fails to switch itself into virtual mode. Otherwise there would be machines out there that simply wouldn't boot.

This being the case, there doesn't seem to be a whole lot of point in making sure the error path cleans up correctly. In fact, perhaps the error path should be removed all together or just changed to BUG("unable to switch EFI into virtual mode");

2. It seems to me that the loop in efi_enter_virtual_mode() could be rewritten as the following without changing its behaviour, other than debugging output. I have not tested this theory.

   for (p = efi_map_start; p < efi_map_end; p += efi_desc_size) {
	md = p;
	if (! (md->attribute & EFI_MEMORY_RUNTIME))
		continue;
	md->virt_addr = (u64) ioremap(md->phys_addr, 0);

}

Horms
  H: http://www.vergenet.net/~horms/
  W: http://www.valinux.co.jp/en/

Yes, you are right, this was what I had in mind:
--
if (status != EFI_SUCCESS) {
+ for (p = efi_map_start; p < efi_map_end; p += efi_desc_size) {
+ md = p;
+ if ((md->attribute & EFI_MEMORY_RUNTIME) &&
+ (md->attribute & EFI_MEMORY_UC) ||
+ (md->attribute & EFI_MEMORY_WC))
+ iounmap(md->virt_addr);
+ }
printk(KERN_WARNING "warning: unable to switch EFI into virtual mode "
"(status=%lu)\n", status);
return;
---

Like this?

---
if (status != EFI_SUCCESS) {
+ for (p = efi_map_start; p < efi_map_end; p += efi_desc_size) {
+ md = p;
+ if ((md->attribute & EFI_MEMORY_RUNTIME) &&
+ (md->attribute & EFI_MEMORY_UC) ||
+ (md->attribute & EFI_MEMORY_WC))
+ md->virt_addr = 0;
+ }
printk(KERN_WARNING "warning: unable to switch EFI into virtual mode "
"(status=%lu)\n", status);
return;
---

I fear the remainder of your review is beyond my scope. My field of expertise is biomedical sciences :)

Roel

			if ((md->attribute & EFI_MEMORY_RUNTIME) &&
			    (md->attribute &
			     EFI_MEMORY_UC|EFI_MEMORY_WC|EFI_MEMORY_WT))

Yes, though I guess that iounmap() should also be called. And as I mentioned before, I'm not sure if doing this is worthwhile or not.

On a fresh boot its likely that md->virt_addr is initialised to 0 for each md (I ought to check the spec). But, for instance, in a second kernel botoed by kexec, virt_addr may well be non-zero, indicating the values that were set in the previous boot. Incidently efi_call_phys() really ought to fail on in that case, though it doesn't as kexec replaces it with a noop.

So I'm not sure if reseting the value to 0 has any use. Though it does feel clean to me.

Horms, California Edition