Re: [PATCH v2 04/17] libxl/arm: prepare for constructing ACPI tables

[Julien Grall <julien.grall@arm.com>]

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,

-- 
Julien Grall

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