Re: [PATCH v2 22/41] arm : acpi create min DT stub for DOM0

[Julien Grall <julien.grall@citrix.com>]

Hi Parth,

On 17/05/15 21:03, Parth Dixit wrote:
> Create a DT for DOM0 for ACPI-case only.
> DT contains minmal required informations such as

s/minmal/minimal/

> DOM0 bootargs, initrd, efi description table
> and address of uefi memory table.
> Add placeholder for tables to be marked as
> reserved in efi table. This is requird for

s/requird/required/

> DT function's signature.

Well, you can modify later the prototype. It's usually better to define
where it's used because we can understand why you need this how you will
use it.

Also, a link the description of the minimal DT in the Linux doc would
have been nice.

> Signed-off-by: Naresh Bhat <naresh.bhat@linaro.org>
> Signed-off-by: Parth Dixit <parth.dixit@linaro.org>
> ---
>  xen/arch/arm/domain_build.c | 75 ++++++++++++++++++++++++++++++++++++++++++++-
>  xen/include/asm-arm/acpi.h  | 10 ++++++
>  2 files changed, 84 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 1e545fe..c830702 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -9,6 +9,7 @@
>  #include <asm/regs.h>
>  #include <xen/errno.h>
>  #include <xen/device_tree.h>
> +#include <xen/acpi.h>
>  #include <xen/libfdt/libfdt.h>
>  #include <xen/guest_access.h>
>  #include <xen/iocap.h>
> @@ -18,6 +19,7 @@
>  #include <asm/psci.h>
>  #include <asm/setup.h>
>  #include <asm/cpufeature.h>
> +#include <asm/acpi.h>

Not necessary, xen/acpi.h already includes asm/acpi.h

>  
>  #include <asm/gic.h>
>  #include <xen/irq.h>
> @@ -61,6 +63,11 @@ custom_param("dom0_mem", parse_dom0_mem);
>   */
>  #define DOM0_FDT_EXTRA_SIZE (128 + sizeof(struct fdt_reserve_entry))
>  
> +#ifdef CONFIG_ACPI
> +/* Reserve DOM0 FDT size in ACPI case only */
> +#define DOM0_FDT_MIN_SIZE 4096

This can be moved within the big #ifdef CONFIG_ACPI below.
Also please rename the define to show that it's ACPI specific.

> +#endif
> +
>  struct vcpu *__init alloc_dom0_vcpu0(struct domain *dom0)
>  {
>      if ( opt_dom0_max_vcpus == 0 )
> @@ -1211,7 +1218,68 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
>  
>      return res;
>  }

Newline.

> +#ifdef CONFIG_ACPI
> +/*
> + * Prepare a minimal DTB for DOM0 which contains
> + * bootargs, initrd, memory information,
> + * EFI table.
> + */
> +static int create_acpi_dtb(struct domain *d, struct kernel_info *kinfo, struct membank tbl_add[])
> +{
> +    int new_size;
> +    int ret;
> +
> +    DPRINT("Prepare a min DTB for DOM0\n");
> +
> +    /* Allocate min size for DT */
> +    new_size = DOM0_FDT_MIN_SIZE;
> +    kinfo->fdt = xmalloc_bytes(DOM0_FDT_MIN_SIZE);

Why not using new_size here? It would be less error-prone.

> +
> +    if ( kinfo->fdt == NULL )
> +        return -ENOMEM;
> +
> +    /* Create a new empty DT for DOM0 */
> +    ret = fdt_create(kinfo->fdt, new_size);
> +    if ( ret < 0 )
> +        goto err;
> +
> +    ret = fdt_finish_reservemap(kinfo->fdt);
> +    if ( ret < 0 )
> +        goto err;
> +
> +    ret = fdt_begin_node(kinfo->fdt, "/");
> +    if ( ret < 0 )
> +        goto err;
> +
> +    ret = fdt_property_cell(kinfo->fdt, "#address-cells", 2);
> +    if ( ret )
> +        return ret;
>  
> +    ret = fdt_property_cell(kinfo->fdt, "#size-cells", 1);
> +    if ( ret )
> +        return ret;
> +
> +    ret = fdt_end_node(kinfo->fdt);
> +    if ( ret < 0 )
> +        goto err;
> +
> +    ret = fdt_finish(kinfo->fdt);
> +    if ( ret < 0 )
> +        goto err;
> +
> +    return 0;
> +
> +  err:
> +    printk("Device tree generation failed (%d).\n", ret);
> +    xfree(kinfo->fdt);
> +    return -EINVAL;
> +}
> +#else
> +static int create_acpi_dtb(struct domain *d, struct kernel_info *kinfo, struct membank tbl_add[])
> +{

Please add a comment and a BUG_ON to make clear this should never be called.

> +    return -EINVAL;
> +}
> +#endif
>  static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
>  {
>      const void *fdt;
> @@ -1370,6 +1438,7 @@ int construct_dom0(struct domain *d)
>      struct kernel_info kinfo = {};
>      struct vcpu *saved_current;
>      int rc, i, cpu;
> +    struct membank tbl_add[TBL_MMAX] = {};
>  
>      struct vcpu *v = d->vcpu[0];
>      struct cpu_user_regs *regs = &v->arch.cpu_info->guest_cpu_user_regs;
> @@ -1403,7 +1472,11 @@ int construct_dom0(struct domain *d)
>  
>      allocate_memory(d, &kinfo);
>  
> -    rc = prepare_dtb(d, &kinfo);
> +    if (acpi_disabled)

if ( ... )

> +        rc = prepare_dtb(d, &kinfo);
> +    else
> +        rc = create_acpi_dtb(d, &kinfo, tbl_add);
> +
>      if ( rc < 0 )
>          return rc;
>  
> diff --git a/xen/include/asm-arm/acpi.h b/xen/include/asm-arm/acpi.h
> index 482cc5b..2df9ae0 100644
> --- a/xen/include/asm-arm/acpi.h
> +++ b/xen/include/asm-arm/acpi.h
> @@ -50,6 +50,16 @@ static inline void disable_acpi(void)
>      acpi_disabled = 1;
>  }
>  
> +/* Tables marked as reserved in efi table */
> +enum EFI_MEM_RES{
> +    TBL_STAO,
> +    TBL_XENV,
> +    TBL_XSDT,
> +    TBL_EFIT,
> +    TBL_MMAP,
> +    TBL_MMAX,
> +};
> +

This doesn't belong to this patch ...

>  #define ACPI_GTDT_INTR_MASK ( ACPI_GTDT_INTERRUPT_MODE | ACPI_GTDT_INTERRUPT_POLARITY )
>  
>  /**
> 

Regards,

-- 
Julien Grall