Re: [Xen-devel] [PATCH v6 10/15] x86/boot: implement early command line parser in C

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.09.16 at 22:18, <daniel.kiper@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/boot/Makefile
> +++ b/xen/arch/x86/boot/Makefile
> @@ -1,9 +1,16 @@
>  obj-bin-y += head.o
>  
> -RELOC_DEPS = $(BASEDIR)/include/asm-x86/config.h 
> $(BASEDIR)/include/xen/multiboot.h \
> +DEFS_H_DEPS = $(BASEDIR)/include/xen/stdbool.h
> +
> +CMDLINE_DEPS = defs.h $(DEFS_H_DEPS) video.h
> +
> +RELOC_DEPS = defs.h $(DEFS_H_DEPS) $(BASEDIR)/include/xen/multiboot.h \

I think it would be more natural for DEFS_H_DEPS to include defs.h,
as I can't see a valid use of one without the other; perhaps it would
then better be renamed.

> --- /dev/null
> +++ b/xen/arch/x86/boot/defs.h
> @@ -0,0 +1,52 @@
> +/*
> + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program.  If not, see <http://www.gnu.org/licenses/>.
> + *
> + * max() was copied from xen/xen/include/xen/kernel.h.
> + */
> +
> +#ifndef __BOOT_DEFS_H__
> +#define __BOOT_DEFS_H__
> +
> +#include "../../../include/xen/stdbool.h"
> +
> +#define __packed     __attribute__((__packed__))
> +#define __stdcall    __attribute__((__stdcall__))
> +
> +#define NULL         ((void *)0)
> +
> +#define ALIGN_UP(arg, align) \
> +                (((arg) + (align) - 1) & ~((typeof(arg))(align) - 1))
> +
> +#define max(x,y) ({ \
> +        const typeof(x) _x = (x);       \
> +        const typeof(y) _y = (y);       \
> +        (void) (&_x == &_y);            \
> +        _x > _y ? _x : _y; })

Please don't add max() without also adding min().

> --- a/xen/arch/x86/boot/trampoline.S
> +++ b/xen/arch/x86/boot/trampoline.S
> @@ -220,8 +220,22 @@ trampoline_boot_cpu_entry:
>          /* Jump to the common bootstrap entry point. */
>          jmp     trampoline_protmode_entry
>  
> +#include "video.h"
> +
> +        .align  2
> +        .byte   0

While this odd placement of the requested padding byte will fulfill
the immediate purpose, please do it the originally requested more
conventional way (putting it inside the structure and adding a
respective field to the C representation). For someone wanting to
add another field it'll then be far more obvious what to do without
re-introducing mis-alignment.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel