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

2016-06-23 Thread Julien Grall

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 
  #include 
@@ -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 000..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 


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 
+#include 
+
+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 000..5899210
--- /dev/null
+++ b/tools/libxl/libxl_arm_acpi.h
@@ -0,0 +1,32 @@


[...]


+#include "libxl_internal.h"
+#include "libxl_arch.h"
+
+#include 
+
+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


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

2016-06-23 Thread Stefano Stabellini
On Thu, 23 Jun 2016, Shannon Zhao wrote:
> 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 000..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 
> >> + * Copyright (C) 2006 Fabrice Bellard
> >> + * Copyright (C) 2013 Red Hat Inc
> >> + *
> >> + * Author: Michael S. Tsirkin 
> >> + *
> >> + * Copyright (c) 2015 HUAWEI TECHNOLOGIES CO.,LTD.
> >> + *
> >> + * Author: Shannon Zhao 
> >> + *
> >> + * Copyright (C) 2016  Linaro Ltd.
> >> + *
> >> + * Author: Shannon Zhao 
> > 
> > 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. :)

I see. It's important to keep all the right signed-off-by, acked-by and
reviewed-by in the commit message. But the author list on source files
doesn't make much sense these days.___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel


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

2016-06-23 Thread Shannon Zhao
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 000..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 
>> + * Copyright (C) 2006 Fabrice Bellard
>> + * Copyright (C) 2013 Red Hat Inc
>> + *
>> + * Author: Michael S. Tsirkin 
>> + *
>> + * Copyright (c) 2015 HUAWEI TECHNOLOGIES CO.,LTD.
>> + *
>> + * Author: Shannon Zhao 
>> + *
>> + * Copyright (C) 2016  Linaro Ltd.
>> + *
>> + * Author: Shannon Zhao 
> 
> 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,
-- 
Shannon

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


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

2016-06-23 Thread Stefano Stabellini
On Thu, 23 Jun 2016, Shannon Zhao wrote:
> From: Shannon Zhao 
> 
> 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 
> ---
>  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
> 
> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
> index 9fee752..264b6ef 100644
> --- a/tools/libxl/Makefile
> +++ b/tools/libxl/Makefile
> @@ -77,6 +77,10 @@ endif
>  
>  LIBXL_OBJS-$(CONFIG_X86) += libxl_cpuid.o libxl_x86.o libxl_psr.o
>  LIBXL_OBJS-$(CONFIG_ARM) += libxl_nocpuid.o libxl_arm.o libxl_libfdt_compat.o
> +LIBXL_OBJS-$(CONFIG_ARM) += libxl_arm_acpi.o
> +
> +libxl_arm_acpi.o: libxl_arm_acpi.c
> + $(CC) -c $(CFLAGS) -I../../xen/include/ -o $@ libxl_arm_acpi.c
>  
>  ifeq ($(CONFIG_NetBSD),y)
>  LIBXL_OBJS-y += libxl_netbsd.o
> 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 
>  #include 
> @@ -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;

Shouldn't we return 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 000..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 
> + * Copyright (C) 2006 Fabrice Bellard
> + * Copyright (C) 2013 Red Hat Inc
> + *
> + * Author: Michael S. Tsirkin 
> + *
> + * Copyright (c) 2015 HUAWEI TECHNOLOGIES CO.,LTD.
> + *
> + * Author: Shannon Zhao 
> + *
> + * Copyright (C) 2016  Linaro Ltd.
> + *
> + * Author: Shannon Zhao 

Uhm... If in doubt just remove all the Author lines: git log provides
more than enough information about who wrote this code.


> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU Lesser General Public License as published
> + * by the Free Software Foundation; version 2.1 only. with the special
> + * exception on linking described in file LICENSE.
> + *
> + * 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 Lesser General Public License for more details.
> + */
> +
> +#include "libxl_arm_acpi.h"
> +
> +#include 
> +typedef uint8_t u8;
> +typedef uint16_t u16;
> +typedef uint32_t u32;
> +typedef uint64_t u64;
> +#include 
> +#include 
> +
> +enum {
> +RSDP,
> +XSDT,
> +GTDT,
> +MADT,
> +FADT,
> +DSDT,
> +NUMS,
> +};
> +
> +struct acpitable {
> +void *table;
> +size_t size;
> +};
> +
> +static struct acpitable acpitables[NUMS];
> +
> +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_

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

2016-06-22 Thread Shannon Zhao
From: Shannon Zhao 

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 
---
 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

diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index 9fee752..264b6ef 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -77,6 +77,10 @@ endif
 
 LIBXL_OBJS-$(CONFIG_X86) += libxl_cpuid.o libxl_x86.o libxl_psr.o
 LIBXL_OBJS-$(CONFIG_ARM) += libxl_nocpuid.o libxl_arm.o libxl_libfdt_compat.o
+LIBXL_OBJS-$(CONFIG_ARM) += libxl_arm_acpi.o
+
+libxl_arm_acpi.o: libxl_arm_acpi.c
+   $(CC) -c $(CFLAGS) -I../../xen/include/ -o $@ libxl_arm_acpi.c
 
 ifeq ($(CONFIG_NetBSD),y)
 LIBXL_OBJS-y += libxl_netbsd.o
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 
 #include 
@@ -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;
+}
+
+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 000..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 
+ * Copyright (C) 2006 Fabrice Bellard
+ * Copyright (C) 2013 Red Hat Inc
+ *
+ * Author: Michael S. Tsirkin 
+ *
+ * Copyright (c) 2015 HUAWEI TECHNOLOGIES CO.,LTD.
+ *
+ * Author: Shannon Zhao 
+ *
+ * Copyright (C) 2016  Linaro Ltd.
+ *
+ * Author: Shannon Zhao 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only. with the special
+ * exception on linking described in file LICENSE.
+ *
+ * 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 Lesser General Public License for more details.
+ */
+
+#include "libxl_arm_acpi.h"
+
+#include 
+typedef uint8_t u8;
+typedef uint16_t u16;
+typedef uint32_t u32;
+typedef uint64_t u64;
+#include 
+#include 
+
+enum {
+RSDP,
+XSDT,
+GTDT,
+MADT,
+FADT,
+DSDT,
+NUMS,
+};
+
+struct acpitable {
+void *table;
+size_t size;
+};
+
+static struct acpitable acpitables[NUMS];
+
+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 000..5899210
--- /dev/null
+++ b/tools/libxl/libxl_arm_acpi.h
@@ -0,0 +1,32 @@
+/*
+ * Copyright (C) 2016  Linaro Ltd.
+ *
+ * Author: Shannon Zhao 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only. with the special
+ * exception on linking described in file LICENSE.
+ *
+ * This program is distributed in th