Re: [Xen-devel] [PATCH] Xen: EFI: Parse DT parameters for Xen specific UEFI

[Shannon Zhao <shannon.zhao@xxxxxxxxxx>]

On 2016年05月11日 20:27, Matt Fleming wrote:
> On Fri, 06 May, at 04:32:06PM, Shannon Zhao wrote:
>> > From: Shannon Zhao <shannon.zhao@xxxxxxxxxx>
>> > 
>> > Add a new function to parse DT parameters for Xen specific UEFI just
>> > like the way for normal UEFI. Then it could reuse the existing codes.
>> > 
>> > If Xen supports EFI, initialize runtime services.
>  
> This commit log would benefit from a little expansion. I'd like to see
> information that answers the following questions,
> 
>  - How do the Xen DT paramters differ from bare metal?
>  - What existing code is reused with your patch?
>  - How are Xen runtime services different from bare metal?
>  - Why is it OK to force enable EFI runtime services for Xen?
> 
> I think it would also be good to explicitly state that we do not
> expect to find both Xen EFI DT parameters and bare metal EFI DT
> parameters when performing the search; the two should be mutually
> exclusive.
> 
>> > CC: Matt Fleming <matt@xxxxxxxxxxxxxxxxxxx>
>> > Signed-off-by: Shannon Zhao <shannon.zhao@xxxxxxxxxx>
>> > ---
>> > Drop using of EFI_PARAVIRT. Discussion can be found from [1].
>> > [1] https://lkml.org/lkml/2016/4/29/8
>> > ---
>> >  arch/arm/include/asm/efi.h         |  2 +
>> >  arch/arm/xen/enlighten.c           | 16 ++++++++
>> >  arch/arm64/include/asm/efi.h       |  2 +
>> >  drivers/firmware/efi/arm-runtime.c | 70 +++++++++++++++++++++++---------
>> >  drivers/firmware/efi/efi.c         | 81 
>> > ++++++++++++++++++++++++++++++--------
>> >  5 files changed, 137 insertions(+), 34 deletions(-)
>> > 
>> > diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
>> > index e0eea72..5ba4343 100644
>> > --- a/arch/arm/include/asm/efi.h
>> > +++ b/arch/arm/include/asm/efi.h
>> > @@ -52,9 +52,11 @@ static inline void efi_set_pgd(struct mm_struct *mm)
>> >  
>> >  void efi_virtmap_load(void);
>> >  void efi_virtmap_unload(void);
>> > +int xen_arm_enable_runtime_services(void);
>> >  
>> >  #else
>> >  #define efi_init()
>> > +#define xen_arm_enable_runtime_services() (0)
>> >  #endif /* CONFIG_EFI */
>> >  
>> >  /* arch specific definitions used by the stub code */
>> > diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
>> > index 13e3e9f..3362870 100644
>> > --- a/arch/arm/xen/enlighten.c
>> > +++ b/arch/arm/xen/enlighten.c
>> > @@ -16,6 +16,7 @@
>> >  #include <asm/xen/hypervisor.h>
>> >  #include <asm/xen/hypercall.h>
>> >  #include <asm/system_misc.h>
>> > +#include <asm/efi.h>
>> >  #include <linux/interrupt.h>
>> >  #include <linux/irqreturn.h>
>> >  #include <linux/module.h>
>> > @@ -261,6 +262,13 @@ static int __init fdt_find_hyper_node(unsigned long 
>> > node, const char *uname,
>> >        !strncmp(hyper_node.prefix, s, strlen(hyper_node.prefix)))
>> >            hyper_node.version = s + strlen(hyper_node.prefix);
>> >  
>> > +  if (IS_ENABLED(CONFIG_XEN_EFI)) {
>> > +          /* Check if Xen supports EFI */
>> > +          if ((of_get_flat_dt_subnode_by_name(node, "uefi") > 0) &&
>> > +              !efi_runtime_disabled())
>> > +                  set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
>> > +  }
>> > +
>> >    return 0;
>> >  }
>> >  
> The above comment could do with including similar information to the
> commit log as to why we want to force enable runtime services. For x86
> we have this,
> 
>        *
>        * When EFI_PARAVIRT is in force then we could not map runtime
>        * service memory region because we do not have direct access to it.
>        * However, runtime services are available through proxy functions
>        * (e.g. in case of Xen dom0 EFI implementation they call special
>        * hypercall which executes relevant EFI functions) and that is why
>        * they are always enabled.
>        */
> 
> We need something similar here.
> 
>> > @@ -338,6 +346,7 @@ static int __init xen_guest_init(void)
>> >  {
>> >    struct xen_add_to_physmap xatp;
>> >    struct shared_info *shared_info_page = NULL;
>> > +  int ret;
>> >  
>> >    if (!xen_domain())
>> >            return 0;
>> > @@ -352,6 +361,13 @@ static int __init xen_guest_init(void)
>> >            return -ENODEV;
>> >    }
>> >  
>> > +  if (IS_ENABLED(CONFIG_XEN_EFI) && efi_enabled(EFI_BOOT) &&
>> > +      efi_enabled(EFI_RUNTIME_SERVICES)) {
>> > +          ret = xen_arm_enable_runtime_services();
>> > +          if (ret)
>> > +                  return ret;
>> > +  }
>> > +
> Is it ever possible to have EFI_RUNTIME_SERVICES set but
> efi_enabled(EFI_BOOT) and IS_ENABLED(CONFIG_XEN_EFI) be false inside
> this function? If the answer is "no", I'd suggest just reducing this
> down to,
> 
>       /*
>        * The fdt parsing code will have set EFI_RUNTIME_SERVICES if
>        * it found Xen EFI parameters. Force enable runtime services.
>        */ 
>       if (efi_enabled(EFI_RUNTIME_SERVICES)) {
>               ret = xen_arm_enable_runtime_services();
>               if (ret)
>                       return ret;
>       }
> 
> But first, see my comments below.
> 
>> > @@ -39,6 +40,33 @@ static struct mm_struct efi_mm = {
>> >    .mmlist                 = LIST_HEAD_INIT(efi_mm.mmlist),
>> >  };
>> >  
>> > +static int __init efi_remap_init(void)
>> > +{
>> > +  u64 mapsize;
>> > +
>> > +  pr_info("Remapping and enabling EFI services.\n");
>> > +
>> > +  mapsize = memmap.map_end - memmap.map;
>> > +  memmap.map = (__force void *)ioremap_cache(memmap.phys_map,
>> > +                                             mapsize);
>> > +  if (!memmap.map) {
>> > +          pr_err("Failed to remap EFI memory map\n");
>> > +          return -ENOMEM;
>> > +  }
>> > +  memmap.map_end = memmap.map + mapsize;
>> > +  efi.memmap = &memmap;
>> > +
>> > +  efi.systab = (__force void *)ioremap_cache(efi_system_table,
>> > +                                             sizeof(efi_system_table_t));
>> > +  if (!efi.systab) {
>> > +          pr_err("Failed to remap EFI System Table\n");
>> > +          return -ENOMEM;
>> > +  }
>> > +  set_bit(EFI_SYSTEM_TABLES, &efi.flags);
>> > +
>> > +  return 0;
>> > +}
>> > +
>> >  static bool __init efi_virtmap_init(void)
>> >  {
>> >    efi_memory_desc_t *md;
>> > @@ -75,7 +103,7 @@ static bool __init efi_virtmap_init(void)
>> >   */
>> >  static int __init arm_enable_runtime_services(void)
>> >  {
>> > -  u64 mapsize;
>> > +  int ret;
>> >  
>> >    if (!efi_enabled(EFI_BOOT)) {
>> >            pr_info("EFI services will not be available.\n");
>> > @@ -87,25 +115,14 @@ static int __init arm_enable_runtime_services(void)
>> >            return 0;
>> >    }
>> >  
>> > -  pr_info("Remapping and enabling EFI services.\n");
>> > -
>> > -  mapsize = memmap.map_end - memmap.map;
>> > -  memmap.map = (__force void *)ioremap_cache(memmap.phys_map,
>> > -                                             mapsize);
>> > -  if (!memmap.map) {
>> > -          pr_err("Failed to remap EFI memory map\n");
>> > -          return -ENOMEM;
>> > +  if (efi_enabled(EFI_RUNTIME_SERVICES)) {
>> > +          pr_info("EFI runtime services access via paravirt.\n");
>> > +          return 0;
>> >    }
>> > -  memmap.map_end = memmap.map + mapsize;
>> > -  efi.memmap = &memmap;
>> >  
>> > -  efi.systab = (__force void *)ioremap_cache(efi_system_table,
>> > -                                             sizeof(efi_system_table_t));
>> > -  if (!efi.systab) {
>> > -          pr_err("Failed to remap EFI System Table\n");
>> > -          return -ENOMEM;
>> > -  }
>> > -  set_bit(EFI_SYSTEM_TABLES, &efi.flags);
>> > +  ret = efi_remap_init();
>> > +  if (ret)
>> > +          return ret;
>> >  
>> >    if (!efi_virtmap_init()) {
>> >            pr_err("No UEFI virtual mapping was installed -- runtime 
>> > services will not be available\n");
>> > @@ -122,6 +139,23 @@ static int __init arm_enable_runtime_services(void)
>> >  }
>> >  early_initcall(arm_enable_runtime_services);
>> >  
>> > +int __init xen_arm_enable_runtime_services(void)
>> > +{
>> > +  int ret;
>> > +
>> > +  ret = efi_remap_init();
>> > +  if (ret)
>> > +          return ret;
>> > +
>> > +  if (IS_ENABLED(CONFIG_XEN_EFI)) {
>> > +          /* Set up runtime services function pointers for Xen Dom0 */
>> > +          xen_efi_runtime_setup();
>> > +          efi.runtime_version = efi.systab->hdr.revision;
>> > +  }
>> > +
>> > +  return 0;
>> > +}
> Since you call efi_remap_init() in both of these functions, couldn't
> you leave the existing code alone and bail after setting up the memory
> map and systab in arm_enable_runtime_services()?
> 
> Also, why do you need to setup efi.runtime_version here? Isn't that
> done inside uefi_init()?
> 
I don't see any codes which setup efi.runtime_version in uefi_init().

> Here is how I would have expected this patch to look:
> 
>   - Add FDT code to find hypervisor EFI params
> 
>   - Force enable EFI_RUNTIME_SERVICES for Xen and call
>     xen_efi_runtime_setup() inside xen_guest_init()
> 
>   - Change arm_enable_runtime_services() to handle the case where
>     EFI_RUNTIME_SERVICES is already set
> 
> Am I misunderstanding some ordering issues?

Since xen_guest_init() and arm_enable_runtime_services() are both
early_initcall and the calling order is random I think. And when we call
xen_efi_runtime_setup() and setup efi.runtime_version in
xen_guest_init(), the efi.systab might be NULL. So it needs to map the
systanle explicitly before we use efi.systab.

Thanks,
-- 
Shannon

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel