Re: [Xen-devel] [PATCH RFC 13/20] acpi/hvmloader: Add stdio.h, string.h and x86.h

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.04.16 at 03:25, <boris.ostrovsky@xxxxxxxxxx> wrote:
> Users of ACPI builder will need to provide their own implemetations of
> strncpy(), memcpy, memset() and printf declared in stdio.h and string.h.
> For hvmloader we provide those two as wrappers around utul.h.
> 
> Some x86-specific definitions move to ACPI builder's x86.h file so that
> users won't have to re-define them themselves.
> 
> Explicitly define a few things that are currently available to to build.c
> via util.h.

Considering the first paragraph, this last sentence was more
confusing than clarifying to me: I expected util.h to get changed
in some way, but aiui having gone through the entire patch this is
just a (very different) re-statement of what you've said already.

>  create mode 100644 tools/firmware/hvmloader/acpi/x86.h
>  create mode 100644 tools/firmware/hvmloader/stdio.h
>  create mode 100644 tools/firmware/hvmloader/string.h

I have to admit that I'm not really looking forward to see the
hypervisor gain a file stdio.h. I'm also not convinced libxc'e
planned use of the libacpi/ would fit well with uses of printf() in
that subtree.

> --- a/tools/firmware/hvmloader/Makefile
> +++ b/tools/firmware/hvmloader/Makefile
> @@ -36,7 +36,7 @@ ACPI_PATH = $(XEN_ROOT)/tools/firmware/hvmloader/acpi
>  vpath %.c $(ACPI_PATH)
>  ACPI_FILES = dsdt_anycpu.c dsdt_15cpu.c static_tables.c 
> dsdt_anycpu_qemu_xen.c build.c
>  ACPI_SRC = $(patsubst %.c,$(ACPI_PATH)/%.c,$(ACPI_FILES))
> -CFLAGS += -I$(ACPI_PATH)
> +CFLAGS += -I$(ACPI_PATH) -I.

With the subtree symlink approach, such an addition could then be
limited to just the libacpi/ files, i.e. not affecting core hvmloader ones.

> @@ -30,6 +32,12 @@
>  
>  #define align16(sz)        (((sz) + 15) & ~15)
>  #define fixed_strcpy(d, s) strncpy((d), (s), sizeof(d))
> +#ifndef offsetof
> +#define offsetof(t, m) ((unsigned long)&((t *)0)->m)
> +#endif
> +#ifndef NULL
> +#define NULL ((void *)0)
> +#endif

These should be provided by the consumer too, I would say
(stddef.h).

> @@ -41,6 +49,11 @@ extern struct acpi_20_waet Waet;
>  /* Number of processor objects in the chosen DSDT. */
>  static unsigned int nr_processor_objects;
>  
> +static inline int __test_bit(unsigned int b, void *p)
> +{
> +    return !!(((uint8_t *)p)[b>>3] & (1u<<(b&7)));
> +}

This one too, without the leading underscores (bitops.h or some
other sbtraction). All three potential consumers of this code already
have test_bit().

> @@ -238,8 +251,6 @@ static struct acpi_20_srat *construct_srat(struct 
> acpi_config *config)
>          memory++;
>      }
>  
> -    ASSERT(((unsigned long)memory) - ((unsigned long)p) == size);

I think this was put here for a reason, and hence shouldn't be
removed without good reason (I wouldn't count ASSERT() not being
available as one).

> --- /dev/null
> +++ b/tools/firmware/hvmloader/acpi/x86.h
> @@ -0,0 +1,14 @@
> +#ifndef __ACPI_X86_H__
> +#define __ACPI_X86_H__
> +
> +#define IOAPIC_BASE_ADDRESS 0xfec00000
> +#define IOAPIC_ID           0x01
> +#define IOAPIC_VERSION      0x11
> +
> +#define LAPIC_BASE_ADDRESS  0xfee00000
> +#define LAPIC_ID(vcpu_id)   ((vcpu_id) * 2)
> +
> +#define PCI_ISA_DEVFN       0x08    /* dev 1, fn 0 */
> +#define PCI_ISA_IRQ_MASK    0x0c20U /* ISA IRQs 5,10,11 are PCI connected */

Only the latter of those last two is used by build.c. Please limit what
you move to the very minimum. I.e. IOAPIC_VERSION shouldn't move
either imo (and I notice there's a disconnect between it and the
hypervisor's VIOAPIC_VERSION_ID, but that's a pre-existing issue).

> --- a/tools/firmware/hvmloader/hvmloader.c
> +++ b/tools/firmware/hvmloader/hvmloader.c
> @@ -24,6 +24,7 @@
>  #include "config.h"
>  #include "pci_regs.h"
>  #include "apic_regs.h"
> +#include "x86.h"

Looking at an inclusion point like this, it is completely unclear that this
comes from libacpi/. One more argument in favor of keeping the
subtree.

Jan


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