From mboxrd@z Thu Jan 1 00:00:00 1970 From: Parth Dixit Subject: Re: [PATCH v2 22/41] arm : acpi create min DT stub for DOM0 Date: Sun, 5 Jul 2015 18:45:32 +0530 Message-ID: References: <1431893048-5214-1-git-send-email-parth.dixit@linaro.org> <1431893048-5214-23-git-send-email-parth.dixit@linaro.org> <556DE774.8060805@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <556DE774.8060805@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Julien Grall Cc: keir@xen.org, Ian Campbell , andrew.cooper3@citrix.com, tim@xen.org, xen-devel , Stefano Stabellini , shannon.zhao@linaro.org, Jan Beulich , Christoffer Dall List-Id: xen-devel@lists.xenproject.org +shannon On 2 June 2015 at 22:57, Julien Grall 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 >> Signed-off-by: Parth Dixit >> --- >> 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 >> #include >> #include >> +#include >> #include >> #include >> #include >> @@ -18,6 +19,7 @@ >> #include >> #include >> #include >> +#include > > Not necessary, xen/acpi.h already includes asm/acpi.h > >> >> #include >> #include >> @@ -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