Re: [PATCH RFC 20/20] acpi: Make ACPI builder available to hypervisor code

["Jan Beulich" <JBeulich@suse.com>]

>>> On 06.04.16 at 03:25, <boris.ostrovsky@oracle.com> wrote:
> --- a/xen/common/libacpi/Makefile
> +++ b/xen/common/libacpi/Makefile
> @@ -21,7 +21,16 @@ C_SRC  = build.c dsdt_anycpu.c dsdt_15cpu.c static_tables.c
>  C_SRC += dsdt_anycpu_qemu_xen.c dsdt_empty.c
>  OBJS  = $(patsubst %.c,%.o,$(C_SRC))
>  
> -CFLAGS_xeninclude = -I$(XEN_ROOT)/tools/include
> +ifneq (,$(findstring -D__XEN__,$(CFLAGS)))
> +# Hypervisor
> +obj-y	= $(OBJS)
> +$(OBJS): all
> +CFLAGS  += -I$(XEN_ROOT)/xen/include/public
> +CFLAGS_MKDSDT = -I$(XEN_ROOT)/xen/include/public/hvm -D__XEN__
> +else
> +# Toolstack
> +CFLAGS_MKDSDT = -I$(XEN_ROOT)/tools/include
> +endif

This again speaks against makefile sharing. Note that libelf doesn't
have anything like that.

> --- a/xen/common/libacpi/acpi2_0.h
> +++ b/xen/common/libacpi/acpi2_0.h
> @@ -17,10 +17,16 @@
>  #ifndef _ACPI_2_0_H_
>  #define _ACPI_2_0_H_
>  
> +#ifndef __XEN__
>  #include <stdint.h>
>  #include <xen/xen.h>
>  #include <xen/hvm/ioreq.h>
>  #include <xen/memory.h>
> +#else
> +#include <xen/lib.h>
> +#include <memory.h>
> +#include <hvm/ioreq.h>
> +#endif

I'm not sure this should be done here, albeit I do see the analogy to
how libelf deals with the dual use in this regard (perhaps doing this
in .c files of header other than this particular one would be fine). In
any event I am missing public/ prefixes above (preferably you would
drop the explicit .../public include path, but even if that's needed for
some reason, please don't obscure the inclusion of public headers
here by omitting that prefix).

> --- a/xen/common/libacpi/build.c
> +++ b/xen/common/libacpi/build.c
> @@ -15,8 +15,13 @@
>   * this program; If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> +#ifndef __XEN__
>  #include <stdio.h>
>  #include <string.h>
> +#else
> +#include <xen/lib.h>
> +#define printf printk
> +#endif

I think we should strive to not introduce bunches of new log-level-less
printk()-s. (But I do appreciate you not introducing stdio.h here.)

> @@ -24,14 +29,24 @@
>  #include "ssdt_tpm.h"
>  #include "ssdt_pm.h"
>  #include "x86.h"
> +#ifndef __XEN__
>  #include <xen/hvm/hvm_info_table.h>
>  #include <xen/hvm/hvm_xs_strings.h>
>  #include <xen/hvm/params.h>
> +#else
> +#include <hvm/hvm_info_table.h>
> +#include <hvm/hvm_xs_strings.h>
> +#include <hvm/params.h>
> +#endif
>  
>  #define ACPI_MAX_SECONDARY_TABLES 16
>  
>  #define align16(sz)        (((sz) + 15) & ~15)
> +#ifndef __XEN__
>  #define fixed_strcpy(d, s) strncpy((d), (s), sizeof(d))
> +#else
> +#define fixed_strcpy(d, s) strlcpy((d), (s), sizeof(d))
> +#endif

I'd rather see you ditch the stray parentheses in the pre-existing
instance than introduce further ones.

Jan


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