Re: [PATCH RFC 02/35] xen: arm64: ACPI: Support common ACPI drivers

[Julien Grall <julien.grall@linaro.org>]

Hi Parth,

On 04/02/2015 14:01, parth.dixit@linaro.org wrote:
> From: Naresh Bhat <naresh.bhat@linaro.org>
>
> xen hypervisor will use ACPI for initialisation in the same manner that
> current x86/x86_64 ones do. Add the calls to initialise the ACPI tables
> and load devices using the xen/drivers/acpi subsytem.

All changes in this patch are mostly unrelated, and most of them 
requires an explanation why we need to it.

I would split this patch in small chunk.

See below for others comment.

> Signed-off-by: Naresh Bhat <naresh.bhat@linaro.org>
> ---
>   xen/common/sysctl.c                   |   2 +
>   xen/drivers/acpi/osl.c                |   6 ++
>   xen/drivers/acpi/utilities/utglobal.c |   1 +
>   xen/include/asm-arm/acpi.h            | 106 ++++++++++++++++++++++++++++++++++
>   xen/include/asm-arm/arm64/page.h      |   2 +
>   5 files changed, 117 insertions(+)
>   create mode 100644 xen/include/asm-arm/acpi.h
>
> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
> index 0cb6ee1..a700a16 100644
> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -170,6 +170,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>           op->u.availheap.avail_bytes <<= PAGE_SHIFT;
>           break;
>
> +#ifdef CONFIG_X86
>   #ifdef HAS_ACPI

#if defined(CONFIG_X86) && defined(HAS_ACPI) ?

Also, this change seems to be related to the patch #1.

I would make sense to create a separate patch which will disable the 
compilation of pmstate and adding this #ifdef.

>       case XEN_SYSCTL_get_pmstat:
>           ret = do_get_pm_info(&op->u.get_pmstat);
> @@ -181,6 +182,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>               copyback = 1;
>           break;
>   #endif
> +#endif
>
>       case XEN_SYSCTL_page_offline_op:
>       {
> diff --git a/xen/drivers/acpi/osl.c b/xen/drivers/acpi/osl.c
> index 93c983c..73da9d9 100644
> --- a/xen/drivers/acpi/osl.c
> +++ b/xen/drivers/acpi/osl.c
> @@ -96,7 +96,11 @@ acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
>   			return __va(phys);
>   		return __vmap(&pfn, PFN_UP(offs + size), 1, 1, PAGE_HYPERVISOR_NOCACHE) + offs;
>   	}
> +#ifdef CONFIG_X86
>   	return __acpi_map_table(phys, size);
> +#else
> +	return __va(phys);

I remembered to have a discussion about this change with Naresh few 
month ago.

__va should only be used when the memory is direct-mapped to Xen (i.e 
accessible directly). On ARM64, this only the case for the RAM. Can you 
confirm that ACPI will always reside to the RAM?

Futhermore, the code of this function seems x86-specific. The low 1MB 
may not be mapped on ARM64.

I would move the whole function (acpi_os_map_memory) per-architecture.

> +#endif
>   }


>   void acpi_os_unmap_memory(void __iomem * virt, acpi_size size)
> @@ -105,6 +109,7 @@ void acpi_os_unmap_memory(void __iomem * virt, acpi_size size)
>   		vunmap((void *)((unsigned long)virt & PAGE_MASK));
>   }
>
> +#ifdef CONFIG_X86
>   acpi_status acpi_os_read_port(acpi_io_address port, u32 * value, u32 width)
>   {
>   	u32 dummy;
> @@ -140,6 +145,7 @@ acpi_status acpi_os_write_port(acpi_io_address port, u32 value, u32 width)
>
>   	return AE_OK;
>   }
> +#endif

Why only x86? Linux seems to define it also for ARM64.

>   acpi_status
>   acpi_os_read_memory(acpi_physical_address phys_addr, u32 * value, u32 width)
> diff --git a/xen/drivers/acpi/utilities/utglobal.c b/xen/drivers/acpi/utilities/utglobal.c
> index 7dbc964..65c918e 100644
> --- a/xen/drivers/acpi/utilities/utglobal.c
> +++ b/xen/drivers/acpi/utilities/utglobal.c
> @@ -43,6 +43,7 @@
>
>   #define DEFINE_ACPI_GLOBALS
>
> +#include <asm/system.h>

Why it's necessary?

>   #include <xen/config.h>
>   #include <xen/init.h>
>   #include <xen/lib.h>
> diff --git a/xen/include/asm-arm/acpi.h b/xen/include/asm-arm/acpi.h
> new file mode 100644
> index 0000000..f6284b5
> --- /dev/null
> +++ b/xen/include/asm-arm/acpi.h
> @@ -0,0 +1,106 @@
> +/*
> + *  Copyright (C) 2014, Naresh Bhat <naresh.bhat@linaro.org>
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + *
> + *  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.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with this program; if not, write to the Free Software
> + *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + */
> +
> +#ifndef _ASM_ARM64_ACPI_H
> +#define _ASM_ARM64_ACPI_H
> +
> +#include <xen/init.h>
> +
> +#define COMPILER_DEPENDENT_INT64   long long
> +#define COMPILER_DEPENDENT_UINT64  unsigned long long
> +
> +#define MAX_LOCAL_APIC 256
> +#define MAX_IO_APICS 64
> +
> +/*
> + * Calling conventions:
> + *
> + * ACPI_SYSTEM_XFACE        - Interfaces to host OS (handlers, threads)
> + * ACPI_EXTERNAL_XFACE      - External ACPI interfaces
> + * ACPI_INTERNAL_XFACE      - Internal ACPI interfaces
> + * ACPI_INTERNAL_VAR_XFACE  - Internal variable-parameter list interfaces
> + */
> +#define ACPI_SYSTEM_XFACE
> +#define ACPI_EXTERNAL_XFACE
> +#define ACPI_INTERNAL_XFACE
> +#define ACPI_INTERNAL_VAR_XFACE
> +
> +/* Asm macros */
> +#define ACPI_ASM_MACROS
> +#define BREAKPOINT3

It's never used on common code.

> +#define ACPI_DISABLE_IRQS() local_irq_disable()
> +#define ACPI_ENABLE_IRQS()  local_irq_enable()
> +#define ACPI_FLUSH_CPU_CACHE() flush_cache_all()
> +
> +/* Blob handling macros */
> +#define ACPI_BLOB_HEADER_SIZE   8
> +
> +/* Basic configuration for ACPI */
> +#ifdef  CONFIG_ACPI
> +extern int acpi_disabled;
> +extern int acpi_noirq;
> +extern int acpi_pci_disabled;
> +extern int acpi_strict;

I think it would be better to define common external variable in a 
common headers. Although I'm an ACPI maintainers...

> +/* map logic cpu id to physical APIC id
> + * APIC = GIC cpu interface on ARM
> + */
> +extern volatile int arm_cpu_to_apicid[NR_CPUS];
> +extern int boot_cpu_apic_id;
> +#define cpu_physical_id(cpu) arm_cpu_to_apicid[cpu]
> +
> +struct acpi_arm_root {
> +    paddr_t phys_address;
> +    unsigned long size;
> +};

Can you document this structure?

> +extern struct acpi_arm_root acpi_arm_rsdp_info;

This external variable is defined but seem to never be used in the whole 
series.

> +void arm_acpi_reserve_memory(void);
> +
> +/* Low-level suspend routine. */
> +extern int (*acpi_suspend_lowlevel)(void);
> +
> +extern void prefill_possible_map(void);

Those 3 functions are never implemented neither used.

> +#define acpi_wakeup_address (0)

This macro is never used.

> +static inline void disable_acpi(void)
> +{
> +    acpi_disabled = 1;
> +    acpi_pci_disabled = 1;
> +    acpi_noirq = 1;
> +}
> +static inline void acpi_noirq_set(void) { acpi_noirq = 1; }
> +static inline void acpi_disable_pci(void)
> +{
> +    acpi_pci_disabled = 1;
> +    acpi_noirq_set();
> +}

Ditto for acpi_noirq_set and acpi_disable_pci

> +#else   /* !CONFIG_ACPI */
> +#define acpi_disabled 1 /* ACPI sometimes enabled on ARM */
> +#define acpi_noirq 1    /* ACPI sometimes enabled on ARM */
> +#define acpi_pci_disabled 1 /* ACPI PCI sometimes enabled on ARM */
> +#define acpi_strict 1   /* no ACPI spec workarounds on ARM */

The comments are unclear to me here.

> +#endif
> +
> +#endif /*_ASM_ARM_ACPI_H*/
> diff --git a/xen/include/asm-arm/arm64/page.h b/xen/include/asm-arm/arm64/page.h
> index 1fd416d..13b0ea1 100644
> --- a/xen/include/asm-arm/arm64/page.h
> +++ b/xen/include/asm-arm/arm64/page.h
> @@ -1,6 +1,8 @@
>   #ifndef __ARM_ARM64_PAGE_H__
>   #define __ARM_ARM64_PAGE_H__
>
> +#include <asm-arm/system.h>
> +

Why?

Regards,

-- 
Julien Grall