Re: [PATCH 3/5] x86_64 EFI support -v3: EFI runtime support

["Huang, Ying" <ying.huang@intel.com>]

On Mon, 2007-07-30 at 22:30 -0600, Eric W. Biederman wrote:
> > This patch adds runtime service support for EFI x86_64 system.
> >
> > The EFI support for emergency_restart and RTC clock is added. The EFI
> > based implementation and legacy BIOS or CMOS based implementation are
> > put in separate functions and are chosen based on the value of
> > efi_enabled.
> 
> The patches to the reboot path are wrong (see below).
> 
> Why do we need to do this anyway?  Why do we need any EFI runtime
> support?  We already have ACPI.  Isn't that good enough to abstract
> out the runtime parts of the hardware?
> 
> Why do we need to replace working code that directly talks to the
> architecturally defined hardware, with firmware calls?
> 
> What is the point?  What is the advantage?
> 
> The disadvantage of having more code to maintain and having
> to deal with more BIOS bugs should be obvious.

The reboot EFI runtime service is needed because otherwise we have no
way to tell firmware whether a warm or cold reboot is requested. The
"0x472" protocol is used by legacy BIOS, but not by EFI based firmware.
For EFI based firmware, the parameter to runtime service call is the
only way to tell firmware whether a warm or cold reboot is requested.

That is not implemented by current EFI support patch yet, but we will
add it in the next version.

> > Signed-off-by: Chandramouli Narayanan <mouli@linux.intel.com>
> > Signed-off-by: Huang Ying <ying.huang@intel.com>
> >
> > ---
> >
> >  reboot.c |   11 ++++++++++-
> >  time.c   |   47 +++++++++++++++++++++++++++++++----------------
> >  2 files changed, 41 insertions(+), 17 deletions(-)
> >
> > Index: linux-2.6.23-rc1/arch/x86_64/kernel/reboot.c
> > ===================================================================
> > --- linux-2.6.23-rc1.orig/arch/x86_64/kernel/reboot.c 2007-07-23
> > 04:41:00.000000000 +0800
> > +++ linux-2.6.23-rc1/arch/x86_64/kernel/reboot.c 2007-07-30 09:26:56.000000000
> > +0800
> > @@ -9,6 +9,7 @@
> >  #include <linux/pm.h>
> >  #include <linux/kdebug.h>
> >  #include <linux/sched.h>
> > +#include <linux/efi.h>
> >  #include <asm/io.h>
> >  #include <asm/delay.h>
> >  #include <asm/hw_irq.h>
> > @@ -117,7 +118,7 @@
> >  	pci_iommu_shutdown();
> >  }
> >  
> > -void machine_emergency_restart(void)
> > +static inline void bios_emergency_restart(void)
> >  {
> >  	int i;
> >  
> > @@ -145,6 +146,14 @@
> >  	}      
> >  }
> >  
> > +void machine_emergency_restart(void)
> > +{
> > +	if (efi_enabled)
> > +		efi_emergency_restart();
> > +	else
> > +		bios_emergency_restart();
> > +}
> > +
> 
> A EFI is a bios so naming the current machine_emergency_restart bios_emergency_restart is
> a misnomer, especially since pounds the hardware not the firmware.
> Second we already have a perfectly capable mechanism in the
> reboot_type variable so you should just need to add one more type and
> handle this properly. 

Yes. Reboot via EFI should be implemented as a new type of
"reboot_type".

Best Regards,
Huang Ying