From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefano Stabellini Subject: Re: [PATCH 08/10] xen: arm: support bootmodule type detection by ordering Date: Wed, 18 Jun 2014 16:19:32 +0100 Message-ID: References: <1402919079.14907.22.camel@kazak.uk.xensource.com> <1402919103-29642-8-git-send-email-ian.campbell@citrix.com> <1403102716.6568.65.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1403102716.6568.65.camel@kazak.uk.xensource.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: Ian Campbell Cc: Stefano Stabellini , Naresh Bhat , julien.grall@linaro.org, tim@xen.org, xen-devel@lists.xen.org, Roy Franz , Fu Wei List-Id: xen-devel@lists.xenproject.org On Wed, 18 Jun 2014, Ian Campbell wrote: > On Wed, 2014-06-18 at 15:40 +0100, Stefano Stabellini wrote: > > On Mon, 16 Jun 2014, Ian Campbell wrote: > > > Assign modules types based on the order in which they are defined in the FDT. > > > This is supported only for the dom0 kernel and ramdisk when given as the first > > > and second modules respectively, similar to how > > > http://wiki.xen.org/wiki?title=Xen_ARM_with_Virtualization_Extensions/Multiboot&oldid=11824 > > > defined the default types from the bootloader side. > > > > > > This is compatible with how Xen interprets the modules with x86 multiboot and I > > > think simplifies things for bootloaders which now need not contain similar > > > guessing code if they only care about the most basic case. > > > > > > Signed-off-by: Ian Campbell > > > --- > > > xen/arch/arm/bootfdt.c | 10 +++++++--- > > > xen/arch/arm/setup.c | 14 ++++++++++++++ > > > xen/include/asm-arm/setup.h | 11 ++++++++++- > > > 3 files changed, 31 insertions(+), 4 deletions(-) > > > > > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c > > > index e983aa7..a80055c 100644 > > > --- a/xen/arch/arm/bootfdt.c > > > +++ b/xen/arch/arm/bootfdt.c > > > @@ -163,6 +163,7 @@ static void __init process_multiboot_node(const void *fdt, int node, > > > const char *name, > > > u32 address_cells, u32 size_cells) > > > { > > > + static bootmodulekind kind_guess = BOOTMOD_LAST_PRESERVE + 1; > > > > kind_guess = BOOTMOD_KERNEL would be clearer > > Yeah. > > > > const struct fdt_property *prop; > > > const __be32 *cell; > > > bootmodulekind kind; > > > @@ -178,8 +179,10 @@ static void __init process_multiboot_node(const void *fdt, int node, > > > kind = BOOTMOD_RAMDISK; > > > else if ( fdt_node_check_compatible(fdt, node, "xen,xsm-policy") == 0 ) > > > kind = BOOTMOD_XSM; > > > + else if ( kind_guess < BOOTMOD_UNKNOWN ) > > > + kind = kind_guess++; > > > > This would allow for BOOTMOD_XSM to be specified this way. Do we want > > that? > > No, explicitly not. And we don't I think because BOOTMOD_XSM >= > BOOTMOD_UNKNOWN (for exactly this reason, see comment in the enum defn) I didn't spot that you moved BOOTMOD_UNKNOWN before BOOTMOD_XSM in this patch. I would rather avoid it for the principle of least astonishment. I would expect the "unknown" type to be the last one in the enum. > > If so, we should write to the commit message and to the wiki. > > Otherwise I would prefer: > > Really? The point is trying to avoid relying on the integer ordering in the enum, otherwise we loose the benefits of using an enum instead of the old macros. > > else if ( kind_guess == BOOTMOD_KERNEL || kind_guess == BOOTMOD_RAMDISK ) > > { > > kind = kind_guess; > > kind_guess++; > > } > > Ian >