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

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

[Roel Kluin]

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

Signed-off-by: Roel Kluin <12o3l@tiscali.nl>
---
diff --git a/arch/ia64/kernel/efi.c b/arch/ia64/kernel/efi.c
index 3f7ea13..af925ab 100644
--- a/arch/ia64/kernel/efi.c
+++ b/arch/ia64/kernel/efi.c
@@ -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;

Re: [PATCH] iounmap after ioremap in efi_enter_virtual_mode, file arch/ia64/kernel/efi.c

[Simon Horman]

On Sun, Nov 04, 2007 at 12:30:14AM +0100, Roel Kluin wrote:
> I am less certain about this one, so please review
> --
> Iounmap when EFI won't switch to virtual mode
> 
> Signed-off-by: Roel Kluin <12o3l@tiscali.nl>
> ---
> diff --git a/arch/ia64/kernel/efi.c b/arch/ia64/kernel/efi.c
> index 3f7ea13..af925ab 100644
> --- a/arch/ia64/kernel/efi.c
> +++ b/arch/ia64/kernel/efi.c
> @@ -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;
> -
> To unsubscribe from this list: send the line "unsubscribe linux-ia64" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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/

Re: [PATCH] iounmap after ioremap in efi_enter_virtual_mode, file

[Roel Kluin]

Simon Horman wrote:
> On Sun, Nov 04, 2007 at 12:30:14AM +0100, Roel Kluin wrote:
>> I am less certain about this one, so please review
>> --
>> Iounmap when EFI won't switch to virtual mode
>>

> 
> 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?

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;
---

> 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?
> 

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

Re: [PATCH] iounmap after ioremap in efi_enter_virtual_mode, file arch/ia64/kernel/efi.c

[Simon Horman]

On Mon, Nov 05, 2007 at 09:45:27AM +0100, Roel Kluin wrote:
> Simon Horman wrote:
> > On Sun, Nov 04, 2007 at 12:30:14AM +0100, Roel Kluin wrote:
> >> I am less certain about this one, so please review
> >> --
> >> Iounmap when EFI won't switch to virtual mode
> >>
> 
> > 
> > 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?
> 
> 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))

I think that you also need || (md->attribute & EFI_MEMORY_WT)

or in other words
			if ((md->attribute & EFI_MEMORY_RUNTIME) &&
			    (md->attribute &
			     EFI_MEMORY_UC|EFI_MEMORY_WC|EFI_MEMORY_WT))

> +				iounmap(md->virt_addr);
> +		}
>  		printk(KERN_WARNING "warning: unable to switch EFI into virtual mode "
>  		       "(status=%lu)\n", status);
>  		return;

Otherwise, yes, that is what I was thinking.

> ---
> 
> > 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?
> > 
> 
> Like this?

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.

> 
> ---
>  	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
> -
> To unsubscribe from this list: send the line "unsubscribe linux-ia64" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Horms, California Edition