[Xen-devel,v2,04/17] libxl/arm: prepare for constructing ACPI tables

From: Shannon Zhao <shannon.zhao@linaro.org>

It only constructs the ACPI tables for 64-bit ARM DomU when user enables
acpi because 32-bit DomU doesn't support ACPI.

Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 tools/libxl/Makefile         |  4 +++
 tools/libxl/libxl_arm.c      | 19 +++++++++-
 tools/libxl/libxl_arm_acpi.c | 85 ++++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_arm_acpi.h | 32 +++++++++++++++++
 4 files changed, 139 insertions(+), 1 deletion(-)
 create mode 100644 tools/libxl/libxl_arm_acpi.c
 create mode 100644 tools/libxl/libxl_arm_acpi.h

On 2016年06月23日 21:37, Stefano Stabellini wrote:
> On Thu, 23 Jun 2016, Shannon Zhao wrote:
>> +
>> +    if (strcmp(dom->guest_type, "xen-3.0-aarch64")) {
>> +        LOG(DEBUG, "Do not generate ACPI tables for %s", dom->guest_type);
>> +        state->config.acpi = false;
> 
> Shouldn't we return here?
> 
Ah, right, thanks!
> 
>> +    }
>> +
>> +    return libxl__prepare_acpi(gc, info, state, dom);
>>  }
>>  
>>  static void finalise_one_memory_node(libxl__gc *gc, void *fdt,
>> diff --git a/tools/libxl/libxl_arm_acpi.c b/tools/libxl/libxl_arm_acpi.c
>> new file mode 100644
>> index 0000000..60c31f9
>> --- /dev/null
>> +++ b/tools/libxl/libxl_arm_acpi.c
>> @@ -0,0 +1,85 @@
>> +/*
>> + * ARM DmoU ACPI generation
>> + *
>> + * Copyright (C) 2008-2010  Kevin O'Connor <kevin@koconnor.net>
>> + * Copyright (C) 2006 Fabrice Bellard
>> + * Copyright (C) 2013 Red Hat Inc
>> + *
>> + * Author: Michael S. Tsirkin <mst@redhat.com>
>> + *
>> + * Copyright (c) 2015 HUAWEI TECHNOLOGIES CO.,LTD.
>> + *
>> + * Author: Shannon Zhao <zhaoshenglong@huawei.com>
>> + *
>> + * Copyright (C) 2016      Linaro Ltd.
>> + *
>> + * Author: Shannon Zhao <shannon.zhao@linaro.org>
> 
> Uhm... If in doubt just remove all the Author lines: git log provides
> more than enough information about who wrote this code.
> 
Ahm... Julien suggested I should import the copyright from QEMU since I
refer to that. While what I didn't say before is that even if I didn't
refer to QEMU codes, the implementation will be like this because for
the tables except DSDT we just fill in the contents of the tables. But
for DSDT generation, it's totally different compared with QEMU.

I think I'll remove this. :)

Thanks,

Hi Shannon,

On 23/06/16 04:16, Shannon Zhao wrote:

[...]

> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> index cc5a717..f5db74b 100644
> --- a/tools/libxl/libxl_arm.c
> +++ b/tools/libxl/libxl_arm.c
> @@ -1,6 +1,7 @@
>   #include "libxl_internal.h"
>   #include "libxl_arch.h"
>   #include "libxl_libfdt_compat.h"
> +#include "libxl_arm_acpi.h"
>
>   #include <xc_dom.h>
>   #include <stdbool.h>
> @@ -888,8 +889,24 @@ int libxl__arch_domain_init_hw_description(libxl__gc *gc,
>                                              libxl__domain_build_state *state,
>                                              struct xc_dom_image *dom)
>   {
> +    int rc;
> +
>       assert(info->type == LIBXL_DOMAIN_TYPE_PV);
> -    return libxl__prepare_dtb(gc, info, state, dom);
> +    rc = libxl__prepare_dtb(gc, info, state, dom);
> +    if (rc)
> +        return rc;
> +
> +    if (!state->config.acpi) {
> +        LOG(DEBUG, "Generating ACPI tables is disabled by user.");
> +        return 0;
> +    }
> +
> +    if (strcmp(dom->guest_type, "xen-3.0-aarch64")) {
> +        LOG(DEBUG, "Do not generate ACPI tables for %s", dom->guest_type);
> +        state->config.acpi = false;

Silently ignores the user configuration is usually not a good thing. So 
should not we return an error here?

> +    }
> +
> +    return libxl__prepare_acpi(gc, info, state, dom);
>   }
>
>   static void finalise_one_memory_node(libxl__gc *gc, void *fdt,
> diff --git a/tools/libxl/libxl_arm_acpi.c b/tools/libxl/libxl_arm_acpi.c
> new file mode 100644
> index 0000000..60c31f9
> --- /dev/null
> +++ b/tools/libxl/libxl_arm_acpi.c
> @@ -0,0 +1,85 @@
> +/*
> + * ARM DmoU ACPI generation

s/DmoU/DomU/

[...]

> +#include "libxl_arm_acpi.h"
> +
> +#include <stdint.h>

Please add a newline here for clarity.

> +typedef uint8_t u8;
> +typedef uint16_t u16;
> +typedef uint32_t u32;
> +typedef uint64_t u64;

Ditto.

> +#include <acpi/acconfig.h>
> +#include <acpi/actbl.h>
> +
> +enum {
> +    RSDP,
> +    XSDT,
> +    GTDT,
> +    MADT,
> +    FADT,
> +    DSDT,
> +    NUMS,
> +};
> +
> +struct acpitable {
> +    void *table;
> +    size_t size;
> +};
> +
> +static struct acpitable acpitables[NUMS];

libxl is a library which can be used to implement daemon. So anything 
domain specific should not be static nor global.

> +
> +int libxl__prepare_acpi(libxl__gc *gc, libxl_domain_build_info *info,
> +                        libxl__domain_build_state *state,
> +                        struct xc_dom_image *dom)
> +{
> +    const libxl_version_info *vers;
> +    int rc;
> +
> +    /* convenience aliases */
> +    xc_domain_configuration_t *xc_config = &state->config;
> +
> +    vers = libxl_get_version_info(CTX);
> +    if (vers == NULL)
> +        return ERROR_FAIL;
> +
> +    LOG(DEBUG, "constructing ACPI tables for Xen version %d.%d guest",
> +        vers->xen_version_major, vers->xen_version_minor);
> +
> +    dom->acpitable_blob = NULL;
> +    dom->acpitable_size = 0;
> +
> +    return 0;
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/tools/libxl/libxl_arm_acpi.h b/tools/libxl/libxl_arm_acpi.h
> new file mode 100644
> index 0000000..5899210
> --- /dev/null
> +++ b/tools/libxl/libxl_arm_acpi.h
> @@ -0,0 +1,32 @@

[...]

> +#include "libxl_internal.h"
> +#include "libxl_arch.h"
> +
> +#include <xc_dom.h>
> +
> +int libxl__prepare_acpi(libxl__gc *gc, libxl_domain_build_info *info,

The function should be prefixed with _hidden.

> +                        libxl__domain_build_state *state,
> +                        struct xc_dom_image *dom);
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
>

Regards,