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

[Wei Liu]

On Mon, Jan 22, 2018 at 06:35:14AM -0700, Jan Beulich wrote:
> >>> On 22.01.18 at 14:21,  wrote:
> > On Mon, Jan 22, 2018 at 01:03:14PM +, Roger Pau Monné wrote:
> >> On Mon, Jan 22, 2018 at 12:47:10PM +, Wei Liu wrote:
> >> > --- a/xen/drivers/acpi/osl.c
> >> > +++ b/xen/drivers/acpi/osl.c
> >> > @@ -38,6 +38,10 @@
> >> >  #include 
> >> >  #include 
> >> >  
> >> > +#ifdef CONFIG_PVH_GUEST
> >> > +#include 
> >> > +#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?
> 
> That's not so much the question here imo. Instead the issue I
> see is that the way you code it it's really a layering violation.
> Similar hackery was also rejected in Linux recently, iirc.
> 

OK. I buy this argument.

Let me invent a rsdp_hint instead.

Wei.

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

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

[Jan Beulich]

>>> On 22.01.18 at 14:21,  wrote:
> On Mon, Jan 22, 2018 at 01:03:14PM +, Roger Pau Monné wrote:
>> On Mon, Jan 22, 2018 at 12:47:10PM +, Wei Liu wrote:
>> > --- a/xen/drivers/acpi/osl.c
>> > +++ b/xen/drivers/acpi/osl.c
>> > @@ -38,6 +38,10 @@
>> >  #include 
>> >  #include 
>> >  
>> > +#ifdef CONFIG_PVH_GUEST
>> > +#include 
>> > +#endif
>> > +
>> >  #define _COMPONENTACPI_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?

That's not so much the question here imo. Instead the issue I
see is that the way you code it it's really a layering violation.
Similar hackery was also rejected in Linux recently, iirc.

Jan

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

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

[Wei Liu]

On Mon, Jan 22, 2018 at 01:03:14PM +, Roger Pau Monné wrote:
> On Mon, Jan 22, 2018 at 12:47:10PM +, 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é 
> > Signed-off-by: Wei Liu 
> > ---
> > Cc: Jan Beulich 
> > Cc: Andrew Cooper 
> > Cc: Roger Pau Monné 
> > 
> > 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 
> >  #include 
> >  
> > +#ifdef CONFIG_PVH_GUEST
> > +#include 
> > +#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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Roger Pau Monné]

On Mon, Jan 22, 2018 at 12:47:10PM +, 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é 
> Signed-off-by: Wei Liu 
> ---
> Cc: Jan Beulich 
> Cc: Andrew Cooper 
> Cc: Roger Pau Monné 
> 
> 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.

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

>  }
>  
>  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 
>  #include 
>  
> +#ifdef CONFIG_PVH_GUEST
> +#include 
> +#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?

Thanks, Roger.

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

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

[Wei Liu]

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é 
Signed-off-by: Wei Liu 
---
Cc: Jan Beulich 
Cc: Andrew Cooper 
Cc: Roger Pau Monné 

What about PVH + EFI?
---
 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;
 
 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;;
 }
 
 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 
 #include 
 
+#ifdef CONFIG_PVH_GUEST
+#include 
+#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;
 
diff --git a/xen/include/asm-x86/guest/pvh-boot.h 
b/xen/include/asm-x86/guest/pvh-boot.h
index 1b429f9401..995500e4da 100644
--- a/xen/include/asm-x86/guest/pvh-boot.h
+++ b/xen/include/asm-x86/guest/pvh-boot.h
@@ -24,6 +24,7 @@
 #ifdef CONFIG_PVH_GUEST
 
 extern bool pvh_boot;
+extern unsigned long pvh_rsdp_pa;
 
 multiboot_info_t *pvh_init(void);
 void pvh_print_info(void);
-- 
2.11.0


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