Xen-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/4] xen/arm: Unbreak ACPI
@ 2020-09-26 20:55 Julien Grall
  2020-09-26 20:55 ` [PATCH 1/4] xen/acpi: Rework acpi_os_map_memory() and acpi_os_unmap_memory() Julien Grall
                   ` (7 more replies)
  0 siblings, 8 replies; 51+ messages in thread
From: Julien Grall @ 2020-09-26 20:55 UTC (permalink / raw)
  To: xen-devel
  Cc: alex.bennee, masami.hiramatsu, ehem+xen, bertrand.marquis,
	andre.przywara, Julien Grall, Stefano Stabellini, Julien Grall,
	Volodymyr Babchuk, Andrew Cooper, George Dunlap, Ian Jackson,
	Jan Beulich, Wei Liu, Roger Pau Monné

From: Julien Grall <jgrall@amazon.com>

Hi all,

Xen on ARM has been broken for quite a while on ACPI systems. This
series aims to fix it.

Unfortunately I don't have a system with ACPI v6.0 or later (QEMU seems
to only support 5.1). So I did only some light testing.

I have only build tested the x86 side so far.

Cheers,

*** BLURB HERE ***

Julien Grall (4):
  xen/acpi: Rework acpi_os_map_memory() and acpi_os_unmap_memory()
  xen/arm: acpi: The fixmap area should always be cleared during
    failure/unmap
  xen/arm: Check if the platform is not using ACPI before initializing
    Dom0less
  xen/arm: Introduce fw_unreserved_regions() and use it

 xen/arch/arm/acpi/lib.c     | 79 ++++++++++++++++++++++++++++++-------
 xen/arch/arm/kernel.c       |  2 +-
 xen/arch/arm/setup.c        | 25 +++++++++---
 xen/arch/x86/acpi/lib.c     | 18 +++++++++
 xen/drivers/acpi/osl.c      | 34 ++++++++--------
 xen/include/asm-arm/setup.h |  2 +-
 xen/include/xen/acpi.h      |  1 +
 7 files changed, 123 insertions(+), 38 deletions(-)

-- 
2.17.1



^ permalink raw reply	[flat|nested] 51+ messages in thread

* [PATCH 1/4] xen/acpi: Rework acpi_os_map_memory() and acpi_os_unmap_memory()
  2020-09-26 20:55 [PATCH 0/4] xen/arm: Unbreak ACPI Julien Grall
@ 2020-09-26 20:55 ` Julien Grall
  2020-09-28  8:18   ` Jan Beulich
                     ` (2 more replies)
  2020-09-26 20:55 ` [PATCH 2/4] xen/arm: acpi: The fixmap area should always be cleared during failure/unmap Julien Grall
                   ` (6 subsequent siblings)
  7 siblings, 3 replies; 51+ messages in thread
From: Julien Grall @ 2020-09-26 20:55 UTC (permalink / raw)
  To: xen-devel
  Cc: alex.bennee, masami.hiramatsu, ehem+xen, bertrand.marquis,
	andre.przywara, Julien Grall, Stefano Stabellini, Julien Grall,
	Volodymyr Babchuk, Andrew Cooper, George Dunlap, Ian Jackson,
	Jan Beulich, Wei Liu, Roger Pau Monné

From: Julien Grall <jgrall@amazon.com>

The functions acpi_os_{un,}map_memory() are meant to be arch-agnostic
while the __acpi_os_{un,}map_memory() are meant to be arch-specific.

Currently, the former are still containing x86 specific code.

To avoid this rather strange split, the generic helpers are reworked so
they are arch-agnostic. This requires the introduction of a new helper
__acpi_os_unmap_memory() that will undo any mapping done by
__acpi_os_map_memory().

Currently, the arch-helper for unmap is basically a no-op so it only
returns whether the mapping was arch specific. But this will change
in the future.

Note that the x86 version of acpi_os_map_memory() was already able to
able the 1MB region. Hence why there is no addition of new code.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/arch/arm/acpi/lib.c | 10 ++++++++++
 xen/arch/x86/acpi/lib.c | 18 ++++++++++++++++++
 xen/drivers/acpi/osl.c  | 34 ++++++++++++++++++----------------
 xen/include/xen/acpi.h  |  1 +
 4 files changed, 47 insertions(+), 16 deletions(-)

diff --git a/xen/arch/arm/acpi/lib.c b/xen/arch/arm/acpi/lib.c
index 4fc6e17322c1..2192a5519171 100644
--- a/xen/arch/arm/acpi/lib.c
+++ b/xen/arch/arm/acpi/lib.c
@@ -30,6 +30,10 @@ char *__acpi_map_table(paddr_t phys, unsigned long size)
     unsigned long base, offset, mapped_size;
     int idx;
 
+    /* No arch specific implementation after early boot */
+    if ( system_state >= SYS_STATE_boot )
+        return NULL;
+
     offset = phys & (PAGE_SIZE - 1);
     mapped_size = PAGE_SIZE - offset;
     set_fixmap(FIXMAP_ACPI_BEGIN, maddr_to_mfn(phys), PAGE_HYPERVISOR);
@@ -49,6 +53,12 @@ char *__acpi_map_table(paddr_t phys, unsigned long size)
     return ((char *) base + offset);
 }
 
+bool __acpi_unmap_table(void *ptr, unsigned long size)
+{
+    return ( vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) &&
+             vaddr < (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE) );
+}
+
 /* True to indicate PSCI 0.2+ is implemented */
 bool __init acpi_psci_present(void)
 {
diff --git a/xen/arch/x86/acpi/lib.c b/xen/arch/x86/acpi/lib.c
index 265b9ad81905..77803f4d4c63 100644
--- a/xen/arch/x86/acpi/lib.c
+++ b/xen/arch/x86/acpi/lib.c
@@ -46,6 +46,10 @@ char *__acpi_map_table(paddr_t phys, unsigned long size)
 	if ((phys + size) <= (1 * 1024 * 1024))
 		return __va(phys);
 
+	/* No arch specific implementation after early boot */
+	if (system_state >= SYS_STATE_boot)
+		return NULL;
+
 	offset = phys & (PAGE_SIZE - 1);
 	mapped_size = PAGE_SIZE - offset;
 	set_fixmap(FIX_ACPI_END, phys);
@@ -66,6 +70,20 @@ char *__acpi_map_table(paddr_t phys, unsigned long size)
 	return ((char *) base + offset);
 }
 
+bool __acpi_unmap_table(void *ptr, unsigned long size)
+{
+	unsigned long vaddr = (unsigned long)ptr;
+
+	if (vaddr >= DIRECTMAP_VIRT_START &&
+	    vaddr < DIRECTMAP_VIRT_END) {
+		ASSERT(!((__pa(ptr) + size - 1) >> 20));
+		return true;
+	}
+
+	return (vaddr >= __fix_to_virt(FIX_ACPI_END)) &&
+		(vaddr < (__fix_to_virt(FIX_ACPI_BEGIN) + PAGE_SIZE));
+}
+
 unsigned int acpi_get_processor_id(unsigned int cpu)
 {
 	unsigned int acpiid, apicid;
diff --git a/xen/drivers/acpi/osl.c b/xen/drivers/acpi/osl.c
index 4c8bb7839eda..100eee72def2 100644
--- a/xen/drivers/acpi/osl.c
+++ b/xen/drivers/acpi/osl.c
@@ -92,27 +92,29 @@ acpi_physical_address __init acpi_os_get_root_pointer(void)
 void __iomem *
 acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
 {
-	if (system_state >= SYS_STATE_boot) {
-		mfn_t mfn = _mfn(PFN_DOWN(phys));
-		unsigned int offs = phys & (PAGE_SIZE - 1);
-
-		/* The low first Mb is always mapped on x86. */
-		if (IS_ENABLED(CONFIG_X86) && !((phys + size - 1) >> 20))
-			return __va(phys);
-		return __vmap(&mfn, PFN_UP(offs + size), 1, 1,
-			      ACPI_MAP_MEM_ATTR, VMAP_DEFAULT) + offs;
-	}
-	return __acpi_map_table(phys, size);
+	void *ptr;
+	mfn_t mfn = _mfn(PFN_DOWN(phys));
+	unsigned int offs = phys & (PAGE_SIZE - 1);
+
+	/* Try the arch specific implementation first */
+	ptr = __acpi_map_table(phys, size);
+	if (ptr)
+		return ptr;
+
+	/* No common implementation for early boot map */
+	if (unlikely(system_state < SYS_STATE_boot))
+	     return NULL;
+
+	ptr = __vmap(&mfn, PFN_UP(offs + size), 1, 1,
+		     ACPI_MAP_MEM_ATTR, VMAP_DEFAULT);
+
+	return !ptr ? NULL : (ptr + offs);
 }
 
 void acpi_os_unmap_memory(void __iomem * virt, acpi_size size)
 {
-	if (IS_ENABLED(CONFIG_X86) &&
-	    (unsigned long)virt >= DIRECTMAP_VIRT_START &&
-	    (unsigned long)virt < DIRECTMAP_VIRT_END) {
-		ASSERT(!((__pa(virt) + size - 1) >> 20));
+	if (__acpi_unmap_table(virt, size))
 		return;
-	}
 
 	if (system_state >= SYS_STATE_boot)
 		vunmap((void *)((unsigned long)virt & PAGE_MASK));
diff --git a/xen/include/xen/acpi.h b/xen/include/xen/acpi.h
index c945ab05c864..5a84a4bf54e0 100644
--- a/xen/include/xen/acpi.h
+++ b/xen/include/xen/acpi.h
@@ -68,6 +68,7 @@ typedef int (*acpi_table_entry_handler) (struct acpi_subtable_header *header, co
 
 unsigned int acpi_get_processor_id (unsigned int cpu);
 char * __acpi_map_table (paddr_t phys_addr, unsigned long size);
+bool __acpi_unmap_table(void *ptr, unsigned long size);
 int acpi_boot_init (void);
 int acpi_boot_table_init (void);
 int acpi_numa_init (void);
-- 
2.17.1



^ permalink raw reply	[flat|nested] 51+ messages in thread

* [PATCH 2/4] xen/arm: acpi: The fixmap area should always be cleared during failure/unmap
  2020-09-26 20:55 [PATCH 0/4] xen/arm: Unbreak ACPI Julien Grall
  2020-09-26 20:55 ` [PATCH 1/4] xen/acpi: Rework acpi_os_map_memory() and acpi_os_unmap_memory() Julien Grall
@ 2020-09-26 20:55 ` Julien Grall
  2020-09-29 11:13   ` Rahul Singh
  2020-10-01  0:30   ` Stefano Stabellini
  2020-09-26 20:55 ` [PATCH 3/4] xen/arm: Check if the platform is not using ACPI before initializing Dom0less Julien Grall
                   ` (5 subsequent siblings)
  7 siblings, 2 replies; 51+ messages in thread
From: Julien Grall @ 2020-09-26 20:55 UTC (permalink / raw)
  To: xen-devel
  Cc: alex.bennee, masami.hiramatsu, ehem+xen, bertrand.marquis,
	andre.przywara, Julien Grall, Stefano Stabellini, Julien Grall,
	Volodymyr Babchuk, Wei Xu

From: Julien Grall <jgrall@amazon.com>

Commit 022387ee1ad3 "xen/arm: mm: Don't open-code Xen PT update in
{set, clear}_fixmap()" enforced that each set_fixmap() should be
paired with a clear_fixmap(). Any failure to follow the model would
result to a platform crash.

Unfortunately, the use of fixmap in the ACPI code was overlooked as it
is calling set_fixmap() but not clear_fixmap().

The function __acpi_os_map_table() is reworked so:
    - We know before the mapping whether the fixmap region is big
    enough for the mapping.
    - It will fail if the fixmap is always inuse.

The function __acpi_os_unmap_table() will now call clear_fixmap().

Reported-by: Wei Xu <xuwei5@hisilicon.com>
Signed-off-by: Julien Grall <jgrall@amazon.com>

---

The discussion on the original thread [1] suggested to also zap it on
x86. This is technically not necessary today, so it is left alone for
now.

I looked at making the fixmap code common but the index are inverted
between Arm and x86.

[1] https://lore.kernel.org/xen-devel/5E26C935.9080107@hisilicon.com/
---
 xen/arch/arm/acpi/lib.c | 75 +++++++++++++++++++++++++++++++----------
 1 file changed, 58 insertions(+), 17 deletions(-)

diff --git a/xen/arch/arm/acpi/lib.c b/xen/arch/arm/acpi/lib.c
index 2192a5519171..eebaca695562 100644
--- a/xen/arch/arm/acpi/lib.c
+++ b/xen/arch/arm/acpi/lib.c
@@ -25,38 +25,79 @@
 #include <xen/init.h>
 #include <xen/mm.h>
 
+static bool fixmap_inuse;
+
 char *__acpi_map_table(paddr_t phys, unsigned long size)
 {
-    unsigned long base, offset, mapped_size;
-    int idx;
+    unsigned long base, offset;
+    mfn_t mfn;
+    unsigned int idx;
 
     /* No arch specific implementation after early boot */
     if ( system_state >= SYS_STATE_boot )
         return NULL;
 
     offset = phys & (PAGE_SIZE - 1);
-    mapped_size = PAGE_SIZE - offset;
-    set_fixmap(FIXMAP_ACPI_BEGIN, maddr_to_mfn(phys), PAGE_HYPERVISOR);
-    base = FIXMAP_ADDR(FIXMAP_ACPI_BEGIN);
+    base = FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) + offset;
+
+    /* Check the fixmap is big enough to map the region */
+    if ( (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE - base) < size )
+        return NULL;
+
+    /* With the fixmap, we can only map one region at the time */
+    if ( fixmap_inuse )
+        return NULL;
 
-    /* Most cases can be covered by the below. */
+    fixmap_inuse = true;
+
+    size += offset;
+    mfn = maddr_to_mfn(phys);
     idx = FIXMAP_ACPI_BEGIN;
-    while ( mapped_size < size )
-    {
-        if ( ++idx > FIXMAP_ACPI_END )
-            return NULL;    /* cannot handle this */
-        phys += PAGE_SIZE;
-        set_fixmap(idx, maddr_to_mfn(phys), PAGE_HYPERVISOR);
-        mapped_size += PAGE_SIZE;
-    }
 
-    return ((char *) base + offset);
+    do {
+        set_fixmap(idx, mfn, PAGE_HYPERVISOR);
+        size -= min(size, (unsigned long)PAGE_SIZE);
+        mfn = mfn_add(mfn, 1);
+        idx++;
+    } while ( size > 0 );
+
+    return (char *)base;
 }
 
 bool __acpi_unmap_table(void *ptr, unsigned long size)
 {
-    return ( vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) &&
-             vaddr < (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE) );
+    vaddr_t vaddr = (vaddr_t)ptr;
+    unsigned int idx;
+
+    /* We are only handling fixmap address in the arch code */
+    if ( vaddr < FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) ||
+         vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_END) )
+        return false;
+
+    /*
+     * __acpi_map_table() will always return a pointer in the first page
+     * for the ACPI fixmap region. The caller is expected to free with
+     * the same address.
+     */
+    ASSERT((vaddr & PAGE_MASK) == FIXMAP_ADDR(FIXMAP_ACPI_BEGIN));
+
+    /* The region allocated fit in the ACPI fixmap region. */
+    ASSERT(size < (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE - vaddr));
+    ASSERT(fixmap_inuse);
+
+    fixmap_inuse = false;
+
+    size += FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) - vaddr;
+    idx = FIXMAP_ACPI_BEGIN;
+
+    do
+    {
+        clear_fixmap(idx);
+        size -= min(size, (unsigned long)PAGE_SIZE);
+        idx++;
+    } while ( size > 0 );
+
+    return true;
 }
 
 /* True to indicate PSCI 0.2+ is implemented */
-- 
2.17.1



^ permalink raw reply	[flat|nested] 51+ messages in thread

* [PATCH 3/4] xen/arm: Check if the platform is not using ACPI before initializing Dom0less
  2020-09-26 20:55 [PATCH 0/4] xen/arm: Unbreak ACPI Julien Grall
  2020-09-26 20:55 ` [PATCH 1/4] xen/acpi: Rework acpi_os_map_memory() and acpi_os_unmap_memory() Julien Grall
  2020-09-26 20:55 ` [PATCH 2/4] xen/arm: acpi: The fixmap area should always be cleared during failure/unmap Julien Grall
@ 2020-09-26 20:55 ` Julien Grall
  2020-09-29 11:17   ` Rahul Singh
  2020-09-30 23:26   ` Stefano Stabellini
  2020-09-26 20:55 ` [PATCH 4/4] xen/arm: Introduce fw_unreserved_regions() and use it Julien Grall
                   ` (4 subsequent siblings)
  7 siblings, 2 replies; 51+ messages in thread
From: Julien Grall @ 2020-09-26 20:55 UTC (permalink / raw)
  To: xen-devel
  Cc: alex.bennee, masami.hiramatsu, ehem+xen, bertrand.marquis,
	andre.przywara, Julien Grall, Stefano Stabellini, Julien Grall,
	Volodymyr Babchuk

From: Julien Grall <jgrall@amazon.com>

Dom0less requires a device-tree. However, since commit 6e3e77120378
"xen/arm: setup: Relocate the Device-Tree later on in the boot", the
device-tree will not get unflatten when using ACPI.

This will lead to a crash during boot.

Given the complexity to setup dom0less with ACPI (for instance how to
assign device?), we should skip any code related to Dom0less when using
ACPI.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/arch/arm/setup.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index f16b33fa87a2..35e5bee04efa 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -987,7 +987,8 @@ void __init start_xen(unsigned long boot_phys_offset,
 
     system_state = SYS_STATE_active;
 
-    create_domUs();
+    if ( acpi_disabled )
+        create_domUs();
 
     domain_unpause_by_systemcontroller(dom0);
 
-- 
2.17.1



^ permalink raw reply	[flat|nested] 51+ messages in thread

* [PATCH 4/4] xen/arm: Introduce fw_unreserved_regions() and use it
  2020-09-26 20:55 [PATCH 0/4] xen/arm: Unbreak ACPI Julien Grall
                   ` (2 preceding siblings ...)
  2020-09-26 20:55 ` [PATCH 3/4] xen/arm: Check if the platform is not using ACPI before initializing Dom0less Julien Grall
@ 2020-09-26 20:55 ` Julien Grall
  2020-09-30 23:40   ` Stefano Stabellini
  2020-09-27  1:47 ` [PATCH 0/4] xen/arm: Unbreak ACPI Elliott Mitchell
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 51+ messages in thread
From: Julien Grall @ 2020-09-26 20:55 UTC (permalink / raw)
  To: xen-devel
  Cc: alex.bennee, masami.hiramatsu, ehem+xen, bertrand.marquis,
	andre.przywara, Julien Grall, Stefano Stabellini, Julien Grall,
	Volodymyr Babchuk

From: Julien Grall <jgrall@amazon.com>

Since commit 6e3e77120378 "xen/arm: setup: Relocate the Device-Tree
later on in the boot", the device-tree will not be kept mapped when
using ACPI.

However, a few places are calling dt_unreserved_regions() which expects
a valid DT. This will lead to a crash.

As the DT should not be used for ACPI (other than for detecting the
modules), a new function fw_unreserved_regions() is introduced.

It will behave the same way on DT system. On ACPI system, it will
unreserve the whole region.

Signed-off-by: Julien Grall <jgrall@amazon.com>

---

Is there any region we should exclude on ACPI?
---
 xen/arch/arm/kernel.c       |  2 +-
 xen/arch/arm/setup.c        | 22 +++++++++++++++++-----
 xen/include/asm-arm/setup.h |  2 +-
 3 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index 032923853f2c..ab78689ed2a6 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -307,7 +307,7 @@ static __init int kernel_decompress(struct bootmodule *mod)
      * Free the original kernel, update the pointers to the
      * decompressed kernel
      */
-    dt_unreserved_regions(addr, addr + size, init_domheap_pages, 0);
+    fw_unreserved_regions(addr, addr + size, init_domheap_pages, 0);
 
     return 0;
 }
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 35e5bee04efa..7fcff9af2a7e 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -196,8 +196,9 @@ static void __init processor_id(void)
     processor_setup();
 }
 
-void __init dt_unreserved_regions(paddr_t s, paddr_t e,
-                                  void (*cb)(paddr_t, paddr_t), int first)
+static void __init dt_unreserved_regions(paddr_t s, paddr_t e,
+                                         void (*cb)(paddr_t, paddr_t),
+                                         int first)
 {
     int i, nr = fdt_num_mem_rsv(device_tree_flattened);
 
@@ -244,6 +245,17 @@ void __init dt_unreserved_regions(paddr_t s, paddr_t e,
     cb(s, e);
 }
 
+void __init fw_unreserved_regions(paddr_t s, paddr_t e,
+                                  void (*cb)(paddr_t, paddr_t), int first)
+{
+    if ( acpi_disabled )
+        dt_unreserved_regions(s, e, cb, first);
+    else
+        cb(s, e);
+}
+
+
+
 struct bootmodule __init *add_boot_module(bootmodule_kind kind,
                                           paddr_t start, paddr_t size,
                                           bool domU)
@@ -405,7 +417,7 @@ void __init discard_initial_modules(void)
              !mfn_valid(maddr_to_mfn(e)) )
             continue;
 
-        dt_unreserved_regions(s, e, init_domheap_pages, 0);
+        fw_unreserved_regions(s, e, init_domheap_pages, 0);
     }
 
     mi->nr_mods = 0;
@@ -712,7 +724,7 @@ static void __init setup_mm(void)
                 n = mfn_to_maddr(mfn_add(xenheap_mfn_start, xenheap_pages));
             }
 
-            dt_unreserved_regions(s, e, init_boot_pages, 0);
+            fw_unreserved_regions(s, e, init_boot_pages, 0);
 
             s = n;
         }
@@ -765,7 +777,7 @@ static void __init setup_mm(void)
             if ( e > bank_end )
                 e = bank_end;
 
-            dt_unreserved_regions(s, e, init_boot_pages, 0);
+            fw_unreserved_regions(s, e, init_boot_pages, 0);
             s = n;
         }
     }
diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
index 2f8f24e286ed..34df23247b87 100644
--- a/xen/include/asm-arm/setup.h
+++ b/xen/include/asm-arm/setup.h
@@ -96,7 +96,7 @@ int construct_dom0(struct domain *d);
 void create_domUs(void);
 
 void discard_initial_modules(void);
-void dt_unreserved_regions(paddr_t s, paddr_t e,
+void fw_unreserved_regions(paddr_t s, paddr_t e,
                            void (*cb)(paddr_t, paddr_t), int first);
 
 size_t boot_fdt_info(const void *fdt, paddr_t paddr);
-- 
2.17.1



^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 0/4] xen/arm: Unbreak ACPI
  2020-09-26 20:55 [PATCH 0/4] xen/arm: Unbreak ACPI Julien Grall
                   ` (3 preceding siblings ...)
  2020-09-26 20:55 ` [PATCH 4/4] xen/arm: Introduce fw_unreserved_regions() and use it Julien Grall
@ 2020-09-27  1:47 ` Elliott Mitchell
  2020-09-29 15:28   ` Elliott Mitchell
  2020-09-28  6:47 ` Masami Hiramatsu
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 51+ messages in thread
From: Elliott Mitchell @ 2020-09-27  1:47 UTC (permalink / raw)
  To: Julien Grall
  Cc: xen-devel, alex.bennee, masami.hiramatsu, bertrand.marquis,
	andre.przywara, Julien Grall, Stefano Stabellini,
	Volodymyr Babchuk, Andrew Cooper, George Dunlap, Ian Jackson,
	Jan Beulich, Wei Liu, Roger Pau Monn??

On Sat, Sep 26, 2020 at 09:55:38PM +0100, Julien Grall wrote:
> Xen on ARM has been broken for quite a while on ACPI systems. This
> series aims to fix it.
> 
> Unfortunately I don't have a system with ACPI v6.0 or later (QEMU seems
> to only support 5.1). So I did only some light testing.
> 
> I have only build tested the x86 side so far.

On the ARM device I'm trying to get operational the boot got distinctly
further along so appears to be a distinct ACPI improvement here.


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg@m5p.com  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445




^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 0/4] xen/arm: Unbreak ACPI
  2020-09-26 20:55 [PATCH 0/4] xen/arm: Unbreak ACPI Julien Grall
                   ` (4 preceding siblings ...)
  2020-09-27  1:47 ` [PATCH 0/4] xen/arm: Unbreak ACPI Elliott Mitchell
@ 2020-09-28  6:47 ` Masami Hiramatsu
  2020-09-28 13:00   ` Masami Hiramatsu
                     ` (3 more replies)
  2020-09-29 11:10 ` Rahul Singh
  2020-09-29 15:29 ` Alex Bennée
  7 siblings, 4 replies; 51+ messages in thread
From: Masami Hiramatsu @ 2020-09-28  6:47 UTC (permalink / raw)
  To: Julien Grall
  Cc: xen-devel, Alex Bennée, ehem+xen, bertrand.marquis,
	andre.przywara, Julien Grall, Stefano Stabellini,
	Volodymyr Babchuk, Andrew Cooper, George Dunlap, Ian Jackson,
	Jan Beulich, Wei Liu, Roger Pau Monné


[-- Attachment #1: Type: text/plain, Size: 1375 bytes --]

Hello,

This made progress with my Xen boot on DeveloperBox (
https://www.96boards.org/product/developerbox/ ) with ACPI.

Thank you,


2020年9月27日(日) 5:56 Julien Grall <julien@xen.org>:

>
> From: Julien Grall <jgrall@amazon.com>
>
> Hi all,
>
> Xen on ARM has been broken for quite a while on ACPI systems. This
> series aims to fix it.
>
> Unfortunately I don't have a system with ACPI v6.0 or later (QEMU seems
> to only support 5.1). So I did only some light testing.
>
> I have only build tested the x86 side so far.
>
> Cheers,
>
> *** BLURB HERE ***
>
> Julien Grall (4):
>   xen/acpi: Rework acpi_os_map_memory() and acpi_os_unmap_memory()
>   xen/arm: acpi: The fixmap area should always be cleared during
>     failure/unmap
>   xen/arm: Check if the platform is not using ACPI before initializing
>     Dom0less
>   xen/arm: Introduce fw_unreserved_regions() and use it
>
>  xen/arch/arm/acpi/lib.c     | 79 ++++++++++++++++++++++++++++++-------
>  xen/arch/arm/kernel.c       |  2 +-
>  xen/arch/arm/setup.c        | 25 +++++++++---
>  xen/arch/x86/acpi/lib.c     | 18 +++++++++
>  xen/drivers/acpi/osl.c      | 34 ++++++++--------
>  xen/include/asm-arm/setup.h |  2 +-
>  xen/include/xen/acpi.h      |  1 +
>  7 files changed, 123 insertions(+), 38 deletions(-)
>
> --
> 2.17.1
>


--
Masami Hiramatsu

[-- Attachment #2: xen-acpi-hide-uart-address --]
[-- Type: application/octet-stream, Size: 2134 bytes --]

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/4] xen/acpi: Rework acpi_os_map_memory() and acpi_os_unmap_memory()
  2020-09-26 20:55 ` [PATCH 1/4] xen/acpi: Rework acpi_os_map_memory() and acpi_os_unmap_memory() Julien Grall
@ 2020-09-28  8:18   ` Jan Beulich
  2020-09-28  9:58     ` Julien Grall
  2020-09-29 11:10   ` Rahul Singh
  2020-10-01  0:06   ` Stefano Stabellini
  2 siblings, 1 reply; 51+ messages in thread
From: Jan Beulich @ 2020-09-28  8:18 UTC (permalink / raw)
  To: Julien Grall
  Cc: xen-devel, alex.bennee, masami.hiramatsu, ehem+xen,
	bertrand.marquis, andre.przywara, Julien Grall,
	Stefano Stabellini, Volodymyr Babchuk, Andrew Cooper,
	George Dunlap, Ian Jackson, Wei Liu, Roger Pau Monné

On 26.09.2020 22:55, Julien Grall wrote:
> @@ -49,6 +53,12 @@ char *__acpi_map_table(paddr_t phys, unsigned long size)
>      return ((char *) base + offset);
>  }
>  
> +bool __acpi_unmap_table(void *ptr, unsigned long size)
> +{
> +    return ( vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) &&
> +             vaddr < (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE) );

Nit: If this wasn't Arm code, I'd ask for the parentheses to be
dropped altogether, but at least the stray blanks should go away
imo.

> --- a/xen/arch/x86/acpi/lib.c
> +++ b/xen/arch/x86/acpi/lib.c
> @@ -46,6 +46,10 @@ char *__acpi_map_table(paddr_t phys, unsigned long size)
>  	if ((phys + size) <= (1 * 1024 * 1024))
>  		return __va(phys);
>  
> +	/* No arch specific implementation after early boot */
> +	if (system_state >= SYS_STATE_boot)
> +		return NULL;

Considering the code in context above, the comment isn't entirely
correct.

> @@ -66,6 +70,20 @@ char *__acpi_map_table(paddr_t phys, unsigned long size)
>  	return ((char *) base + offset);
>  }
>  
> +bool __acpi_unmap_table(void *ptr, unsigned long size)

Is there anything standing in the way of making ptr "const void *"?

> +{
> +	unsigned long vaddr = (unsigned long)ptr;
> +
> +	if (vaddr >= DIRECTMAP_VIRT_START &&
> +	    vaddr < DIRECTMAP_VIRT_END) {
> +		ASSERT(!((__pa(ptr) + size - 1) >> 20));
> +		return true;
> +	}
> +
> +	return (vaddr >= __fix_to_virt(FIX_ACPI_END)) &&
> +		(vaddr < (__fix_to_virt(FIX_ACPI_BEGIN) + PAGE_SIZE));

Indentation is slightly wrong here. Also please consider reducing
the number of parentheses here; at the very least the return and
the earlier if() want to be consistent in when ones are(n't) used.

> --- a/xen/drivers/acpi/osl.c
> +++ b/xen/drivers/acpi/osl.c
> @@ -92,27 +92,29 @@ acpi_physical_address __init acpi_os_get_root_pointer(void)
>  void __iomem *
>  acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
>  {
> -	if (system_state >= SYS_STATE_boot) {
> -		mfn_t mfn = _mfn(PFN_DOWN(phys));
> -		unsigned int offs = phys & (PAGE_SIZE - 1);
> -
> -		/* The low first Mb is always mapped on x86. */
> -		if (IS_ENABLED(CONFIG_X86) && !((phys + size - 1) >> 20))
> -			return __va(phys);
> -		return __vmap(&mfn, PFN_UP(offs + size), 1, 1,
> -			      ACPI_MAP_MEM_ATTR, VMAP_DEFAULT) + offs;
> -	}
> -	return __acpi_map_table(phys, size);
> +	void *ptr;
> +	mfn_t mfn = _mfn(PFN_DOWN(phys));
> +	unsigned int offs = phys & (PAGE_SIZE - 1);

Open-coding PAGE_OFFSET()?

> +	/* Try the arch specific implementation first */
> +	ptr = __acpi_map_table(phys, size);
> +	if (ptr)
> +		return ptr;
> +
> +	/* No common implementation for early boot map */
> +	if (unlikely(system_state < SYS_STATE_boot))
> +	     return NULL;

Consistently hard tabs for indentation here, please.

> +	ptr = __vmap(&mfn, PFN_UP(offs + size), 1, 1,
> +		     ACPI_MAP_MEM_ATTR, VMAP_DEFAULT);
> +
> +	return !ptr ? NULL : (ptr + offs);

Slightly odd that you don't let the success case go first, the
more that it's minimally shorter:

	return ptr ? ptr + offs : NULL;

Jan


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/4] xen/acpi: Rework acpi_os_map_memory() and acpi_os_unmap_memory()
  2020-09-28  8:18   ` Jan Beulich
@ 2020-09-28  9:58     ` Julien Grall
  2020-09-28 10:09       ` Jan Beulich
  0 siblings, 1 reply; 51+ messages in thread
From: Julien Grall @ 2020-09-28  9:58 UTC (permalink / raw)
  To: Jan Beulich
  Cc: xen-devel, alex.bennee, masami.hiramatsu, ehem+xen,
	bertrand.marquis, andre.przywara, Julien Grall,
	Stefano Stabellini, Volodymyr Babchuk, Andrew Cooper,
	George Dunlap, Ian Jackson, Wei Liu, Roger Pau Monné

Hi Jan,

On 28/09/2020 09:18, Jan Beulich wrote:
> On 26.09.2020 22:55, Julien Grall wrote:
>> @@ -49,6 +53,12 @@ char *__acpi_map_table(paddr_t phys, unsigned long size)
>>       return ((char *) base + offset);
>>   }
>>   
>> +bool __acpi_unmap_table(void *ptr, unsigned long size)
>> +{
>> +    return ( vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) &&
>> +             vaddr < (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE) );
> 
> Nit: If this wasn't Arm code, I'd ask for the parentheses to be
> dropped altogether, but at least the stray blanks should go away
> imo.

I will drop the stray blanks.

> 
>> --- a/xen/arch/x86/acpi/lib.c
>> +++ b/xen/arch/x86/acpi/lib.c
>> @@ -46,6 +46,10 @@ char *__acpi_map_table(paddr_t phys, unsigned long size)
>>   	if ((phys + size) <= (1 * 1024 * 1024))
>>   		return __va(phys);
>>   
>> +	/* No arch specific implementation after early boot */
>> +	if (system_state >= SYS_STATE_boot)
>> +		return NULL;
> 
> Considering the code in context above, the comment isn't entirely
> correct.

How about "No arch specific implementation after early boot but for the 
first 1MB"?

> 
>> @@ -66,6 +70,20 @@ char *__acpi_map_table(paddr_t phys, unsigned long size)
>>   	return ((char *) base + offset);
>>   }
>>   
>> +bool __acpi_unmap_table(void *ptr, unsigned long size)
> 
> Is there anything standing in the way of making ptr "const void *"?

It might be possible, I will have a look.

>> +{
>> +	unsigned long vaddr = (unsigned long)ptr;
>> +
>> +	if (vaddr >= DIRECTMAP_VIRT_START &&
>> +	    vaddr < DIRECTMAP_VIRT_END) {
>> +		ASSERT(!((__pa(ptr) + size - 1) >> 20));
>> +		return true;
>> +	}
>> +
>> +	return (vaddr >= __fix_to_virt(FIX_ACPI_END)) &&
>> +		(vaddr < (__fix_to_virt(FIX_ACPI_BEGIN) + PAGE_SIZE));
> 
> Indentation is slightly wrong here.

This is Linux coding style and therefore is using hard tab. Care to 
explain the problem?

> Also please consider reducing
> the number of parentheses here; at the very least the return and
> the earlier if() want to be consistent in when ones are(n't) used.

I will add extra parentheses in the if.

> 
>> --- a/xen/drivers/acpi/osl.c
>> +++ b/xen/drivers/acpi/osl.c
>> @@ -92,27 +92,29 @@ acpi_physical_address __init acpi_os_get_root_pointer(void)
>>   void __iomem *
>>   acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
>>   {
>> -	if (system_state >= SYS_STATE_boot) {
>> -		mfn_t mfn = _mfn(PFN_DOWN(phys));
>> -		unsigned int offs = phys & (PAGE_SIZE - 1);
>> -
>> -		/* The low first Mb is always mapped on x86. */
>> -		if (IS_ENABLED(CONFIG_X86) && !((phys + size - 1) >> 20))
>> -			return __va(phys);
>> -		return __vmap(&mfn, PFN_UP(offs + size), 1, 1,
>> -			      ACPI_MAP_MEM_ATTR, VMAP_DEFAULT) + offs;
>> -	}
>> -	return __acpi_map_table(phys, size);
>> +	void *ptr;
>> +	mfn_t mfn = _mfn(PFN_DOWN(phys));
>> +	unsigned int offs = phys & (PAGE_SIZE - 1);
> 
> Open-coding PAGE_OFFSET()?

I was looking for a macro to avoid open-coding but I couldn't find it. I 
will use it in the next version.

>> +	/* Try the arch specific implementation first */
>> +	ptr = __acpi_map_table(phys, size);
>> +	if (ptr)
>> +		return ptr;
>> +
>> +	/* No common implementation for early boot map */
>> +	if (unlikely(system_state < SYS_STATE_boot))
>> +	     return NULL;
> 
> Consistently hard tabs for indentation here, please.

Will do.

>> +	ptr = __vmap(&mfn, PFN_UP(offs + size), 1, 1,
>> +		     ACPI_MAP_MEM_ATTR, VMAP_DEFAULT);
>> +
>> +	return !ptr ? NULL : (ptr + offs);
> 
> Slightly odd that you don't let the success case go first,

I don't really see the problem. Are you nitpicking?

> the more that it's minimally shorter:
> 
> 	return ptr ? ptr + offs : NULL;
Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/4] xen/acpi: Rework acpi_os_map_memory() and acpi_os_unmap_memory()
  2020-09-28  9:58     ` Julien Grall
@ 2020-09-28 10:09       ` Jan Beulich
  2020-09-28 10:39         ` Julien Grall
  0 siblings, 1 reply; 51+ messages in thread
From: Jan Beulich @ 2020-09-28 10:09 UTC (permalink / raw)
  To: Julien Grall
  Cc: xen-devel, alex.bennee, masami.hiramatsu, ehem+xen,
	bertrand.marquis, andre.przywara, Julien Grall,
	Stefano Stabellini, Volodymyr Babchuk, Andrew Cooper,
	George Dunlap, Ian Jackson, Wei Liu, Roger Pau Monné

On 28.09.2020 11:58, Julien Grall wrote:
> On 28/09/2020 09:18, Jan Beulich wrote:
>> On 26.09.2020 22:55, Julien Grall wrote:
>>> --- a/xen/arch/x86/acpi/lib.c
>>> +++ b/xen/arch/x86/acpi/lib.c
>>> @@ -46,6 +46,10 @@ char *__acpi_map_table(paddr_t phys, unsigned long size)
>>>   	if ((phys + size) <= (1 * 1024 * 1024))
>>>   		return __va(phys);
>>>   
>>> +	/* No arch specific implementation after early boot */
>>> +	if (system_state >= SYS_STATE_boot)
>>> +		return NULL;
>>
>> Considering the code in context above, the comment isn't entirely
>> correct.
> 
> How about "No arch specific implementation after early boot but for the 
> first 1MB"?

That or simply "No further ...".

>>> +{
>>> +	unsigned long vaddr = (unsigned long)ptr;
>>> +
>>> +	if (vaddr >= DIRECTMAP_VIRT_START &&
>>> +	    vaddr < DIRECTMAP_VIRT_END) {
>>> +		ASSERT(!((__pa(ptr) + size - 1) >> 20));
>>> +		return true;
>>> +	}
>>> +
>>> +	return (vaddr >= __fix_to_virt(FIX_ACPI_END)) &&
>>> +		(vaddr < (__fix_to_virt(FIX_ACPI_BEGIN) + PAGE_SIZE));
>>
>> Indentation is slightly wrong here.
> 
> This is Linux coding style and therefore is using hard tab. Care to 
> explain the problem?

The two opening parentheses should align with one another,
shouldn't they?

>>> +	ptr = __vmap(&mfn, PFN_UP(offs + size), 1, 1,
>>> +		     ACPI_MAP_MEM_ATTR, VMAP_DEFAULT);
>>> +
>>> +	return !ptr ? NULL : (ptr + offs);
>>
>> Slightly odd that you don't let the success case go first,
> 
> I don't really see the problem. Are you nitpicking?
> 
>> the more that it's minimally shorter:
>>
>> 	return ptr ? ptr + offs : NULL;

Well, I said "slightly odd" as sort of a replacement of "Nit:".
But I really think it would be more logical the other way
around, not so much for how it looks like, but to aid the not
uncommon compiler heuristics of assuming the "true" path is
the common one.

Jan


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/4] xen/acpi: Rework acpi_os_map_memory() and acpi_os_unmap_memory()
  2020-09-28 10:09       ` Jan Beulich
@ 2020-09-28 10:39         ` Julien Grall
  2020-10-10  9:49           ` Julien Grall
  0 siblings, 1 reply; 51+ messages in thread
From: Julien Grall @ 2020-09-28 10:39 UTC (permalink / raw)
  To: Jan Beulich
  Cc: xen-devel, alex.bennee, masami.hiramatsu, ehem+xen,
	bertrand.marquis, andre.przywara, Julien Grall,
	Stefano Stabellini, Volodymyr Babchuk, Andrew Cooper,
	George Dunlap, Ian Jackson, Wei Liu, Roger Pau Monné



On 28/09/2020 11:09, Jan Beulich wrote:
> On 28.09.2020 11:58, Julien Grall wrote:
>> On 28/09/2020 09:18, Jan Beulich wrote:
>>> On 26.09.2020 22:55, Julien Grall wrote:
>>>> --- a/xen/arch/x86/acpi/lib.c
>>>> +++ b/xen/arch/x86/acpi/lib.c
>>>> @@ -46,6 +46,10 @@ char *__acpi_map_table(paddr_t phys, unsigned long size)
>>>>    	if ((phys + size) <= (1 * 1024 * 1024))
>>>>    		return __va(phys);
>>>>    
>>>> +	/* No arch specific implementation after early boot */
>>>> +	if (system_state >= SYS_STATE_boot)
>>>> +		return NULL;
>>>
>>> Considering the code in context above, the comment isn't entirely
>>> correct.
>>
>> How about "No arch specific implementation after early boot but for the
>> first 1MB"?
> 
> That or simply "No further ...".

I will do that.

>>>> +{
>>>> +	unsigned long vaddr = (unsigned long)ptr;
>>>> +
>>>> +	if (vaddr >= DIRECTMAP_VIRT_START &&
>>>> +	    vaddr < DIRECTMAP_VIRT_END) {
>>>> +		ASSERT(!((__pa(ptr) + size - 1) >> 20));
>>>> +		return true;
>>>> +	}
>>>> +
>>>> +	return (vaddr >= __fix_to_virt(FIX_ACPI_END)) &&
>>>> +		(vaddr < (__fix_to_virt(FIX_ACPI_BEGIN) + PAGE_SIZE));
>>>
>>> Indentation is slightly wrong here.
>>
>> This is Linux coding style and therefore is using hard tab. Care to
>> explain the problem?
> 
> The two opening parentheses should align with one another,
> shouldn't they?

Hmmm... somehow vim wants to indent this way. I am not entirely sure why...

I will force the indentation manually.

> 
>>>> +	ptr = __vmap(&mfn, PFN_UP(offs + size), 1, 1,
>>>> +		     ACPI_MAP_MEM_ATTR, VMAP_DEFAULT);
>>>> +
>>>> +	return !ptr ? NULL : (ptr + offs);
>>>
>>> Slightly odd that you don't let the success case go first,
>>
>> I don't really see the problem. Are you nitpicking?
>>
>>> the more that it's minimally shorter:
>>>
>>> 	return ptr ? ptr + offs : NULL;
> 
> Well, I said "slightly odd" as sort of a replacement of "Nit:".
> But I really think it would be more logical the other way
> around, not so much for how it looks like, but to aid the not
> uncommon compiler heuristics of assuming the "true" path is
> the common one.

Ah, you are trying to outsmart the compilers again...

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 0/4] xen/arm: Unbreak ACPI
  2020-09-28  6:47 ` Masami Hiramatsu
@ 2020-09-28 13:00   ` Masami Hiramatsu
  2020-10-16 22:33     ` Xen-ARM EFI/ACPI problems (was: Re: [PATCH 0/4] xen/arm: Unbreak ACPI) Elliott Mitchell
  2020-10-08 18:39   ` [PATCH 0/4] xen/arm: Unbreak ACPI Elliott Mitchell
                     ` (2 subsequent siblings)
  3 siblings, 1 reply; 51+ messages in thread
From: Masami Hiramatsu @ 2020-09-28 13:00 UTC (permalink / raw)
  To: Julien Grall
  Cc: xen-devel, Alex Bennée, ehem+xen, bertrand.marquis,
	andre.przywara, Julien Grall, Stefano Stabellini,
	Volodymyr Babchuk, Andrew Cooper, George Dunlap, Ian Jackson,
	Jan Beulich, Wei Liu, Roger Pau Monné

Hi,

I've missed the explanation of the attached patch. This prototype
patch was also needed for booting up the Xen on my box (for the system
which has no SPCR).

Thank you,

2020年9月28日(月) 15:47 Masami Hiramatsu <masami.hiramatsu@linaro.org>:
>
> Hello,
>
> This made progress with my Xen boot on DeveloperBox (
> https://www.96boards.org/product/developerbox/ ) with ACPI.
>
> Thank you,
>
>
> 2020年9月27日(日) 5:56 Julien Grall <julien@xen.org>:
>
> >
> > From: Julien Grall <jgrall@amazon.com>
> >
> > Hi all,
> >
> > Xen on ARM has been broken for quite a while on ACPI systems. This
> > series aims to fix it.
> >
> > Unfortunately I don't have a system with ACPI v6.0 or later (QEMU seems
> > to only support 5.1). So I did only some light testing.
> >
> > I have only build tested the x86 side so far.
> >
> > Cheers,
> >
> > *** BLURB HERE ***
> >
> > Julien Grall (4):
> >   xen/acpi: Rework acpi_os_map_memory() and acpi_os_unmap_memory()
> >   xen/arm: acpi: The fixmap area should always be cleared during
> >     failure/unmap
> >   xen/arm: Check if the platform is not using ACPI before initializing
> >     Dom0less
> >   xen/arm: Introduce fw_unreserved_regions() and use it
> >
> >  xen/arch/arm/acpi/lib.c     | 79 ++++++++++++++++++++++++++++++-------
> >  xen/arch/arm/kernel.c       |  2 +-
> >  xen/arch/arm/setup.c        | 25 +++++++++---
> >  xen/arch/x86/acpi/lib.c     | 18 +++++++++
> >  xen/drivers/acpi/osl.c      | 34 ++++++++--------
> >  xen/include/asm-arm/setup.h |  2 +-
> >  xen/include/xen/acpi.h      |  1 +
> >  7 files changed, 123 insertions(+), 38 deletions(-)
> >
> > --
> > 2.17.1
> >
>
>
> --
> Masami Hiramatsu



-- 
Masami Hiramatsu


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 0/4] xen/arm: Unbreak ACPI
  2020-09-26 20:55 [PATCH 0/4] xen/arm: Unbreak ACPI Julien Grall
                   ` (5 preceding siblings ...)
  2020-09-28  6:47 ` Masami Hiramatsu
@ 2020-09-29 11:10 ` Rahul Singh
  2020-09-29 15:29 ` Alex Bennée
  7 siblings, 0 replies; 51+ messages in thread
From: Rahul Singh @ 2020-09-29 11:10 UTC (permalink / raw)
  To: Julien Grall
  Cc: xen-devel, alex.bennee, masami.hiramatsu, ehem+xen,
	Bertrand Marquis, Andre Przywara, Julien Grall,
	Stefano Stabellini, Volodymyr Babchuk, Andrew Cooper,
	George Dunlap, Ian Jackson, Jan Beulich, Wei Liu,
	Roger Pau Monné

Hello,

> On 26 Sep 2020, at 9:55 pm, Julien Grall <julien@xen.org> wrote:
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> Hi all,
> 
> Xen on ARM has been broken for quite a while on ACPI systems. This
> series aims to fix it.

We also observed the panic after enabling the ACPI for ARM N1SDP board earlier.

(XEN) Xen call trace:
(XEN)    [<0000000000271b24>] set_fixmap+0x28/0x2c (PC)
(XEN)    [<0000000000271b18>] set_fixmap+0x1c/0x2c (LR)
(XEN)    [<0000000000267d58>] __acpi_map_table+0x38/0x94
(XEN)    [<00000000002501a4>] acpi_os_map_memory+0x18/0x64

After applying this patch series panic is fixed and XEN is able to boot with ACPI enabled. 

> 
> Unfortunately I don't have a system with ACPI v6.0 or later (QEMU seems
> to only support 5.1). So I did only some light testing.
> 
> I have only build tested the x86 side so far.
> 
> Cheers,
> 
> *** BLURB HERE ***
> 
> Julien Grall (4):
>  xen/acpi: Rework acpi_os_map_memory() and acpi_os_unmap_memory()
>  xen/arm: acpi: The fixmap area should always be cleared during
>    failure/unmap
>  xen/arm: Check if the platform is not using ACPI before initializing
>    Dom0less
>  xen/arm: Introduce fw_unreserved_regions() and use it
> 
> xen/arch/arm/acpi/lib.c     | 79 ++++++++++++++++++++++++++++++-------
> xen/arch/arm/kernel.c       |  2 +-
> xen/arch/arm/setup.c        | 25 +++++++++---
> xen/arch/x86/acpi/lib.c     | 18 +++++++++
> xen/drivers/acpi/osl.c      | 34 ++++++++--------
> xen/include/asm-arm/setup.h |  2 +-
> xen/include/xen/acpi.h      |  1 +
> 7 files changed, 123 insertions(+), 38 deletions(-)
> 
> -- 
> 2.17.1
> 
> 



^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/4] xen/acpi: Rework acpi_os_map_memory() and acpi_os_unmap_memory()
  2020-09-26 20:55 ` [PATCH 1/4] xen/acpi: Rework acpi_os_map_memory() and acpi_os_unmap_memory() Julien Grall
  2020-09-28  8:18   ` Jan Beulich
@ 2020-09-29 11:10   ` Rahul Singh
  2020-10-01  0:06   ` Stefano Stabellini
  2 siblings, 0 replies; 51+ messages in thread
From: Rahul Singh @ 2020-09-29 11:10 UTC (permalink / raw)
  To: Julien Grall
  Cc: xen-devel, alex.bennee, masami.hiramatsu, ehem+xen,
	Bertrand Marquis, Andre Przywara, Julien Grall,
	Stefano Stabellini, Volodymyr Babchuk, Andrew Cooper,
	George Dunlap, Ian Jackson, Jan Beulich, Wei Liu,
	Roger Pau Monné

Hello,

> On 26 Sep 2020, at 9:55 pm, Julien Grall <julien@xen.org> wrote:
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> The functions acpi_os_{un,}map_memory() are meant to be arch-agnostic
> while the __acpi_os_{un,}map_memory() are meant to be arch-specific.
> 
> Currently, the former are still containing x86 specific code.
> 
> To avoid this rather strange split, the generic helpers are reworked so
> they are arch-agnostic. This requires the introduction of a new helper
> __acpi_os_unmap_memory() that will undo any mapping done by
> __acpi_os_map_memory().
> 
> Currently, the arch-helper for unmap is basically a no-op so it only
> returns whether the mapping was arch specific. But this will change
> in the future.
> 
> Note that the x86 version of acpi_os_map_memory() was already able to
> able the 1MB region. Hence why there is no addition of new code.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Reviewed-by: Rahul Singh <rahul.singh@arm.com>
Tested-by: Rahul Singh <rahul.singh@arm.com>
> ---
> xen/arch/arm/acpi/lib.c | 10 ++++++++++
> xen/arch/x86/acpi/lib.c | 18 ++++++++++++++++++
> xen/drivers/acpi/osl.c  | 34 ++++++++++++++++++----------------
> xen/include/xen/acpi.h  |  1 +
> 4 files changed, 47 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/arch/arm/acpi/lib.c b/xen/arch/arm/acpi/lib.c
> index 4fc6e17322c1..2192a5519171 100644
> --- a/xen/arch/arm/acpi/lib.c
> +++ b/xen/arch/arm/acpi/lib.c
> @@ -30,6 +30,10 @@ char *__acpi_map_table(paddr_t phys, unsigned long size)
>     unsigned long base, offset, mapped_size;
>     int idx;
> 
> +    /* No arch specific implementation after early boot */
> +    if ( system_state >= SYS_STATE_boot )
> +        return NULL;
> +
>     offset = phys & (PAGE_SIZE - 1);
>     mapped_size = PAGE_SIZE - offset;
>     set_fixmap(FIXMAP_ACPI_BEGIN, maddr_to_mfn(phys), PAGE_HYPERVISOR);
> @@ -49,6 +53,12 @@ char *__acpi_map_table(paddr_t phys, unsigned long size)
>     return ((char *) base + offset);
> }
> 
> +bool __acpi_unmap_table(void *ptr, unsigned long size)
> +{
> +    return ( vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) &&
> +             vaddr < (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE) );
> +}
> +
> /* True to indicate PSCI 0.2+ is implemented */
> bool __init acpi_psci_present(void)
> {
> diff --git a/xen/arch/x86/acpi/lib.c b/xen/arch/x86/acpi/lib.c
> index 265b9ad81905..77803f4d4c63 100644
> --- a/xen/arch/x86/acpi/lib.c
> +++ b/xen/arch/x86/acpi/lib.c
> @@ -46,6 +46,10 @@ char *__acpi_map_table(paddr_t phys, unsigned long size)
> 	if ((phys + size) <= (1 * 1024 * 1024))
> 		return __va(phys);
> 
> +	/* No arch specific implementation after early boot */
> +	if (system_state >= SYS_STATE_boot)
> +		return NULL;
> +
> 	offset = phys & (PAGE_SIZE - 1);
> 	mapped_size = PAGE_SIZE - offset;
> 	set_fixmap(FIX_ACPI_END, phys);
> @@ -66,6 +70,20 @@ char *__acpi_map_table(paddr_t phys, unsigned long size)
> 	return ((char *) base + offset);
> }
> 
> +bool __acpi_unmap_table(void *ptr, unsigned long size)
> +{
> +	unsigned long vaddr = (unsigned long)ptr;
> +
> +	if (vaddr >= DIRECTMAP_VIRT_START &&
> +	    vaddr < DIRECTMAP_VIRT_END) {
> +		ASSERT(!((__pa(ptr) + size - 1) >> 20));
> +		return true;
> +	}
> +
> +	return (vaddr >= __fix_to_virt(FIX_ACPI_END)) &&
> +		(vaddr < (__fix_to_virt(FIX_ACPI_BEGIN) + PAGE_SIZE));
> +}
> +
> unsigned int acpi_get_processor_id(unsigned int cpu)
> {
> 	unsigned int acpiid, apicid;
> diff --git a/xen/drivers/acpi/osl.c b/xen/drivers/acpi/osl.c
> index 4c8bb7839eda..100eee72def2 100644
> --- a/xen/drivers/acpi/osl.c
> +++ b/xen/drivers/acpi/osl.c
> @@ -92,27 +92,29 @@ acpi_physical_address __init acpi_os_get_root_pointer(void)
> void __iomem *
> acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
> {
> -	if (system_state >= SYS_STATE_boot) {
> -		mfn_t mfn = _mfn(PFN_DOWN(phys));
> -		unsigned int offs = phys & (PAGE_SIZE - 1);
> -
> -		/* The low first Mb is always mapped on x86. */
> -		if (IS_ENABLED(CONFIG_X86) && !((phys + size - 1) >> 20))
> -			return __va(phys);
> -		return __vmap(&mfn, PFN_UP(offs + size), 1, 1,
> -			      ACPI_MAP_MEM_ATTR, VMAP_DEFAULT) + offs;
> -	}
> -	return __acpi_map_table(phys, size);
> +	void *ptr;
> +	mfn_t mfn = _mfn(PFN_DOWN(phys));
> +	unsigned int offs = phys & (PAGE_SIZE - 1);
> +
> +	/* Try the arch specific implementation first */
> +	ptr = __acpi_map_table(phys, size);
> +	if (ptr)
> +		return ptr;
> +
> +	/* No common implementation for early boot map */
> +	if (unlikely(system_state < SYS_STATE_boot))
> +	     return NULL;
> +
> +	ptr = __vmap(&mfn, PFN_UP(offs + size), 1, 1,
> +		     ACPI_MAP_MEM_ATTR, VMAP_DEFAULT);
> +
> +	return !ptr ? NULL : (ptr + offs);
> }
> 
> void acpi_os_unmap_memory(void __iomem * virt, acpi_size size)
> {
> -	if (IS_ENABLED(CONFIG_X86) &&
> -	    (unsigned long)virt >= DIRECTMAP_VIRT_START &&
> -	    (unsigned long)virt < DIRECTMAP_VIRT_END) {
> -		ASSERT(!((__pa(virt) + size - 1) >> 20));
> +	if (__acpi_unmap_table(virt, size))
> 		return;
> -	}
> 
> 	if (system_state >= SYS_STATE_boot)
> 		vunmap((void *)((unsigned long)virt & PAGE_MASK));
> diff --git a/xen/include/xen/acpi.h b/xen/include/xen/acpi.h
> index c945ab05c864..5a84a4bf54e0 100644
> --- a/xen/include/xen/acpi.h
> +++ b/xen/include/xen/acpi.h
> @@ -68,6 +68,7 @@ typedef int (*acpi_table_entry_handler) (struct acpi_subtable_header *header, co
> 
> unsigned int acpi_get_processor_id (unsigned int cpu);
> char * __acpi_map_table (paddr_t phys_addr, unsigned long size);
> +bool __acpi_unmap_table(void *ptr, unsigned long size);
> int acpi_boot_init (void);
> int acpi_boot_table_init (void);
> int acpi_numa_init (void);
> -- 
> 2.17.1
> 
> 

Regards,
Rahul



^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 2/4] xen/arm: acpi: The fixmap area should always be cleared during failure/unmap
  2020-09-26 20:55 ` [PATCH 2/4] xen/arm: acpi: The fixmap area should always be cleared during failure/unmap Julien Grall
@ 2020-09-29 11:13   ` Rahul Singh
  2020-10-01  0:30   ` Stefano Stabellini
  1 sibling, 0 replies; 51+ messages in thread
From: Rahul Singh @ 2020-09-29 11:13 UTC (permalink / raw)
  To: Julien Grall
  Cc: xen-devel, alex.bennee, masami.hiramatsu, ehem+xen,
	Bertrand Marquis, Andre Przywara, Julien Grall,
	Stefano Stabellini, Volodymyr Babchuk, Wei Xu

Hello,

> On 26 Sep 2020, at 9:55 pm, Julien Grall <julien@xen.org> wrote:
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> Commit 022387ee1ad3 "xen/arm: mm: Don't open-code Xen PT update in
> {set, clear}_fixmap()" enforced that each set_fixmap() should be
> paired with a clear_fixmap(). Any failure to follow the model would
> result to a platform crash.
> 
> Unfortunately, the use of fixmap in the ACPI code was overlooked as it
> is calling set_fixmap() but not clear_fixmap().
> 
> The function __acpi_os_map_table() is reworked so:
>    - We know before the mapping whether the fixmap region is big
>    enough for the mapping.
>    - It will fail if the fixmap is always inuse.
> 
> The function __acpi_os_unmap_table() will now call clear_fixmap().
> 
> Reported-by: Wei Xu <xuwei5@hisilicon.com>
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Reviewed-by: Rahul Singh <rahul.singh@arm.com>
Tested-by: Rahul Singh <rahul.singh@arm.com>

> 
> ---
> 
> The discussion on the original thread [1] suggested to also zap it on
> x86. This is technically not necessary today, so it is left alone for
> now.
> 
> I looked at making the fixmap code common but the index are inverted
> between Arm and x86.
> 
> [1] https://lore.kernel.org/xen-devel/5E26C935.9080107@hisilicon.com/
> ---
> xen/arch/arm/acpi/lib.c | 75 +++++++++++++++++++++++++++++++----------
> 1 file changed, 58 insertions(+), 17 deletions(-)
> 
> diff --git a/xen/arch/arm/acpi/lib.c b/xen/arch/arm/acpi/lib.c
> index 2192a5519171..eebaca695562 100644
> --- a/xen/arch/arm/acpi/lib.c
> +++ b/xen/arch/arm/acpi/lib.c
> @@ -25,38 +25,79 @@
> #include <xen/init.h>
> #include <xen/mm.h>
> 
> +static bool fixmap_inuse;
> +
> char *__acpi_map_table(paddr_t phys, unsigned long size)
> {
> -    unsigned long base, offset, mapped_size;
> -    int idx;
> +    unsigned long base, offset;
> +    mfn_t mfn;
> +    unsigned int idx;
> 
>     /* No arch specific implementation after early boot */
>     if ( system_state >= SYS_STATE_boot )
>         return NULL;
> 
>     offset = phys & (PAGE_SIZE - 1);
> -    mapped_size = PAGE_SIZE - offset;
> -    set_fixmap(FIXMAP_ACPI_BEGIN, maddr_to_mfn(phys), PAGE_HYPERVISOR);
> -    base = FIXMAP_ADDR(FIXMAP_ACPI_BEGIN);
> +    base = FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) + offset;
> +
> +    /* Check the fixmap is big enough to map the region */
> +    if ( (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE - base) < size )
> +        return NULL;
> +
> +    /* With the fixmap, we can only map one region at the time */
> +    if ( fixmap_inuse )
> +        return NULL;
> 
> -    /* Most cases can be covered by the below. */
> +    fixmap_inuse = true;
> +
> +    size += offset;
> +    mfn = maddr_to_mfn(phys);
>     idx = FIXMAP_ACPI_BEGIN;
> -    while ( mapped_size < size )
> -    {
> -        if ( ++idx > FIXMAP_ACPI_END )
> -            return NULL;    /* cannot handle this */
> -        phys += PAGE_SIZE;
> -        set_fixmap(idx, maddr_to_mfn(phys), PAGE_HYPERVISOR);
> -        mapped_size += PAGE_SIZE;
> -    }
> 
> -    return ((char *) base + offset);
> +    do {
> +        set_fixmap(idx, mfn, PAGE_HYPERVISOR);
> +        size -= min(size, (unsigned long)PAGE_SIZE);
> +        mfn = mfn_add(mfn, 1);
> +        idx++;
> +    } while ( size > 0 );
> +
> +    return (char *)base;
> }
> 
> bool __acpi_unmap_table(void *ptr, unsigned long size)
> {
> -    return ( vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) &&
> -             vaddr < (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE) );
> +    vaddr_t vaddr = (vaddr_t)ptr;
> +    unsigned int idx;
> +
> +    /* We are only handling fixmap address in the arch code */
> +    if ( vaddr < FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) ||
> +         vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_END) )
> +        return false;
> +
> +    /*
> +     * __acpi_map_table() will always return a pointer in the first page
> +     * for the ACPI fixmap region. The caller is expected to free with
> +     * the same address.
> +     */
> +    ASSERT((vaddr & PAGE_MASK) == FIXMAP_ADDR(FIXMAP_ACPI_BEGIN));
> +
> +    /* The region allocated fit in the ACPI fixmap region. */
> +    ASSERT(size < (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE - vaddr));
> +    ASSERT(fixmap_inuse);
> +
> +    fixmap_inuse = false;
> +
> +    size += FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) - vaddr;
> +    idx = FIXMAP_ACPI_BEGIN;
> +
> +    do
> +    {
> +        clear_fixmap(idx);
> +        size -= min(size, (unsigned long)PAGE_SIZE);
> +        idx++;
> +    } while ( size > 0 );
> +
> +    return true;
> }
> 
> /* True to indicate PSCI 0.2+ is implemented */
> -- 
> 2.17.1
> 
> 

Regards,
Rahul



^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 3/4] xen/arm: Check if the platform is not using ACPI before initializing Dom0less
  2020-09-26 20:55 ` [PATCH 3/4] xen/arm: Check if the platform is not using ACPI before initializing Dom0less Julien Grall
@ 2020-09-29 11:17   ` Rahul Singh
  2020-09-30 23:26   ` Stefano Stabellini
  1 sibling, 0 replies; 51+ messages in thread
From: Rahul Singh @ 2020-09-29 11:17 UTC (permalink / raw)
  To: Julien Grall
  Cc: xen-devel, alex.bennee, masami.hiramatsu, ehem+xen,
	Bertrand Marquis, Andre Przywara, Julien Grall,
	Stefano Stabellini, Volodymyr Babchuk

Hello	

> On 26 Sep 2020, at 9:55 pm, Julien Grall <julien@xen.org> wrote:
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> Dom0less requires a device-tree. However, since commit 6e3e77120378
> "xen/arm: setup: Relocate the Device-Tree later on in the boot", the
> device-tree will not get unflatten when using ACPI.
> 
> This will lead to a crash during boot.
> 
> Given the complexity to setup dom0less with ACPI (for instance how to
> assign device?), we should skip any code related to Dom0less when using
> ACPI.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Rahul Singh <rahul.singh@arm.com>
Tested-by: Rahul Singh <rahul.singh@arm.com>
> ---
> xen/arch/arm/setup.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index f16b33fa87a2..35e5bee04efa 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -987,7 +987,8 @@ void __init start_xen(unsigned long boot_phys_offset,
> 
>     system_state = SYS_STATE_active;
> 
> -    create_domUs();
> +    if ( acpi_disabled )
> +        create_domUs();
> 
>     domain_unpause_by_systemcontroller(dom0);
> 
> -- 
> 2.17.1
> 
> 

Regards,
Rahul



^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 0/4] xen/arm: Unbreak ACPI
  2020-09-27  1:47 ` [PATCH 0/4] xen/arm: Unbreak ACPI Elliott Mitchell
@ 2020-09-29 15:28   ` Elliott Mitchell
  0 siblings, 0 replies; 51+ messages in thread
From: Elliott Mitchell @ 2020-09-29 15:28 UTC (permalink / raw)
  To: Julien Grall
  Cc: xen-devel, alex.bennee, masami.hiramatsu, bertrand.marquis,
	andre.przywara, Julien Grall, Stefano Stabellini,
	Volodymyr Babchuk, Andrew Cooper, George Dunlap, Ian Jackson,
	Jan Beulich, Wei Liu, Roger Pau Monn??

On Sat, Sep 26, 2020 at 06:47:08PM -0700, Elliott Mitchell wrote:
> On the ARM device I'm trying to get operational the boot got distinctly
> further along so appears to be a distinct ACPI improvement here.

Just in case it wasn't clear, that does amount to:

Tested-by: Elliott Mitchell <ehem+xen@m5p.com>

Now to look over a different set of shoulders and find out whether the
next problem is what they've been running into, or a distinct one...


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg@m5p.com  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445




^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 0/4] xen/arm: Unbreak ACPI
  2020-09-26 20:55 [PATCH 0/4] xen/arm: Unbreak ACPI Julien Grall
                   ` (6 preceding siblings ...)
  2020-09-29 11:10 ` Rahul Singh
@ 2020-09-29 15:29 ` Alex Bennée
  2020-09-29 17:07   ` Julien Grall
  7 siblings, 1 reply; 51+ messages in thread
From: Alex Bennée @ 2020-09-29 15:29 UTC (permalink / raw)
  To: Julien Grall
  Cc: xen-devel, masami.hiramatsu, ehem+xen, bertrand.marquis,
	andre.przywara, Julien Grall, Stefano Stabellini,
	Volodymyr Babchuk, Andrew Cooper, George Dunlap, Ian Jackson,
	Jan Beulich, Wei Liu, Roger Pau Monné


Julien Grall <julien@xen.org> writes:

> From: Julien Grall <jgrall@amazon.com>
>
> Hi all,
>
> Xen on ARM has been broken for quite a while on ACPI systems. This
> series aims to fix it.
>
> Unfortunately I don't have a system with ACPI v6.0 or later (QEMU seems
> to only support 5.1). So I did only some light testing.

I was hoping to get more diagnostics out to get it working under QEMU
TCG so I think must of missed a step:

  Loading Xen 4.15-unstable ...
  Loading Linux 4.19.0-11-arm64 ...
  Loading initial ramdisk ...
  Using modules provided by bootloader in FDT
  Xen 4.15-unstable (c/s Sat Sep 26 21:55:42 2020 +0100 git:72f3d495d0) EFI loader
  ...silence...

I have a grub installed from testing on a buster base:

  dpkg --status grub-arm64-efi
  Version: 2.04-8

With:

  GRUB_CMDLINE_LINUX_DEFAULT=""
  GRUB_CMDLINE_LINUX="console=ttyAMA0"
  GRUB_CMDLINE_LINUX_XEN_REPLACE="console=hvc0 earlyprintk=xen"
  GRUB_CMDLINE_XEN="loglvl=all guest_loglvl=all com1=115200,8n1,0x3e8,5console=com1,vg"

And I built Xen with --enable-systemd and tweaked the hypervisor .config:

  CONFIG_EXPERT=y
  CONFIG_ACPI=y

So any pointers to make it more verbose would be helpful.

>
> I have only build tested the x86 side so far.
>
> Cheers,
>
> *** BLURB HERE ***
>
> Julien Grall (4):
>   xen/acpi: Rework acpi_os_map_memory() and acpi_os_unmap_memory()
>   xen/arm: acpi: The fixmap area should always be cleared during
>     failure/unmap
>   xen/arm: Check if the platform is not using ACPI before initializing
>     Dom0less
>   xen/arm: Introduce fw_unreserved_regions() and use it
>
>  xen/arch/arm/acpi/lib.c     | 79 ++++++++++++++++++++++++++++++-------
>  xen/arch/arm/kernel.c       |  2 +-
>  xen/arch/arm/setup.c        | 25 +++++++++---
>  xen/arch/x86/acpi/lib.c     | 18 +++++++++
>  xen/drivers/acpi/osl.c      | 34 ++++++++--------
>  xen/include/asm-arm/setup.h |  2 +-
>  xen/include/xen/acpi.h      |  1 +
>  7 files changed, 123 insertions(+), 38 deletions(-)


-- 
Alex Bennée


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 0/4] xen/arm: Unbreak ACPI
  2020-09-29 15:29 ` Alex Bennée
@ 2020-09-29 17:07   ` Julien Grall
  2020-09-29 21:11     ` Alex Bennée
  0 siblings, 1 reply; 51+ messages in thread
From: Julien Grall @ 2020-09-29 17:07 UTC (permalink / raw)
  To: Alex Bennée
  Cc: xen-devel, masami.hiramatsu, ehem+xen, bertrand.marquis,
	andre.przywara, Julien Grall, Stefano Stabellini,
	Volodymyr Babchuk, Andrew Cooper, George Dunlap, Ian Jackson,
	Jan Beulich, Wei Liu, Roger Pau Monné

Hi Alex,

On 29/09/2020 16:29, Alex Bennée wrote:
> 
> Julien Grall <julien@xen.org> writes:
> 
>> From: Julien Grall <jgrall@amazon.com>
>>
>> Hi all,
>>
>> Xen on ARM has been broken for quite a while on ACPI systems. This
>> series aims to fix it.
>>
>> Unfortunately I don't have a system with ACPI v6.0 or later (QEMU seems
>> to only support 5.1). So I did only some light testing.
> 
> I was hoping to get more diagnostics out to get it working under QEMU
> TCG so I think must of missed a step:
> 
>    Loading Xen 4.15-unstable ...
>    Loading Linux 4.19.0-11-arm64 ...
>    Loading initial ramdisk ...
>    Using modules provided by bootloader in FDT
>    Xen 4.15-unstable (c/s Sat Sep 26 21:55:42 2020 +0100 git:72f3d495d0) EFI loader
>    ...silence...
> 
> I have a grub installed from testing on a buster base:
> 
>    dpkg --status grub-arm64-efi
>    Version: 2.04-8
> 
> With:
> 
>    GRUB_CMDLINE_LINUX_DEFAULT=""
>    GRUB_CMDLINE_LINUX="console=ttyAMA0"
>    GRUB_CMDLINE_LINUX_XEN_REPLACE="console=hvc0 earlyprintk=xen"
>    GRUB_CMDLINE_XEN="loglvl=all guest_loglvl=all com1=115200,8n1,0x3e8,5console=com1,vg"
> 
> And I built Xen with --enable-systemd and tweaked the hypervisor .config:
> 
>    CONFIG_EXPERT=y
>    CONFIG_ACPI=y
> 
> So any pointers to make it more verbose would be helpful.

The error is hapenning before Xen setup the console. You can get early 
output on QEMU if you rebuild Xen with the following .config options:

CONFIG_DEBUG=y
CONFIG_EARLY_UART_CHOICE_PL011=y
CONFIG_EARLY_UART_PL011=y
CONFIG_EARLY_PRINTK=y
CONFIG_EARLY_UART_BASE_ADDRESS=0x09000000
CONFIG_EARLY_UART_PL011_BAUD_RATE=0
CONFIG_EARLY_PRINTK_INC="debug-pl011.inc"

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 0/4] xen/arm: Unbreak ACPI
  2020-09-29 17:07   ` Julien Grall
@ 2020-09-29 21:11     ` Alex Bennée
  2020-09-29 23:39       ` André Przywara
  2020-09-30  9:42       ` Julien Grall
  0 siblings, 2 replies; 51+ messages in thread
From: Alex Bennée @ 2020-09-29 21:11 UTC (permalink / raw)
  To: Julien Grall
  Cc: xen-devel, masami.hiramatsu, ehem+xen, bertrand.marquis,
	andre.przywara, Julien Grall, Stefano Stabellini,
	Volodymyr Babchuk, Andrew Cooper, George Dunlap, Ian Jackson,
	Jan Beulich, Wei Liu, Roger Pau Monné


Julien Grall <julien@xen.org> writes:

> Hi Alex,
>
> On 29/09/2020 16:29, Alex Bennée wrote:
>> 
>> Julien Grall <julien@xen.org> writes:
>> 
>>> From: Julien Grall <jgrall@amazon.com>
>>>
>>> Hi all,
>>>
>>> Xen on ARM has been broken for quite a while on ACPI systems. This
>>> series aims to fix it.
>>>
>>> Unfortunately I don't have a system with ACPI v6.0 or later (QEMU seems
>>> to only support 5.1). So I did only some light testing.
>> 
>> I was hoping to get more diagnostics out to get it working under QEMU
>> TCG so I think must of missed a step:
>> 
>>    Loading Xen 4.15-unstable ...
>>    Loading Linux 4.19.0-11-arm64 ...
>>    Loading initial ramdisk ...
>>    Using modules provided by bootloader in FDT
>>    Xen 4.15-unstable (c/s Sat Sep 26 21:55:42 2020 +0100 git:72f3d495d0) EFI loader
>>    ...silence...
>> 
>> I have a grub installed from testing on a buster base:
>> 
>>    dpkg --status grub-arm64-efi
>>    Version: 2.04-8
>> 
>> With:
>> 
>>    GRUB_CMDLINE_LINUX_DEFAULT=""
>>    GRUB_CMDLINE_LINUX="console=ttyAMA0"
>>    GRUB_CMDLINE_LINUX_XEN_REPLACE="console=hvc0 earlyprintk=xen"
>>    GRUB_CMDLINE_XEN="loglvl=all guest_loglvl=all com1=115200,8n1,0x3e8,5console=com1,vg"
>> 
>> And I built Xen with --enable-systemd and tweaked the hypervisor .config:
>> 
>>    CONFIG_EXPERT=y
>>    CONFIG_ACPI=y
>> 
>> So any pointers to make it more verbose would be helpful.
>
> The error is hapenning before Xen setup the console. You can get early 
> output on QEMU if you rebuild Xen with the following .config options:
>
> CONFIG_DEBUG=y
> CONFIG_EARLY_UART_CHOICE_PL011=y
> CONFIG_EARLY_UART_PL011=y
> CONFIG_EARLY_PRINTK=y
> CONFIG_EARLY_UART_BASE_ADDRESS=0x09000000
> CONFIG_EARLY_UART_PL011_BAUD_RATE=0
> CONFIG_EARLY_PRINTK_INC="debug-pl011.inc"

OK I can see it fails on the ACPI and then tries to fall back to FDT and
then fails to find the GIC:

  (XEN) CMDLINE[00000000f7bbe000]:chosen placeholder root=UUID=cf00cd3a-066b-4146-bedf-f811d3343077 ro console=hvc0 earlyprintk=xen
  (XEN)
  (XEN) Command line: placeholder loglvl=all guest_loglvl=all com1=115200,8n1,0x3e8,5console=com1,vg no-real-mode edd=off
  (XEN) parameter "placeholder" unknown!
  (XEN) parameter "no-real-mode" unknown!
  (XEN) parameter "edd" unknown!
  (XEN) ACPI: RSDP 138560000, 0024 (r2 BOCHS )
  (XEN) ACPI: XSDT 138550000, 004C (r1 BOCHS  BXPCFACP        1       1000013)
  (XEN) ACPI: FACP 138510000, 010C (r5 BOCHS  BXPCFACP        1 BXPC        1)
  (XEN) ACPI: DSDT 138520000, 14A6 (r2 BOCHS  BXPCDSDT        1 BXPC        1)
  (XEN) ACPI: APIC 138500000, 018C (r3 BOCHS  BXPCAPIC        1 BXPC        1)
  (XEN) ACPI: GTDT 1384F0000, 0060 (r2 BOCHS  BXPCGTDT        1 BXPC        1)
  (XEN) ACPI: MCFG 1384E0000, 003C (r1 BOCHS  BXPCMCFG        1 BXPC        1)
  (XEN) ACPI: SPCR 1384D0000, 0050 (r2 BOCHS  BXPCSPCR        1 BXPC        1)
  (XEN) Unsupported FADT revision 5.1, should be 6.0+, will disable ACPI
  (XEN) acpi_boot_table_init: FADT not found (-22)
  (XEN) Domain heap initialised
  (XEN) Booting using Device Tree
  (XEN) Platform: Generic System
  (XEN)
  (XEN) ****************************************
  (XEN) Panic on CPU 0:
  (XEN) Unable to find compatible GIC in the device tree
  (XEN) ****************************************
  (XEN)
  (XEN) Reboot in five seconds...

Despite saying it is going to reboot it never manages to. Any idea how
it is trying to reset the system?

-- 
Alex Bennée


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 0/4] xen/arm: Unbreak ACPI
  2020-09-29 21:11     ` Alex Bennée
@ 2020-09-29 23:39       ` André Przywara
  2020-09-30  8:51         ` Alex Bennée
  2020-09-30 10:35         ` Julien Grall
  2020-09-30  9:42       ` Julien Grall
  1 sibling, 2 replies; 51+ messages in thread
From: André Przywara @ 2020-09-29 23:39 UTC (permalink / raw)
  To: Alex Bennée, Julien Grall
  Cc: xen-devel, masami.hiramatsu, ehem+xen, bertrand.marquis,
	Julien Grall, Stefano Stabellini, Volodymyr Babchuk,
	Andrew Cooper, George Dunlap, Ian Jackson, Jan Beulich, Wei Liu,
	Roger Pau Monné

On 29/09/2020 22:11, Alex Bennée wrote:

Hi,

> Julien Grall <julien@xen.org> writes:
> 
>> Hi Alex,
>>
>> On 29/09/2020 16:29, Alex Bennée wrote:
>>>
>>> Julien Grall <julien@xen.org> writes:
>>>
>>>> From: Julien Grall <jgrall@amazon.com>
>>>>
>>>> Hi all,
>>>>
>>>> Xen on ARM has been broken for quite a while on ACPI systems. This
>>>> series aims to fix it.
>>>>
>>>> Unfortunately I don't have a system with ACPI v6.0 or later (QEMU seems
>>>> to only support 5.1).

Does QEMU provide ACPI tables? Or is this actually EDK2 generating them?

> So I did only some light testing.

So about that v6.0 or later: it seems like the requirement comes from:
commit 1c9bd43019cd "arm/acpi: Parse FADT table and get PSCI flags":
"Since STAO table and the GIC version are introduced by ACPI 6.0, we
will check the version and only parse FADT table with version >= 6.0. If
firmware provides ACPI tables with ACPI version less than 6.0, OS will
be messed up with those information, so disable ACPI if we get an FADT
table with version less than 6.0."

STAO (and XENV) have indeed been introduced by ACPI v6.0, but those
tables are supposed to be *generated* by Xen and handed down to Dom0,
they will never be provided by firmware (or parsed) by the Xen
hypervisor. Checking the Linux code it seems to actually not (yet?)
support the STAO named list, and currently finds and uses the STAO (UART
block bit) regardless of the FADT version number.

I can't find anything GIC related in the FADT, the whole GIC information
is described in MADT.

Linux/arm64 seems to be happy with ACPI >= v5.1:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/kernel/acpi.c#n148

So can we relax the v6.0 requirement, to be actually >= v5.1? That is
among the first to support ARM anyway, IIRC.

I have only a smattering about ACPI, so happy to stand corrected.

Cheers,
Andre

>>>
>>> I was hoping to get more diagnostics out to get it working under QEMU
>>> TCG so I think must of missed a step:
>>>
>>>    Loading Xen 4.15-unstable ...
>>>    Loading Linux 4.19.0-11-arm64 ...
>>>    Loading initial ramdisk ...
>>>    Using modules provided by bootloader in FDT
>>>    Xen 4.15-unstable (c/s Sat Sep 26 21:55:42 2020 +0100 git:72f3d495d0) EFI loader
>>>    ...silence...
>>>
>>> I have a grub installed from testing on a buster base:
>>>
>>>    dpkg --status grub-arm64-efi
>>>    Version: 2.04-8
>>>
>>> With:
>>>
>>>    GRUB_CMDLINE_LINUX_DEFAULT=""
>>>    GRUB_CMDLINE_LINUX="console=ttyAMA0"
>>>    GRUB_CMDLINE_LINUX_XEN_REPLACE="console=hvc0 earlyprintk=xen"
>>>    GRUB_CMDLINE_XEN="loglvl=all guest_loglvl=all com1=115200,8n1,0x3e8,5console=com1,vg"
>>>
>>> And I built Xen with --enable-systemd and tweaked the hypervisor .config:
>>>
>>>    CONFIG_EXPERT=y
>>>    CONFIG_ACPI=y
>>>
>>> So any pointers to make it more verbose would be helpful.
>>
>> The error is hapenning before Xen setup the console. You can get early 
>> output on QEMU if you rebuild Xen with the following .config options:
>>
>> CONFIG_DEBUG=y
>> CONFIG_EARLY_UART_CHOICE_PL011=y
>> CONFIG_EARLY_UART_PL011=y
>> CONFIG_EARLY_PRINTK=y
>> CONFIG_EARLY_UART_BASE_ADDRESS=0x09000000
>> CONFIG_EARLY_UART_PL011_BAUD_RATE=0
>> CONFIG_EARLY_PRINTK_INC="debug-pl011.inc"
> 
> OK I can see it fails on the ACPI and then tries to fall back to FDT and
> then fails to find the GIC:
> 
>   (XEN) CMDLINE[00000000f7bbe000]:chosen placeholder root=UUID=cf00cd3a-066b-4146-bedf-f811d3343077 ro console=hvc0 earlyprintk=xen
>   (XEN)
>   (XEN) Command line: placeholder loglvl=all guest_loglvl=all com1=115200,8n1,0x3e8,5console=com1,vg no-real-mode edd=off
>   (XEN) parameter "placeholder" unknown!
>   (XEN) parameter "no-real-mode" unknown!
>   (XEN) parameter "edd" unknown!
>   (XEN) ACPI: RSDP 138560000, 0024 (r2 BOCHS )
>   (XEN) ACPI: XSDT 138550000, 004C (r1 BOCHS  BXPCFACP        1       1000013)
>   (XEN) ACPI: FACP 138510000, 010C (r5 BOCHS  BXPCFACP        1 BXPC        1)
>   (XEN) ACPI: DSDT 138520000, 14A6 (r2 BOCHS  BXPCDSDT        1 BXPC        1)
>   (XEN) ACPI: APIC 138500000, 018C (r3 BOCHS  BXPCAPIC        1 BXPC        1)
>   (XEN) ACPI: GTDT 1384F0000, 0060 (r2 BOCHS  BXPCGTDT        1 BXPC        1)
>   (XEN) ACPI: MCFG 1384E0000, 003C (r1 BOCHS  BXPCMCFG        1 BXPC        1)
>   (XEN) ACPI: SPCR 1384D0000, 0050 (r2 BOCHS  BXPCSPCR        1 BXPC        1)
>   (XEN) Unsupported FADT revision 5.1, should be 6.0+, will disable ACPI
>   (XEN) acpi_boot_table_init: FADT not found (-22)
>   (XEN) Domain heap initialised
>   (XEN) Booting using Device Tree
>   (XEN) Platform: Generic System
>   (XEN)
>   (XEN) ****************************************
>   (XEN) Panic on CPU 0:
>   (XEN) Unable to find compatible GIC in the device tree
>   (XEN) ****************************************
>   (XEN)
>   (XEN) Reboot in five seconds...
> 
> Despite saying it is going to reboot it never manages to. Any idea how
> it is trying to reset the system?
> 



^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 0/4] xen/arm: Unbreak ACPI
  2020-09-29 23:39       ` André Przywara
@ 2020-09-30  8:51         ` Alex Bennée
  2020-09-30 10:35         ` Julien Grall
  1 sibling, 0 replies; 51+ messages in thread
From: Alex Bennée @ 2020-09-30  8:51 UTC (permalink / raw)
  To: André Przywara
  Cc: Julien Grall, xen-devel, masami.hiramatsu, ehem+xen,
	bertrand.marquis, Julien Grall, Stefano Stabellini,
	Volodymyr Babchuk, Andrew Cooper, George Dunlap, Ian Jackson,
	Jan Beulich, Wei Liu, Roger Pau Monné


André Przywara <andre.przywara@arm.com> writes:

> On 29/09/2020 22:11, Alex Bennée wrote:
>
> Hi,
>
>> Julien Grall <julien@xen.org> writes:
>> 
>>> Hi Alex,
>>>
>>> On 29/09/2020 16:29, Alex Bennée wrote:
>>>>
>>>> Julien Grall <julien@xen.org> writes:
>>>>
>>>>> From: Julien Grall <jgrall@amazon.com>
>>>>>
>>>>> Hi all,
>>>>>
>>>>> Xen on ARM has been broken for quite a while on ACPI systems. This
>>>>> series aims to fix it.
>>>>>
>>>>> Unfortunately I don't have a system with ACPI v6.0 or later (QEMU seems
>>>>> to only support 5.1).
>
> Does QEMU provide ACPI tables? Or is this actually EDK2 generating
> them?

It certainly does - whether EDK2 mangles them afterwards is another
question. For the -M virt machine we currently generate:

  /* FADT */
  static void build_fadt_rev5(GArray *table_data, BIOSLinker *linker,
                              VirtMachineState *vms, unsigned dsdt_tbl_offset)
  {
      /* ACPI v5.1 */
      AcpiFadtData fadt = {
          .rev = 5,
          .minor_ver = 1,
          .flags = 1 << ACPI_FADT_F_HW_REDUCED_ACPI,
          .xdsdt_tbl_offset = &dsdt_tbl_offset,
      };

      switch (vms->psci_conduit) {
      case QEMU_PSCI_CONDUIT_DISABLED:
          fadt.arm_boot_arch = 0;
          break;
      case QEMU_PSCI_CONDUIT_HVC:
          fadt.arm_boot_arch = ACPI_FADT_ARM_PSCI_COMPLIANT |
                               ACPI_FADT_ARM_PSCI_USE_HVC;
          break;
      case QEMU_PSCI_CONDUIT_SMC:
          fadt.arm_boot_arch = ACPI_FADT_ARM_PSCI_COMPLIANT;
          break;
      default:
          g_assert_not_reached();
      }

      build_fadt(table_data, linker, &fadt, NULL, NULL);
  }

Looking through the code it seems we stop around 5.0 - even the new
fancy x86 microvm ACPI tables are described as /* ACPI 5.0: 4.1
Hardware-Reduced ACPI */.

>
>> So I did only some light testing.
>
> So about that v6.0 or later: it seems like the requirement comes from:
> commit 1c9bd43019cd "arm/acpi: Parse FADT table and get PSCI flags":
> "Since STAO table and the GIC version are introduced by ACPI 6.0, we
> will check the version and only parse FADT table with version >= 6.0. If
> firmware provides ACPI tables with ACPI version less than 6.0, OS will
> be messed up with those information, so disable ACPI if we get an FADT
> table with version less than 6.0."
>
> STAO (and XENV) have indeed been introduced by ACPI v6.0, but those
> tables are supposed to be *generated* by Xen and handed down to Dom0,
> they will never be provided by firmware (or parsed) by the Xen
> hypervisor. Checking the Linux code it seems to actually not (yet?)
> support the STAO named list, and currently finds and uses the STAO (UART
> block bit) regardless of the FADT version number.
>
> I can't find anything GIC related in the FADT, the whole GIC information
> is described in MADT.

I think we are generating full data for both GIC2 and GIC3:

  /* MADT */
  static void
  build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
  {
      VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
      int madt_start = table_data->len;
      const MemMapEntry *memmap = vms->memmap;
      const int *irqmap = vms->irqmap;
      AcpiMadtGenericDistributor *gicd;
      AcpiMadtGenericMsiFrame *gic_msi;
      int i;

      acpi_data_push(table_data, sizeof(AcpiMultipleApicTable));

      gicd = acpi_data_push(table_data, sizeof *gicd);
      gicd->type = ACPI_APIC_GENERIC_DISTRIBUTOR;
      gicd->length = sizeof(*gicd);
      gicd->base_address = cpu_to_le64(memmap[VIRT_GIC_DIST].base);
      gicd->version = vms->gic_version;

      for (i = 0; i < vms->smp_cpus; i++) {
          AcpiMadtGenericCpuInterface *gicc = acpi_data_push(table_data,
                                                             sizeof(*gicc));
          ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(i));

          gicc->type = ACPI_APIC_GENERIC_CPU_INTERFACE;
          gicc->length = sizeof(*gicc);
          if (vms->gic_version == 2) {
              gicc->base_address = cpu_to_le64(memmap[VIRT_GIC_CPU].base);
              gicc->gich_base_address = cpu_to_le64(memmap[VIRT_GIC_HYP].base);
              gicc->gicv_base_address = cpu_to_le64(memmap[VIRT_GIC_VCPU].base);
          }
          gicc->cpu_interface_number = cpu_to_le32(i);
          gicc->arm_mpidr = cpu_to_le64(armcpu->mp_affinity);
          gicc->uid = cpu_to_le32(i);
          gicc->flags = cpu_to_le32(ACPI_MADT_GICC_ENABLED);

          if (arm_feature(&armcpu->env, ARM_FEATURE_PMU)) {
              gicc->performance_interrupt = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ));
          }
          if (vms->virt) {
              gicc->vgic_interrupt = cpu_to_le32(PPI(ARCH_GIC_MAINT_IRQ));
          }
      }

      if (vms->gic_version == 3) {
          AcpiMadtGenericTranslator *gic_its;
          int nb_redist_regions = virt_gicv3_redist_region_count(vms);
          AcpiMadtGenericRedistributor *gicr = acpi_data_push(table_data,
                                                           sizeof *gicr);

          gicr->type = ACPI_APIC_GENERIC_REDISTRIBUTOR;
          gicr->length = sizeof(*gicr);
          gicr->base_address = cpu_to_le64(memmap[VIRT_GIC_REDIST].base);
          gicr->range_length = cpu_to_le32(memmap[VIRT_GIC_REDIST].size);

          if (nb_redist_regions == 2) {
              gicr = acpi_data_push(table_data, sizeof(*gicr));
              gicr->type = ACPI_APIC_GENERIC_REDISTRIBUTOR;
              gicr->length = sizeof(*gicr);
              gicr->base_address =
                  cpu_to_le64(memmap[VIRT_HIGH_GIC_REDIST2].base);
              gicr->range_length =
                  cpu_to_le32(memmap[VIRT_HIGH_GIC_REDIST2].size);
          }

          if (its_class_name() && !vmc->no_its) {
              gic_its = acpi_data_push(table_data, sizeof *gic_its);
              gic_its->type = ACPI_APIC_GENERIC_TRANSLATOR;
              gic_its->length = sizeof(*gic_its);
              gic_its->translation_id = 0;
              gic_its->base_address = cpu_to_le64(memmap[VIRT_GIC_ITS].base);
          }
      } else {
          gic_msi = acpi_data_push(table_data, sizeof *gic_msi);
          gic_msi->type = ACPI_APIC_GENERIC_MSI_FRAME;
          gic_msi->length = sizeof(*gic_msi);
          gic_msi->gic_msi_frame_id = 0;
          gic_msi->base_address = cpu_to_le64(memmap[VIRT_GIC_V2M].base);
          gic_msi->flags = cpu_to_le32(1);
          gic_msi->spi_count = cpu_to_le16(NUM_GICV2M_SPIS);
          gic_msi->spi_base = cpu_to_le16(irqmap[VIRT_GIC_V2M] + ARM_SPI_BASE);
      }

      build_header(linker, table_data,
                   (void *)(table_data->data + madt_start), "APIC",
                   table_data->len - madt_start, 3, NULL, NULL);
  }


>
> Linux/arm64 seems to be happy with ACPI >= v5.1:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/kernel/acpi.c#n148
>
> So can we relax the v6.0 requirement, to be actually >= v5.1? That is
> among the first to support ARM anyway, IIRC.
>
> I have only a smattering about ACPI, so happy to stand corrected.

I'd certainly be happy with that as reduces the number of components
that need to be uprevved to get support.

>
> Cheers,
> Andre
>
>>>>
>>>> I was hoping to get more diagnostics out to get it working under QEMU
>>>> TCG so I think must of missed a step:
>>>>
>>>>    Loading Xen 4.15-unstable ...
>>>>    Loading Linux 4.19.0-11-arm64 ...
>>>>    Loading initial ramdisk ...
>>>>    Using modules provided by bootloader in FDT
>>>>    Xen 4.15-unstable (c/s Sat Sep 26 21:55:42 2020 +0100 git:72f3d495d0) EFI loader
>>>>    ...silence...
>>>>
>>>> I have a grub installed from testing on a buster base:
>>>>
>>>>    dpkg --status grub-arm64-efi
>>>>    Version: 2.04-8
>>>>
>>>> With:
>>>>
>>>>    GRUB_CMDLINE_LINUX_DEFAULT=""
>>>>    GRUB_CMDLINE_LINUX="console=ttyAMA0"
>>>>    GRUB_CMDLINE_LINUX_XEN_REPLACE="console=hvc0 earlyprintk=xen"
>>>>    GRUB_CMDLINE_XEN="loglvl=all guest_loglvl=all com1=115200,8n1,0x3e8,5console=com1,vg"
>>>>
>>>> And I built Xen with --enable-systemd and tweaked the hypervisor .config:
>>>>
>>>>    CONFIG_EXPERT=y
>>>>    CONFIG_ACPI=y
>>>>
>>>> So any pointers to make it more verbose would be helpful.
>>>
>>> The error is hapenning before Xen setup the console. You can get early 
>>> output on QEMU if you rebuild Xen with the following .config options:
>>>
>>> CONFIG_DEBUG=y
>>> CONFIG_EARLY_UART_CHOICE_PL011=y
>>> CONFIG_EARLY_UART_PL011=y
>>> CONFIG_EARLY_PRINTK=y
>>> CONFIG_EARLY_UART_BASE_ADDRESS=0x09000000
>>> CONFIG_EARLY_UART_PL011_BAUD_RATE=0
>>> CONFIG_EARLY_PRINTK_INC="debug-pl011.inc"
>> 
>> OK I can see it fails on the ACPI and then tries to fall back to FDT and
>> then fails to find the GIC:
>> 
>>   (XEN) CMDLINE[00000000f7bbe000]:chosen placeholder root=UUID=cf00cd3a-066b-4146-bedf-f811d3343077 ro console=hvc0 earlyprintk=xen
>>   (XEN)
>>   (XEN) Command line: placeholder loglvl=all guest_loglvl=all com1=115200,8n1,0x3e8,5console=com1,vg no-real-mode edd=off
>>   (XEN) parameter "placeholder" unknown!
>>   (XEN) parameter "no-real-mode" unknown!
>>   (XEN) parameter "edd" unknown!
>>   (XEN) ACPI: RSDP 138560000, 0024 (r2 BOCHS )
>>   (XEN) ACPI: XSDT 138550000, 004C (r1 BOCHS  BXPCFACP        1       1000013)
>>   (XEN) ACPI: FACP 138510000, 010C (r5 BOCHS  BXPCFACP        1 BXPC        1)
>>   (XEN) ACPI: DSDT 138520000, 14A6 (r2 BOCHS  BXPCDSDT        1 BXPC        1)
>>   (XEN) ACPI: APIC 138500000, 018C (r3 BOCHS  BXPCAPIC        1 BXPC        1)
>>   (XEN) ACPI: GTDT 1384F0000, 0060 (r2 BOCHS  BXPCGTDT        1 BXPC        1)
>>   (XEN) ACPI: MCFG 1384E0000, 003C (r1 BOCHS  BXPCMCFG        1 BXPC        1)
>>   (XEN) ACPI: SPCR 1384D0000, 0050 (r2 BOCHS  BXPCSPCR        1 BXPC        1)
>>   (XEN) Unsupported FADT revision 5.1, should be 6.0+, will disable ACPI
>>   (XEN) acpi_boot_table_init: FADT not found (-22)
>>   (XEN) Domain heap initialised
>>   (XEN) Booting using Device Tree
>>   (XEN) Platform: Generic System
>>   (XEN)
>>   (XEN) ****************************************
>>   (XEN) Panic on CPU 0:
>>   (XEN) Unable to find compatible GIC in the device tree
>>   (XEN) ****************************************
>>   (XEN)
>>   (XEN) Reboot in five seconds...
>> 
>> Despite saying it is going to reboot it never manages to. Any idea how
>> it is trying to reset the system?
>> 


-- 
Alex Bennée


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 0/4] xen/arm: Unbreak ACPI
  2020-09-29 21:11     ` Alex Bennée
  2020-09-29 23:39       ` André Przywara
@ 2020-09-30  9:42       ` Julien Grall
  2020-09-30 10:38         ` Alex Bennée
  1 sibling, 1 reply; 51+ messages in thread
From: Julien Grall @ 2020-09-30  9:42 UTC (permalink / raw)
  To: Alex Bennée
  Cc: xen-devel, masami.hiramatsu, ehem+xen, bertrand.marquis,
	andre.przywara, Julien Grall, Stefano Stabellini,
	Volodymyr Babchuk, Andrew Cooper, George Dunlap, Ian Jackson,
	Jan Beulich, Wei Liu, Roger Pau Monné

Hi Alex,

On 29/09/2020 22:11, Alex Bennée wrote:
> 
> Julien Grall <julien@xen.org> writes:
> 
>> Hi Alex,
>>
>> On 29/09/2020 16:29, Alex Bennée wrote:
>>>
>>> Julien Grall <julien@xen.org> writes:
>>>
>>>> From: Julien Grall <jgrall@amazon.com>
>>>>
>>>> Hi all,
>>>>
>>>> Xen on ARM has been broken for quite a while on ACPI systems. This
>>>> series aims to fix it.
>>>>
>>>> Unfortunately I don't have a system with ACPI v6.0 or later (QEMU seems
>>>> to only support 5.1). So I did only some light testing.
>>>
>>> I was hoping to get more diagnostics out to get it working under QEMU
>>> TCG so I think must of missed a step:
>>>
>>>     Loading Xen 4.15-unstable ...
>>>     Loading Linux 4.19.0-11-arm64 ...
>>>     Loading initial ramdisk ...
>>>     Using modules provided by bootloader in FDT
>>>     Xen 4.15-unstable (c/s Sat Sep 26 21:55:42 2020 +0100 git:72f3d495d0) EFI loader
>>>     ...silence...
>>>
>>> I have a grub installed from testing on a buster base:
>>>
>>>     dpkg --status grub-arm64-efi
>>>     Version: 2.04-8
>>>
>>> With:
>>>
>>>     GRUB_CMDLINE_LINUX_DEFAULT=""
>>>     GRUB_CMDLINE_LINUX="console=ttyAMA0"
>>>     GRUB_CMDLINE_LINUX_XEN_REPLACE="console=hvc0 earlyprintk=xen"
>>>     GRUB_CMDLINE_XEN="loglvl=all guest_loglvl=all com1=115200,8n1,0x3e8,5console=com1,vg"
>>>
>>> And I built Xen with --enable-systemd and tweaked the hypervisor .config:
>>>
>>>     CONFIG_EXPERT=y
>>>     CONFIG_ACPI=y
>>>
>>> So any pointers to make it more verbose would be helpful.
>>
>> The error is hapenning before Xen setup the console. You can get early
>> output on QEMU if you rebuild Xen with the following .config options:
>>
>> CONFIG_DEBUG=y
>> CONFIG_EARLY_UART_CHOICE_PL011=y
>> CONFIG_EARLY_UART_PL011=y
>> CONFIG_EARLY_PRINTK=y
>> CONFIG_EARLY_UART_BASE_ADDRESS=0x09000000
>> CONFIG_EARLY_UART_PL011_BAUD_RATE=0
>> CONFIG_EARLY_PRINTK_INC="debug-pl011.inc"
> 
> OK I can see it fails on the ACPI and then tries to fall back to FDT and
> then fails to find the GIC:
> 
>    (XEN) CMDLINE[00000000f7bbe000]:chosen placeholder root=UUID=cf00cd3a-066b-4146-bedf-f811d3343077 ro console=hvc0 earlyprintk=xen
>    (XEN)
>    (XEN) Command line: placeholder loglvl=all guest_loglvl=all com1=115200,8n1,0x3e8,5console=com1,vg no-real-mode edd=off
>    (XEN) parameter "placeholder" unknown!
>    (XEN) parameter "no-real-mode" unknown!
>    (XEN) parameter "edd" unknown!
>    (XEN) ACPI: RSDP 138560000, 0024 (r2 BOCHS )
>    (XEN) ACPI: XSDT 138550000, 004C (r1 BOCHS  BXPCFACP        1       1000013)
>    (XEN) ACPI: FACP 138510000, 010C (r5 BOCHS  BXPCFACP        1 BXPC        1)
>    (XEN) ACPI: DSDT 138520000, 14A6 (r2 BOCHS  BXPCDSDT        1 BXPC        1)
>    (XEN) ACPI: APIC 138500000, 018C (r3 BOCHS  BXPCAPIC        1 BXPC        1)
>    (XEN) ACPI: GTDT 1384F0000, 0060 (r2 BOCHS  BXPCGTDT        1 BXPC        1)
>    (XEN) ACPI: MCFG 1384E0000, 003C (r1 BOCHS  BXPCMCFG        1 BXPC        1)
>    (XEN) ACPI: SPCR 1384D0000, 0050 (r2 BOCHS  BXPCSPCR        1 BXPC        1)
>    (XEN) Unsupported FADT revision 5.1, should be 6.0+, will disable ACPI
>    (XEN) acpi_boot_table_init: FADT not found (-22)
>    (XEN) Domain heap initialised
>    (XEN) Booting using Device Tree
>    (XEN) Platform: Generic System
>    (XEN)
>    (XEN) ****************************************
>    (XEN) Panic on CPU 0:
>    (XEN) Unable to find compatible GIC in the device tree
>    (XEN) ****************************************
>    (XEN)
>    (XEN) Reboot in five seconds...
> 
> Despite saying it is going to reboot it never manages to. Any idea how
> it is trying to reset the system?

This is a bit of chicken and eggs problem. To know the reset method, you 
need to parse the ACPI tables. As we can't parse then we don't know the 
reset method. So, Xen will just do an infinite loop.

It would probably be good to be more forthcoming with the users and say 
it will not reboot.

Also, IIRC, the time subsystem is not yet initialized. So it might be 
possible to mdelay() doesn't work properly.

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 0/4] xen/arm: Unbreak ACPI
  2020-09-29 23:39       ` André Przywara
  2020-09-30  8:51         ` Alex Bennée
@ 2020-09-30 10:35         ` Julien Grall
  1 sibling, 0 replies; 51+ messages in thread
From: Julien Grall @ 2020-09-30 10:35 UTC (permalink / raw)
  To: André Przywara, Alex Bennée
  Cc: xen-devel, masami.hiramatsu, ehem+xen, bertrand.marquis,
	Julien Grall, Stefano Stabellini, Volodymyr Babchuk

(Removing the x86 folks from the CC list)

Hi André,

On 30/09/2020 00:39, André Przywara wrote:
> On 29/09/2020 22:11, Alex Bennée wrote:
> 
> Hi,
> 
>> Julien Grall <julien@xen.org> writes:
>>
>>> Hi Alex,
>>>
>>> On 29/09/2020 16:29, Alex Bennée wrote:
>>>>
>>>> Julien Grall <julien@xen.org> writes:
>>>>
>>>>> From: Julien Grall <jgrall@amazon.com>
>>>>>
>>>>> Hi all,
>>>>>
>>>>> Xen on ARM has been broken for quite a while on ACPI systems. This
>>>>> series aims to fix it.
>>>>>
>>>>> Unfortunately I don't have a system with ACPI v6.0 or later (QEMU seems
>>>>> to only support 5.1).
> 
> Does QEMU provide ACPI tables? Or is this actually EDK2 generating them?
> 
>> So I did only some light testing.
> 
> So about that v6.0 or later: it seems like the requirement comes from:
> commit 1c9bd43019cd "arm/acpi: Parse FADT table and get PSCI flags":
> "Since STAO table and the GIC version are introduced by ACPI 6.0, we
> will check the version and only parse FADT table with version >= 6.0. If
> firmware provides ACPI tables with ACPI version less than 6.0, OS will
> be messed up with those information, so disable ACPI if we get an FADT
> table with version less than 6.0."
> 
> STAO (and XENV) have indeed been introduced by ACPI v6.0, but those
> tables are supposed to be *generated* by Xen and handed down to Dom0,
> they will never be provided by firmware (or parsed) by the Xen
> hypervisor. Checking the Linux code it seems to actually not (yet?)
> support the STAO named list,

I may be because we don't yet use the named list in Xen. I am not sure 
if other hypervisor ever used the STAO.

> and currently finds and uses the STAO (UART
> block bit) regardless of the FADT version number.

IIRC, the concern at the time is an OS may decide to bail out if it 
finds a table that is not meant to exist.

> I can't find anything GIC related in the FADT, the whole GIC information
> is described in MADT.

My memory is quite fuzzy... IIRC the problem is (was?) related to how 
Linux used to check the size of the MADT.

Before commit [1], Linux was checking that the MADT entry was matching 
the ACPI version. As Xen generates the MADT using the ACPI 6.0 layout, 
it wouldn't be possible to boot Linux.

> 
> Linux/arm64 seems to be happy with ACPI >= v5.1:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/kernel/acpi.c#n148
> 
> So can we relax the v6.0 requirement, to be actually >= v5.1? That is
> among the first to support ARM anyway, IIRC.

I remember trying to relax the requiring in the past. However, I can't 
remember why I didn't upstream it. There was possibly some issues I 
could not get around?

I think supporting ACPI 5.1 would be useful. So I would suggest to 
attempt it again and see what breaks.

Cheers,

[1]
commit 9eb1c92b47c73249465d388eaa394fe436a3b489
Author: Jeremy Linton <jeremy.linton@arm.com>
Date:   Tue Nov 27 17:59:12 2018 +0000

     arm64: acpi: Prepare for longer MADTs

     The BAD_MADT_GICC_ENTRY check is a little too strict because
     it rejects MADT entries that don't match the currently known
     lengths. We should remove this restriction to avoid problems
     if the table length changes. Future code which might depend on
     additional fields should be written to validate those fields
     before using them, rather than trying to globally check
     known MADT version lengths.

     Link: 
https://lkml.kernel.org/r/20181012192937.3819951-1-jeremy.linton@arm.com
     Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
     [lorenzo.pieralisi@arm.com: added MADT macro comments]
     Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
     Acked-by: Sudeep Holla <sudeep.holla@arm.com>
     Cc: Will Deacon <will.deacon@arm.com>
     Cc: Catalin Marinas <catalin.marinas@arm.com>
     Cc: Al Stone <ahs3@redhat.com>
     Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
     Signed-off-by: Will Deacon <will.deacon@arm.com>

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 0/4] xen/arm: Unbreak ACPI
  2020-09-30  9:42       ` Julien Grall
@ 2020-09-30 10:38         ` Alex Bennée
  2020-09-30 11:10           ` Julien Grall
  0 siblings, 1 reply; 51+ messages in thread
From: Alex Bennée @ 2020-09-30 10:38 UTC (permalink / raw)
  To: Julien Grall
  Cc: xen-devel, masami.hiramatsu, ehem+xen, bertrand.marquis,
	andre.przywara, Julien Grall, Stefano Stabellini,
	Volodymyr Babchuk, Andrew Cooper, George Dunlap, Ian Jackson,
	Jan Beulich, Wei Liu, Roger Pau Monné


Julien Grall <julien@xen.org> writes:

> Hi Alex,
>
> On 29/09/2020 22:11, Alex Bennée wrote:
>> 
>> Julien Grall <julien@xen.org> writes:
>> 
>>> Hi Alex,
>>>
>>> On 29/09/2020 16:29, Alex Bennée wrote:
>>>>
>>>> Julien Grall <julien@xen.org> writes:
>>>>
>>>>> From: Julien Grall <jgrall@amazon.com>
>>>>>
>>>>> Hi all,
>>>>>
>>>>> Xen on ARM has been broken for quite a while on ACPI systems. This
>>>>> series aims to fix it.
>>>>>
>>>>> Unfortunately I don't have a system with ACPI v6.0 or later (QEMU seems
>>>>> to only support 5.1). So I did only some light testing.
>>>>
>>>> I was hoping to get more diagnostics out to get it working under QEMU
>>>> TCG so I think must of missed a step:
>>>>
>>>>     Loading Xen 4.15-unstable ...
>>>>     Loading Linux 4.19.0-11-arm64 ...
>>>>     Loading initial ramdisk ...
>>>>     Using modules provided by bootloader in FDT
>>>>     Xen 4.15-unstable (c/s Sat Sep 26 21:55:42 2020 +0100 git:72f3d495d0) EFI loader
>>>>     ...silence...
>>>>
>>>> I have a grub installed from testing on a buster base:
>>>>
>>>>     dpkg --status grub-arm64-efi
>>>>     Version: 2.04-8
>>>>
>>>> With:
>>>>
>>>>     GRUB_CMDLINE_LINUX_DEFAULT=""
>>>>     GRUB_CMDLINE_LINUX="console=ttyAMA0"
>>>>     GRUB_CMDLINE_LINUX_XEN_REPLACE="console=hvc0 earlyprintk=xen"
>>>>     GRUB_CMDLINE_XEN="loglvl=all guest_loglvl=all com1=115200,8n1,0x3e8,5console=com1,vg"
>>>>
>>>> And I built Xen with --enable-systemd and tweaked the hypervisor .config:
>>>>
>>>>     CONFIG_EXPERT=y
>>>>     CONFIG_ACPI=y
>>>>
>>>> So any pointers to make it more verbose would be helpful.
>>>
>>> The error is hapenning before Xen setup the console. You can get early
>>> output on QEMU if you rebuild Xen with the following .config options:
>>>
>>> CONFIG_DEBUG=y
>>> CONFIG_EARLY_UART_CHOICE_PL011=y
>>> CONFIG_EARLY_UART_PL011=y
>>> CONFIG_EARLY_PRINTK=y
>>> CONFIG_EARLY_UART_BASE_ADDRESS=0x09000000
>>> CONFIG_EARLY_UART_PL011_BAUD_RATE=0
>>> CONFIG_EARLY_PRINTK_INC="debug-pl011.inc"
>> 
>> OK I can see it fails on the ACPI and then tries to fall back to FDT and
>> then fails to find the GIC:
>> 
>>    (XEN) CMDLINE[00000000f7bbe000]:chosen placeholder root=UUID=cf00cd3a-066b-4146-bedf-f811d3343077 ro console=hvc0 earlyprintk=xen
>>    (XEN)
>>    (XEN) Command line: placeholder loglvl=all guest_loglvl=all com1=115200,8n1,0x3e8,5console=com1,vg no-real-mode edd=off
>>    (XEN) parameter "placeholder" unknown!
>>    (XEN) parameter "no-real-mode" unknown!
>>    (XEN) parameter "edd" unknown!
>>    (XEN) ACPI: RSDP 138560000, 0024 (r2 BOCHS )
>>    (XEN) ACPI: XSDT 138550000, 004C (r1 BOCHS  BXPCFACP        1       1000013)
>>    (XEN) ACPI: FACP 138510000, 010C (r5 BOCHS  BXPCFACP        1 BXPC        1)
>>    (XEN) ACPI: DSDT 138520000, 14A6 (r2 BOCHS  BXPCDSDT        1 BXPC        1)
>>    (XEN) ACPI: APIC 138500000, 018C (r3 BOCHS  BXPCAPIC        1 BXPC        1)
>>    (XEN) ACPI: GTDT 1384F0000, 0060 (r2 BOCHS  BXPCGTDT        1 BXPC        1)
>>    (XEN) ACPI: MCFG 1384E0000, 003C (r1 BOCHS  BXPCMCFG        1 BXPC        1)
>>    (XEN) ACPI: SPCR 1384D0000, 0050 (r2 BOCHS  BXPCSPCR        1 BXPC        1)
>>    (XEN) Unsupported FADT revision 5.1, should be 6.0+, will disable ACPI
>>    (XEN) acpi_boot_table_init: FADT not found (-22)
>>    (XEN) Domain heap initialised
>>    (XEN) Booting using Device Tree
>>    (XEN) Platform: Generic System
>>    (XEN)
>>    (XEN) ****************************************
>>    (XEN) Panic on CPU 0:
>>    (XEN) Unable to find compatible GIC in the device tree
>>    (XEN) ****************************************
>>    (XEN)
>>    (XEN) Reboot in five seconds...
>> 
>> Despite saying it is going to reboot it never manages to. Any idea how
>> it is trying to reset the system?
>
> This is a bit of chicken and eggs problem. To know the reset method, you 
> need to parse the ACPI tables. As we can't parse then we don't know the 
> reset method. So, Xen will just do an infinite loop.

Well you do get some ACPI tables - downgrading the minimum at least
restores the reset method detection. I wonder if it would be worth
defaulting to PSCI if you don't know rather than hang indefinitely?

FWIW the failure after that is failing to find the GIC - I'm just
looking at the MADT table parsing now. Why am I getting a sense of
DejaVu?

> It would probably be good to be more forthcoming with the users and say 
> it will not reboot.
>
> Also, IIRC, the time subsystem is not yet initialized. So it might be 
> possible to mdelay() doesn't work properly.

Surely that's an architectural subsystem so there is no reason that
couldn't be up and running.

>
> Cheers,


-- 
Alex Bennée


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 0/4] xen/arm: Unbreak ACPI
  2020-09-30 10:38         ` Alex Bennée
@ 2020-09-30 11:10           ` Julien Grall
  0 siblings, 0 replies; 51+ messages in thread
From: Julien Grall @ 2020-09-30 11:10 UTC (permalink / raw)
  To: Alex Bennée
  Cc: xen-devel, masami.hiramatsu, ehem+xen, bertrand.marquis,
	andre.przywara, Julien Grall, Stefano Stabellini,
	Volodymyr Babchuk

Hi Alex,

On 30/09/2020 11:38, Alex Bennée wrote:
> 
> Julien Grall <julien@xen.org> writes:
> 
>> Hi Alex,
>>
>> On 29/09/2020 22:11, Alex Bennée wrote:
>>>
>>> Julien Grall <julien@xen.org> writes:
>>>
>>>> Hi Alex,
>>>>
>>>> On 29/09/2020 16:29, Alex Bennée wrote:
>>>>>
>>>>> Julien Grall <julien@xen.org> writes:
>>>>>
>>>>>> From: Julien Grall <jgrall@amazon.com>
>>>>>>
>>>>>> Hi all,
>>>>>>
>>>>>> Xen on ARM has been broken for quite a while on ACPI systems. This
>>>>>> series aims to fix it.
>>>>>>
>>>>>> Unfortunately I don't have a system with ACPI v6.0 or later (QEMU seems
>>>>>> to only support 5.1). So I did only some light testing.
>>>>>
>>>>> I was hoping to get more diagnostics out to get it working under QEMU
>>>>> TCG so I think must of missed a step:
>>>>>
>>>>>      Loading Xen 4.15-unstable ...
>>>>>      Loading Linux 4.19.0-11-arm64 ...
>>>>>      Loading initial ramdisk ...
>>>>>      Using modules provided by bootloader in FDT
>>>>>      Xen 4.15-unstable (c/s Sat Sep 26 21:55:42 2020 +0100 git:72f3d495d0) EFI loader
>>>>>      ...silence...
>>>>>
>>>>> I have a grub installed from testing on a buster base:
>>>>>
>>>>>      dpkg --status grub-arm64-efi
>>>>>      Version: 2.04-8
>>>>>
>>>>> With:
>>>>>
>>>>>      GRUB_CMDLINE_LINUX_DEFAULT=""
>>>>>      GRUB_CMDLINE_LINUX="console=ttyAMA0"
>>>>>      GRUB_CMDLINE_LINUX_XEN_REPLACE="console=hvc0 earlyprintk=xen"
>>>>>      GRUB_CMDLINE_XEN="loglvl=all guest_loglvl=all com1=115200,8n1,0x3e8,5console=com1,vg"
>>>>>
>>>>> And I built Xen with --enable-systemd and tweaked the hypervisor .config:
>>>>>
>>>>>      CONFIG_EXPERT=y
>>>>>      CONFIG_ACPI=y
>>>>>
>>>>> So any pointers to make it more verbose would be helpful.
>>>>
>>>> The error is hapenning before Xen setup the console. You can get early
>>>> output on QEMU if you rebuild Xen with the following .config options:
>>>>
>>>> CONFIG_DEBUG=y
>>>> CONFIG_EARLY_UART_CHOICE_PL011=y
>>>> CONFIG_EARLY_UART_PL011=y
>>>> CONFIG_EARLY_PRINTK=y
>>>> CONFIG_EARLY_UART_BASE_ADDRESS=0x09000000
>>>> CONFIG_EARLY_UART_PL011_BAUD_RATE=0
>>>> CONFIG_EARLY_PRINTK_INC="debug-pl011.inc"
>>>
>>> OK I can see it fails on the ACPI and then tries to fall back to FDT and
>>> then fails to find the GIC:
>>>
>>>     (XEN) CMDLINE[00000000f7bbe000]:chosen placeholder root=UUID=cf00cd3a-066b-4146-bedf-f811d3343077 ro console=hvc0 earlyprintk=xen
>>>     (XEN)
>>>     (XEN) Command line: placeholder loglvl=all guest_loglvl=all com1=115200,8n1,0x3e8,5console=com1,vg no-real-mode edd=off
>>>     (XEN) parameter "placeholder" unknown!
>>>     (XEN) parameter "no-real-mode" unknown!
>>>     (XEN) parameter "edd" unknown!
>>>     (XEN) ACPI: RSDP 138560000, 0024 (r2 BOCHS )
>>>     (XEN) ACPI: XSDT 138550000, 004C (r1 BOCHS  BXPCFACP        1       1000013)
>>>     (XEN) ACPI: FACP 138510000, 010C (r5 BOCHS  BXPCFACP        1 BXPC        1)
>>>     (XEN) ACPI: DSDT 138520000, 14A6 (r2 BOCHS  BXPCDSDT        1 BXPC        1)
>>>     (XEN) ACPI: APIC 138500000, 018C (r3 BOCHS  BXPCAPIC        1 BXPC        1)
>>>     (XEN) ACPI: GTDT 1384F0000, 0060 (r2 BOCHS  BXPCGTDT        1 BXPC        1)
>>>     (XEN) ACPI: MCFG 1384E0000, 003C (r1 BOCHS  BXPCMCFG        1 BXPC        1)
>>>     (XEN) ACPI: SPCR 1384D0000, 0050 (r2 BOCHS  BXPCSPCR        1 BXPC        1)
>>>     (XEN) Unsupported FADT revision 5.1, should be 6.0+, will disable ACPI
>>>     (XEN) acpi_boot_table_init: FADT not found (-22)
>>>     (XEN) Domain heap initialised
>>>     (XEN) Booting using Device Tree
>>>     (XEN) Platform: Generic System
>>>     (XEN)
>>>     (XEN) ****************************************
>>>     (XEN) Panic on CPU 0:
>>>     (XEN) Unable to find compatible GIC in the device tree
>>>     (XEN) ****************************************
>>>     (XEN)
>>>     (XEN) Reboot in five seconds...
>>>
>>> Despite saying it is going to reboot it never manages to. Any idea how
>>> it is trying to reset the system?
>>
>> This is a bit of chicken and eggs problem. To know the reset method, you
>> need to parse the ACPI tables. As we can't parse then we don't know the
>> reset method. So, Xen will just do an infinite loop.
> 
> Well you do get some ACPI tables - downgrading the minimum at least
> restores the reset method detection. I wonder if it would be worth
> defaulting to PSCI if you don't know rather than hang indefinitely?

The risk is probably low enough to try to use PSCI even on platform not 
supporting it.

Although, it might be worth to check if EL3 is present to avoid 
panicking again and again on XGene.

> 
> FWIW the failure after that is failing to find the GIC - I'm just
> looking at the MADT table parsing now. Why am I getting a sense of
> DejaVu?

The ACPI code in Xen is based on the first ACPI implementation in Linux. 
So it is quite possible you encountered the bug there :).

>> It would probably be good to be more forthcoming with the users and say
>> it will not reboot.
>>
>> Also, IIRC, the time subsystem is not yet initialized. So it might be
>> possible to mdelay() doesn't work properly.
> 
> Surely that's an architectural subsystem so there is no reason that
> couldn't be up and running.

In theory yes, but the code is also catering some interesting/weird 
platforms behavior:
    1) There are (were?) platform where CNTFREQ was not set correctly
    2) Some platforms, such as the one with Exynos 5, (used to?) require 
specific code to enable the arch timer.

We are still using the Arndale for automated testing. So we would need 
to keep the hacks.

But it would be possible to rework the code and try to make the timer 
available earlier for well-behaved platforms.

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 3/4] xen/arm: Check if the platform is not using ACPI before initializing Dom0less
  2020-09-26 20:55 ` [PATCH 3/4] xen/arm: Check if the platform is not using ACPI before initializing Dom0less Julien Grall
  2020-09-29 11:17   ` Rahul Singh
@ 2020-09-30 23:26   ` Stefano Stabellini
  1 sibling, 0 replies; 51+ messages in thread
From: Stefano Stabellini @ 2020-09-30 23:26 UTC (permalink / raw)
  To: Julien Grall
  Cc: xen-devel, alex.bennee, masami.hiramatsu, ehem+xen,
	bertrand.marquis, andre.przywara, Julien Grall,
	Stefano Stabellini, Volodymyr Babchuk

On Sat, 26 Sep 2020, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> Dom0less requires a device-tree. However, since commit 6e3e77120378
> "xen/arm: setup: Relocate the Device-Tree later on in the boot", the
> device-tree will not get unflatten when using ACPI.
> 
> This will lead to a crash during boot.
> 
> Given the complexity to setup dom0less with ACPI (for instance how to
> assign device?), we should skip any code related to Dom0less when using
> ACPI.

Yeah...

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> Signed-off-by: Julien Grall <jgrall@amazon.com>
> ---
>  xen/arch/arm/setup.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index f16b33fa87a2..35e5bee04efa 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -987,7 +987,8 @@ void __init start_xen(unsigned long boot_phys_offset,
>  
>      system_state = SYS_STATE_active;
>  
> -    create_domUs();
> +    if ( acpi_disabled )
> +        create_domUs();
>  
>      domain_unpause_by_systemcontroller(dom0);
>  
> -- 
> 2.17.1
> 


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 4/4] xen/arm: Introduce fw_unreserved_regions() and use it
  2020-09-26 20:55 ` [PATCH 4/4] xen/arm: Introduce fw_unreserved_regions() and use it Julien Grall
@ 2020-09-30 23:40   ` Stefano Stabellini
  2020-10-01 15:34     ` Julien Grall
  0 siblings, 1 reply; 51+ messages in thread
From: Stefano Stabellini @ 2020-09-30 23:40 UTC (permalink / raw)
  To: Julien Grall
  Cc: xen-devel, alex.bennee, masami.hiramatsu, ehem+xen,
	bertrand.marquis, andre.przywara, Julien Grall,
	Stefano Stabellini, Volodymyr Babchuk

On Sat, 26 Sep 2020, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> Since commit 6e3e77120378 "xen/arm: setup: Relocate the Device-Tree
> later on in the boot", the device-tree will not be kept mapped when
> using ACPI.
> 
> However, a few places are calling dt_unreserved_regions() which expects
> a valid DT. This will lead to a crash.
> 
> As the DT should not be used for ACPI (other than for detecting the
> modules), a new function fw_unreserved_regions() is introduced.
> 
> It will behave the same way on DT system. On ACPI system, it will
> unreserve the whole region.

The patch is good.

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


I have a small suggestion for improvement that could be done on commit:
given that bootinfo is actually used on EFI systems (granted, not
bootinfo.reserved_mem but bootinfo.mem, see
xen/arch/arm/efi/efi-boot.h:efi_process_memory_map_bootinfo) so
technically bootinfo could be in-use with ACPI, maybe we could add a
comment on top of xen/include/asm-arm/setup.h:bootinfo to say that
reserved_mem is device tree only?



> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> ---
> 
> Is there any region we should exclude on ACPI?
> ---
>  xen/arch/arm/kernel.c       |  2 +-
>  xen/arch/arm/setup.c        | 22 +++++++++++++++++-----
>  xen/include/asm-arm/setup.h |  2 +-
>  3 files changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> index 032923853f2c..ab78689ed2a6 100644
> --- a/xen/arch/arm/kernel.c
> +++ b/xen/arch/arm/kernel.c
> @@ -307,7 +307,7 @@ static __init int kernel_decompress(struct bootmodule *mod)
>       * Free the original kernel, update the pointers to the
>       * decompressed kernel
>       */
> -    dt_unreserved_regions(addr, addr + size, init_domheap_pages, 0);
> +    fw_unreserved_regions(addr, addr + size, init_domheap_pages, 0);
>  
>      return 0;
>  }
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 35e5bee04efa..7fcff9af2a7e 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -196,8 +196,9 @@ static void __init processor_id(void)
>      processor_setup();
>  }
>  
> -void __init dt_unreserved_regions(paddr_t s, paddr_t e,
> -                                  void (*cb)(paddr_t, paddr_t), int first)
> +static void __init dt_unreserved_regions(paddr_t s, paddr_t e,
> +                                         void (*cb)(paddr_t, paddr_t),
> +                                         int first)
>  {
>      int i, nr = fdt_num_mem_rsv(device_tree_flattened);
>  
> @@ -244,6 +245,17 @@ void __init dt_unreserved_regions(paddr_t s, paddr_t e,
>      cb(s, e);
>  }
>  
> +void __init fw_unreserved_regions(paddr_t s, paddr_t e,
> +                                  void (*cb)(paddr_t, paddr_t), int first)
> +{
> +    if ( acpi_disabled )
> +        dt_unreserved_regions(s, e, cb, first);
> +    else
> +        cb(s, e);
> +}
> +
> +
> +
>  struct bootmodule __init *add_boot_module(bootmodule_kind kind,
>                                            paddr_t start, paddr_t size,
>                                            bool domU)
> @@ -405,7 +417,7 @@ void __init discard_initial_modules(void)
>               !mfn_valid(maddr_to_mfn(e)) )
>              continue;
>  
> -        dt_unreserved_regions(s, e, init_domheap_pages, 0);
> +        fw_unreserved_regions(s, e, init_domheap_pages, 0);
>      }
>  
>      mi->nr_mods = 0;
> @@ -712,7 +724,7 @@ static void __init setup_mm(void)
>                  n = mfn_to_maddr(mfn_add(xenheap_mfn_start, xenheap_pages));
>              }
>  
> -            dt_unreserved_regions(s, e, init_boot_pages, 0);
> +            fw_unreserved_regions(s, e, init_boot_pages, 0);
>  
>              s = n;
>          }
> @@ -765,7 +777,7 @@ static void __init setup_mm(void)
>              if ( e > bank_end )
>                  e = bank_end;
>  
> -            dt_unreserved_regions(s, e, init_boot_pages, 0);
> +            fw_unreserved_regions(s, e, init_boot_pages, 0);
>              s = n;
>          }
>      }
> diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
> index 2f8f24e286ed..34df23247b87 100644
> --- a/xen/include/asm-arm/setup.h
> +++ b/xen/include/asm-arm/setup.h
> @@ -96,7 +96,7 @@ int construct_dom0(struct domain *d);
>  void create_domUs(void);
>  
>  void discard_initial_modules(void);
> -void dt_unreserved_regions(paddr_t s, paddr_t e,
> +void fw_unreserved_regions(paddr_t s, paddr_t e,
>                             void (*cb)(paddr_t, paddr_t), int first);
>  
>  size_t boot_fdt_info(const void *fdt, paddr_t paddr);
> -- 
> 2.17.1
> 


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/4] xen/acpi: Rework acpi_os_map_memory() and acpi_os_unmap_memory()
  2020-09-26 20:55 ` [PATCH 1/4] xen/acpi: Rework acpi_os_map_memory() and acpi_os_unmap_memory() Julien Grall
  2020-09-28  8:18   ` Jan Beulich
  2020-09-29 11:10   ` Rahul Singh
@ 2020-10-01  0:06   ` Stefano Stabellini
  2020-10-01 15:09     ` Julien Grall
  2 siblings, 1 reply; 51+ messages in thread
From: Stefano Stabellini @ 2020-10-01  0:06 UTC (permalink / raw)
  To: Julien Grall
  Cc: xen-devel, alex.bennee, masami.hiramatsu, ehem+xen,
	bertrand.marquis, andre.przywara, Julien Grall,
	Stefano Stabellini, Volodymyr Babchuk, Andrew Cooper,
	George Dunlap, Ian Jackson, Jan Beulich, Wei Liu,
	Roger Pau Monné

On Sat, 26 Sep 2020, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> The functions acpi_os_{un,}map_memory() are meant to be arch-agnostic
> while the __acpi_os_{un,}map_memory() are meant to be arch-specific.
> 
> Currently, the former are still containing x86 specific code.
> 
> To avoid this rather strange split, the generic helpers are reworked so
> they are arch-agnostic. This requires the introduction of a new helper
> __acpi_os_unmap_memory() that will undo any mapping done by
> __acpi_os_map_memory().
> 
> Currently, the arch-helper for unmap is basically a no-op so it only
> returns whether the mapping was arch specific. But this will change
> in the future.
> 
> Note that the x86 version of acpi_os_map_memory() was already able to
> able the 1MB region. Hence why there is no addition of new code.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> ---
>  xen/arch/arm/acpi/lib.c | 10 ++++++++++
>  xen/arch/x86/acpi/lib.c | 18 ++++++++++++++++++
>  xen/drivers/acpi/osl.c  | 34 ++++++++++++++++++----------------
>  xen/include/xen/acpi.h  |  1 +
>  4 files changed, 47 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/arch/arm/acpi/lib.c b/xen/arch/arm/acpi/lib.c
> index 4fc6e17322c1..2192a5519171 100644
> --- a/xen/arch/arm/acpi/lib.c
> +++ b/xen/arch/arm/acpi/lib.c
> @@ -30,6 +30,10 @@ char *__acpi_map_table(paddr_t phys, unsigned long size)
>      unsigned long base, offset, mapped_size;
>      int idx;
>  
> +    /* No arch specific implementation after early boot */
> +    if ( system_state >= SYS_STATE_boot )
> +        return NULL;
> +
>      offset = phys & (PAGE_SIZE - 1);
>      mapped_size = PAGE_SIZE - offset;
>      set_fixmap(FIXMAP_ACPI_BEGIN, maddr_to_mfn(phys), PAGE_HYPERVISOR);
> @@ -49,6 +53,12 @@ char *__acpi_map_table(paddr_t phys, unsigned long size)
>      return ((char *) base + offset);
>  }
>  
> +bool __acpi_unmap_table(void *ptr, unsigned long size)
> +{
> +    return ( vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) &&
> +             vaddr < (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE) );
> +}

vaddr or ptr?  :-)

lib.c: In function '__acpi_unmap_table':
lib.c:58:14: error: 'vaddr' undeclared (first use in this function)
     return ( vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) &&
              ^
lib.c:58:14: note: each undeclared identifier is reported only once for each function it appears in
lib.c:60:1: error: control reaches end of non-void function [-Werror=return-type]
 }
 ^
cc1: all warnings being treated as errors



>  /* True to indicate PSCI 0.2+ is implemented */
>  bool __init acpi_psci_present(void)
>  {
> diff --git a/xen/arch/x86/acpi/lib.c b/xen/arch/x86/acpi/lib.c
> index 265b9ad81905..77803f4d4c63 100644
> --- a/xen/arch/x86/acpi/lib.c
> +++ b/xen/arch/x86/acpi/lib.c
> @@ -46,6 +46,10 @@ char *__acpi_map_table(paddr_t phys, unsigned long size)
>  	if ((phys + size) <= (1 * 1024 * 1024))
>  		return __va(phys);
>  
> +	/* No arch specific implementation after early boot */
> +	if (system_state >= SYS_STATE_boot)
> +		return NULL;
> +
>  	offset = phys & (PAGE_SIZE - 1);
>  	mapped_size = PAGE_SIZE - offset;
>  	set_fixmap(FIX_ACPI_END, phys);
> @@ -66,6 +70,20 @@ char *__acpi_map_table(paddr_t phys, unsigned long size)
>  	return ((char *) base + offset);
>  }
>  
> +bool __acpi_unmap_table(void *ptr, unsigned long size)
> +{
> +	unsigned long vaddr = (unsigned long)ptr;
> +
> +	if (vaddr >= DIRECTMAP_VIRT_START &&
> +	    vaddr < DIRECTMAP_VIRT_END) {
> +		ASSERT(!((__pa(ptr) + size - 1) >> 20));
> +		return true;
> +	}
> +
> +	return (vaddr >= __fix_to_virt(FIX_ACPI_END)) &&
> +		(vaddr < (__fix_to_virt(FIX_ACPI_BEGIN) + PAGE_SIZE));
> +}
> +
>  unsigned int acpi_get_processor_id(unsigned int cpu)
>  {
>  	unsigned int acpiid, apicid;
> diff --git a/xen/drivers/acpi/osl.c b/xen/drivers/acpi/osl.c
> index 4c8bb7839eda..100eee72def2 100644
> --- a/xen/drivers/acpi/osl.c
> +++ b/xen/drivers/acpi/osl.c
> @@ -92,27 +92,29 @@ acpi_physical_address __init acpi_os_get_root_pointer(void)
>  void __iomem *
>  acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
>  {
> -	if (system_state >= SYS_STATE_boot) {
> -		mfn_t mfn = _mfn(PFN_DOWN(phys));
> -		unsigned int offs = phys & (PAGE_SIZE - 1);
> -		/* The low first Mb is always mapped on x86. */
> -		if (IS_ENABLED(CONFIG_X86) && !((phys + size - 1) >> 20))
> -			return __va(phys);
> -		return __vmap(&mfn, PFN_UP(offs + size), 1, 1,
> -			      ACPI_MAP_MEM_ATTR, VMAP_DEFAULT) + offs;
> -	}
> -	return __acpi_map_table(phys, size);
> +	void *ptr;
> +	mfn_t mfn = _mfn(PFN_DOWN(phys));
> +	unsigned int offs = phys & (PAGE_SIZE - 1);
> +
> +	/* Try the arch specific implementation first */
> +	ptr = __acpi_map_table(phys, size);
> +	if (ptr)
> +		return ptr;
> +
> +	/* No common implementation for early boot map */
> +	if (unlikely(system_state < SYS_STATE_boot))
> +	     return NULL;
> +
> +	ptr = __vmap(&mfn, PFN_UP(offs + size), 1, 1,
> +		     ACPI_MAP_MEM_ATTR, VMAP_DEFAULT);
> +
> +	return !ptr ? NULL : (ptr + offs);
>  }
>  
>  void acpi_os_unmap_memory(void __iomem * virt, acpi_size size)
>  {
> -	if (IS_ENABLED(CONFIG_X86) &&
> -	    (unsigned long)virt >= DIRECTMAP_VIRT_START &&
> -	    (unsigned long)virt < DIRECTMAP_VIRT_END) {
> -		ASSERT(!((__pa(virt) + size - 1) >> 20));
> +	if (__acpi_unmap_table(virt, size))
>  		return;
> -	}
>  
>  	if (system_state >= SYS_STATE_boot)
>  		vunmap((void *)((unsigned long)virt & PAGE_MASK));
> diff --git a/xen/include/xen/acpi.h b/xen/include/xen/acpi.h
> index c945ab05c864..5a84a4bf54e0 100644
> --- a/xen/include/xen/acpi.h
> +++ b/xen/include/xen/acpi.h
> @@ -68,6 +68,7 @@ typedef int (*acpi_table_entry_handler) (struct acpi_subtable_header *header, co
>  
>  unsigned int acpi_get_processor_id (unsigned int cpu);
>  char * __acpi_map_table (paddr_t phys_addr, unsigned long size);
> +bool __acpi_unmap_table(void *ptr, unsigned long size);
>  int acpi_boot_init (void);
>  int acpi_boot_table_init (void);
>  int acpi_numa_init (void);
> -- 
> 2.17.1
> 


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 2/4] xen/arm: acpi: The fixmap area should always be cleared during failure/unmap
  2020-09-26 20:55 ` [PATCH 2/4] xen/arm: acpi: The fixmap area should always be cleared during failure/unmap Julien Grall
  2020-09-29 11:13   ` Rahul Singh
@ 2020-10-01  0:30   ` Stefano Stabellini
  2020-10-01 15:14     ` Julien Grall
  1 sibling, 1 reply; 51+ messages in thread
From: Stefano Stabellini @ 2020-10-01  0:30 UTC (permalink / raw)
  To: Julien Grall
  Cc: xen-devel, alex.bennee, masami.hiramatsu, ehem+xen,
	bertrand.marquis, andre.przywara, Julien Grall,
	Stefano Stabellini, Volodymyr Babchuk, Wei Xu

On Sat, 26 Sep 2020, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> Commit 022387ee1ad3 "xen/arm: mm: Don't open-code Xen PT update in
> {set, clear}_fixmap()" enforced that each set_fixmap() should be
> paired with a clear_fixmap(). Any failure to follow the model would
> result to a platform crash.
> 
> Unfortunately, the use of fixmap in the ACPI code was overlooked as it
> is calling set_fixmap() but not clear_fixmap().
> 
> The function __acpi_os_map_table() is reworked so:
>     - We know before the mapping whether the fixmap region is big
>     enough for the mapping.
>     - It will fail if the fixmap is always inuse.

I take you mean "it will fail if the fixmap is *already* in use"?

If so, can it be a problem? Or the expectation is that in practice
__acpi_os_map_table() will only get called once before SYS_STATE_boot?

Looking at the code it would seem that even before this patch
__acpi_os_map_table() wasn't able to handle multiple calls before
SYS_STATE_boot.


> 
> The function __acpi_os_unmap_table() will now call clear_fixmap().
> 
> Reported-by: Wei Xu <xuwei5@hisilicon.com>
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> ---
> 
> The discussion on the original thread [1] suggested to also zap it on
> x86. This is technically not necessary today, so it is left alone for
> now.
> 
> I looked at making the fixmap code common but the index are inverted
> between Arm and x86.
> 
> [1] https://lore.kernel.org/xen-devel/5E26C935.9080107@hisilicon.com/
> ---
>  xen/arch/arm/acpi/lib.c | 75 +++++++++++++++++++++++++++++++----------
>  1 file changed, 58 insertions(+), 17 deletions(-)
> 
> diff --git a/xen/arch/arm/acpi/lib.c b/xen/arch/arm/acpi/lib.c
> index 2192a5519171..eebaca695562 100644
> --- a/xen/arch/arm/acpi/lib.c
> +++ b/xen/arch/arm/acpi/lib.c
> @@ -25,38 +25,79 @@
>  #include <xen/init.h>
>  #include <xen/mm.h>
>  
> +static bool fixmap_inuse;
> +
>  char *__acpi_map_table(paddr_t phys, unsigned long size)
>  {
> -    unsigned long base, offset, mapped_size;
> -    int idx;
> +    unsigned long base, offset;
> +    mfn_t mfn;
> +    unsigned int idx;
>  
>      /* No arch specific implementation after early boot */
>      if ( system_state >= SYS_STATE_boot )
>          return NULL;
>  
>      offset = phys & (PAGE_SIZE - 1);
> -    mapped_size = PAGE_SIZE - offset;
> -    set_fixmap(FIXMAP_ACPI_BEGIN, maddr_to_mfn(phys), PAGE_HYPERVISOR);
> -    base = FIXMAP_ADDR(FIXMAP_ACPI_BEGIN);
> +    base = FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) + offset;
> +
> +    /* Check the fixmap is big enough to map the region */
> +    if ( (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE - base) < size )
> +        return NULL;
> +
> +    /* With the fixmap, we can only map one region at the time */
> +    if ( fixmap_inuse )
> +        return NULL;
>  
> -    /* Most cases can be covered by the below. */
> +    fixmap_inuse = true;
> +
> +    size += offset;
> +    mfn = maddr_to_mfn(phys);
>      idx = FIXMAP_ACPI_BEGIN;
> -    while ( mapped_size < size )
> -    {
> -        if ( ++idx > FIXMAP_ACPI_END )
> -            return NULL;    /* cannot handle this */
> -        phys += PAGE_SIZE;
> -        set_fixmap(idx, maddr_to_mfn(phys), PAGE_HYPERVISOR);
> -        mapped_size += PAGE_SIZE;
> -    }
>  
> -    return ((char *) base + offset);
> +    do {
> +        set_fixmap(idx, mfn, PAGE_HYPERVISOR);
> +        size -= min(size, (unsigned long)PAGE_SIZE);
> +        mfn = mfn_add(mfn, 1);
> +        idx++;
> +    } while ( size > 0 );
> +
> +    return (char *)base;
>  }
>  
>  bool __acpi_unmap_table(void *ptr, unsigned long size)
>  {
> -    return ( vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) &&
> -             vaddr < (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE) );
> +    vaddr_t vaddr = (vaddr_t)ptr;
> +    unsigned int idx;
> +
> +    /* We are only handling fixmap address in the arch code */
> +    if ( vaddr < FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) ||
> +         vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_END) )

The "+ PAGE_SIZE" got lost


> +        return false;
> +
> +    /*
> +     * __acpi_map_table() will always return a pointer in the first page
> +     * for the ACPI fixmap region. The caller is expected to free with
> +     * the same address.
> +     */
> +    ASSERT((vaddr & PAGE_MASK) == FIXMAP_ADDR(FIXMAP_ACPI_BEGIN));
> +
> +    /* The region allocated fit in the ACPI fixmap region. */
> +    ASSERT(size < (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE - vaddr));
> +    ASSERT(fixmap_inuse);
> +
> +    fixmap_inuse = false;
> +
> +    size += FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) - vaddr;

Sorry I got confused.. Shouldn't this be:

  size += vaddr - FIXMAP_ADDR(FIXMAP_ACPI_BEGIN);

?


> +    idx = FIXMAP_ACPI_BEGIN;
> +
> +    do
> +    {
> +        clear_fixmap(idx);
> +        size -= min(size, (unsigned long)PAGE_SIZE);
> +        idx++;
> +    } while ( size > 0 );
> +
> +    return true;
>  }
>  
>  /* True to indicate PSCI 0.2+ is implemented */
> -- 
> 2.17.1
> 


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/4] xen/acpi: Rework acpi_os_map_memory() and acpi_os_unmap_memory()
  2020-10-01  0:06   ` Stefano Stabellini
@ 2020-10-01 15:09     ` Julien Grall
  0 siblings, 0 replies; 51+ messages in thread
From: Julien Grall @ 2020-10-01 15:09 UTC (permalink / raw)
  To: Stefano Stabellini
  Cc: xen-devel, alex.bennee, masami.hiramatsu, ehem+xen,
	bertrand.marquis, andre.przywara, Julien Grall,
	Volodymyr Babchuk, Andrew Cooper, George Dunlap, Ian Jackson,
	Jan Beulich, Wei Liu, Roger Pau Monné

Hi Stefano,

On 01/10/2020 01:06, Stefano Stabellini wrote:
> On Sat, 26 Sep 2020, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> The functions acpi_os_{un,}map_memory() are meant to be arch-agnostic
>> while the __acpi_os_{un,}map_memory() are meant to be arch-specific.
>>
>> Currently, the former are still containing x86 specific code.
>>
>> To avoid this rather strange split, the generic helpers are reworked so
>> they are arch-agnostic. This requires the introduction of a new helper
>> __acpi_os_unmap_memory() that will undo any mapping done by
>> __acpi_os_map_memory().
>>
>> Currently, the arch-helper for unmap is basically a no-op so it only
>> returns whether the mapping was arch specific. But this will change
>> in the future.
>>
>> Note that the x86 version of acpi_os_map_memory() was already able to
>> able the 1MB region. Hence why there is no addition of new code.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>> ---
>>   xen/arch/arm/acpi/lib.c | 10 ++++++++++
>>   xen/arch/x86/acpi/lib.c | 18 ++++++++++++++++++
>>   xen/drivers/acpi/osl.c  | 34 ++++++++++++++++++----------------
>>   xen/include/xen/acpi.h  |  1 +
>>   4 files changed, 47 insertions(+), 16 deletions(-)
>>
>> diff --git a/xen/arch/arm/acpi/lib.c b/xen/arch/arm/acpi/lib.c
>> index 4fc6e17322c1..2192a5519171 100644
>> --- a/xen/arch/arm/acpi/lib.c
>> +++ b/xen/arch/arm/acpi/lib.c
>> @@ -30,6 +30,10 @@ char *__acpi_map_table(paddr_t phys, unsigned long size)
>>       unsigned long base, offset, mapped_size;
>>       int idx;
>>   
>> +    /* No arch specific implementation after early boot */
>> +    if ( system_state >= SYS_STATE_boot )
>> +        return NULL;
>> +
>>       offset = phys & (PAGE_SIZE - 1);
>>       mapped_size = PAGE_SIZE - offset;
>>       set_fixmap(FIXMAP_ACPI_BEGIN, maddr_to_mfn(phys), PAGE_HYPERVISOR);
>> @@ -49,6 +53,12 @@ char *__acpi_map_table(paddr_t phys, unsigned long size)
>>       return ((char *) base + offset);
>>   }
>>   
>> +bool __acpi_unmap_table(void *ptr, unsigned long size)
>> +{
>> +    return ( vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) &&
>> +             vaddr < (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE) );
>> +}
> 
> vaddr or ptr?  :-)
> 
> lib.c: In function '__acpi_unmap_table':
> lib.c:58:14: error: 'vaddr' undeclared (first use in this function)
>       return ( vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) &&
>                ^
> lib.c:58:14: note: each undeclared identifier is reported only once for each function it appears in
> lib.c:60:1: error: control reaches end of non-void function [-Werror=return-type]
>   }
>   ^
> cc1: all warnings being treated as errors

The whole series builds because 'vaddr' is added in the next patch. But 
I forgot to build patch by patch :(.

I will fix it in the next version.

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 2/4] xen/arm: acpi: The fixmap area should always be cleared during failure/unmap
  2020-10-01  0:30   ` Stefano Stabellini
@ 2020-10-01 15:14     ` Julien Grall
  0 siblings, 0 replies; 51+ messages in thread
From: Julien Grall @ 2020-10-01 15:14 UTC (permalink / raw)
  To: Stefano Stabellini
  Cc: xen-devel, alex.bennee, masami.hiramatsu, ehem+xen,
	bertrand.marquis, andre.przywara, Julien Grall,
	Volodymyr Babchuk, Wei Xu

Hi Stefano,

On 01/10/2020 01:30, Stefano Stabellini wrote:
> On Sat, 26 Sep 2020, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> Commit 022387ee1ad3 "xen/arm: mm: Don't open-code Xen PT update in
>> {set, clear}_fixmap()" enforced that each set_fixmap() should be
>> paired with a clear_fixmap(). Any failure to follow the model would
>> result to a platform crash.
>>
>> Unfortunately, the use of fixmap in the ACPI code was overlooked as it
>> is calling set_fixmap() but not clear_fixmap().
>>
>> The function __acpi_os_map_table() is reworked so:
>>      - We know before the mapping whether the fixmap region is big
>>      enough for the mapping.
>>      - It will fail if the fixmap is always inuse.
> 
> I take you mean "it will fail if the fixmap is *already* in use"?

Yes.

> 
> If so, can it be a problem? Or the expectation is that in practice
> __acpi_os_map_table() will only get called once before SYS_STATE_boot?
> 
> Looking at the code it would seem that even before this patch
> __acpi_os_map_table() wasn't able to handle multiple calls before
> SYS_STATE_boot.

Correct, I am not changing any expectation here. It is only making 
clearer because before commit 022387ee1ad3 we would just overwrite the 
existing mapping with no warning.

After commit 022387ee1ad3, we would just hit the BUG_ON() in set_fixmap().

I will clarify it in the commit message.

[...]

>>   bool __acpi_unmap_table(void *ptr, unsigned long size)
>>   {
>> -    return ( vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) &&
>> -             vaddr < (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE) );
>> +    vaddr_t vaddr = (vaddr_t)ptr;
>> +    unsigned int idx;
>> +
>> +    /* We are only handling fixmap address in the arch code */
>> +    if ( vaddr < FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) ||
>> +         vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_END) )
> 
> The "+ PAGE_SIZE" got lost

Hmmm yes.

> 
> 
>> +        return false;
>> +
>> +    /*
>> +     * __acpi_map_table() will always return a pointer in the first page
>> +     * for the ACPI fixmap region. The caller is expected to free with
>> +     * the same address.
>> +     */
>> +    ASSERT((vaddr & PAGE_MASK) == FIXMAP_ADDR(FIXMAP_ACPI_BEGIN));
>> +
>> +    /* The region allocated fit in the ACPI fixmap region. */
>> +    ASSERT(size < (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE - vaddr));
>> +    ASSERT(fixmap_inuse);
>> +
>> +    fixmap_inuse = false;
>> +
>> +    size += FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) - vaddr;
> 
> Sorry I got confused.. Shouldn't this be:
> 
>    size += vaddr - FIXMAP_ADDR(FIXMAP_ACPI_BEGIN);
> 
> ?

It should be. :) I guess this was unoticed because vaddr == 
FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) in my testing.

I will fix it.

> 
> 
>> +    idx = FIXMAP_ACPI_BEGIN;
>> +
>> +    do
>> +    {
>> +        clear_fixmap(idx);
>> +        size -= min(size, (unsigned long)PAGE_SIZE);
>> +        idx++;
>> +    } while ( size > 0 );
>> +
>> +    return true;
>>   }
>>   
>>   /* True to indicate PSCI 0.2+ is implemented */
>> -- 
>> 2.17.1
>>

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 4/4] xen/arm: Introduce fw_unreserved_regions() and use it
  2020-09-30 23:40   ` Stefano Stabellini
@ 2020-10-01 15:34     ` Julien Grall
  0 siblings, 0 replies; 51+ messages in thread
From: Julien Grall @ 2020-10-01 15:34 UTC (permalink / raw)
  To: Stefano Stabellini
  Cc: xen-devel, alex.bennee, masami.hiramatsu, ehem+xen,
	bertrand.marquis, andre.przywara, Julien Grall,
	Volodymyr Babchuk

Hi Stefano,

On 01/10/2020 00:40, Stefano Stabellini wrote:
> On Sat, 26 Sep 2020, Julien Grall wrote:
> I have a small suggestion for improvement that could be done on commit:
> given that bootinfo is actually used on EFI systems (granted, not
> bootinfo.reserved_mem but bootinfo.mem, see
> xen/arch/arm/efi/efi-boot.h:efi_process_memory_map_bootinfo) so
> technically bootinfo could be in-use with ACPI, maybe we could add a
> comment on top of xen/include/asm-arm/setup.h:bootinfo to say that
> reserved_mem is device tree only?

That's fine with me. I will need to resend the rest of the series, so I 
will update it at the same time.

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 0/4] xen/arm: Unbreak ACPI
  2020-09-28  6:47 ` Masami Hiramatsu
  2020-09-28 13:00   ` Masami Hiramatsu
@ 2020-10-08 18:39   ` Elliott Mitchell
  2020-10-09  9:39     ` Julien Grall
  2020-10-09 21:49   ` Elliott Mitchell
  2020-10-10 11:02   ` Julien Grall
  3 siblings, 1 reply; 51+ messages in thread
From: Elliott Mitchell @ 2020-10-08 18:39 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Julien Grall, xen-devel, Alex Benn??e, bertrand.marquis,
	andre.przywara, Julien Grall, Stefano Stabellini,
	Volodymyr Babchuk, Andrew Cooper, George Dunlap, Ian Jackson,
	Jan Beulich, Wei Liu, Roger Pau Monn??

On Mon, Sep 28, 2020 at 03:47:52PM +0900, Masami Hiramatsu wrote:
> This made progress with my Xen boot on DeveloperBox (
> https://www.96boards.org/product/developerbox/ ) with ACPI.

Adding your patch on top of Julien Grall's patch appears to push the Xen
boot of my target device (Raspberry PI 4B with Tianocore) further.  Still
yet to see any output attributable to the Domain 0 kernel though.

(XEN) *** LOADING DOMAIN 0 ***
(XEN) Loading d0 kernel from boot module @ 0000000032234000
(XEN) Loading ramdisk from boot module @ 0000000031747000
(XEN) Allocating 1:1 mappings totalling 2048MB for dom0:
(XEN) BANK[0] 0x00000020000000-0x00000030000000 (256MB)
(XEN) BANK[1] 0x00000040000000-0x000000b0000000 (1792MB)
(XEN) Grant table range: 0x000000315f3000-0x00000031633000
(XEN) Allocating PPI 16 for event channel interrupt
(XEN) Loading zImage from 0000000032234000 to 0000000020080000-0000000021359200
(XEN) Loading d0 initrd from 0000000031747000 to 0x0000000028200000-0x0000000028cebfe4
(XEN) Loading d0 DTB to 0x0000000028000000-0x0000000028000273
(XEN) Initial low memory virq threshold set at 0x4000 pages.
(XEN) Scrubbing Free RAM in background
(XEN) Std. Loglevel: All
(XEN) Guest Loglevel: All
(XEN) *** Serial input to DOM0 (type 'CTRL-a' three times to switch input)
(XEN) Freed 396kB init memory.

The line, "Loading d0 DTB to 0x0000000028000000-0x0000000028000273" seems
rather suspicious as I would expect Domain 0 to see ACPI tables, not a
device tree.

Your (Masami Hiramatsu) patch seems plausible, but things haven't
progressed enough for me to endorse it.  Looks like something closer to
the core of ACPI still needs further work, Julien Grall?


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg@m5p.com  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445




^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 0/4] xen/arm: Unbreak ACPI
  2020-10-08 18:39   ` [PATCH 0/4] xen/arm: Unbreak ACPI Elliott Mitchell
@ 2020-10-09  9:39     ` Julien Grall
  2020-10-09 14:22       ` Elliott Mitchell
  0 siblings, 1 reply; 51+ messages in thread
From: Julien Grall @ 2020-10-09  9:39 UTC (permalink / raw)
  To: Elliott Mitchell, Masami Hiramatsu
  Cc: xen-devel, Alex Benn??e, bertrand.marquis, andre.przywara,
	Julien Grall, Stefano Stabellini, Volodymyr Babchuk,
	Andrew Cooper, George Dunlap, Ian Jackson, Jan Beulich, Wei Liu,
	Roger Pau Monn??

Hello,

On 08/10/2020 19:39, Elliott Mitchell wrote:
> On Mon, Sep 28, 2020 at 03:47:52PM +0900, Masami Hiramatsu wrote:
>> This made progress with my Xen boot on DeveloperBox (
>> https://www.96boards.org/product/developerbox/ ) with ACPI.
> 
> Adding your patch on top of Julien Grall's patch appears to push the Xen
> boot of my target device (Raspberry PI 4B with Tianocore) further.  Still
> yet to see any output attributable to the Domain 0 kernel though.
> 
> (XEN) *** LOADING DOMAIN 0 ***
> (XEN) Loading d0 kernel from boot module @ 0000000032234000
> (XEN) Loading ramdisk from boot module @ 0000000031747000
> (XEN) Allocating 1:1 mappings totalling 2048MB for dom0:
> (XEN) BANK[0] 0x00000020000000-0x00000030000000 (256MB)
> (XEN) BANK[1] 0x00000040000000-0x000000b0000000 (1792MB)
> (XEN) Grant table range: 0x000000315f3000-0x00000031633000
> (XEN) Allocating PPI 16 for event channel interrupt
> (XEN) Loading zImage from 0000000032234000 to 0000000020080000-0000000021359200
> (XEN) Loading d0 initrd from 0000000031747000 to 0x0000000028200000-0x0000000028cebfe4
> (XEN) Loading d0 DTB to 0x0000000028000000-0x0000000028000273
> (XEN) Initial low memory virq threshold set at 0x4000 pages.
> (XEN) Scrubbing Free RAM in background
> (XEN) Std. Loglevel: All
> (XEN) Guest Loglevel: All
> (XEN) *** Serial input to DOM0 (type 'CTRL-a' three times to switch input)
> (XEN) Freed 396kB init memory.
> 
> The line, "Loading d0 DTB to 0x0000000028000000-0x0000000028000273" seems
> rather suspicious as I would expect Domain 0 to see ACPI tables, not a
> device tree.

This is normal, we are creating a small device-tree to pass some 
information to dom0 (such as the ACPI tables, command line, initrd).

> 
> Your (Masami Hiramatsu) patch seems plausible, but things haven't
> progressed enough for me to endorse it.  Looks like something closer to
> the core of ACPI still needs further work, Julien Grall?

I didn't go very far during my testing because QEMU is providing ACPI 
5.1 (Xen only supports 6.0+ so far).

For your log above, Xen finished to boot and now dom0 should start 
booting. The lack of console output may be because of a crash in Linux 
during earlyboot.

Do you have the early console enabled Linux? This can be done by adding 
earlycon=xenboot on the Linux command line.

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 0/4] xen/arm: Unbreak ACPI
  2020-10-09  9:39     ` Julien Grall
@ 2020-10-09 14:22       ` Elliott Mitchell
  2020-10-09 18:15         ` Julien Grall
  0 siblings, 1 reply; 51+ messages in thread
From: Elliott Mitchell @ 2020-10-09 14:22 UTC (permalink / raw)
  To: Julien Grall
  Cc: Elliott Mitchell, Masami Hiramatsu, xen-devel, Alex Benn??e,
	bertrand.marquis, andre.przywara, Julien Grall,
	Stefano Stabellini, Volodymyr Babchuk, Andrew Cooper,
	George Dunlap, Ian Jackson, Jan Beulich, Wei Liu,
	Roger Pau Monn??

On Fri, Oct 09, 2020 at 10:39:26AM +0100, Julien Grall wrote:
> On 08/10/2020 19:39, Elliott Mitchell wrote:
> > Your (Masami Hiramatsu) patch seems plausible, but things haven't
> > progressed enough for me to endorse it.  Looks like something closer to
> > the core of ACPI still needs further work, Julien Grall?
> 
> I didn't go very far during my testing because QEMU is providing ACPI 
> 5.1 (Xen only supports 6.0+ so far).
> 
> For your log above, Xen finished to boot and now dom0 should start 
> booting. The lack of console output may be because of a crash in Linux 
> during earlyboot.
> 
> Do you have the early console enabled Linux? This can be done by adding 
> earlycon=xenboot on the Linux command line.

Finding all the command-line console settings can be a challenge.  I had
thought it was supposed to be "console=hvc0 earlycon=hvc0".

With that though I finally have some output which claims to come from the
Linux kernel (yay! finally hit this point!).  As we were both guessing,
very early kernel panic:

[    0.000000] efi: Getting EFI parameters from FDT:
[    0.000000] efi: Can't find 'System Table' in device tree!
[    0.000000] cma: Failed to reserve 64 MiB
[    0.000000] Kernel panic - not syncing: Failed to allocate page table page

I don't know whether this is a problem with the mini-DT which was passed
in versus ACPI tables.  I note a complete lack of ACPI table information.
The kernel is from a 5.6-based kernel tree.  I'm unsure which portion to
try updating next.


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg@m5p.com  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445




^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 0/4] xen/arm: Unbreak ACPI
  2020-10-09 14:22       ` Elliott Mitchell
@ 2020-10-09 18:15         ` Julien Grall
  2020-10-09 22:36           ` Elliott Mitchell
  0 siblings, 1 reply; 51+ messages in thread
From: Julien Grall @ 2020-10-09 18:15 UTC (permalink / raw)
  To: Elliott Mitchell
  Cc: Masami Hiramatsu, xen-devel, Alex Benn??e, bertrand.marquis,
	andre.przywara, Julien Grall, Stefano Stabellini,
	Volodymyr Babchuk, Andrew Cooper, George Dunlap, Ian Jackson,
	Jan Beulich, Wei Liu, Roger Pau Monn??

Hi Elliott,

On 09/10/2020 15:22, Elliott Mitchell wrote:
> On Fri, Oct 09, 2020 at 10:39:26AM +0100, Julien Grall wrote:
>> On 08/10/2020 19:39, Elliott Mitchell wrote:
>>> Your (Masami Hiramatsu) patch seems plausible, but things haven't
>>> progressed enough for me to endorse it.  Looks like something closer to
>>> the core of ACPI still needs further work, Julien Grall?
>>
>> I didn't go very far during my testing because QEMU is providing ACPI
>> 5.1 (Xen only supports 6.0+ so far).
>>
>> For your log above, Xen finished to boot and now dom0 should start
>> booting. The lack of console output may be because of a crash in Linux
>> during earlyboot.
>>
>> Do you have the early console enabled Linux? This can be done by adding
>> earlycon=xenboot on the Linux command line.
> 
> Finding all the command-line console settings can be a challenge.  I had
> thought it was supposed to be "console=hvc0 earlycon=hvc0".
> 
> With that though I finally have some output which claims to come from the
> Linux kernel (yay! finally hit this point!).  As we were both guessing,
> very early kernel panic:
> 
> [    0.000000] efi: Getting EFI parameters from FDT:
> [    0.000000] efi: Can't find 'System Table' in device tree!

Thank you for sending part of the log. Looking at Linux 5.6 code, the 
error message is printed by efi_get_fdt_params() (see 
drivers/firmware/efi.c) when one of the property is missing.

'System Table' suggests that Linux wasn't enable to find 
"linux,uefi-system-table" or "xen,uefi-system-table".

Xen will only create later. Would it be possible to add some code in 
__find_uefi_params() to confirm which property Linux thinks is missing?

> [    0.000000] cma: Failed to reserve 64 MiB
> [    0.000000] Kernel panic - not syncing: Failed to allocate page table page
> 
> I don't know whether this is a problem with the mini-DT which was passed
> in versus ACPI tables.  I note a complete lack of ACPI table information.

I think this is normal because, IIRC, the ACPI root pointer will be 
found using the System Table.

> The kernel is from a 5.6-based kernel tree.  I'm unsure which portion to
> try updating next.
> 
> 

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 0/4] xen/arm: Unbreak ACPI
  2020-09-28  6:47 ` Masami Hiramatsu
  2020-09-28 13:00   ` Masami Hiramatsu
  2020-10-08 18:39   ` [PATCH 0/4] xen/arm: Unbreak ACPI Elliott Mitchell
@ 2020-10-09 21:49   ` Elliott Mitchell
  2020-10-10 11:02   ` Julien Grall
  3 siblings, 0 replies; 51+ messages in thread
From: Elliott Mitchell @ 2020-10-09 21:49 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Julien Grall, xen-devel, Alex Benn??e, bertrand.marquis,
	andre.przywara, Julien Grall, Stefano Stabellini,
	Volodymyr Babchuk, Andrew Cooper, George Dunlap, Ian Jackson,
	Jan Beulich, Wei Liu, Roger Pau Monn??

On Mon, Sep 28, 2020 at 03:47:52PM +0900, Masami Hiramatsu wrote:
> This made progress with my Xen boot on DeveloperBox (
> https://www.96boards.org/product/developerbox/ ) with ACPI.

Now things have progressed a bit more and I can confirm the patch
provides useful progress.  I cannot say whether there is a better way
since I'm not familiar enough with the code.

As such, for both Masami Hiramatsu's "Hide UART address only if STAO..."
and Julien Grall's set of four:

Tested-by: Elliott Mitchell <ehem+xen@m5p.com>


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg@m5p.com  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445




^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 0/4] xen/arm: Unbreak ACPI
  2020-10-09 18:15         ` Julien Grall
@ 2020-10-09 22:36           ` Elliott Mitchell
  0 siblings, 0 replies; 51+ messages in thread
From: Elliott Mitchell @ 2020-10-09 22:36 UTC (permalink / raw)
  To: Julien Grall
  Cc: Masami Hiramatsu, xen-devel, Alex Benn??e, bertrand.marquis,
	andre.przywara, Julien Grall, Stefano Stabellini,
	Volodymyr Babchuk, Andrew Cooper, George Dunlap, Ian Jackson,
	Jan Beulich, Wei Liu, Roger Pau Monn??

On Fri, Oct 09, 2020 at 07:15:20PM +0100, Julien Grall wrote:
> On 09/10/2020 15:22, Elliott Mitchell wrote:
> > Finding all the command-line console settings can be a challenge.  I had
> > thought it was supposed to be "console=hvc0 earlycon=hvc0".
> > 
> > With that though I finally have some output which claims to come from the
> > Linux kernel (yay! finally hit this point!).  As we were both guessing,
> > very early kernel panic:
> > 
> > [    0.000000] efi: Getting EFI parameters from FDT:
> > [    0.000000] efi: Can't find 'System Table' in device tree!
> 
> Thank you for sending part of the log. Looking at Linux 5.6 code, the 
> error message is printed by efi_get_fdt_params() (see 
> drivers/firmware/efi.c) when one of the property is missing.
> 
> 'System Table' suggests that Linux wasn't enable to find 
> "linux,uefi-system-table" or "xen,uefi-system-table".
> 
> Xen will only create later. Would it be possible to add some code in 
> __find_uefi_params() to confirm which property Linux thinks is missing?

Trying to rebuild a configuration long after the prior build can be
entertaining.  Finally identified one patch appears to be for 4.19, but
breaks 5.x.  With that and an appropriate adjustment:

[    0.000000] efi: __find_uefi_params(): Missing "linux,uefi-system-table"
[    0.000000] efi: __find_uefi_params(): Missing "linux,uefi-system-table"


Good news is I had been meaning to try a later kernel for a while and
thus tried building a 5.8 kernel.  Once built, the 5.8 kernel booted
successfully.  As such the next issue is the driver for the graphics chip
doesn't function under Xen as of the 5.8 kernel and Xen 4.14.

I can now state I've gotten success with a combination of Julien Grall's
patches and Masami Hiramatsu.  Certainly qualifies for:

Tested-by: Elliott Mitchell <ehem+xen@m5p.com>

For all 5 patches.


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg@m5p.com  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445




^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/4] xen/acpi: Rework acpi_os_map_memory() and acpi_os_unmap_memory()
  2020-09-28 10:39         ` Julien Grall
@ 2020-10-10  9:49           ` Julien Grall
  2020-10-10 10:04             ` Julien Grall
  0 siblings, 1 reply; 51+ messages in thread
From: Julien Grall @ 2020-10-10  9:49 UTC (permalink / raw)
  To: Jan Beulich
  Cc: xen-devel, alex.bennee, masami.hiramatsu, ehem+xen,
	bertrand.marquis, andre.przywara, Julien Grall,
	Stefano Stabellini, Volodymyr Babchuk, Andrew Cooper,
	George Dunlap, Ian Jackson, Wei Liu, Roger Pau Monné

Hi,

On 28/09/2020 11:39, Julien Grall wrote:
> 
> 
> On 28/09/2020 11:09, Jan Beulich wrote:
>> On 28.09.2020 11:58, Julien Grall wrote:
>>> On 28/09/2020 09:18, Jan Beulich wrote:
>>>> On 26.09.2020 22:55, Julien Grall wrote:
>>>>> --- a/xen/arch/x86/acpi/lib.c
>>>>> +++ b/xen/arch/x86/acpi/lib.c
>>>>> @@ -46,6 +46,10 @@ char *__acpi_map_table(paddr_t phys, unsigned 
>>>>> long size)
>>>>>        if ((phys + size) <= (1 * 1024 * 1024))
>>>>>            return __va(phys);
>>>>> +    /* No arch specific implementation after early boot */
>>>>> +    if (system_state >= SYS_STATE_boot)
>>>>> +        return NULL;
>>>>
>>>> Considering the code in context above, the comment isn't entirely
>>>> correct.
>>>
>>> How about "No arch specific implementation after early boot but for the
>>> first 1MB"?
>>
>> That or simply "No further ...".
> 
> I will do that.
> 
>>>>> +{
>>>>> +    unsigned long vaddr = (unsigned long)ptr;
>>>>> +
>>>>> +    if (vaddr >= DIRECTMAP_VIRT_START &&
>>>>> +        vaddr < DIRECTMAP_VIRT_END) {
>>>>> +        ASSERT(!((__pa(ptr) + size - 1) >> 20));
>>>>> +        return true;
>>>>> +    }
>>>>> +
>>>>> +    return (vaddr >= __fix_to_virt(FIX_ACPI_END)) &&
>>>>> +        (vaddr < (__fix_to_virt(FIX_ACPI_BEGIN) + PAGE_SIZE));
>>>>
>>>> Indentation is slightly wrong here.
>>>
>>> This is Linux coding style and therefore is using hard tab. Care to
>>> explain the problem?
>>
>> The two opening parentheses should align with one another,
>> shouldn't they?
> 
> Hmmm... somehow vim wants to indent this way. I am not entirely sure why...

Looking at the Linux codebase this is the expected indentation. This is 
because the first ( on the first line is not closed and until the last ) 
on the second line.

So I will stick with this code.

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/4] xen/acpi: Rework acpi_os_map_memory() and acpi_os_unmap_memory()
  2020-10-10  9:49           ` Julien Grall
@ 2020-10-10 10:04             ` Julien Grall
  0 siblings, 0 replies; 51+ messages in thread
From: Julien Grall @ 2020-10-10 10:04 UTC (permalink / raw)
  To: Jan Beulich
  Cc: xen-devel, alex.bennee, masami.hiramatsu, ehem+xen,
	bertrand.marquis, andre.przywara, Julien Grall,
	Stefano Stabellini, Volodymyr Babchuk, Andrew Cooper,
	George Dunlap, Ian Jackson, Wei Liu, Roger Pau Monné



On 10/10/2020 10:49, Julien Grall wrote:
> Hi,
> 
> On 28/09/2020 11:39, Julien Grall wrote:
>>
>>
>> On 28/09/2020 11:09, Jan Beulich wrote:
>>> On 28.09.2020 11:58, Julien Grall wrote:
>>>> On 28/09/2020 09:18, Jan Beulich wrote:
>>>>> On 26.09.2020 22:55, Julien Grall wrote:
>>>>>> --- a/xen/arch/x86/acpi/lib.c
>>>>>> +++ b/xen/arch/x86/acpi/lib.c
>>>>>> @@ -46,6 +46,10 @@ char *__acpi_map_table(paddr_t phys, unsigned 
>>>>>> long size)
>>>>>>        if ((phys + size) <= (1 * 1024 * 1024))
>>>>>>            return __va(phys);
>>>>>> +    /* No arch specific implementation after early boot */
>>>>>> +    if (system_state >= SYS_STATE_boot)
>>>>>> +        return NULL;
>>>>>
>>>>> Considering the code in context above, the comment isn't entirely
>>>>> correct.
>>>>
>>>> How about "No arch specific implementation after early boot but for the
>>>> first 1MB"?
>>>
>>> That or simply "No further ...".
>>
>> I will do that.
>>
>>>>>> +{
>>>>>> +    unsigned long vaddr = (unsigned long)ptr;
>>>>>> +
>>>>>> +    if (vaddr >= DIRECTMAP_VIRT_START &&
>>>>>> +        vaddr < DIRECTMAP_VIRT_END) {
>>>>>> +        ASSERT(!((__pa(ptr) + size - 1) >> 20));
>>>>>> +        return true;
>>>>>> +    }
>>>>>> +
>>>>>> +    return (vaddr >= __fix_to_virt(FIX_ACPI_END)) &&
>>>>>> +        (vaddr < (__fix_to_virt(FIX_ACPI_BEGIN) + PAGE_SIZE));
>>>>>
>>>>> Indentation is slightly wrong here.
>>>>
>>>> This is Linux coding style and therefore is using hard tab. Care to
>>>> explain the problem?
>>>
>>> The two opening parentheses should align with one another,
>>> shouldn't they?
>>
>> Hmmm... somehow vim wants to indent this way. I am not entirely sure 
>> why...
> 
> Looking at the Linux codebase this is the expected indentation. This is 
> because the first ( on the first line is not closed and until the last ) 
> on the second line.

Hrmm... I obviously misread the code I wrote... Sorry for the noise.

> 
> So I will stick with this code.
> 
> Cheers,
> 

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 0/4] xen/arm: Unbreak ACPI
  2020-09-28  6:47 ` Masami Hiramatsu
                     ` (2 preceding siblings ...)
  2020-10-09 21:49   ` Elliott Mitchell
@ 2020-10-10 11:02   ` Julien Grall
  2020-10-12 19:02     ` Stefano Stabellini
  3 siblings, 1 reply; 51+ messages in thread
From: Julien Grall @ 2020-10-10 11:02 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: xen-devel, Alex Bennée, ehem+xen, bertrand.marquis,
	andre.przywara, Julien Grall, Stefano Stabellini,
	Volodymyr Babchuk

On 28/09/2020 07:47, Masami Hiramatsu wrote:
> Hello,

Hi Masami,

> This made progress with my Xen boot on DeveloperBox (
> https://www.96boards.org/product/developerbox/ ) with ACPI.
> 

I have reviewed the patch attached and I have a couple of remarks about it.

The STAO table was originally created to allow an hypervisor to hide 
devices from a controller domain (such as Dom0). If this table is not 
present, then it means the OS/hypervisor can use any device listed in 
the ACPI table.

Additionally, the STAO table should never be present in the host ACPI table.

Therefore, I think the code should not try to find the STAO. Instead, it 
should check whether the SPCR table is present.

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 0/4] xen/arm: Unbreak ACPI
  2020-10-10 11:02   ` Julien Grall
@ 2020-10-12 19:02     ` Stefano Stabellini
  2020-10-12 21:34       ` Elliott Mitchell
  2020-10-14 17:44       ` Julien Grall
  0 siblings, 2 replies; 51+ messages in thread
From: Stefano Stabellini @ 2020-10-12 19:02 UTC (permalink / raw)
  To: Julien Grall
  Cc: Masami Hiramatsu, xen-devel, Alex Bennée, ehem+xen,
	bertrand.marquis, andre.przywara, Julien Grall,
	Stefano Stabellini, Volodymyr Babchuk

On Sat, 10 Oct 2020, Julien Grall wrote:
> On 28/09/2020 07:47, Masami Hiramatsu wrote:
> > Hello,
> 
> Hi Masami,
> 
> > This made progress with my Xen boot on DeveloperBox (
> > https://www.96boards.org/product/developerbox/ ) with ACPI.
> > 
> 
> I have reviewed the patch attached and I have a couple of remarks about it.
> 
> The STAO table was originally created to allow an hypervisor to hide devices
> from a controller domain (such as Dom0). If this table is not present, then it
> means the OS/hypervisor can use any device listed in the ACPI table.
> 
> Additionally, the STAO table should never be present in the host ACPI table.
> 
> Therefore, I think the code should not try to find the STAO. Instead, it
> should check whether the SPCR table is present.

Yes, that makes sense, but that brings me to the next question.

SPCR seems to be required by SBBR, however, Masami wrote that he could
boot on a system without SPCR, which gets me very confused for two
reasons:

1) Why there is no SPCR? Isn't it supposed to be mandatory? Is it
because there no UART on Masami's system?

2) If there is no SPCR, how did Masami manage to boot Xen?
I take without any serial output? Just with the framebuffer?


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 0/4] xen/arm: Unbreak ACPI
  2020-10-12 19:02     ` Stefano Stabellini
@ 2020-10-12 21:34       ` Elliott Mitchell
  2020-10-14  1:06         ` Stefano Stabellini
  2020-10-14 17:44       ` Julien Grall
  1 sibling, 1 reply; 51+ messages in thread
From: Elliott Mitchell @ 2020-10-12 21:34 UTC (permalink / raw)
  To: Stefano Stabellini
  Cc: Julien Grall, Masami Hiramatsu, xen-devel, Alex Benn??e,
	bertrand.marquis, andre.przywara, Julien Grall,
	Volodymyr Babchuk

On Mon, Oct 12, 2020 at 12:02:14PM -0700, Stefano Stabellini wrote:
> On Sat, 10 Oct 2020, Julien Grall wrote:
> > Therefore, I think the code should not try to find the STAO. Instead, it
> > should check whether the SPCR table is present.
> 
> Yes, that makes sense, but that brings me to the next question.
> 
> SPCR seems to be required by SBBR, however, Masami wrote that he could
> boot on a system without SPCR, which gets me very confused for two
> reasons:
> 
> 1) Why there is no SPCR? Isn't it supposed to be mandatory? Is it
> because there no UART on Masami's system?

I'm on different hardware, but some folks have setup Tianocore for it.
According to Documentation/arm64/acpi_object_usage.rst,
"Required: DSDT, FADT, GTDT, MADT, MCFG, RSDP, SPCR, XSDT".  Yet when
booting a Linux kernel directly on the hardware it lists APIC, BGRT,
CSRT, DSDT, DBG2, FACP, GTDT, PPTT, RSDP, and XSDT.

I don't know whether Linux's ACPI code omits mention of some required
tables and merely panics if they're absent.  Yet I'm speculating the list
of required tables has shrunk, SPCR is no longer required, and the
documentation is out of date.  Perhaps SPCR was required in early Linux
ACPI implementations, but more recent ones removed that requirement?

> 2) If there is no SPCR, how did Masami manage to boot Xen?
> I take without any serial output? Just with the framebuffer?

On my board the provided tables are sufficient to let Linux identify
ttyAMA0.  Linux's efifb driver finds the framebuffer and apparently has
it usable.


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg@m5p.com  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445




^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 0/4] xen/arm: Unbreak ACPI
  2020-10-12 21:34       ` Elliott Mitchell
@ 2020-10-14  1:06         ` Stefano Stabellini
  2020-10-14  1:37           ` Elliott Mitchell
  0 siblings, 1 reply; 51+ messages in thread
From: Stefano Stabellini @ 2020-10-14  1:06 UTC (permalink / raw)
  To: Elliott Mitchell
  Cc: Stefano Stabellini, Julien Grall, Masami Hiramatsu, xen-devel,
	Alex Benn??e, bertrand.marquis, andre.przywara, Julien Grall,
	Volodymyr Babchuk

On Mon, 12 Oct 2020, Elliott Mitchell wrote:
> On Mon, Oct 12, 2020 at 12:02:14PM -0700, Stefano Stabellini wrote:
> > On Sat, 10 Oct 2020, Julien Grall wrote:
> > > Therefore, I think the code should not try to find the STAO. Instead, it
> > > should check whether the SPCR table is present.
> > 
> > Yes, that makes sense, but that brings me to the next question.
> > 
> > SPCR seems to be required by SBBR, however, Masami wrote that he could
> > boot on a system without SPCR, which gets me very confused for two
> > reasons:
> > 
> > 1) Why there is no SPCR? Isn't it supposed to be mandatory? Is it
> > because there no UART on Masami's system?
> 
> I'm on different hardware, but some folks have setup Tianocore for it.
> According to Documentation/arm64/acpi_object_usage.rst,
> "Required: DSDT, FADT, GTDT, MADT, MCFG, RSDP, SPCR, XSDT".  Yet when
> booting a Linux kernel directly on the hardware it lists APIC, BGRT,
> CSRT, DSDT, DBG2, FACP, GTDT, PPTT, RSDP, and XSDT.
> 
> I don't know whether Linux's ACPI code omits mention of some required
> tables and merely panics if they're absent.  Yet I'm speculating the list
> of required tables has shrunk, SPCR is no longer required, and the
> documentation is out of date.  Perhaps SPCR was required in early Linux
> ACPI implementations, but more recent ones removed that requirement?

I have just checked and SPCR is still a mandatory table in the latest
SBBR specification. It is probably one of those cases where the firmware
claims to be SBBR compliant, but it is not, and it happens to work with
Linux.


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 0/4] xen/arm: Unbreak ACPI
  2020-10-14  1:06         ` Stefano Stabellini
@ 2020-10-14  1:37           ` Elliott Mitchell
  2020-10-14 17:47             ` Julien Grall
  0 siblings, 1 reply; 51+ messages in thread
From: Elliott Mitchell @ 2020-10-14  1:37 UTC (permalink / raw)
  To: Stefano Stabellini
  Cc: Julien Grall, Masami Hiramatsu, xen-devel, Alex Benn??e,
	bertrand.marquis, andre.przywara, Julien Grall,
	Volodymyr Babchuk

On Tue, Oct 13, 2020 at 06:06:26PM -0700, Stefano Stabellini wrote:
> On Mon, 12 Oct 2020, Elliott Mitchell wrote:
> > I'm on different hardware, but some folks have setup Tianocore for it.
> > According to Documentation/arm64/acpi_object_usage.rst,
> > "Required: DSDT, FADT, GTDT, MADT, MCFG, RSDP, SPCR, XSDT".  Yet when
> > booting a Linux kernel directly on the hardware it lists APIC, BGRT,
> > CSRT, DSDT, DBG2, FACP, GTDT, PPTT, RSDP, and XSDT.
> > 
> > I don't know whether Linux's ACPI code omits mention of some required
> > tables and merely panics if they're absent.  Yet I'm speculating the list
> > of required tables has shrunk, SPCR is no longer required, and the
> > documentation is out of date.  Perhaps SPCR was required in early Linux
> > ACPI implementations, but more recent ones removed that requirement?
> 
> I have just checked and SPCR is still a mandatory table in the latest
> SBBR specification. It is probably one of those cases where the firmware
> claims to be SBBR compliant, but it is not, and it happens to work with
> Linux.

Is meeting the SBBR specification supposed to be a requirement of running
Xen-ARM?

I don't seen any mention of such.
`find docs xen/arch/arm -type f -print0 | xargs -0 grep -eSBBR` produces
no output.

Perhaps you've been adding this as a presumptive requirement since
previously the only hardware capable of running Xen due to an
appropriately unlocked bootloader was SBBR compliant?  If so, it seems
time to either add this as an explicit requirement and document it, or
else remove this implicit requirement and start acting as such.

The Raspberry PI 4B has a UEFI implementation available which is based on
Tianocore.  No statement has been made of it qualifying as SBBR.  Yet it
is clearly mostly able to boot Xen, just this is exposing issues.


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg@m5p.com  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445




^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 0/4] xen/arm: Unbreak ACPI
  2020-10-12 19:02     ` Stefano Stabellini
  2020-10-12 21:34       ` Elliott Mitchell
@ 2020-10-14 17:44       ` Julien Grall
  1 sibling, 0 replies; 51+ messages in thread
From: Julien Grall @ 2020-10-14 17:44 UTC (permalink / raw)
  To: Stefano Stabellini
  Cc: Masami Hiramatsu, xen-devel, Alex Bennée, ehem+xen,
	bertrand.marquis, andre.przywara, Julien Grall,
	Volodymyr Babchuk

Hi,

On 12/10/2020 20:02, Stefano Stabellini wrote:
> On Sat, 10 Oct 2020, Julien Grall wrote:
>> On 28/09/2020 07:47, Masami Hiramatsu wrote:
>>> Hello,
>>
>> Hi Masami,
>>
>>> This made progress with my Xen boot on DeveloperBox (
>>> https://www.96boards.org/product/developerbox/ ) with ACPI.
>>>
>>
>> I have reviewed the patch attached and I have a couple of remarks about it.
>>
>> The STAO table was originally created to allow an hypervisor to hide devices
>> from a controller domain (such as Dom0). If this table is not present, then it
>> means the OS/hypervisor can use any device listed in the ACPI table.
>>
>> Additionally, the STAO table should never be present in the host ACPI table.
>>
>> Therefore, I think the code should not try to find the STAO. Instead, it
>> should check whether the SPCR table is present.
> 
> Yes, that makes sense, but that brings me to the next question.
> 
> SPCR seems to be required by SBBR, however, Masami wrote that he could
> boot on a system without SPCR, which gets me very confused for two
> reasons:
> 
> 1) Why there is no SPCR? Isn't it supposed to be mandatory? Is it
> because there no UART on Masami's system?

I can't comment specically on Masami's system, but I can make two broad 
comments:
    1) Not all the systems have to be compliant with the SBBR. But in 
theory, only systems passing the SBBR test can be claimed to be 
complained. This brings to the second point...
    2) "Mandatory" is what the specs aim to enforce. In my experience, 
some of the vendors may bend the rule and still claim they are compliant.

Even Linux documentation says that the SPCR is mandatory. But in the 
reality, the implementation will just ignore it.

 From my understanding of the code, Linux will only use it to discover 
the preferred console and enable earlycon. Without it, Linux will 
require the user to specify console=<...> or earlycon=<...>.

The UART will then be discovered via the DSDT.

> 
> 2) If there is no SPCR, how did Masami manage to boot Xen?
> I take without any serial output? Just with the framebuffer?

After his patch, Xen can boot without the SPCR. You will just see no 
logs. But I belive, he enabled earlyprintk for his platform.

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 0/4] xen/arm: Unbreak ACPI
  2020-10-14  1:37           ` Elliott Mitchell
@ 2020-10-14 17:47             ` Julien Grall
  2020-10-15 18:00               ` Stefano Stabellini
  0 siblings, 1 reply; 51+ messages in thread
From: Julien Grall @ 2020-10-14 17:47 UTC (permalink / raw)
  To: Elliott Mitchell, Stefano Stabellini
  Cc: Masami Hiramatsu, xen-devel, Alex Benn??e, bertrand.marquis,
	andre.przywara, Julien Grall, Volodymyr Babchuk

Hi Elliot,

On 14/10/2020 02:37, Elliott Mitchell wrote:
> On Tue, Oct 13, 2020 at 06:06:26PM -0700, Stefano Stabellini wrote:
>> On Mon, 12 Oct 2020, Elliott Mitchell wrote:
>>> I'm on different hardware, but some folks have setup Tianocore for it.
>>> According to Documentation/arm64/acpi_object_usage.rst,
>>> "Required: DSDT, FADT, GTDT, MADT, MCFG, RSDP, SPCR, XSDT".  Yet when
>>> booting a Linux kernel directly on the hardware it lists APIC, BGRT,
>>> CSRT, DSDT, DBG2, FACP, GTDT, PPTT, RSDP, and XSDT.
>>>
>>> I don't know whether Linux's ACPI code omits mention of some required
>>> tables and merely panics if they're absent.  Yet I'm speculating the list
>>> of required tables has shrunk, SPCR is no longer required, and the
>>> documentation is out of date.  Perhaps SPCR was required in early Linux
>>> ACPI implementations, but more recent ones removed that requirement?
>>
>> I have just checked and SPCR is still a mandatory table in the latest
>> SBBR specification. It is probably one of those cases where the firmware
>> claims to be SBBR compliant, but it is not, and it happens to work with
>> Linux.
> 
> Is meeting the SBBR specification supposed to be a requirement of running
> Xen-ARM?

This is not my goal. We should try to get Xen running everywhere as long 
as this doesn't require a lot of extra code. IOW, don't ask me to 
review/accept a port of Xen to RPI3 ;).

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 0/4] xen/arm: Unbreak ACPI
  2020-10-14 17:47             ` Julien Grall
@ 2020-10-15 18:00               ` Stefano Stabellini
  0 siblings, 0 replies; 51+ messages in thread
From: Stefano Stabellini @ 2020-10-15 18:00 UTC (permalink / raw)
  To: Julien Grall
  Cc: Elliott Mitchell, Stefano Stabellini, Masami Hiramatsu,
	xen-devel, Alex Benn??e, bertrand.marquis, andre.przywara,
	Julien Grall, Volodymyr Babchuk

On Wed, 14 Oct 2020, Julien Grall wrote:
> Hi Elliot,
> 
> On 14/10/2020 02:37, Elliott Mitchell wrote:
> > On Tue, Oct 13, 2020 at 06:06:26PM -0700, Stefano Stabellini wrote:
> > > On Mon, 12 Oct 2020, Elliott Mitchell wrote:
> > > > I'm on different hardware, but some folks have setup Tianocore for it.
> > > > According to Documentation/arm64/acpi_object_usage.rst,
> > > > "Required: DSDT, FADT, GTDT, MADT, MCFG, RSDP, SPCR, XSDT".  Yet when
> > > > booting a Linux kernel directly on the hardware it lists APIC, BGRT,
> > > > CSRT, DSDT, DBG2, FACP, GTDT, PPTT, RSDP, and XSDT.
> > > > 
> > > > I don't know whether Linux's ACPI code omits mention of some required
> > > > tables and merely panics if they're absent.  Yet I'm speculating the
> > > > list
> > > > of required tables has shrunk, SPCR is no longer required, and the
> > > > documentation is out of date.  Perhaps SPCR was required in early Linux
> > > > ACPI implementations, but more recent ones removed that requirement?
> > > 
> > > I have just checked and SPCR is still a mandatory table in the latest
> > > SBBR specification. It is probably one of those cases where the firmware
> > > claims to be SBBR compliant, but it is not, and it happens to work with
> > > Linux.
> > 
> > Is meeting the SBBR specification supposed to be a requirement of running
> > Xen-ARM?
> 
> This is not my goal. We should try to get Xen running everywhere as long as
> this doesn't require a lot of extra code. IOW, don't ask me to review/accept a
> port of Xen to RPI3 ;).

I agree with Julien's statement.

For context, my previous comment in regards to SBBR was because I am
positive that Masami's platform is expected to be SBBR compliant.


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Xen-ARM EFI/ACPI problems (was: Re: [PATCH 0/4] xen/arm: Unbreak ACPI)
  2020-09-28 13:00   ` Masami Hiramatsu
@ 2020-10-16 22:33     ` Elliott Mitchell
  2020-10-17  5:12       ` Elliott Mitchell
  0 siblings, 1 reply; 51+ messages in thread
From: Elliott Mitchell @ 2020-10-16 22:33 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Julien Grall, xen-devel, Alex Benn??e, bertrand.marquis,
	andre.przywara, Julien Grall, Stefano Stabellini,
	Volodymyr Babchuk, Andrew Cooper, George Dunlap, Ian Jackson,
	Jan Beulich, Wei Liu, Roger Pau Monn??

On Mon, Sep 28, 2020 at 10:00:35PM +0900, Masami Hiramatsu wrote:
> I've missed the explanation of the attached patch. This prototype
> patch was also needed for booting up the Xen on my box (for the system
> which has no SPCR).

I'm pretty sure of this on the hardware I'm dealing with, but don't know
about the hardware you're dealing with.

Does your device have a framebuffer?  Have you ever tried/succeeded
booting your device with the framebuffer disabled? (booted with a
HDMI/DVI cable disconnected or the device on the other end *completely*
powered down)

Based upon the back and forth both on xen-devel and some messages which
were split off due to not being general.  An observation I had made a
while ago finally caused me to try recreating it.

On the device I'm on (Raspberry PI 4B with Tianocore -> GRUB -> Xen) I
discovered a SPCR table shows up if I boot with the device the output is
plugged into is powered down.  I'm guessing this causes Tianocore to
advise GRUB/Linux/Xen to boot with a serial console (presenting a Serial
Port Console Redirect table), whereas if the display device is
functioning the absense of SPCR is supposed to indicate console on
framebuffer.

This means the ACPI_FAILURE case in acpi_iomem_deny_access() simply needs
to be filled in similar to how it likely is on x86.  Allocate a serial
port for Xen's use as console, present it to domain 0 as hvc0, and hide
it from domain 0.



Next issue for me will be getting the framebuffer operational.
Apparently the Xen-ARM EFI implementation doesn't provide any EFI
variables to domain 0?  Jan Beulich, your name was mentioned as person
likely to have ideas for getting Linux's efifb code operational Xen-ARM.


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg@m5p.com  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445




^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: Xen-ARM EFI/ACPI problems (was: Re: [PATCH 0/4] xen/arm: Unbreak ACPI)
  2020-10-16 22:33     ` Xen-ARM EFI/ACPI problems (was: Re: [PATCH 0/4] xen/arm: Unbreak ACPI) Elliott Mitchell
@ 2020-10-17  5:12       ` Elliott Mitchell
  0 siblings, 0 replies; 51+ messages in thread
From: Elliott Mitchell @ 2020-10-17  5:12 UTC (permalink / raw)
  To: Masami Hiramatsu, Julien Grall, xen-devel, Alex Benn??e,
	bertrand.marquis, andre.przywara, Julien Grall,
	Stefano Stabellini, Volodymyr Babchuk, Andrew Cooper,
	George Dunlap, Ian Jackson, Jan Beulich, Wei Liu,
	Roger Pau Monn??

On Fri, Oct 16, 2020 at 03:33:23PM -0700, Elliott Mitchell wrote:
> On the device I'm on (Raspberry PI 4B with Tianocore -> GRUB -> Xen) I
> discovered a SPCR table shows up if I boot with the device the output is
> plugged into is powered down.  I'm guessing this causes Tianocore to
> advise GRUB/Linux/Xen to boot with a serial console (presenting a Serial
> Port Console Redirect table), whereas if the display device is
> functioning the absense of SPCR is supposed to indicate console on
> framebuffer.
> 
> This means the ACPI_FAILURE case in acpi_iomem_deny_access() simply needs
> to be filled in similar to how it likely is on x86.  Allocate a serial
> port for Xen's use as console, present it to domain 0 as hvc0, and hide
> it from domain 0.

Looks like things are worse than I thought.

Upon examining some of my `dmesg` copies it looks like Linux interprets
the ignore_uart field in STAO tables as applying strictly to the UART
referenced in the SPCR table.  As such, when booted with the framebuffer
available, Linux thinks it can freely access the UART found in the
hardware table.  The specification for the STAO table is apparently
garbage since it only allows a hypervisor to tell a VM to ignore a
single UART.  Instead it really needs to be possible to mask arbitrary
devices.  :-(

As such, for ARM devices which can include framebuffers, I'm guessing Xen
will need to either pass a modified table to domain 0, or simulate the
device sufficiently to prevent concurrent access.  This could be as
simple as simulating a MMIO page which discards all writes.





> Next issue for me will be getting the framebuffer operational.
> Apparently the Xen-ARM EFI implementation doesn't provide any EFI
> variables to domain 0?  Jan Beulich, your name was mentioned as person
> likely to have ideas for getting Linux's efifb code operational Xen-ARM.

There may be multiple pieces to this.

For the framebuffer this might be as simple as parsing the BGRT table and
ensuring the addresses are directly mapped to domain 0.  I just noticed
in dmesg, "efi_bgrt: Ignoring BGRT: invalid image address".  I'm guessing
one thing got remapped, but a second didn't.

The other need for EFI variable access is for modifying EFI's boot
process.  While I suspect it may be feasible for most users to reboot to
a kernel directly on hardware to update GRUB/other bootloader, adding an
extra step increases the potential for a failure at the Wrong Time.


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg@m5p.com  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445




^ permalink raw reply	[flat|nested] 51+ messages in thread

end of thread, back to index

Thread overview: 51+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-26 20:55 [PATCH 0/4] xen/arm: Unbreak ACPI Julien Grall
2020-09-26 20:55 ` [PATCH 1/4] xen/acpi: Rework acpi_os_map_memory() and acpi_os_unmap_memory() Julien Grall
2020-09-28  8:18   ` Jan Beulich
2020-09-28  9:58     ` Julien Grall
2020-09-28 10:09       ` Jan Beulich
2020-09-28 10:39         ` Julien Grall
2020-10-10  9:49           ` Julien Grall
2020-10-10 10:04             ` Julien Grall
2020-09-29 11:10   ` Rahul Singh
2020-10-01  0:06   ` Stefano Stabellini
2020-10-01 15:09     ` Julien Grall
2020-09-26 20:55 ` [PATCH 2/4] xen/arm: acpi: The fixmap area should always be cleared during failure/unmap Julien Grall
2020-09-29 11:13   ` Rahul Singh
2020-10-01  0:30   ` Stefano Stabellini
2020-10-01 15:14     ` Julien Grall
2020-09-26 20:55 ` [PATCH 3/4] xen/arm: Check if the platform is not using ACPI before initializing Dom0less Julien Grall
2020-09-29 11:17   ` Rahul Singh
2020-09-30 23:26   ` Stefano Stabellini
2020-09-26 20:55 ` [PATCH 4/4] xen/arm: Introduce fw_unreserved_regions() and use it Julien Grall
2020-09-30 23:40   ` Stefano Stabellini
2020-10-01 15:34     ` Julien Grall
2020-09-27  1:47 ` [PATCH 0/4] xen/arm: Unbreak ACPI Elliott Mitchell
2020-09-29 15:28   ` Elliott Mitchell
2020-09-28  6:47 ` Masami Hiramatsu
2020-09-28 13:00   ` Masami Hiramatsu
2020-10-16 22:33     ` Xen-ARM EFI/ACPI problems (was: Re: [PATCH 0/4] xen/arm: Unbreak ACPI) Elliott Mitchell
2020-10-17  5:12       ` Elliott Mitchell
2020-10-08 18:39   ` [PATCH 0/4] xen/arm: Unbreak ACPI Elliott Mitchell
2020-10-09  9:39     ` Julien Grall
2020-10-09 14:22       ` Elliott Mitchell
2020-10-09 18:15         ` Julien Grall
2020-10-09 22:36           ` Elliott Mitchell
2020-10-09 21:49   ` Elliott Mitchell
2020-10-10 11:02   ` Julien Grall
2020-10-12 19:02     ` Stefano Stabellini
2020-10-12 21:34       ` Elliott Mitchell
2020-10-14  1:06         ` Stefano Stabellini
2020-10-14  1:37           ` Elliott Mitchell
2020-10-14 17:47             ` Julien Grall
2020-10-15 18:00               ` Stefano Stabellini
2020-10-14 17:44       ` Julien Grall
2020-09-29 11:10 ` Rahul Singh
2020-09-29 15:29 ` Alex Bennée
2020-09-29 17:07   ` Julien Grall
2020-09-29 21:11     ` Alex Bennée
2020-09-29 23:39       ` André Przywara
2020-09-30  8:51         ` Alex Bennée
2020-09-30 10:35         ` Julien Grall
2020-09-30  9:42       ` Julien Grall
2020-09-30 10:38         ` Alex Bennée
2020-09-30 11:10           ` Julien Grall

Xen-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/xen-devel/0 xen-devel/git/0.git
	git clone --mirror https://lore.kernel.org/xen-devel/1 xen-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 xen-devel xen-devel/ https://lore.kernel.org/xen-devel \
		xen-devel@lists.xenproject.org xen-devel@lists.xen.org
	public-inbox-index xen-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.xenproject.lists.xen-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git