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

[Parth Dixit <parth.dixit@linaro.org>]

+shannon

On 2 June 2015 at 22:57, Julien Grall <julien.grall@citrix.com> wrote:
> 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