Re: [PATCH v4 1/3] xen/arm: gic: acpi: Guard helpers to build the MADT with CONFIG_ACPI

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien,

> On 19 Nov 2020, at 17:08, Julien Grall <julien@xxxxxxx> wrote:
> 
> From: Julien Grall <jgrall@xxxxxxxxxx>
> 
> gic_make_hwdom_madt() and gic_get_hwdom_madt_size() are ACPI specific.
> 
> While they build fine today, this will change in a follow-up patch.
> Rather than trying to fix the build on ACPI, it is best to avoid
> compiling the helpers and the associated callbacks when CONFIG_ACPI=n.
> 
> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
Reviewed-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>

I also tested the serie on FVP without ACPI and Xen is still booting properly 
Dom0.

Cheers
Bertrand

> 
> ---
>    Changes in v4:
>        - Patch added
> ---
> xen/arch/arm/gic-v2.c     |  8 +++-----
> xen/arch/arm/gic-v3.c     | 11 ++---------
> xen/arch/arm/gic.c        |  2 ++
> xen/include/asm-arm/gic.h | 10 ++++++++--
> 4 files changed, 15 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 0f747538dbcd..581ea5ba6b2c 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -1114,12 +1114,12 @@ static int gicv2_iomem_deny_access(const struct 
> domain *d)
>     return iomem_deny_access(d, mfn, mfn + nr);
> }
> 
> +#ifdef CONFIG_ACPI
> static unsigned long gicv2_get_hwdom_extra_madt_size(const struct domain *d)
> {
>     return 0;
> }
> 
> -#ifdef CONFIG_ACPI
> static int gicv2_make_hwdom_madt(const struct domain *d, u32 offset)
> {
>     struct acpi_subtable_header *header;
> @@ -1248,10 +1248,6 @@ static void __init gicv2_acpi_init(void)
> }
> #else
> static void __init gicv2_acpi_init(void) { }
> -static int gicv2_make_hwdom_madt(const struct domain *d, u32 offset)
> -{
> -    return 0;
> -}
> #endif
> 
> static int __init gicv2_init(void)
> @@ -1357,8 +1353,10 @@ const static struct gic_hw_operations gicv2_ops = {
>     .read_apr            = gicv2_read_apr,
>     .read_pending_state  = gicv2_read_pending_state,
>     .make_hwdom_dt_node  = gicv2_make_hwdom_dt_node,
> +#ifdef CONFIG_ACPI
>     .make_hwdom_madt     = gicv2_make_hwdom_madt,
>     .get_hwdom_extra_madt_size = gicv2_get_hwdom_extra_madt_size,
> +#endif
>     .map_hwdom_extra_mappings = gicv2_map_hwdown_extra_mappings,
>     .iomem_deny_access   = gicv2_iomem_deny_access,
>     .do_LPI              = gicv2_do_LPI,
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 0f6cbf6224e9..2a344393a0e4 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1735,15 +1735,6 @@ static void __init gicv3_acpi_init(void)
> }
> #else
> static void __init gicv3_acpi_init(void) { }
> -static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset)
> -{
> -    return 0;
> -}
> -
> -static unsigned long gicv3_get_hwdom_extra_madt_size(const struct domain *d)
> -{
> -    return 0;
> -}
> #endif
> 
> static bool gic_dist_supports_lpis(void)
> @@ -1858,8 +1849,10 @@ static const struct gic_hw_operations gicv3_ops = {
>     .read_pending_state  = gicv3_read_pending_state,
>     .secondary_init      = gicv3_secondary_cpu_init,
>     .make_hwdom_dt_node  = gicv3_make_hwdom_dt_node,
> +#ifdef CONFIG_ACPI
>     .make_hwdom_madt     = gicv3_make_hwdom_madt,
>     .get_hwdom_extra_madt_size = gicv3_get_hwdom_extra_madt_size,
> +#endif
>     .iomem_deny_access   = gicv3_iomem_deny_access,
>     .do_LPI              = gicv3_do_LPI,
> };
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index d623c57cb9fa..fe60619e99cf 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -443,6 +443,7 @@ int gic_make_hwdom_dt_node(const struct domain *d,
>     return gic_hw_ops->make_hwdom_dt_node(d, gic, fdt);
> }
> 
> +#ifdef CONFIG_ACPI
> int gic_make_hwdom_madt(const struct domain *d, u32 offset)
> {
>     return gic_hw_ops->make_hwdom_madt(d, offset);
> @@ -459,6 +460,7 @@ unsigned long gic_get_hwdom_madt_size(const struct domain 
> *d)
> 
>     return madt_size;
> }
> +#endif
> 
> int gic_iomem_deny_access(const struct domain *d)
> {
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index ba870523bb2a..ad0f7452d005 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -378,12 +378,14 @@ struct gic_hw_operations {
>     /* Create GIC node for the hardware domain */
>     int (*make_hwdom_dt_node)(const struct domain *d,
>                               const struct dt_device_node *gic, void *fdt);
> +#ifdef CONFIG_ACPI
>     /* Create MADT table for the hardware domain */
>     int (*make_hwdom_madt)(const struct domain *d, u32 offset);
> -    /* Map extra GIC MMIO, irqs and other hw stuffs to the hardware domain. 
> */
> -    int (*map_hwdom_extra_mappings)(struct domain *d);
>     /* Query the size of hardware domain madt table */
>     unsigned long (*get_hwdom_extra_madt_size)(const struct domain *d);
> +#endif
> +    /* Map extra GIC MMIO, irqs and other hw stuffs to the hardware domain. 
> */
> +    int (*map_hwdom_extra_mappings)(struct domain *d);
>     /* Deny access to GIC regions */
>     int (*iomem_deny_access)(const struct domain *d);
>     /* Handle LPIs, which require special handling */
> @@ -435,8 +437,12 @@ void register_gic_ops(const struct gic_hw_operations 
> *ops);
> int gic_make_hwdom_dt_node(const struct domain *d,
>                            const struct dt_device_node *gic,
>                            void *fdt);
> +
> +#ifdef CONFIG_ACPI
> int gic_make_hwdom_madt(const struct domain *d, u32 offset);
> unsigned long gic_get_hwdom_madt_size(const struct domain *d);
> +#endif
> +
> int gic_map_hwdom_extra_mappings(struct domain *d);
> int gic_iomem_deny_access(const struct domain *d);
> 
> -- 
> 2.17.1
>