Re: [Xen-devel] [PATCH] xen/shim: stash RSDP address for ACPI driver

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Mon, Jan 22, 2018 at 01:03:14PM +0000, Roger Pau Monné wrote:
> On Mon, Jan 22, 2018 at 12:47:10PM +0000, Wei Liu wrote:
> > It used to the case that we placed RSDP under 1MB and let Xen search
> > for it. We moved the placement to under 4GB in 4a5733771, so the
> > search wouldn't work.
> > 
> > Stash the RSDP address to solve this problem.
> > 
> > Suggested-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> > ---
> > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > Cc: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > 
> > What about PVH + EFI?
> 
> PVH guests using EFI firmware will get the RSDP address from the EFI
> tables. Ie: EFI firmware will use the PVH entry point, thus fetching
> the RSDP from start_info, but the kernel loaded from EFI should be
> using the EFI entry point, and thus fetching the RSDP pointer from the
> EFI tables. Or at least that was my thinking.

Good. That means no addition is needed to the EFI path in
acpi_os_get_root_pointer.

> 
> > ---
> >  xen/arch/x86/guest/pvh-boot.c        | 4 ++++
> >  xen/drivers/acpi/osl.c               | 9 +++++++++
> >  xen/include/asm-x86/guest/pvh-boot.h | 1 +
> >  3 files changed, 14 insertions(+)
> > 
> > diff --git a/xen/arch/x86/guest/pvh-boot.c b/xen/arch/x86/guest/pvh-boot.c
> > index be3122b16c..427f9ea6b1 100644
> > --- a/xen/arch/x86/guest/pvh-boot.c
> > +++ b/xen/arch/x86/guest/pvh-boot.c
> > @@ -30,6 +30,7 @@
> >  /* Initialised in head.S, before .bss is zeroed. */
> >  bool __initdata pvh_boot;
> >  uint32_t __initdata pvh_start_info_pa;
> > +unsigned long __initdata pvh_rsdp_pa;
> 
> uint64_t maybe to use the same type as start_info.h.
> 
> >  
> >  static multiboot_info_t __initdata pvh_mbi;
> >  static module_t __initdata pvh_mbi_mods[8];
> > @@ -69,6 +70,9 @@ static void __init convert_pvh_info(void)
> >          mod[i].mod_end   = entry[i].paddr + entry[i].size;
> >          mod[i].string    = entry[i].cmdline_paddr;
> >      }
> > +
> > +    /* Stash RSDP pointer so ACPI driver can get it */
> > +    pvh_rsdp_pa = pvh_info->rsdp_paddr;;
> 
> Double ';'.
> 
> Is this too early to panic? IMHO we should add:
> 
> if ( !pvh_info->rsdp_paddr )
>     panic("Unable to boot in PVH mode without ACPI tables");
> 
> Preferably here or at acpi_os_get_root_pointer.

It is too early to panic here. Even those BUG_ONs are problematic (yes
they do stop booting but no useful message is printed). But I couldn't
come up with a better way when I wrote the patch.

It is better to do that later in acpi_os_get_root_pointer.

> 
> >  }
> >  
> >  static void __init get_memory_map(void)
> > diff --git a/xen/drivers/acpi/osl.c b/xen/drivers/acpi/osl.c
> > index 52c9b4ba9a..6a81de1707 100644
> > --- a/xen/drivers/acpi/osl.c
> > +++ b/xen/drivers/acpi/osl.c
> > @@ -38,6 +38,10 @@
> >  #include <xen/efi.h>
> >  #include <xen/vmap.h>
> >  
> > +#ifdef CONFIG_PVH_GUEST
> > +#include <asm/guest/pvh-boot.h>
> > +#endif
> > +
> >  #define _COMPONENT         ACPI_OS_SERVICES
> >  ACPI_MODULE_NAME("osl")
> >  
> > @@ -74,6 +78,11 @@ acpi_physical_address __init 
> > acpi_os_get_root_pointer(void)
> >                            "System description tables not found\n");
> >                     return 0;
> >             }
> > +#ifdef CONFIG_PVH_GUEST
> > +   } else if (pvh_boot) {
> > +           ASSERT(pvh_rsdp_pa);
> > +           return pvh_rsdp_pa;
> > +#endif
> >     } else if (IS_ENABLED(CONFIG_ACPI_LEGACY_TABLES_LOOKUP)) {
> >             acpi_physical_address pa = 0;
> 
> Can this be done in a non-PVH specific way?
> 
> Can we have a global rsdp_hint variable or similar that would be used
> here if set?

Who will be the anticipated user(s) other than PVH?

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel