[PATCH v4 00/10] Refactor DT specific codes preparing for ACPI support on ARM64

[PATCH v4 00/10] Refactor DT specific codes preparing for ACPI support on ARM64

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

These patches are Part 2 of the previous patch set I sent which adds
ACPI support for arm64 on Xen[1]. Split them as an individual set for
convenient reviewing.

The first two patches ports two ACPI changes from Linux kernel, which
are missed at Part 1.

The second three patches solve pmstat compiling errors for ACPI on ARM64
and refactor acpi_os_map_memory to be architecturally independent.

The last five patches refactor some ARM codes into generic and DT
specific parts.

Cc: Jan Beulich <jbeulich@suse.com>

Thanks,
Shannon
[1] http://lists.xenproject.org/archives/html/xen-devel/2015-11/msg01831.html


Ashwin Chaugule (1):
  ACPI / table: Add new function to get table entries

Len Brown (1):
  ACPI: check acpi_disabled in acpi_table_parse() and
    acpi_table_parse_entries()

Parth Dixit (1):
  acpi/pmstat: Build pmstat for x86 only

Shannon Zhao (7):
  acpi: Don't do traditional BIOS table scan for ARM64
  acpi: Refactor acpi_os_map_memory to be architecturally independent
  arm/smpboot: Move dt specific code in smp to seperate functions
  arm/gic-v2: Refactor gicv2_init into generic and dt specific parts
  arm/gic-v3: Refactor gicv3_init into generic and dt specific parts
  arm/uart: Rename dt-uart.c to arm-uart.c
  pl011: Refactor pl011 driver to dt and common initialization parts

 MAINTAINERS                        |   2 +-
 xen/arch/arm/arm64/smpboot.c       |   7 ++-
 xen/arch/arm/gic-v2.c              |  21 ++++---
 xen/arch/arm/gic-v3.c              | 114 ++++++++++++++++++++-----------------
 xen/arch/arm/smpboot.c             |  29 ++++++----
 xen/arch/x86/acpi/lib.c            |  16 ++++++
 xen/common/sysctl.c                |   2 +-
 xen/drivers/acpi/Makefile          |   2 +-
 xen/drivers/acpi/osl.c             |  12 +---
 xen/drivers/acpi/tables.c          |  56 ++++++++++++++----
 xen/drivers/acpi/tables/tbxfroot.c |   7 +++
 xen/drivers/char/Makefile          |   2 +-
 xen/drivers/char/arm-uart.c        | 107 ++++++++++++++++++++++++++++++++++
 xen/drivers/char/dt-uart.c         | 107 ----------------------------------
 xen/drivers/char/pl011.c           |  64 ++++++++++++---------
 xen/include/xen/acpi.h             |   6 ++
 16 files changed, 322 insertions(+), 232 deletions(-)
 create mode 100644 xen/drivers/char/arm-uart.c
 delete mode 100644 xen/drivers/char/dt-uart.c

-- 
2.0.4

[PATCH v4 01/10] ACPI: check acpi_disabled in acpi_table_parse() and acpi_table_parse_entries()

[Shannon Zhao]

From: Len Brown <len.brown@intel.com>

Allow consumers of the acpi_table_parse()/acpi_table_parse_entries() API
to gracefully handle the acpi_disabled=1 case via return value
rather than checking the global flag themselves.

Signed-off-by: Feng Tang <feng.tang@intel.com>
Signed-off-by: Len Brown <len.brown@intel.com>
[Linux commit e5b8fc6ac158f65598f58dba2c0d52ba3b412f52]
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
Cc: Jan Beulich <jbeulich@suse.com>
---
 xen/drivers/acpi/tables.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/xen/drivers/acpi/tables.c b/xen/drivers/acpi/tables.c
index 60c4ab1..39f1223 100644
--- a/xen/drivers/acpi/tables.c
+++ b/xen/drivers/acpi/tables.c
@@ -210,6 +210,9 @@ acpi_table_parse_entries(char *id,
 	unsigned int count = 0;
 	unsigned long table_end;
 
+	if (acpi_disabled)
+		return -ENODEV;
+
 	if (!handler)
 		return -EINVAL;
 
@@ -279,6 +282,9 @@ int __init acpi_table_parse(char *id, acpi_table_handler handler)
 {
 	struct acpi_table_header *table = NULL;
 
+	if (acpi_disabled)
+		return -ENODEV;
+
 	if (!handler)
 		return -EINVAL;
 
-- 
2.0.4

[PATCH v4 02/10] ACPI / table: Add new function to get table entries

[Shannon Zhao]

From: Ashwin Chaugule <ashwin.chaugule@linaro.org>

The acpi_table_parse() function has a callback that
passes a pointer to a table_header. Add a new function
which takes this pointer and parses its entries. This
eliminates the need to re-traverse all the tables for
each call. e.g. as in acpi_table_parse_madt() which is
normally called after acpi_table_parse().

Acked-by: Grant Likely <grant.likely@linaro.org>
Signed-off-by: Ashwin Chaugule <ashwin.chaugule@linaro.org>
Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
[Linux commit f08bb472bff3c0397fb7d6f47bc5cec41dad76e3]
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
Cc: Jan Beulich <jbeulich@suse.com>
---
 xen/drivers/acpi/tables.c | 50 +++++++++++++++++++++++++++++++++++------------
 xen/include/xen/acpi.h    |  4 ++++
 2 files changed, 42 insertions(+), 12 deletions(-)

diff --git a/xen/drivers/acpi/tables.c b/xen/drivers/acpi/tables.c
index 39f1223..4e590de 100644
--- a/xen/drivers/acpi/tables.c
+++ b/xen/drivers/acpi/tables.c
@@ -199,27 +199,23 @@ void __init acpi_table_print_madt_entry(struct acpi_subtable_header *header)
 
 
 int __init
-acpi_table_parse_entries(char *id,
-			     unsigned long table_size,
-			     int entry_id,
-			     acpi_table_entry_handler handler,
-			     unsigned int max_entries)
+acpi_parse_entries(char *id, unsigned long table_size,
+		   acpi_table_entry_handler handler,
+		   struct acpi_table_header *table_header,
+		   int entry_id, unsigned int max_entries)
 {
-	struct acpi_table_header *table_header = NULL;
 	struct acpi_subtable_header *entry;
-	unsigned int count = 0;
+	int count = 0;
 	unsigned long table_end;
 
 	if (acpi_disabled)
 		return -ENODEV;
 
-	if (!handler)
+	if (!id || !handler)
 		return -EINVAL;
 
-	if (strncmp(id, ACPI_SIG_MADT, 4) == 0)
-		acpi_get_table(id, acpi_apic_instance, &table_header);
-	else
-		acpi_get_table(id, 0, &table_header);
+	if (!table_size)
+		return -EINVAL;
 
 	if (!table_header) {
 		printk(KERN_WARNING PREFIX "%4.4s not present\n", id);
@@ -252,6 +248,7 @@ acpi_table_parse_entries(char *id,
 		entry = (struct acpi_subtable_header *)
 		    ((unsigned long)entry + entry->length);
 	}
+
 	if (max_entries && count > max_entries) {
 		printk(KERN_WARNING PREFIX "[%4.4s:%#x] ignored %i entries of "
 		       "%i found\n", id, entry_id, count - max_entries, count);
@@ -261,6 +258,35 @@ acpi_table_parse_entries(char *id,
 }
 
 int __init
+acpi_table_parse_entries(char *id,
+			 unsigned long table_size,
+			 int entry_id,
+			 acpi_table_entry_handler handler,
+			 unsigned int max_entries)
+{
+	struct acpi_table_header *table_header = NULL;
+	u32 instance = 0;
+
+	if (acpi_disabled)
+		return -ENODEV;
+
+	if (!id || !handler)
+		return -EINVAL;
+
+	if (!strncmp(id, ACPI_SIG_MADT, 4))
+		instance = acpi_apic_instance;
+
+	acpi_get_table(id, instance, &table_header);
+	if (!table_header) {
+		printk(KERN_WARNING PREFIX "%4.4s not present\n", id);
+		return -ENODEV;
+	}
+
+	return acpi_parse_entries(id, table_size, handler, table_header,
+				  entry_id, max_entries);
+}
+
+int __init
 acpi_table_parse_madt(enum acpi_madt_type id,
 		      acpi_table_entry_handler handler, unsigned int max_entries)
 {
diff --git a/xen/include/xen/acpi.h b/xen/include/xen/acpi.h
index 0f1077d..65e53a6 100644
--- a/xen/include/xen/acpi.h
+++ b/xen/include/xen/acpi.h
@@ -64,6 +64,10 @@ void acpi_hest_init(void);
 
 int acpi_table_init (void);
 int acpi_table_parse(char *id, acpi_table_handler handler);
+int acpi_parse_entries(char *id, unsigned long table_size,
+		       acpi_table_entry_handler handler,
+		       struct acpi_table_header *table_header,
+		       int entry_id, unsigned int max_entries);
 int acpi_table_parse_entries(char *id, unsigned long table_size,
 	int entry_id, acpi_table_entry_handler handler, unsigned int max_entries);
 int acpi_table_parse_madt(enum acpi_madt_type id, acpi_table_entry_handler handler, unsigned int max_entries);
-- 
2.0.4

[PATCH v4 03/10] acpi/pmstat: Build pmstat for x86 only

[Shannon Zhao]

From: Parth Dixit <parth.dixit@linaro.org>

Pmstat is currently not supported for ARM in Xen. Configure and build
pmstat for x86 architecture only.

Cc: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Parth Dixit <parth.dixit@linaro.org>
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
v4: keep both CONFIG_HAS_ACPI and CONFIG_HAS_CPUFREQ
---
 xen/common/sysctl.c       | 2 +-
 xen/drivers/acpi/Makefile | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index a3007b8..1624024 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -171,7 +171,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
         op->u.availheap.avail_bytes <<= PAGE_SHIFT;
         break;
 
-#ifdef CONFIG_HAS_ACPI
+#if defined (CONFIG_HAS_ACPI) && defined (CONFIG_HAS_CPUFREQ)
     case XEN_SYSCTL_get_pmstat:
         ret = do_get_pm_info(&op->u.get_pmstat);
         break;
diff --git a/xen/drivers/acpi/Makefile b/xen/drivers/acpi/Makefile
index 3bb626e..0ef3efd 100644
--- a/xen/drivers/acpi/Makefile
+++ b/xen/drivers/acpi/Makefile
@@ -5,7 +5,7 @@ subdir-$(x86) += apei
 obj-bin-y += tables.init.o
 obj-$(HAS_NUMA) += numa.o
 obj-y += osl.o
-obj-y += pmstat.o
+obj-$(CONFIG_HAS_CPUFREQ) += pmstat.o
 
 obj-$(x86) += hwregs.o
 obj-$(x86) += reboot.o
-- 
2.0.4

[PATCH v4 04/10] acpi: Don't do traditional BIOS table scan for ARM64

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

With the addition of ARM64 that does not have a traditional BIOS to
scan, stub out acpi_find_root_pointer to do nothing for ARM.

Cc: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
v4: stub out acpi_find_root_pointer fro ARM
---
 xen/drivers/acpi/tables/tbxfroot.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/xen/drivers/acpi/tables/tbxfroot.c b/xen/drivers/acpi/tables/tbxfroot.c
index 0efb603..90e52e9 100644
--- a/xen/drivers/acpi/tables/tbxfroot.c
+++ b/xen/drivers/acpi/tables/tbxfroot.c
@@ -49,6 +49,12 @@
 #define _COMPONENT          ACPI_TABLES
 ACPI_MODULE_NAME("tbxfroot")
 
+#ifdef CONFIG_ARM
+acpi_status __init acpi_find_root_pointer(acpi_native_uint * table_address)
+{
+	return_ACPI_STATUS(AE_NOT_FOUND);
+}
+#else
 /* Local prototypes */
 static u8 *acpi_tb_scan_memory_for_rsdp(u8 * start_address, u32 length);
 
@@ -271,3 +277,4 @@ static u8 *__init acpi_tb_scan_memory_for_rsdp(u8 * start_address, u32 length)
 			  start_address));
 	return_PTR(NULL);
 }
+#endif
-- 
2.0.4

[PATCH v4 05/10] acpi: Refactor acpi_os_map_memory to be architecturally independent

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

Current acpi_os_map_memory is specific to x86. Refactor it to be
architecturally independent.

Cc: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen/arch/x86/acpi/lib.c | 16 ++++++++++++++++
 xen/drivers/acpi/osl.c  | 12 +-----------
 xen/include/xen/acpi.h  |  2 ++
 3 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/xen/arch/x86/acpi/lib.c b/xen/arch/x86/acpi/lib.c
index cc15ea3..1e2e124 100644
--- a/xen/arch/x86/acpi/lib.c
+++ b/xen/arch/x86/acpi/lib.c
@@ -33,6 +33,22 @@ u8 __read_mostly acpi_disable_value;
 u32 __read_mostly x86_acpiid_to_apicid[MAX_MADT_ENTRIES] =
     {[0 ... MAX_MADT_ENTRIES - 1] = BAD_APICID };
 
+void __iomem *
+arch_acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
+{
+	if (system_state >= SYS_STATE_active) {
+		mfn_t mfn = _mfn(PFN_DOWN(phys));
+		unsigned int offs = phys & (PAGE_SIZE - 1);
+
+		/* The low first Mb is always mapped. */
+		if ( !((phys + size - 1) >> 20) )
+			return __va(phys);
+		return __vmap(&mfn, PFN_UP(offs + size), 1, 1,
+			      PAGE_HYPERVISOR_NOCACHE) + offs;
+	}
+	return __acpi_map_table(phys, size);
+}
+
 /*
  * Important Safety Note:  The fixed ACPI page numbers are *subtracted*
  * from the fixed base.  That's why we start at FIX_ACPI_END and
diff --git a/xen/drivers/acpi/osl.c b/xen/drivers/acpi/osl.c
index ce15470..dc971af 100644
--- a/xen/drivers/acpi/osl.c
+++ b/xen/drivers/acpi/osl.c
@@ -86,17 +86,7 @@ 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_active) {
-		mfn_t mfn = _mfn(PFN_DOWN(phys));
-		unsigned int offs = phys & (PAGE_SIZE - 1);
-
-		/* The low first Mb is always mapped. */
-		if ( !((phys + size - 1) >> 20) )
-			return __va(phys);
-		return __vmap(&mfn, PFN_UP(offs + size), 1, 1,
-			      PAGE_HYPERVISOR_NOCACHE) + offs;
-	}
-	return __acpi_map_table(phys, size);
+	return arch_acpi_os_map_memory(phys, size);
 }
 
 void acpi_os_unmap_memory(void __iomem * virt, acpi_size size)
diff --git a/xen/include/xen/acpi.h b/xen/include/xen/acpi.h
index 65e53a6..9d90d6b 100644
--- a/xen/include/xen/acpi.h
+++ b/xen/include/xen/acpi.h
@@ -54,6 +54,8 @@ typedef int (*acpi_table_handler) (struct acpi_table_header *table);
 
 typedef int (*acpi_table_entry_handler) (struct acpi_subtable_header *header, const unsigned long end);
 
+void __iomem *
+arch_acpi_os_map_memory(acpi_physical_address phys, acpi_size size);
 unsigned int acpi_get_processor_id (unsigned int cpu);
 char * __acpi_map_table (paddr_t phys_addr, unsigned long size);
 int acpi_boot_init (void);
-- 
2.0.4

[PATCH v4 06/10] arm/smpboot: Move dt specific code in smp to seperate functions

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

Partition smp initialization functions into generic and dt specific
parts, this will be useful when introducing new functions for smp
initialization based on acpi.

Signed-off-by: Parth Dixit <parth.dixit@linaro.org>
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen/arch/arm/arm64/smpboot.c |  7 ++++++-
 xen/arch/arm/smpboot.c       | 29 ++++++++++++++++++-----------
 2 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/xen/arch/arm/arm64/smpboot.c b/xen/arch/arm/arm64/smpboot.c
index 62e6abb..7928f69 100644
--- a/xen/arch/arm/arm64/smpboot.c
+++ b/xen/arch/arm/arm64/smpboot.c
@@ -70,7 +70,7 @@ int __init arch_smp_init(void)
     return 0;
 }
 
-int __init arch_cpu_init(int cpu, struct dt_device_node *dn)
+static int __init dt_arch_cpu_init(int cpu, struct dt_device_node *dn)
 {
     const char *enable_method;
 
@@ -94,6 +94,11 @@ int __init arch_cpu_init(int cpu, struct dt_device_node *dn)
     return 0;
 }
 
+int __init arch_cpu_init(int cpu, struct dt_device_node *dn)
+{
+    return dt_arch_cpu_init(cpu, dn);
+}
+
 int __init arch_cpu_up(int cpu)
 {
     if ( !smp_enable_ops[cpu].prepare_cpu )
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index 00b2b2a..b6119d1 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -92,7 +92,7 @@ smp_clear_cpu_maps (void)
  * MPIDR values related to logical cpus
  * Code base on Linux arch/arm/kernel/devtree.c
  */
-void __init smp_init_cpus(void)
+static void __init dt_smp_init_cpus(void)
 {
     register_t mpidr;
     struct dt_device_node *cpus = dt_find_node_by_path("/cpus");
@@ -106,16 +106,6 @@ void __init smp_init_cpus(void)
     bool_t bootcpu_valid = 0;
     int rc;
 
-    /* scan the DTB for a PSCI node and set a global variable */
-    psci_init();
-
-    if ( (rc = arch_smp_init()) < 0 )
-    {
-        printk(XENLOG_WARNING "SMP init failed (%d)\n"
-               "Using only 1 CPU\n", rc);
-        return;
-    }
-
     mpidr = boot_cpu_data.mpidr.bits & MPIDR_HWID_MASK;
 
     if ( !cpus )
@@ -243,6 +233,23 @@ void __init smp_init_cpus(void)
     }
 }
 
+void __init smp_init_cpus(void)
+{
+    int rc;
+
+    /* initialize PSCI and set a global variable */
+    psci_init();
+
+    if ( (rc = arch_smp_init()) < 0 )
+    {
+        printk(XENLOG_WARNING "SMP init failed (%d)\n"
+               "Using only 1 CPU\n", rc);
+        return;
+    }
+
+    dt_smp_init_cpus();
+}
+
 int __init
 smp_get_max_cpus (void)
 {
-- 
2.0.4

[PATCH v4 07/10] arm/gic-v2: Refactor gicv2_init into generic and dt specific parts

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

Refactor gic-v2 related functions into dt and generic parts. This will be
helpful when adding acpi support for gic.

Signed-off-by: Parth Dixit <parth.dixit@linaro.org>
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen/arch/arm/gic-v2.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 793dca7..3fb5823 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -628,13 +628,12 @@ static bool_t gicv2_is_aliased(paddr_t cbase, paddr_t csize)
     return ((val_low & 0xfff0fff) == 0x0202043B && val_low == val_high);
 }
 
-static int __init gicv2_init(void)
+static paddr_t __initdata hbase, dbase, cbase, csize, vbase;
+
+static void __init gicv2_dt_init(void)
 {
     int res;
-    paddr_t hbase, dbase;
-    paddr_t cbase, csize;
-    paddr_t vbase, vsize;
-    uint32_t aliased_offset = 0;
+    paddr_t vsize;
     const struct dt_device_node *node = gicv2_info.node;
 
     res = dt_device_get_address(node, 0, &dbase, NULL);
@@ -680,6 +679,13 @@ static int __init gicv2_init(void)
     if ( csize != vsize )
         panic("GICv2: Sizes of GICC (%#"PRIpaddr") and GICV (%#"PRIpaddr") don't match\n",
                csize, vsize);
+}
+
+static int __init gicv2_init(void)
+{
+    uint32_t aliased_offset = 0;
+
+    gicv2_dt_init();
 
     printk("GICv2 initialization:\n"
               "        gic_dist_addr=%"PRIpaddr"\n"
@@ -765,7 +771,8 @@ const static struct gic_hw_operations gicv2_ops = {
 };
 
 /* Set up the GIC */
-static int __init gicv2_preinit(struct dt_device_node *node, const void *data)
+static int __init gicv2_dt_preinit(struct dt_device_node *node,
+                                   const void *data)
 {
     gicv2_info.hw_version = GIC_V2;
     gicv2_info.node = node;
@@ -783,7 +790,7 @@ static const struct dt_device_match gicv2_dt_match[] __initconst =
 
 DT_DEVICE_START(gicv2, "GICv2", DEVICE_GIC)
         .dt_match = gicv2_dt_match,
-        .init = gicv2_preinit,
+        .init = gicv2_dt_preinit,
 DT_DEVICE_END
 
 /*
-- 
2.0.4

[PATCH v4 08/10] arm/gic-v3: Refactor gicv3_init into generic and dt specific parts

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

Refactor gic-v3 related functions into dt and generic parts. This will be
helpful when adding acpi support for gic-v3.

Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
v4: Use INVALID_PADDR and move ioremap to common init function
---
 xen/arch/arm/gic-v3.c | 114 +++++++++++++++++++++++++++-----------------------
 1 file changed, 61 insertions(+), 53 deletions(-)

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index a245b56..65a4de6 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1138,41 +1138,14 @@ static int __init cmp_rdist(const void *a, const void *b)
     return ( l->base < r->base) ? -1 : 0;
 }
 
+static paddr_t __initdata dbase = INVALID_PADDR, vbase = INVALID_PADDR;
+static paddr_t __initdata cbase = INVALID_PADDR, csize = INVALID_PADDR;
+
 /* If the GICv3 supports GICv2, initialize it */
-static void __init gicv3_init_v2(const struct dt_device_node *node,
-                                 paddr_t dbase)
+static void __init gicv3_init_v2(void)
 {
-    int res;
-    paddr_t cbase, csize;
-    paddr_t vbase, vsize;
-
-    /*
-     * For GICv3 supporting GICv2, GICC and GICV base address will be
-     * provided.
-     */
-    res = dt_device_get_address(node, 1 + gicv3.rdist_count,
-                                &cbase, &csize);
-    if ( res )
-        return;
-
-    res = dt_device_get_address(node, 1 + gicv3.rdist_count + 2,
-                                &vbase, &vsize);
-    if ( res )
-        return;
-
-    /*
-     * We emulate a vGICv2 using a GIC CPU interface of GUEST_GICC_SIZE.
-     * So only support GICv2 on GICv3 when the virtual CPU interface is
-     * at least GUEST_GICC_SIZE.
-     */
-    if ( vsize < GUEST_GICC_SIZE )
-    {
-        printk(XENLOG_WARNING
-               "GICv3: WARNING: Not enabling support for GICv2 compat mode.\n"
-               "Size of GICV (%#"PRIpaddr") must at least be %#llx.\n",
-               vsize, GUEST_GICC_SIZE);
+    if ( cbase == INVALID_PADDR || vbase == INVALID_PADDR )
         return;
-    }
 
     printk("GICv3 compatible with GICv2 cbase %#"PRIpaddr" vbase %#"PRIpaddr"\n",
            cbase, vbase);
@@ -1180,20 +1153,12 @@ static void __init gicv3_init_v2(const struct dt_device_node *node,
     vgic_v2_setup_hw(dbase, cbase, csize, vbase, 0);
 }
 
-/* Set up the GIC */
-static int __init gicv3_init(void)
+static void __init gicv3_dt_init(void)
 {
     struct rdist_region *rdist_regs;
     int res, i;
-    uint32_t reg;
     const struct dt_device_node *node = gicv3_info.node;
-    paddr_t dbase;
-
-    if ( !cpu_has_gicv3 )
-    {
-        dprintk(XENLOG_ERR, "GICv3: driver requires system register support\n");
-        return -ENODEV;
-    }
+    paddr_t vsize;
 
     res = dt_device_get_address(node, 0, &dbase, NULL);
     if ( res )
@@ -1203,14 +1168,6 @@ static int __init gicv3_init(void)
         panic("GICv3:  Found unaligned distributor address %"PRIpaddr"",
               dbase);
 
-    gicv3.map_dbase = ioremap_nocache(dbase, SZ_64K);
-    if ( !gicv3.map_dbase )
-        panic("GICv3: Failed to ioremap for GIC distributor\n");
-
-    reg = readl_relaxed(GICD + GICD_PIDR2) & GIC_PIDR2_ARCH_MASK;
-    if ( reg != GIC_PIDR2_ARCH_GICv3 && reg != GIC_PIDR2_ARCH_GICv4 )
-         panic("GICv3: no distributor detected\n");
-
     if ( !dt_property_read_u32(node, "#redistributor-regions",
                 &gicv3.rdist_count) )
         gicv3.rdist_count = 1;
@@ -1248,6 +1205,57 @@ static int __init gicv3_init(void)
         panic("GICv3: Cannot find the maintenance IRQ");
     gicv3_info.maintenance_irq = res;
 
+    /*
+     * For GICv3 supporting GICv2, GICC and GICV base address will be
+     * provided.
+     */
+    res = dt_device_get_address(node, 1 + gicv3.rdist_count,
+                                &cbase, &csize);
+    if ( res )
+        return;
+
+    res = dt_device_get_address(node, 1 + gicv3.rdist_count + 2,
+                                &vbase, &vsize);
+    if ( res )
+        return;
+
+    /*
+     * We emulate a vGICv2 using a GIC CPU interface of GUEST_GICC_SIZE.
+     * So only support GICv2 on GICv3 when the virtual CPU interface is
+     * at least GUEST_GICC_SIZE.
+     */
+    if ( vsize < GUEST_GICC_SIZE )
+    {
+        printk(XENLOG_WARNING
+               "GICv3: WARNING: Not enabling support for GICv2 compat mode.\n"
+               "Size of GICV (%#"PRIpaddr") must at least be %#llx.\n",
+               vsize, GUEST_GICC_SIZE);
+        return;
+    }
+}
+
+/* Set up the GIC */
+static int __init gicv3_init(void)
+{
+    int res, i;
+    uint32_t reg;
+
+    if ( !cpu_has_gicv3 )
+    {
+        dprintk(XENLOG_ERR, "GICv3: driver requires system register support\n");
+        return -ENODEV;
+    }
+
+    gicv3_dt_init();
+
+    gicv3.map_dbase = ioremap_nocache(dbase, SZ_64K);
+    if ( !gicv3.map_dbase )
+        panic("GICv3: Failed to ioremap for GIC distributor\n");
+
+    reg = readl_relaxed(GICD + GICD_PIDR2) & GIC_PIDR2_ARCH_MASK;
+    if ( reg != GIC_PIDR2_ARCH_GICv3 && reg != GIC_PIDR2_ARCH_GICv4 )
+         panic("GICv3: no distributor detected\n");
+
     for ( i = 0; i < gicv3.rdist_count; i++ )
     {
         /* map dbase & rdist regions */
@@ -1277,7 +1285,7 @@ static int __init gicv3_init(void)
 
     vgic_v3_setup_hw(dbase, gicv3.rdist_count, gicv3.rdist_regions,
                      gicv3.rdist_stride);
-    gicv3_init_v2(node, dbase);
+    gicv3_init_v2();
 
     spin_lock_init(&gicv3.lock);
 
@@ -1317,7 +1325,7 @@ static const struct gic_hw_operations gicv3_ops = {
     .make_hwdom_dt_node  = gicv3_make_hwdom_dt_node,
 };
 
-static int __init gicv3_preinit(struct dt_device_node *node, const void *data)
+static int __init gicv3_dt_preinit(struct dt_device_node *node, const void *data)
 {
     gicv3_info.hw_version = GIC_V3;
     gicv3_info.node = node;
@@ -1335,7 +1343,7 @@ static const struct dt_device_match gicv3_dt_match[] __initconst =
 
 DT_DEVICE_START(gicv3, "GICv3", DEVICE_GIC)
         .dt_match = gicv3_dt_match,
-        .init = gicv3_preinit,
+        .init = gicv3_dt_preinit,
 DT_DEVICE_END
 
 /*
-- 
2.0.4

[PATCH v4 09/10] arm/uart: Rename dt-uart.c to arm-uart.c

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

Since we will add ACPI initialization for UART in this file later,
rename it with a generic name.

Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
v4: split the original patch to renaming this and adding ACPI parts.
---
 MAINTAINERS                 |   2 +-
 xen/drivers/char/Makefile   |   2 +-
 xen/drivers/char/arm-uart.c | 107 ++++++++++++++++++++++++++++++++++++++++++++
 xen/drivers/char/dt-uart.c  | 107 --------------------------------------------
 4 files changed, 109 insertions(+), 109 deletions(-)
 create mode 100644 xen/drivers/char/arm-uart.c
 delete mode 100644 xen/drivers/char/dt-uart.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 09fe823..5d2b354 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -133,7 +133,7 @@ S:	Supported
 L:	xen-devel@lists.xen.org
 F:	xen/arch/arm/
 F:	xen/include/asm-arm/
-F:	xen/drivers/char/dt-uart.c
+F:	xen/drivers/char/arm-uart.c
 F:	xen/drivers/char/exynos4210-uart.c
 F:	xen/drivers/char/omap-uart.c
 F:	xen/drivers/char/pl011.c
diff --git a/xen/drivers/char/Makefile b/xen/drivers/char/Makefile
index aa620fc..aa169d7 100644
--- a/xen/drivers/char/Makefile
+++ b/xen/drivers/char/Makefile
@@ -6,5 +6,5 @@ obj-$(CONFIG_HAS_EXYNOS4210) += exynos4210-uart.o
 obj-$(CONFIG_HAS_OMAP) += omap-uart.o
 obj-$(CONFIG_HAS_SCIF) += scif-uart.o
 obj-$(CONFIG_HAS_EHCI) += ehci-dbgp.o
-obj-$(CONFIG_ARM) += dt-uart.o
+obj-$(CONFIG_ARM) += arm-uart.o
 obj-y += serial.o
diff --git a/xen/drivers/char/arm-uart.c b/xen/drivers/char/arm-uart.c
new file mode 100644
index 0000000..883e615
--- /dev/null
+++ b/xen/drivers/char/arm-uart.c
@@ -0,0 +1,107 @@
+/*
+ * xen/drivers/char/arm-uart.c
+ *
+ * Generic uart retrieved via the device tree
+ *
+ * Julien Grall <julien.grall@linaro.org>
+ * Copyright (c) 2013 Linaro Limited.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <asm/device.h>
+#include <asm/types.h>
+#include <xen/console.h>
+#include <xen/device_tree.h>
+#include <xen/serial.h>
+#include <xen/errno.h>
+
+/*
+ * Configure UART port with a string:
+ * path:options
+ *
+ * @path: full path used in the device tree for the UART. If the path
+ * doesn't start with '/', we assuming that it's an alias.
+ * @options: UART speficic options (see in each UART driver)
+ */
+static char __initdata opt_dtuart[256] = "";
+string_param("dtuart", opt_dtuart);
+
+void __init dt_uart_init(void)
+{
+    struct dt_device_node *dev;
+    int ret;
+    const char *devpath = opt_dtuart;
+    char *options;
+
+    if ( !console_has("dtuart") )
+        return; /* Not for us */
+
+    if ( !strcmp(opt_dtuart, "") )
+    {
+        const struct dt_device_node *chosen = dt_find_node_by_path("/chosen");
+
+        if ( chosen )
+        {
+            const char *stdout;
+
+            ret = dt_property_read_string(chosen, "stdout-path", &stdout);
+            if ( ret >= 0 )
+            {
+                printk("Taking dtuart configuration from /chosen/stdout-path\n");
+                if ( strlcpy(opt_dtuart, stdout, sizeof(opt_dtuart))
+                     >= sizeof(opt_dtuart) )
+                    printk("WARNING: /chosen/stdout-path too long, truncated\n");
+            }
+            else if ( ret != -EINVAL /* Not present */ )
+                printk("Failed to read /chosen/stdout-path (%d)\n", ret);
+        }
+    }
+
+    if ( !strcmp(opt_dtuart, "") )
+    {
+        printk("No dtuart path configured\n");
+        return;
+    }
+
+    options = strchr(opt_dtuart, ':');
+    if ( options != NULL )
+        *(options++) = '\0';
+    else
+        options = "";
+
+    printk("Looking for dtuart at \"%s\", options \"%s\"\n", devpath, options);
+    if ( *devpath == '/' )
+        dev = dt_find_node_by_path(devpath);
+    else
+        dev = dt_find_node_by_alias(devpath);
+
+    if ( !dev )
+    {
+        printk("Unable to find device \"%s\"\n", devpath);
+        return;
+    }
+
+    ret = device_init(dev, DEVICE_SERIAL, options);
+
+    if ( ret )
+        printk("Unable to initialize dtuart: %d\n", ret);
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/drivers/char/dt-uart.c b/xen/drivers/char/dt-uart.c
deleted file mode 100644
index d599322..0000000
--- a/xen/drivers/char/dt-uart.c
+++ /dev/null
@@ -1,107 +0,0 @@
-/*
- * xen/drivers/char/dt-uart.c
- *
- * Generic uart retrieved via the device tree
- *
- * Julien Grall <julien.grall@linaro.org>
- * Copyright (c) 2013 Linaro Limited.
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- */
-
-#include <asm/device.h>
-#include <asm/types.h>
-#include <xen/console.h>
-#include <xen/device_tree.h>
-#include <xen/serial.h>
-#include <xen/errno.h>
-
-/*
- * Configure UART port with a string:
- * path:options
- *
- * @path: full path used in the device tree for the UART. If the path
- * doesn't start with '/', we assuming that it's an alias.
- * @options: UART speficic options (see in each UART driver)
- */
-static char __initdata opt_dtuart[256] = "";
-string_param("dtuart", opt_dtuart);
-
-void __init dt_uart_init(void)
-{
-    struct dt_device_node *dev;
-    int ret;
-    const char *devpath = opt_dtuart;
-    char *options;
-
-    if ( !console_has("dtuart") )
-        return; /* Not for us */
-
-    if ( !strcmp(opt_dtuart, "") )
-    {
-        const struct dt_device_node *chosen = dt_find_node_by_path("/chosen");
-
-        if ( chosen )
-        {
-            const char *stdout;
-
-            ret = dt_property_read_string(chosen, "stdout-path", &stdout);
-            if ( ret >= 0 )
-            {
-                printk("Taking dtuart configuration from /chosen/stdout-path\n");
-                if ( strlcpy(opt_dtuart, stdout, sizeof(opt_dtuart))
-                     >= sizeof(opt_dtuart) )
-                    printk("WARNING: /chosen/stdout-path too long, truncated\n");
-            }
-            else if ( ret != -EINVAL /* Not present */ )
-                printk("Failed to read /chosen/stdout-path (%d)\n", ret);
-        }
-    }
-
-    if ( !strcmp(opt_dtuart, "") )
-    {
-        printk("No dtuart path configured\n");
-        return;
-    }
-
-    options = strchr(opt_dtuart, ':');
-    if ( options != NULL )
-        *(options++) = '\0';
-    else
-        options = "";
-
-    printk("Looking for dtuart at \"%s\", options \"%s\"\n", devpath, options);
-    if ( *devpath == '/' )
-        dev = dt_find_node_by_path(devpath);
-    else
-        dev = dt_find_node_by_alias(devpath);
-
-    if ( !dev )
-    {
-        printk("Unable to find device \"%s\"\n", devpath);
-        return;
-    }
-
-    ret = device_init(dev, DEVICE_SERIAL, options);
-
-    if ( ret )
-        printk("Unable to initialize dtuart: %d\n", ret);
-}
-
-/*
- * Local variables:
- * mode: C
- * c-file-style: "BSD"
- * c-basic-offset: 4
- * tab-width: 4
- * indent-tabs-mode: nil
- * End:
- */
-- 
2.0.4

[PATCH v4 10/10] pl011: Refactor pl011 driver to dt and common initialization parts

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

Refactor pl011 driver to dt and common initialization parts. This will
be useful later when acpi specific uart initialization function is
introduced.

Signed-off-by: Parth Dixit <parth.dixit@linaro.org>
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen/drivers/char/pl011.c | 64 ++++++++++++++++++++++++++++--------------------
 1 file changed, 38 insertions(+), 26 deletions(-)

diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c
index 67e6df5..7e16294 100644
--- a/xen/drivers/char/pl011.c
+++ b/xen/drivers/char/pl011.c
@@ -226,12 +226,42 @@ static struct uart_driver __read_mostly pl011_driver = {
     .vuart_info   = pl011_vuart,
 };
 
+static int __init pl011_uart_init(int irq, u64 addr, u64 size)
+{
+    struct pl011 *uart;
+
+    uart = &pl011_com;
+    uart->irq       = irq;
+    uart->clock_hz  = 0x16e3600;
+    uart->baud      = BAUD_AUTO;
+    uart->data_bits = 8;
+    uart->parity    = PARITY_NONE;
+    uart->stop_bits = 1;
+
+    uart->regs = ioremap_nocache(addr, size);
+    if ( !uart->regs )
+    {
+        printk("pl011: Unable to map the UART memory\n");
+        return -ENOMEM;
+    }
+
+    uart->vuart.base_addr = addr;
+    uart->vuart.size = size;
+    uart->vuart.data_off = DR;
+    uart->vuart.status_off = FR;
+    uart->vuart.status = 0;
+
+    /* Register with generic serial driver. */
+    serial_register_uart(SERHND_DTUART, &pl011_driver, uart);
+
+    return 0;
+}
+
 /* TODO: Parse UART config from the command line */
-static int __init pl011_uart_init(struct dt_device_node *dev,
-                                  const void *data)
+static int __init pl011_dt_uart_init(struct dt_device_node *dev,
+                                     const void *data)
 {
     const char *config = data;
-    struct pl011 *uart;
     int res;
     u64 addr, size;
 
@@ -240,14 +270,6 @@ static int __init pl011_uart_init(struct dt_device_node *dev,
         printk("WARNING: UART configuration is not supported\n");
     }
 
-    uart = &pl011_com;
-
-    uart->clock_hz  = 0x16e3600;
-    uart->baud      = BAUD_AUTO;
-    uart->data_bits = 8;
-    uart->parity    = PARITY_NONE;
-    uart->stop_bits = 1;
-
     res = dt_device_get_address(dev, 0, &addr, &size);
     if ( res )
     {
@@ -262,24 +284,14 @@ static int __init pl011_uart_init(struct dt_device_node *dev,
         printk("pl011: Unable to retrieve the IRQ\n");
         return -EINVAL;
     }
-    uart->irq = res;
 
-    uart->regs = ioremap_nocache(addr, size);
-    if ( !uart->regs )
+    res = pl011_uart_init(res, addr, size);
+    if ( res < 0 )
     {
-        printk("pl011: Unable to map the UART memory\n");
-        return -ENOMEM;
+        printk("pl011: Unable to initialize\n");
+        return res;
     }
 
-    uart->vuart.base_addr = addr;
-    uart->vuart.size = size;
-    uart->vuart.data_off = DR;
-    uart->vuart.status_off = FR;
-    uart->vuart.status = 0;
-
-    /* Register with generic serial driver. */
-    serial_register_uart(SERHND_DTUART, &pl011_driver, uart);
-
     dt_device_set_used_by(dev, DOMID_XEN);
 
     return 0;
@@ -293,7 +305,7 @@ static const struct dt_device_match pl011_dt_match[] __initconst =
 
 DT_DEVICE_START(pl011, "PL011 UART", DEVICE_SERIAL)
         .dt_match = pl011_dt_match,
-        .init = pl011_uart_init,
+        .init = pl011_dt_uart_init,
 DT_DEVICE_END
 
 /*
-- 
2.0.4

Re: [PATCH v4 09/10] arm/uart: Rename dt-uart.c to arm-uart.c

[Jan Beulich]

>>> On 16.01.16 at 06:01, <zhaoshenglong@huawei.com> wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> Since we will add ACPI initialization for UART in this file later,
> rename it with a generic name.
> 
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
> v4: split the original patch to renaming this and adding ACPI parts.
> ---
>  MAINTAINERS                 |   2 +-
>  xen/drivers/char/Makefile   |   2 +-
>  xen/drivers/char/arm-uart.c | 107 ++++++++++++++++++++++++++++++++++++++++++++
>  xen/drivers/char/dt-uart.c  | 107 --------------------------------------------
>  4 files changed, 109 insertions(+), 109 deletions(-)
>  create mode 100644 xen/drivers/char/arm-uart.c
>  delete mode 100644 xen/drivers/char/dt-uart.c

Looks like this really is a rename with little actual changes to
the file, in which case this should be reflected as such in the
presentation of the patch.

Jan

Re: [PATCH v4 04/10] acpi: Don't do traditional BIOS table scan for ARM64

[Jan Beulich]

>>> On 16.01.16 at 06:01, <zhaoshenglong@huawei.com> wrote:
> --- a/xen/drivers/acpi/tables/tbxfroot.c
> +++ b/xen/drivers/acpi/tables/tbxfroot.c
> @@ -49,6 +49,12 @@
>  #define _COMPONENT          ACPI_TABLES
>  ACPI_MODULE_NAME("tbxfroot")
>  
> +#ifdef CONFIG_ARM
> +acpi_status __init acpi_find_root_pointer(acpi_native_uint * table_address)
> +{
> +	return_ACPI_STATUS(AE_NOT_FOUND);
> +}
> +#else
>  /* Local prototypes */
>  static u8 *acpi_tb_scan_memory_for_rsdp(u8 * start_address, u32 length);
>  
> @@ -271,3 +277,4 @@ static u8 *__init acpi_tb_scan_memory_for_rsdp(u8 * start_address, u32 length)
>  			  start_address));
>  	return_PTR(NULL);
>  }
> +#endif

You modify ACPI CA code here, which should be avoided if at all
possible. Why don't you simply port over Linux'es solution (which
changes osl.c instead), the more that now we have Linux-like
Kconfig?

Jan

Re: [PATCH v4 05/10] acpi: Refactor acpi_os_map_memory to be architecturally independent

[Jan Beulich]

>>> On 16.01.16 at 06:01, <zhaoshenglong@huawei.com> wrote:
> --- a/xen/drivers/acpi/osl.c
> +++ b/xen/drivers/acpi/osl.c
> @@ -86,17 +86,7 @@ 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_active) {
> -		mfn_t mfn = _mfn(PFN_DOWN(phys));
> -		unsigned int offs = phys & (PAGE_SIZE - 1);
> -
> -		/* The low first Mb is always mapped. */
> -		if ( !((phys + size - 1) >> 20) )
> -			return __va(phys);
> -		return __vmap(&mfn, PFN_UP(offs + size), 1, 1,
> -			      PAGE_HYPERVISOR_NOCACHE) + offs;
> -	}
> -	return __acpi_map_table(phys, size);
> +	return arch_acpi_os_map_memory(phys, size);
>  }

I'm quite sure I've said before that this goes too far: The __vmap()
part and likely also the early-boot __acpi_map_table() one already
are architecture independent and hence should stay. The factoring
hence should only concern the first Mb handling and maybe the
the mapping attributes passed to __vmap().

Jan

Re: [PATCH v4 09/10] arm/uart: Rename dt-uart.c to arm-uart.c

[Shannon Zhao]

On 2016/1/18 18:40, Jan Beulich wrote:
>>>> On 16.01.16 at 06:01, <zhaoshenglong@huawei.com> wrote:
>> > From: Shannon Zhao <shannon.zhao@linaro.org>
>> > 
>> > Since we will add ACPI initialization for UART in this file later,
>> > rename it with a generic name.
>> > 
>> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>> > ---
>> > v4: split the original patch to renaming this and adding ACPI parts.
>> > ---
>> >  MAINTAINERS                 |   2 +-
>> >  xen/drivers/char/Makefile   |   2 +-
>> >  xen/drivers/char/arm-uart.c | 107 ++++++++++++++++++++++++++++++++++++++++++++
>> >  xen/drivers/char/dt-uart.c  | 107 --------------------------------------------
>> >  4 files changed, 109 insertions(+), 109 deletions(-)
>> >  create mode 100644 xen/drivers/char/arm-uart.c
>> >  delete mode 100644 xen/drivers/char/dt-uart.c
> Looks like this really is a rename with little actual changes to
> the file, 
Yes, but the only change to this file is also to change its name because
it's mentioned in the file header. This intends to make that consistent.

+ * xen/drivers/char/arm-uart.c

Thanks,
-- 
Shannon

Re: [PATCH v4 04/10] acpi: Don't do traditional BIOS table scan for ARM64

[Shannon Zhao]

On 2016/1/18 21:29, Jan Beulich wrote:
>>>> On 16.01.16 at 06:01, <zhaoshenglong@huawei.com> wrote:
>> > --- a/xen/drivers/acpi/tables/tbxfroot.c
>> > +++ b/xen/drivers/acpi/tables/tbxfroot.c
>> > @@ -49,6 +49,12 @@
>> >  #define _COMPONENT          ACPI_TABLES
>> >  ACPI_MODULE_NAME("tbxfroot")
>> >  
>> > +#ifdef CONFIG_ARM
>> > +acpi_status __init acpi_find_root_pointer(acpi_native_uint * table_address)
>> > +{
>> > +	return_ACPI_STATUS(AE_NOT_FOUND);
>> > +}
>> > +#else
>> >  /* Local prototypes */
>> >  static u8 *acpi_tb_scan_memory_for_rsdp(u8 * start_address, u32 length);
>> >  
>> > @@ -271,3 +277,4 @@ static u8 *__init acpi_tb_scan_memory_for_rsdp(u8 * start_address, u32 length)
>> >  			  start_address));
>> >  	return_PTR(NULL);
>> >  }
>> > +#endif
> You modify ACPI CA code here, which should be avoided if at all
> possible. Why don't you simply port over Linux'es solution (which
> changes osl.c instead), the more that now we have Linux-like
> Kconfig?
Of course I can and I suggested this way at v3 but ...
Hope this your final suggestion. Thanks in advance!

-- 
Shannon

Re: [PATCH v4 09/10] arm/uart: Rename dt-uart.c to arm-uart.c

[Jan Beulich]

>>> On 19.01.16 at 04:36, <zhaoshenglong@huawei.com> wrote:

> 
> On 2016/1/18 18:40, Jan Beulich wrote:
>>>>> On 16.01.16 at 06:01, <zhaoshenglong@huawei.com> wrote:
>>> > From: Shannon Zhao <shannon.zhao@linaro.org>
>>> > 
>>> > Since we will add ACPI initialization for UART in this file later,
>>> > rename it with a generic name.
>>> > 
>>> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>>> > ---
>>> > v4: split the original patch to renaming this and adding ACPI parts.
>>> > ---
>>> >  MAINTAINERS                 |   2 +-
>>> >  xen/drivers/char/Makefile   |   2 +-
>>> >  xen/drivers/char/arm-uart.c | 107 ++++++++++++++++++++++++++++++++++++++++++++
>>> >  xen/drivers/char/dt-uart.c  | 107 --------------------------------------------
>>> >  4 files changed, 109 insertions(+), 109 deletions(-)
>>> >  create mode 100644 xen/drivers/char/arm-uart.c
>>> >  delete mode 100644 xen/drivers/char/dt-uart.c
>> Looks like this really is a rename with little actual changes to
>> the file, 
> Yes, but the only change to this file is also to change its name because
> it's mentioned in the file header. This intends to make that consistent.
> 
> + * xen/drivers/char/arm-uart.c

I understand that, but without using git's ability to represent the
rename such that the diff between the files is visible it is hard to
tell what exactly you changed in the file.

Jan

Re: [PATCH v4 09/10] arm/uart: Rename dt-uart.c to arm-uart.c

[Shannon Zhao]

On 2016/1/19 16:39, Jan Beulich wrote:
>>>> On 19.01.16 at 04:36, <zhaoshenglong@huawei.com> wrote:
>> > 
>> > On 2016/1/18 18:40, Jan Beulich wrote:
>>>>>> >>>>> On 16.01.16 at 06:01, <zhaoshenglong@huawei.com> wrote:
>>>>> >>> > From: Shannon Zhao <shannon.zhao@linaro.org>
>>>>> >>> > 
>>>>> >>> > Since we will add ACPI initialization for UART in this file later,
>>>>> >>> > rename it with a generic name.
>>>>> >>> > 
>>>>> >>> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>>>>> >>> > ---
>>>>> >>> > v4: split the original patch to renaming this and adding ACPI parts.
>>>>> >>> > ---
>>>>> >>> >  MAINTAINERS                 |   2 +-
>>>>> >>> >  xen/drivers/char/Makefile   |   2 +-
>>>>> >>> >  xen/drivers/char/arm-uart.c | 107 ++++++++++++++++++++++++++++++++++++++++++++
>>>>> >>> >  xen/drivers/char/dt-uart.c  | 107 --------------------------------------------
>>>>> >>> >  4 files changed, 109 insertions(+), 109 deletions(-)
>>>>> >>> >  create mode 100644 xen/drivers/char/arm-uart.c
>>>>> >>> >  delete mode 100644 xen/drivers/char/dt-uart.c
>>> >> Looks like this really is a rename with little actual changes to
>>> >> the file, 
>> > Yes, but the only change to this file is also to change its name because
>> > it's mentioned in the file header. This intends to make that consistent.
>> > 
>> > + * xen/drivers/char/arm-uart.c
> I understand that, but without using git's ability to represent the
> rename such that the diff between the files is visible it is hard to
> tell what exactly you changed in the file.
Yes, from the git diffstat it's hard to tell the truth. I will add some
words in the commit message.

Thanks,
-- 
Shannon

Re: [PATCH v4 09/10] arm/uart: Rename dt-uart.c to arm-uart.c

[Jan Beulich]

>>> On 19.01.16 at 09:43, <zhaoshenglong@huawei.com> wrote:

> 
> On 2016/1/19 16:39, Jan Beulich wrote:
>>>>> On 19.01.16 at 04:36, <zhaoshenglong@huawei.com> wrote:
>>> > 
>>> > On 2016/1/18 18:40, Jan Beulich wrote:
>>>>>>> >>>>> On 16.01.16 at 06:01, <zhaoshenglong@huawei.com> wrote:
>>>>>> >>> > From: Shannon Zhao <shannon.zhao@linaro.org>
>>>>>> >>> > 
>>>>>> >>> > Since we will add ACPI initialization for UART in this file later,
>>>>>> >>> > rename it with a generic name.
>>>>>> >>> > 
>>>>>> >>> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>>>>>> >>> > ---
>>>>>> >>> > v4: split the original patch to renaming this and adding ACPI parts.
>>>>>> >>> > ---
>>>>>> >>> >  MAINTAINERS                 |   2 +-
>>>>>> >>> >  xen/drivers/char/Makefile   |   2 +-
>>>>>> >>> >  xen/drivers/char/arm-uart.c | 107 
> ++++++++++++++++++++++++++++++++++++++++++++
>>>>>> >>> >  xen/drivers/char/dt-uart.c  | 107 --------------------------------------------
>>>>>> >>> >  4 files changed, 109 insertions(+), 109 deletions(-)
>>>>>> >>> >  create mode 100644 xen/drivers/char/arm-uart.c
>>>>>> >>> >  delete mode 100644 xen/drivers/char/dt-uart.c
>>>> >> Looks like this really is a rename with little actual changes to
>>>> >> the file, 
>>> > Yes, but the only change to this file is also to change its name because
>>> > it's mentioned in the file header. This intends to make that consistent.
>>> > 
>>> > + * xen/drivers/char/arm-uart.c
>> I understand that, but without using git's ability to represent the
>> rename such that the diff between the files is visible it is hard to
>> tell what exactly you changed in the file.
> Yes, from the git diffstat it's hard to tell the truth. I will add some
> words in the commit message.

That's not the point; the point is that if you already use git to
create the patch, you should also use the needed option to
make git represent the patch in a way the change-and-rename
gets reflected suitably. If you weren't using git to create the
patch I'd be fine with the actual change(s) just being mentioned
in the commit message.

Jan

Re: [PATCH v4 05/10] acpi: Refactor acpi_os_map_memory to be architecturally independent

[Shannon Zhao]

On 2016/1/18 21:33, Jan Beulich wrote:
>>>> On 16.01.16 at 06:01, <zhaoshenglong@huawei.com> wrote:
>> > --- a/xen/drivers/acpi/osl.c
>> > +++ b/xen/drivers/acpi/osl.c
>> > @@ -86,17 +86,7 @@ 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_active) {
>> > -		mfn_t mfn = _mfn(PFN_DOWN(phys));
>> > -		unsigned int offs = phys & (PAGE_SIZE - 1);
>> > -
>> > -		/* The low first Mb is always mapped. */
>> > -		if ( !((phys + size - 1) >> 20) )
>> > -			return __va(phys);
>> > -		return __vmap(&mfn, PFN_UP(offs + size), 1, 1,
>> > -			      PAGE_HYPERVISOR_NOCACHE) + offs;
>> > -	}
>> > -	return __acpi_map_table(phys, size);
>> > +	return arch_acpi_os_map_memory(phys, size);
>> >  }
> I'm quite sure I've said before that this goes too far: The __vmap()
> part and likely also the early-boot __acpi_map_table() one already
> are architecture independent and hence should stay. The factoring
> hence should only concern the first Mb handling and maybe the
> the mapping attributes passed to __vmap().

Yes, the first MB handling and __vmap() should refactor. So if it only
moves them to an architecture function, how about below patch?

diff --git a/xen/arch/x86/acpi/lib.c b/xen/arch/x86/acpi/lib.c
index cc15ea3..5885a3a 100644
--- a/xen/arch/x86/acpi/lib.c
+++ b/xen/arch/x86/acpi/lib.c
@@ -33,6 +33,19 @@ u8 __read_mostly acpi_disable_value;
 u32 __read_mostly x86_acpiid_to_apicid[MAX_MADT_ENTRIES] =
     {[0 ... MAX_MADT_ENTRIES - 1] = BAD_APICID };

+void __iomem *
+arch_acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
+{
+       mfn_t mfn = _mfn(PFN_DOWN(phys));
+       unsigned int offs = phys & (PAGE_SIZE - 1);
+
+       /* The low first Mb is always mapped. */
+       if ( !((phys + size - 1) >> 20) )
+               return __va(phys);
+       return __vmap(&mfn, PFN_UP(offs + size), 1, 1,
+                     PAGE_HYPERVISOR_NOCACHE) + offs;
+}
+
 /*
  * Important Safety Note:  The fixed ACPI page numbers are *subtracted*
  * from the fixed base.  That's why we start at FIX_ACPI_END and
diff --git a/xen/drivers/acpi/osl.c b/xen/drivers/acpi/osl.c
index a2fc8c4..e2dda2e 100644
--- a/xen/drivers/acpi/osl.c
+++ b/xen/drivers/acpi/osl.c
@@ -88,16 +88,8 @@ 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_active) {
-               mfn_t mfn = _mfn(PFN_DOWN(phys));
-               unsigned int offs = phys & (PAGE_SIZE - 1);
-
-               /* The low first Mb is always mapped. */
-               if ( !((phys + size - 1) >> 20) )
-                       return __va(phys);
-               return __vmap(&mfn, PFN_UP(offs + size), 1, 1,
-                             PAGE_HYPERVISOR_NOCACHE) + offs;
-       }
+       if (system_state >= SYS_STATE_active)
+               return arch_acpi_os_map_memory(phys, size);
        return __acpi_map_table(phys, size);
 }


-- 
Shannon

Re: [PATCH v4 05/10] acpi: Refactor acpi_os_map_memory to be architecturally independent

[Jan Beulich]

>>> On 22.01.16 at 09:38, <zhaoshenglong@huawei.com> wrote:

> 
> On 2016/1/18 21:33, Jan Beulich wrote:
>>>>> On 16.01.16 at 06:01, <zhaoshenglong@huawei.com> wrote:
>>> > --- a/xen/drivers/acpi/osl.c
>>> > +++ b/xen/drivers/acpi/osl.c
>>> > @@ -86,17 +86,7 @@ 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_active) {
>>> > -		mfn_t mfn = _mfn(PFN_DOWN(phys));
>>> > -		unsigned int offs = phys & (PAGE_SIZE - 1);
>>> > -
>>> > -		/* The low first Mb is always mapped. */
>>> > -		if ( !((phys + size - 1) >> 20) )
>>> > -			return __va(phys);
>>> > -		return __vmap(&mfn, PFN_UP(offs + size), 1, 1,
>>> > -			      PAGE_HYPERVISOR_NOCACHE) + offs;
>>> > -	}
>>> > -	return __acpi_map_table(phys, size);
>>> > +	return arch_acpi_os_map_memory(phys, size);
>>> >  }
>> I'm quite sure I've said before that this goes too far: The __vmap()
>> part and likely also the early-boot __acpi_map_table() one already
>> are architecture independent and hence should stay. The factoring
>> hence should only concern the first Mb handling and maybe the
>> the mapping attributes passed to __vmap().
> 
> Yes, the first MB handling and __vmap() should refactor. So if it only
> moves them to an architecture function, how about below patch?
> 
> diff --git a/xen/arch/x86/acpi/lib.c b/xen/arch/x86/acpi/lib.c
> index cc15ea3..5885a3a 100644
> --- a/xen/arch/x86/acpi/lib.c
> +++ b/xen/arch/x86/acpi/lib.c
> @@ -33,6 +33,19 @@ u8 __read_mostly acpi_disable_value;
>  u32 __read_mostly x86_acpiid_to_apicid[MAX_MADT_ENTRIES] =
>      {[0 ... MAX_MADT_ENTRIES - 1] = BAD_APICID };
> 
> +void __iomem *
> +arch_acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
> +{
> +       mfn_t mfn = _mfn(PFN_DOWN(phys));
> +       unsigned int offs = phys & (PAGE_SIZE - 1);
> +
> +       /* The low first Mb is always mapped. */
> +       if ( !((phys + size - 1) >> 20) )
> +               return __va(phys);
> +       return __vmap(&mfn, PFN_UP(offs + size), 1, 1,
> +                     PAGE_HYPERVISOR_NOCACHE) + offs;
> +}

Well, I had clearly said the vmap() part is generic; if there's
anything architecture dependent here, then the mapping
attributes (and hence only _those_ should be factored out,
not the entire function invocation).

Jan

Re: [PATCH v4 05/10] acpi: Refactor acpi_os_map_memory to be architecturally independent

[Shannon Zhao]

On 2016/1/22 16:47, Jan Beulich wrote:
>>>> On 22.01.16 at 09:38, <zhaoshenglong@huawei.com> wrote:
>> > 
>> > On 2016/1/18 21:33, Jan Beulich wrote:
>>>>>> >>>>> On 16.01.16 at 06:01, <zhaoshenglong@huawei.com> wrote:
>>>>> >>> > --- a/xen/drivers/acpi/osl.c
>>>>> >>> > +++ b/xen/drivers/acpi/osl.c
>>>>> >>> > @@ -86,17 +86,7 @@ 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_active) {
>>>>> >>> > -		mfn_t mfn = _mfn(PFN_DOWN(phys));
>>>>> >>> > -		unsigned int offs = phys & (PAGE_SIZE - 1);
>>>>> >>> > -
>>>>> >>> > -		/* The low first Mb is always mapped. */
>>>>> >>> > -		if ( !((phys + size - 1) >> 20) )
>>>>> >>> > -			return __va(phys);
>>>>> >>> > -		return __vmap(&mfn, PFN_UP(offs + size), 1, 1,
>>>>> >>> > -			      PAGE_HYPERVISOR_NOCACHE) + offs;
>>>>> >>> > -	}
>>>>> >>> > -	return __acpi_map_table(phys, size);
>>>>> >>> > +	return arch_acpi_os_map_memory(phys, size);
>>>>> >>> >  }
>>> >> I'm quite sure I've said before that this goes too far: The __vmap()
>>> >> part and likely also the early-boot __acpi_map_table() one already
>>> >> are architecture independent and hence should stay. The factoring
>>> >> hence should only concern the first Mb handling and maybe the
>>> >> the mapping attributes passed to __vmap().
>> > 
>> > Yes, the first MB handling and __vmap() should refactor. So if it only
>> > moves them to an architecture function, how about below patch?
>> > 
>> > diff --git a/xen/arch/x86/acpi/lib.c b/xen/arch/x86/acpi/lib.c
>> > index cc15ea3..5885a3a 100644
>> > --- a/xen/arch/x86/acpi/lib.c
>> > +++ b/xen/arch/x86/acpi/lib.c
>> > @@ -33,6 +33,19 @@ u8 __read_mostly acpi_disable_value;
>> >  u32 __read_mostly x86_acpiid_to_apicid[MAX_MADT_ENTRIES] =
>> >      {[0 ... MAX_MADT_ENTRIES - 1] = BAD_APICID };
>> > 
>> > +void __iomem *
>> > +arch_acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
>> > +{
>> > +       mfn_t mfn = _mfn(PFN_DOWN(phys));
>> > +       unsigned int offs = phys & (PAGE_SIZE - 1);
>> > +
>> > +       /* The low first Mb is always mapped. */
>> > +       if ( !((phys + size - 1) >> 20) )
>> > +               return __va(phys);
>> > +       return __vmap(&mfn, PFN_UP(offs + size), 1, 1,
>> > +                     PAGE_HYPERVISOR_NOCACHE) + offs;
>> > +}
> Well, I had clearly said the vmap() part is generic; if there's
> anything architecture dependent here, then the mapping
> attributes (and hence only _those_ should be factored out,
> not the entire function invocation).
I know what you said. But how can we change the attribute for ARM in
acpi_os_map_memory() without moving these codes out?

Thanks,
-- 
Shannon

Re: [PATCH v4 05/10] acpi: Refactor acpi_os_map_memory to be architecturally independent

[Jan Beulich]

>>> On 22.01.16 at 10:37, <zhaoshenglong@huawei.com> wrote:

> 
> On 2016/1/22 16:47, Jan Beulich wrote:
>>>>> On 22.01.16 at 09:38, <zhaoshenglong@huawei.com> wrote:
>>> > 
>>> > On 2016/1/18 21:33, Jan Beulich wrote:
>>>>>>> >>>>> On 16.01.16 at 06:01, <zhaoshenglong@huawei.com> wrote:
>>>>>> >>> > --- a/xen/drivers/acpi/osl.c
>>>>>> >>> > +++ b/xen/drivers/acpi/osl.c
>>>>>> >>> > @@ -86,17 +86,7 @@ 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_active) {
>>>>>> >>> > -		mfn_t mfn = _mfn(PFN_DOWN(phys));
>>>>>> >>> > -		unsigned int offs = phys & (PAGE_SIZE - 1);
>>>>>> >>> > -
>>>>>> >>> > -		/* The low first Mb is always mapped. */
>>>>>> >>> > -		if ( !((phys + size - 1) >> 20) )
>>>>>> >>> > -			return __va(phys);
>>>>>> >>> > -		return __vmap(&mfn, PFN_UP(offs + size), 1, 1,
>>>>>> >>> > -			      PAGE_HYPERVISOR_NOCACHE) + offs;
>>>>>> >>> > -	}
>>>>>> >>> > -	return __acpi_map_table(phys, size);
>>>>>> >>> > +	return arch_acpi_os_map_memory(phys, size);
>>>>>> >>> >  }
>>>> >> I'm quite sure I've said before that this goes too far: The __vmap()
>>>> >> part and likely also the early-boot __acpi_map_table() one already
>>>> >> are architecture independent and hence should stay. The factoring
>>>> >> hence should only concern the first Mb handling and maybe the
>>>> >> the mapping attributes passed to __vmap().
>>> > 
>>> > Yes, the first MB handling and __vmap() should refactor. So if it only
>>> > moves them to an architecture function, how about below patch?
>>> > 
>>> > diff --git a/xen/arch/x86/acpi/lib.c b/xen/arch/x86/acpi/lib.c
>>> > index cc15ea3..5885a3a 100644
>>> > --- a/xen/arch/x86/acpi/lib.c
>>> > +++ b/xen/arch/x86/acpi/lib.c
>>> > @@ -33,6 +33,19 @@ u8 __read_mostly acpi_disable_value;
>>> >  u32 __read_mostly x86_acpiid_to_apicid[MAX_MADT_ENTRIES] =
>>> >      {[0 ... MAX_MADT_ENTRIES - 1] = BAD_APICID };
>>> > 
>>> > +void __iomem *
>>> > +arch_acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
>>> > +{
>>> > +       mfn_t mfn = _mfn(PFN_DOWN(phys));
>>> > +       unsigned int offs = phys & (PAGE_SIZE - 1);
>>> > +
>>> > +       /* The low first Mb is always mapped. */
>>> > +       if ( !((phys + size - 1) >> 20) )
>>> > +               return __va(phys);
>>> > +       return __vmap(&mfn, PFN_UP(offs + size), 1, 1,
>>> > +                     PAGE_HYPERVISOR_NOCACHE) + offs;
>>> > +}
>> Well, I had clearly said the vmap() part is generic; if there's
>> anything architecture dependent here, then the mapping
>> attributes (and hence only _those_ should be factored out,
>> not the entire function invocation).
> I know what you said. But how can we change the attribute for ARM in
> acpi_os_map_memory() without moving these codes out?

By having each arch #define their value, and use that constant here?

Jan

Re: [PATCH v4 05/10] acpi: Refactor acpi_os_map_memory to be architecturally independent

[Shannon Zhao]

On 2016/1/22 18:15, Jan Beulich wrote:
>>>> On 22.01.16 at 10:37,<zhaoshenglong@huawei.com>  wrote:
>> >
>> >On 2016/1/22 16:47, Jan Beulich wrote:
>>>>>> >>>>>On 22.01.16 at 09:38,<zhaoshenglong@huawei.com>  wrote:
>>>>> >>> >
>>>>> >>> >On 2016/1/18 21:33, Jan Beulich wrote:
>>>>>>>>>>>>> >>>>>>> >>>>>On 16.01.16 at 06:01,<zhaoshenglong@huawei.com>  wrote:
>>>>>>>>>>> >>>>>> >>> >--- a/xen/drivers/acpi/osl.c
>>>>>>>>>>> >>>>>> >>> >+++ b/xen/drivers/acpi/osl.c
>>>>>>>>>>> >>>>>> >>> >@@ -86,17 +86,7 @@ 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_active) {
>>>>>>>>>>> >>>>>> >>> >-		mfn_t mfn = _mfn(PFN_DOWN(phys));
>>>>>>>>>>> >>>>>> >>> >-		unsigned int offs = phys & (PAGE_SIZE - 1);
>>>>>>>>>>> >>>>>> >>> >-
>>>>>>>>>>> >>>>>> >>> >-		/* The low first Mb is always mapped. */
>>>>>>>>>>> >>>>>> >>> >-		if ( !((phys + size - 1) >> 20) )
>>>>>>>>>>> >>>>>> >>> >-			return __va(phys);
>>>>>>>>>>> >>>>>> >>> >-		return __vmap(&mfn, PFN_UP(offs + size), 1, 1,
>>>>>>>>>>> >>>>>> >>> >-			      PAGE_HYPERVISOR_NOCACHE) + offs;
>>>>>>>>>>> >>>>>> >>> >-	}
>>>>>>>>>>> >>>>>> >>> >-	return __acpi_map_table(phys, size);
>>>>>>>>>>> >>>>>> >>> >+	return arch_acpi_os_map_memory(phys, size);
>>>>>>>>>>> >>>>>> >>> >  }
>>>>>>> >>>> >>I'm quite sure I've said before that this goes too far: The __vmap()
>>>>>>> >>>> >>part and likely also the early-boot __acpi_map_table() one already
>>>>>>> >>>> >>are architecture independent and hence should stay. The factoring
>>>>>>> >>>> >>hence should only concern the first Mb handling and maybe the
>>>>>>> >>>> >>the mapping attributes passed to __vmap().
>>>>> >>> >
>>>>> >>> >Yes, the first MB handling and __vmap() should refactor. So if it only
>>>>> >>> >moves them to an architecture function, how about below patch?
>>>>> >>> >
>>>>> >>> >diff --git a/xen/arch/x86/acpi/lib.c b/xen/arch/x86/acpi/lib.c
>>>>> >>> >index cc15ea3..5885a3a 100644
>>>>> >>> >--- a/xen/arch/x86/acpi/lib.c
>>>>> >>> >+++ b/xen/arch/x86/acpi/lib.c
>>>>> >>> >@@ -33,6 +33,19 @@ u8 __read_mostly acpi_disable_value;
>>>>> >>> >  u32 __read_mostly x86_acpiid_to_apicid[MAX_MADT_ENTRIES] =
>>>>> >>> >      {[0 ... MAX_MADT_ENTRIES - 1] = BAD_APICID };
>>>>> >>> >
>>>>> >>> >+void __iomem *
>>>>> >>> >+arch_acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
>>>>> >>> >+{
>>>>> >>> >+       mfn_t mfn = _mfn(PFN_DOWN(phys));
>>>>> >>> >+       unsigned int offs = phys & (PAGE_SIZE - 1);
>>>>> >>> >+
>>>>> >>> >+       /* The low first Mb is always mapped. */
>>>>> >>> >+       if ( !((phys + size - 1) >> 20) )
>>>>> >>> >+               return __va(phys);
>>>>> >>> >+       return __vmap(&mfn, PFN_UP(offs + size), 1, 1,
>>>>> >>> >+                     PAGE_HYPERVISOR_NOCACHE) + offs;
>>>>> >>> >+}
>>> >>Well, I had clearly said the vmap() part is generic; if there's
>>> >>anything architecture dependent here, then the mapping
>>> >>attributes (and hence only_those_  should be factored out,
>>> >>not the entire function invocation).
>> >I know what you said. But how can we change the attribute for ARM in
>> >acpi_os_map_memory() without moving these codes out?
> By having each arch #define their value, and use that constant here?
You really want that? Even though the way here I use is not too many 
dunplicated codes (and I think it looks clearer).

-- 
Shannon

Re: [PATCH v4 05/10] acpi: Refactor acpi_os_map_memory to be architecturally independent

[Jan Beulich]

>>> On 22.01.16 at 12:55, <shannon.zhao@linaro.org> wrote:

> 
> On 2016/1/22 18:15, Jan Beulich wrote:
>>>>> On 22.01.16 at 10:37,<zhaoshenglong@huawei.com>  wrote:
>>> >
>>> >On 2016/1/22 16:47, Jan Beulich wrote:
>>>>>>> >>>>>On 22.01.16 at 09:38,<zhaoshenglong@huawei.com>  wrote:
>>>>>> >>> >
>>>>>> >>> >On 2016/1/18 21:33, Jan Beulich wrote:
>>>>>>>>>>>>>> >>>>>>> >>>>>On 16.01.16 at 06:01,<zhaoshenglong@huawei.com>  wrote:
>>>>>>>>>>>> >>>>>> >>> >--- a/xen/drivers/acpi/osl.c
>>>>>>>>>>>> >>>>>> >>> >+++ b/xen/drivers/acpi/osl.c
>>>>>>>>>>>> >>>>>> >>> >@@ -86,17 +86,7 @@ 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_active) {
>>>>>>>>>>>> >>>>>> >>> >-		mfn_t mfn = _mfn(PFN_DOWN(phys));
>>>>>>>>>>>> >>>>>> >>> >-		unsigned int offs = phys & (PAGE_SIZE - 1);
>>>>>>>>>>>> >>>>>> >>> >-
>>>>>>>>>>>> >>>>>> >>> >-		/* The low first Mb is always mapped. */
>>>>>>>>>>>> >>>>>> >>> >-		if ( !((phys + size - 1) >> 20) )
>>>>>>>>>>>> >>>>>> >>> >-			return __va(phys);
>>>>>>>>>>>> >>>>>> >>> >-		return __vmap(&mfn, PFN_UP(offs + size), 1, 1,
>>>>>>>>>>>> >>>>>> >>> >-			      PAGE_HYPERVISOR_NOCACHE) + offs;
>>>>>>>>>>>> >>>>>> >>> >-	}
>>>>>>>>>>>> >>>>>> >>> >-	return __acpi_map_table(phys, size);
>>>>>>>>>>>> >>>>>> >>> >+	return arch_acpi_os_map_memory(phys, size);
>>>>>>>>>>>> >>>>>> >>> >  }
>>>>>>>> >>>> >>I'm quite sure I've said before that this goes too far: The __vmap()
>>>>>>>> >>>> >>part and likely also the early-boot __acpi_map_table() one already
>>>>>>>> >>>> >>are architecture independent and hence should stay. The factoring
>>>>>>>> >>>> >>hence should only concern the first Mb handling and maybe the
>>>>>>>> >>>> >>the mapping attributes passed to __vmap().
>>>>>> >>> >
>>>>>> >>> >Yes, the first MB handling and __vmap() should refactor. So if it only
>>>>>> >>> >moves them to an architecture function, how about below patch?
>>>>>> >>> >
>>>>>> >>> >diff --git a/xen/arch/x86/acpi/lib.c b/xen/arch/x86/acpi/lib.c
>>>>>> >>> >index cc15ea3..5885a3a 100644
>>>>>> >>> >--- a/xen/arch/x86/acpi/lib.c
>>>>>> >>> >+++ b/xen/arch/x86/acpi/lib.c
>>>>>> >>> >@@ -33,6 +33,19 @@ u8 __read_mostly acpi_disable_value;
>>>>>> >>> >  u32 __read_mostly x86_acpiid_to_apicid[MAX_MADT_ENTRIES] =
>>>>>> >>> >      {[0 ... MAX_MADT_ENTRIES - 1] = BAD_APICID };
>>>>>> >>> >
>>>>>> >>> >+void __iomem *
>>>>>> >>> >+arch_acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
>>>>>> >>> >+{
>>>>>> >>> >+       mfn_t mfn = _mfn(PFN_DOWN(phys));
>>>>>> >>> >+       unsigned int offs = phys & (PAGE_SIZE - 1);
>>>>>> >>> >+
>>>>>> >>> >+       /* The low first Mb is always mapped. */
>>>>>> >>> >+       if ( !((phys + size - 1) >> 20) )
>>>>>> >>> >+               return __va(phys);
>>>>>> >>> >+       return __vmap(&mfn, PFN_UP(offs + size), 1, 1,
>>>>>> >>> >+                     PAGE_HYPERVISOR_NOCACHE) + offs;
>>>>>> >>> >+}
>>>> >>Well, I had clearly said the vmap() part is generic; if there's
>>>> >>anything architecture dependent here, then the mapping
>>>> >>attributes (and hence only_those_  should be factored out,
>>>> >>not the entire function invocation).
>>> >I know what you said. But how can we change the attribute for ARM in
>>> >acpi_os_map_memory() without moving these codes out?
>> By having each arch #define their value, and use that constant here?
> You really want that?

Yes.

Jan