Re: [PATCH RFC 03/35] xen: arm64: ACPI: Add basic ACPI initialization

[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>
>
> This patch introduce arm-core.c and its related header file
>
> - asm/acpi.h for arch specific variables and functions needed by
>    ACPI driver core;
> - arm-core.c for ARM64 related ACPI implementation for ACPI driver
>    core;
>
> acpi_boot_table_init() will be called in setup_arch()
> to get the RSDP and all the table pointers. with this patch,
> we can get ACPI boot-time tables from firmware on ARM64.
>
> Signed-off-by: Naresh Bhat <naresh.bhat@linaro.org>
> ---
>   xen/arch/arm/arm64/Makefile        |  1 +
>   xen/arch/arm/arm64/acpi/Makefile   |  1 +
>   xen/arch/arm/arm64/acpi/arm-core.c | 97 ++++++++++++++++++++++++++++++++++++++
>   xen/arch/arm/setup.c               | 14 +++++-
>   xen/include/xen/acpi.h             | 11 +++++
>   5 files changed, 123 insertions(+), 1 deletion(-)
>   create mode 100644 xen/arch/arm/arm64/acpi/Makefile
>   create mode 100644 xen/arch/arm/arm64/acpi/arm-core.c
>
> diff --git a/xen/arch/arm/arm64/Makefile b/xen/arch/arm/arm64/Makefile
> index c7243f5..49d135f 100644
> --- a/xen/arch/arm/arm64/Makefile
> +++ b/xen/arch/arm/arm64/Makefile
> @@ -1,4 +1,5 @@
>   subdir-y += lib
> +subdir-y += acpi

It would be more logic to include this directory only when HAS_ACPI is 
defined.

It would be smth like:

subdir-$(HAS_ACPI) += acpi

[...]

> diff --git a/xen/arch/arm/arm64/acpi/arm-core.c b/xen/arch/arm/arm64/acpi/arm-core.c

I think "arm" is pointless in the filename. Maybe core.c would be a 
better name?

> new file mode 100644
> index 0000000..50a83d6
> --- /dev/null
> +++ b/xen/arch/arm/arm64/acpi/arm-core.c
> @@ -0,0 +1,97 @@

[..]

> +#if defined(CONFIG_ARM_64) && defined(CONFIG_ACPI)

Compiling the directory only when HAS_ACPI is enabled would avoid a 
pointless #ifdef here.

> +#include <xen/init.h>
> +#include <xen/acpi.h>
> +
> +#include <asm/acpi.h>

You should be consistent here, the acpi.h headers is in asm so generic 
to all ARM architecture but the ACPI code actually reside in an ARM64 
directory.

Even though ACPI is only ARM64, this code doesn't seem ARM64 specific. 
So I would move the directory in arch/arm/

> +
> +/*
> + * We never plan to use RSDT on arm/arm64 as its deprecated in spec but this
> + * variable is still required by the ACPI core
> + */
> +u32 acpi_rsdt_forced;

I didn't find any usage for this in Xen.

> +int acpi_noirq;                        /* skip ACPI IRQ initialization */

This is set but never used.

> +int acpi_strict;

I didn't find any usage for this in Xen.

> +int acpi_disabled;
> +EXPORT_SYMBOL(acpi_disabled);
> +
> +int acpi_pci_disabled;         /* skip ACPI PCI scan and IRQ initialization */
> +EXPORT_SYMBOL(acpi_pci_disabled);

This is set but never used.

> +enum acpi_irq_model_id acpi_irq_model = ACPI_IRQ_MODEL_PLATFORM;

Why do you define acpi_irq_model to ACPI_IRQ_MODEL_PLATFORM here and in 
patch #15, change to ACPI_IRQ_MODEL_PLATFORM?

Furthermore, you set it, but never use it.

> +
> +struct acpi_arm_root acpi_arm_rsdp_info;     /* info about RSDP from FDT */

I didn't find any usage in Xen.

> +
> +int acpi_gsi_to_irq(u32 gsi, unsigned int *irq)
> +{
> +    *irq = -1;
> +    return 0;
> +}
> +EXPORT_SYMBOL_GPL(acpi_gsi_to_irq);

This function is never used. I found a prototype defined in the headers 
(xen/acpi.h) few years ago but not implemented on x86.

I suspect that we forgot to drop the prototype at some point. Can you 
send a patch to remove it?

> +
> +/*
> + * success: return IRQ number (>0)
> + * failure: return =< 0
> + */
> +//int acpi_register_gsi(struct device *dev, u32 gsi, int trigger, int polarity)
> +unsigned int acpi_register_gsi (u32 gsi, int edge_level, int active_high_low)
> +{
> +    return -1;
> +}
> +EXPORT_SYMBOL_GPL(acpi_register_gsi);
> +
> +void acpi_unregister_gsi(u32 gsi)
> +{
> +}
> +EXPORT_SYMBOL_GPL(acpi_unregister_gsi);

Ditto for these 2 functions.

> +/*
> + * acpi_boot_table_init() called from setup_arch(), always.
> + *      1. find RSDP and get its address, and then find XSDT
> + *      2. extract all tables and checksums them all
> + *
> + * We can parse ACPI boot-time tables such as FADT, MADT after
> + * this function is called.
> + */
> +int __init acpi_boot_table_init(void)
> +{
> +    int error;
> +    /* If acpi_disabled, bail out */
> +    if (acpi_disabled)
> +        return 1;
> +
> +    /* Initialize the ACPI boot-time table parser. */
> +    error = acpi_table_init();
> +    if (error)
> +    {
> +        disable_acpi();
> +        return error;
> +    }
> +
> +    return 0;
> +}
> +#endif
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 3991d64..7ae126b 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -45,6 +45,8 @@
>   #include <asm/procinfo.h>
>   #include <asm/setup.h>
>   #include <xsm/xsm.h>
> +#include <xen/acpi.h>
> +#include <asm/acpi.h>
>
>   struct bootinfo __initdata bootinfo;
>
> @@ -737,7 +739,18 @@ void __init start_xen(unsigned long boot_phys_offset,
>
>       setup_mm(fdt_paddr, fdt_size);
>
> +    system_state = SYS_STATE_boot;
> +

Hmmm why? Is it because of the memory allocator in ACPI? If so, you 
should justify a such change and drop the other system_state = 
SYS_STATE_boot within this function.

>       vm_init();
> +
> +/*
> + * Parse the ACPI tables for possible boot-time configuration
> + */
> +
> +#if defined(CONFIG_ACPI) && defined(CONFIG_ARM_64)

No need to check CONFIG_ARM_64 here, CONFIG_ACPI is only defined for ARM64.

Although, a stub exist when CONFIG_ACPI_BOOT (implicitly enabled with 
CONFIG_ACPI) is not enabled. So maybe we should just drop the #ifdef.

> +    acpi_boot_table_init();

Should not we return try to use the device tree when ACPI return an 
error. What about the Linux boot protocol? I.e prefer DT when it's present?

> +#endif
> +
>       dt_unflatten_host_device_tree();

>       dt_irq_xlate = gic_irq_xlate;

This 2 lines should not be necessary when ACPI is used. Please make sure 
that we never used the device tree in this case. This would help us to 
catch possible error later.

>
> @@ -802,7 +815,6 @@ void __init start_xen(unsigned long boot_phys_offset,
>                   printk("Failed to bring up CPU %u (error %d)\n", i, ret);
>           }
>       }
> -

Spurious change

>       printk("Brought up %ld CPUs\n", (long)num_online_cpus());
>       /* TODO: smp_cpus_done(); */
>
> diff --git a/xen/include/xen/acpi.h b/xen/include/xen/acpi.h
> index 3aeba4a..ff96336 100644
> --- a/xen/include/xen/acpi.h
> +++ b/xen/include/xen/acpi.h
> @@ -42,6 +42,17 @@
>
>   #ifdef CONFIG_ACPI_BOOT
>
> +enum acpi_irq_model_id {
> +        ACPI_IRQ_MODEL_PIC = 0,
> +        ACPI_IRQ_MODEL_IOAPIC,
> +        ACPI_IRQ_MODEL_IOSAPIC,
> +        ACPI_IRQ_MODEL_PLATFORM,
> +        ACPI_IRQ_MODEL_GIC,
> +        ACPI_IRQ_MODEL_COUNT
> +};
> +
> +extern enum acpi_irq_model_id   acpi_irq_model;
> +

If we still need it (see my comment on the declaration above). Then this 
should be moved in asm/acpi.h.

Regards,

-- 
Julien Grall