[PATCH v5 0/3] Add support for GICv2 on GICv3

[PATCH v5 0/3] Add support for GICv2 on GICv3

[Julien Grall]

Hi all,

This patch series adds support for GICv2 on GICv3. This feature is available
only when the GICv3 hardware is compatible with GICv2.

When it's the case, the same interface is provided in order to use a
virtualize GICv2 (i.e GICC and GICV). This will allow us to re-use the
same vGIC driver.

It has been tested on the ARMv8 Foundation Model with GICv2 and GICv3 as
well as changing the vGIC version emulated for the guest (only on GICv3 host).

A branch with all the patches can be found here:
    git://xenbits.xen.org/people/julieng/xen-unstable.git branch gicv2-on-gicv3-v4

For all changes see in each patch.

Sincerely yours,

Julien Grall (3):
  xen/arm: Rename XEN_DOMCTL_CONFIG_GIC_DEFAULT to
    XEN_DOMCTL_CONFIG_GIC_NATIVE
  arm: Allow the user to specify the GIC version
  xen/arm: gic-v3: Add support of vGICv2 when available

 docs/man/xl.cfg.pod.5         | 34 +++++++++++++++++++++++++++
 tools/libxc/xc_domain.c       |  2 +-
 tools/libxl/libxl.h           |  5 ++++
 tools/libxl/libxl_arch.h      |  6 +++++
 tools/libxl/libxl_arm.c       | 35 +++++++++++++++++++++++++++-
 tools/libxl/libxl_create.c    |  4 ++++
 tools/libxl/libxl_types.idl   | 11 +++++++++
 tools/libxl/libxl_x86.c       |  7 ++++++
 tools/libxl/xl_cmdimpl.c      |  9 ++++++++
 xen/arch/arm/domain.c         | 45 +++++++++++++++++++++---------------
 xen/arch/arm/gic-v3.c         | 54 ++++++++++++++++++++++++++++++++++++++++---
 xen/arch/arm/setup.c          |  2 +-
 xen/arch/arm/vgic.c           |  4 ++--
 xen/include/asm-arm/domain.h  |  2 ++
 xen/include/public/arch-arm.h |  2 +-
 15 files changed, 195 insertions(+), 27 deletions(-)

-- 
2.1.4

[PATCH v5 1/3] xen/arm: Rename XEN_DOMCTL_CONFIG_GIC_DEFAULT to XEN_DOMCTL_CONFIG_GIC_NATIVE

[Julien Grall]

This will reflect that we effectively emulate the same version as the
hardware GIC for the guest.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

---
    Changes in v5:
        - Typo
        - Add Ian's ack

    Changes in v4:
        - Patch added
---
 tools/libxc/xc_domain.c       | 2 +-
 tools/libxl/libxl_arm.c       | 2 +-
 xen/arch/arm/domain.c         | 4 ++--
 xen/arch/arm/setup.c          | 2 +-
 xen/include/public/arch-arm.h | 2 +-
 5 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index ce51e69..6db8d13 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -66,7 +66,7 @@ int xc_domain_create(xc_interface *xch,
 #if defined (__i386) || defined(__x86_64__)
     /* No arch-specific configuration for now */
 #elif defined (__arm__) || defined(__aarch64__)
-    config.gic_version = XEN_DOMCTL_CONFIG_GIC_DEFAULT;
+    config.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE;
     config.nr_spis = 0;
 #else
     errno = ENOSYS;
diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
index 93619a5..03a9205 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -61,7 +61,7 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
     xc_config->nr_spis = nr_spis;
     LOG(DEBUG, " - Allocate %u SPIs", nr_spis);
 
-    xc_config->gic_version = XEN_DOMCTL_CONFIG_GIC_DEFAULT;
+    xc_config->gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE;
 
     return 0;
 }
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index d741e4f..b97ab6c 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -562,12 +562,12 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
 
     /*
      * Currently the vGIC is emulating the same version of the
-     * hardware GIC. Only the value XEN_DOMCTL_CONFIG_GIC_DEFAULT
+     * hardware GIC. Only the value XEN_DOMCTL_CONFIG_GIC_NATIVE
      * is allowed. The DOMCTL will return the actual version of the
      * GIC.
      */
     rc = -EOPNOTSUPP;
-    if ( config->gic_version != XEN_DOMCTL_CONFIG_GIC_DEFAULT )
+    if ( config->gic_version != XEN_DOMCTL_CONFIG_GIC_NATIVE )
         goto fail;
 
     switch ( gic_hw_version() )
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 06f8e54..a46c583 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -829,7 +829,7 @@ void __init start_xen(unsigned long boot_phys_offset,
 
     /* Create initial domain 0. */
     /* The vGIC for DOM0 is exactly emulating the hardware GIC */
-    config.gic_version = XEN_DOMCTL_CONFIG_GIC_DEFAULT;
+    config.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE;
     config.nr_spis = gic_number_lines() - 32;
 
     dom0 = domain_create(0, 0, 0, &config);
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index a2e0bf4..e66bc2c 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -303,7 +303,7 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_context_t);
  * struct xen_arch_domainconfig's ABI is covered by
  * XEN_DOMCTL_INTERFACE_VERSION.
  */
-#define XEN_DOMCTL_CONFIG_GIC_DEFAULT   0
+#define XEN_DOMCTL_CONFIG_GIC_NATIVE    0
 #define XEN_DOMCTL_CONFIG_GIC_V2        1
 #define XEN_DOMCTL_CONFIG_GIC_V3        2
 struct xen_arch_domainconfig {
-- 
2.1.4

[PATCH v5 2/3] arm: Allow the user to specify the GIC version

[Julien Grall]

A platform may have a GIC compatible with previous version of the
device.

This is allow to virtualize an unmodified OS on new hardware if the GIC
is compatible with older version.

When a guest is created, the vGIC will emulate same version as the
hardware. Although, the user can specify in the configuration file the
preferred version (currently only GICv2 and GICv3 are supported).

Signed-off-by: Julien Grall <julien.grall@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>

---
    The hypervisor will check if the GIC is able to virtualize the
    version specified by the user (via the DOMCTL createdomain).
    If it's not compatible an error will be send on the Xen console
    which will make the error not obvious for user.

    I left aside a user error reporting for a follow-up as I'm not
    sure how to notify the user which GIC versions are available. May be
    by a new mechanism similar to xen_get_caps?

    It may be possible to rework the libxl code to restrict the scope of
    xc_config in libxl_domain_make. This can be done in a follow-up if
    we figure what to do the frequency field.

    Changes in v5:
        - Typoes
        - Update commit message and doc with Ian suggestion
        - Print directly the GIC version number rather than the string
        in libxl__arch_domain_prepare_config
        - Don't log and directly return the error when
        libxl__arch_domain_save_config is called in libxl__domain_make
        - Pass directly b_info...gic_version when converting the string
        to the GIC version.

    Changes in v4:
        - Update the documentation to specify the default behavior
        - Update the domain configuration with the GIC version returned
        by the hypervisor.

    Changes in v3:
        - Rename GIC_VERSION define in LIBXL_GIC_VERSION_Vn.
        - Change the value of each define
        - Use libxl_gic_version_from_string rather than custom if/else
        - Rename LIBXL_HAVE_BUILDINFO_GIC_VERSION into
        LIBXL_HAVE_BUILDINFO_ARM_GIC_VERSION and update the comment
        - Update doc with Ian's suggestion
        - Typoes

    Changes in v2:
        - Introduce arch_arm in libxl_domain_build_info to store ARM
        specific field
        - Add docs
        - Remove code that is not necessary with the new version
---
 docs/man/xl.cfg.pod.5        | 34 +++++++++++++++++++++++++++++++++
 tools/libxl/libxl.h          |  5 +++++
 tools/libxl/libxl_arch.h     |  6 ++++++
 tools/libxl/libxl_arm.c      | 35 +++++++++++++++++++++++++++++++++-
 tools/libxl/libxl_create.c   |  4 ++++
 tools/libxl/libxl_types.idl  | 11 +++++++++++
 tools/libxl/libxl_x86.c      |  7 +++++++
 tools/libxl/xl_cmdimpl.c     |  9 +++++++++
 xen/arch/arm/domain.c        | 45 ++++++++++++++++++++++++++------------------
 xen/arch/arm/vgic.c          |  4 ++--
 xen/include/asm-arm/domain.h |  2 ++
 11 files changed, 141 insertions(+), 21 deletions(-)

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index a3e0e2e..2e91e60 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -1688,6 +1688,40 @@ The default is B<en-us>.
 
 See L<qemu(1)> for more information.
 
+=head2 Architecture Specific options
+
+=head3 ARM
+
+=over 4
+
+=item B<gic_version="vN">
+
+Version of the GIC emulated for the guest. Currently, the following
+versions are supported:
+
+=over 4
+
+=item B<v2>
+
+Emulate a GICv2
+
+=item B<v3>
+
+Emulate a GICv3. Note that the emulated GIC does not support the
+GICv2 compatibility mode.
+
+=item B<default>
+
+Emulate the same version as the native GIC hardware used by host where
+the domain was created.
+
+=back
+
+This requires hardware compatibility with the requested version. Either
+natively or via hardware backwards compatibility support.
+
+=back
+
 =head1 SEE ALSO
 
 =over 4
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index a1c5d15..2548480 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -200,6 +200,11 @@
 #define LIBXL_HAVE_DEVICETREE_PASSTHROUGH 1
 
 /*
+ * libxl_domain_build_info has the arm.gic_version field.
+ */
+#define LIBXL_HAVE_BUILDINFO_ARM_GIC_VERSION 1
+
+/*
  * libxl ABI compatibility
  *
  * The only guarantee which libxl makes regarding ABI compatibility
diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h
index d04871c..9a80d43 100644
--- a/tools/libxl/libxl_arch.h
+++ b/tools/libxl/libxl_arch.h
@@ -21,6 +21,12 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
                                       libxl_domain_config *d_config,
                                       xc_domain_configuration_t *xc_config);
 
+/* save the arch specific configuration for the domain */
+_hidden
+int libxl__arch_domain_save_config(libxl__gc *gc,
+                                   libxl_domain_config *d_config,
+                                   const xc_domain_configuration_t *xc_config);
+
 /* arch specific internal domain creation function */
 _hidden
 int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
index 03a9205..d306905 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -61,7 +61,40 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
     xc_config->nr_spis = nr_spis;
     LOG(DEBUG, " - Allocate %u SPIs", nr_spis);
 
-    xc_config->gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE;
+    switch (d_config->b_info.arch_arm.gic_version) {
+    case LIBXL_GIC_VERSION_DEFAULT:
+        xc_config->gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE;
+        break;
+    case LIBXL_GIC_VERSION_V2:
+        xc_config->gic_version = XEN_DOMCTL_CONFIG_GIC_V2;
+        break;
+    case LIBXL_GIC_VERSION_V3:
+        xc_config->gic_version = XEN_DOMCTL_CONFIG_GIC_V3;
+        break;
+    default:
+        LOG(ERROR, "Unknown GIC version %d\n",
+            d_config->b_info.arch_arm.gic_version);
+        return ERROR_FAIL;
+    }
+
+    return 0;
+}
+
+int libxl__arch_domain_save_config(libxl__gc *gc,
+                                   libxl_domain_config *d_config,
+                                   const xc_domain_configuration_t *xc_config)
+{
+    switch (xc_config->gic_version) {
+    case XEN_DOMCTL_CONFIG_GIC_V2:
+        d_config->b_info.arch_arm.gic_version = LIBXL_GIC_VERSION_V2;
+        break;
+    case XEN_DOMCTL_CONFIG_GIC_V3:
+        d_config->b_info.arch_arm.gic_version = LIBXL_GIC_VERSION_V3;
+        break;
+    default:
+        LOG(ERROR, "Unexpected gic version %u\n", xc_config->gic_version);
+        return ERROR_FAIL;
+    }
 
     return 0;
 }
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 9c2303c..e43dde4 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -579,6 +579,10 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
         goto out;
     }
 
+    rc = libxl__arch_domain_save_config(gc, d_config, xc_config);
+    if (rc < 0)
+        goto out;
+
     ret = xc_cpupool_movedomain(ctx->xch, info->poolid, *domid);
     if (ret < 0) {
         LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "domain move fail");
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index e1632fa..11f6461 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -369,6 +369,12 @@ libxl_vnode_info = Struct("vnode_info", [
     ("vcpus", libxl_bitmap), # vcpus in this node
     ])
 
+libxl_gic_version = Enumeration("gic_version", [
+    (0, "DEFAULT"),
+    (0x20, "v2"),
+    (0x30, "v3")
+    ], init_val = "LIBXL_GIC_VERSION_DEFAULT")
+
 libxl_domain_build_info = Struct("domain_build_info",[
     ("max_vcpus",       integer),
     ("avail_vcpus",     libxl_bitmap),
@@ -480,6 +486,11 @@ libxl_domain_build_info = Struct("domain_build_info",[
                                       ])),
                  ("invalid", None),
                  ], keyvar_init_val = "LIBXL_DOMAIN_TYPE_INVALID")),
+
+
+    ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
+                              ])),
+
     ], dir=DIR_IN
 )
 
diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
index ed2bd38..8cd15ca 100644
--- a/tools/libxl/libxl_x86.c
+++ b/tools/libxl/libxl_x86.c
@@ -10,6 +10,13 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
     return 0;
 }
 
+int libxl__arch_domain_save_config(libxl__gc *gc,
+                                   libxl_domain_config *d_config,
+                                   const xc_domain_configuration_t *xc_config)
+{
+    return 0;
+}
+
 static const char *e820_names(int type)
 {
     switch (type) {
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 1be3f8b..bc4ee66 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -2250,6 +2250,15 @@ skip_vfb:
         }
     }
 
+    if (!xlu_cfg_get_string (config, "gic_version", &buf, 1)) {
+        e = libxl_gic_version_from_string(buf, &b_info->arch_arm.gic_version);
+        if (e) {
+            fprintf(stderr,
+                    "Unknown gic_version \"%s\" specified\n", buf);
+            exit(-ERROR_FAIL);
+        }
+     }
+
     xlu_cfg_destroy(config);
 }
 
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index b97ab6c..b2bfc7d 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -531,7 +531,6 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
                        struct xen_arch_domainconfig *config)
 {
     int rc;
-    uint8_t gic_version;
 
     d->arch.relmem = RELMEM_not_started;
 
@@ -560,28 +559,38 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
     if ( (rc = p2m_alloc_table(d)) != 0 )
         goto fail;
 
-    /*
-     * Currently the vGIC is emulating the same version of the
-     * hardware GIC. Only the value XEN_DOMCTL_CONFIG_GIC_NATIVE
-     * is allowed. The DOMCTL will return the actual version of the
-     * GIC.
-     */
-    rc = -EOPNOTSUPP;
-    if ( config->gic_version != XEN_DOMCTL_CONFIG_GIC_NATIVE )
-        goto fail;
-
-    switch ( gic_hw_version() )
+    switch ( config->gic_version )
     {
-    case GIC_V3:
-        gic_version = XEN_DOMCTL_CONFIG_GIC_V3;
+    case XEN_DOMCTL_CONFIG_GIC_NATIVE:
+        switch ( gic_hw_version () )
+        {
+        case GIC_V2:
+            config->gic_version = XEN_DOMCTL_CONFIG_GIC_V2;
+            d->arch.vgic.version = GIC_V2;
+            break;
+
+        case GIC_V3:
+            config->gic_version = XEN_DOMCTL_CONFIG_GIC_V3;
+            d->arch.vgic.version = GIC_V3;
+            break;
+
+        default:
+            BUG();
+        }
+        break;
+
+    case XEN_DOMCTL_CONFIG_GIC_V2:
+        d->arch.vgic.version = GIC_V2;
         break;
-    case GIC_V2:
-        gic_version = XEN_DOMCTL_CONFIG_GIC_V2;
+
+    case XEN_DOMCTL_CONFIG_GIC_V3:
+        d->arch.vgic.version = GIC_V3;
         break;
+
     default:
-        BUG();
+        rc = -EOPNOTSUPP;
+        goto fail;
     }
-    config->gic_version = gic_version;
 
     if ( (rc = domain_vgic_init(d, config->nr_spis)) != 0 )
         goto fail;
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 01fc9d8..a6835a8 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -81,7 +81,7 @@ int domain_vgic_init(struct domain *d, unsigned int nr_spis)
 
     d->arch.vgic.nr_spis = nr_spis;
 
-    switch ( gic_hw_version() )
+    switch ( d->arch.vgic.version )
     {
 #ifdef HAS_GICV3
     case GIC_V3:
@@ -95,7 +95,7 @@ int domain_vgic_init(struct domain *d, unsigned int nr_spis)
         break;
     default:
         printk(XENLOG_G_ERR "d%d: Unknown vGIC version %u\n",
-               d->domain_id, gic_hw_version());
+               d->domain_id, d->arch.vgic.version);
         return -ENODEV;
     }
 
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 8f5a689..56aa208 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -77,6 +77,8 @@ struct arch_domain
     } virt_timer_base;
 
     struct {
+        /* Version of the vGIC */
+        enum gic_version version;
         /* GIC HW version specific vGIC driver handler */
         const struct vgic_ops *handler;
         /*
-- 
2.1.4

Re: [PATCH v5 2/3] arm: Allow the user to specify the GIC version

[Ian Campbell]

On Tue, 2015-07-07 at 17:22 +0100, Julien Grall wrote:
> A platform may have a GIC compatible with previous version of the
> device.
> 
> This is allow to virtualize an unmodified OS on new hardware if the GIC
> is compatible with older version.
> 
> When a guest is created, the vGIC will emulate same version as the
> hardware. Although, the user can specify in the configuration file the
> preferred version (currently only GICv2 and GICv3 are supported).
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH v5 2/3] arm: Allow the user to specify the GIC version

[Ian Campbell]

On Tue, 2015-07-07 at 17:22 +0100, Julien Grall wrote:
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index e1632fa..11f6461 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -369,6 +369,12 @@ libxl_vnode_info = Struct("vnode_info", [
>      ("vcpus", libxl_bitmap), # vcpus in this node
>      ])
>  
> +libxl_gic_version = Enumeration("gic_version", [
> +    (0, "DEFAULT"),
> +    (0x20, "v2"),
> +    (0x30, "v3")
> +    ], init_val = "LIBXL_GIC_VERSION_DEFAULT")
> +
>  libxl_domain_build_info = Struct("domain_build_info",[
>      ("max_vcpus",       integer),
>      ("avail_vcpus",     libxl_bitmap),
> @@ -480,6 +486,11 @@ libxl_domain_build_info = Struct("domain_build_info",[
>                                        ])),
>                   ("invalid", None),
>                   ], keyvar_init_val = "LIBXL_DOMAIN_TYPE_INVALID")),
> +
> +
> +    ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
> +                              ])),
> +
>      ], dir=DIR_IN

This results in the following when building the ocaml bindings:

Traceback (most recent call last):
  File "genwrap.py", line 529, in <module>
    ml.write(gen_ocaml_ml(ty, False))
  File "genwrap.py", line 217, in gen_ocaml_ml
    s += gen_struct(ty)
  File "genwrap.py", line 119, in gen_struct
    x = ocaml_instance_of_field(f)
  File "genwrap.py", line 112, in ocaml_instance_of_field
    return "%s : %s" % (munge_name(name), ocaml_type_of(f.type))
  File "genwrap.py", line 90, in ocaml_type_of
    return ty.rawname.capitalize() + ".t"
AttributeError: 'NoneType' object has no attribute 'capitalize'
make[7]: *** No rule to make target '_libxl_types.ml.in', needed by 'xenlight.ml'.  Stop.

I'll take a look.

Re: [PATCH v5 2/3] arm: Allow the user to specify the GIC version

[Ian Campbell]

On Wed, 2015-07-08 at 11:17 +0100, Ian Campbell wrote:
> On Tue, 2015-07-07 at 17:22 +0100, Julien Grall wrote:
> > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> > index e1632fa..11f6461 100644
> > --- a/tools/libxl/libxl_types.idl
> > +++ b/tools/libxl/libxl_types.idl
> > @@ -369,6 +369,12 @@ libxl_vnode_info = Struct("vnode_info", [
> >      ("vcpus", libxl_bitmap), # vcpus in this node
> >      ])
> >  
> > +libxl_gic_version = Enumeration("gic_version", [
> > +    (0, "DEFAULT"),
> > +    (0x20, "v2"),
> > +    (0x30, "v3")
> > +    ], init_val = "LIBXL_GIC_VERSION_DEFAULT")
> > +
> >  libxl_domain_build_info = Struct("domain_build_info",[
> >      ("max_vcpus",       integer),
> >      ("avail_vcpus",     libxl_bitmap),
> > @@ -480,6 +486,11 @@ libxl_domain_build_info = Struct("domain_build_info",[
> >                                        ])),
> >                   ("invalid", None),
> >                   ], keyvar_init_val = "LIBXL_DOMAIN_TYPE_INVALID")),
> > +
> > +
> > +    ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
> > +                              ])),
> > +
> >      ], dir=DIR_IN
> 
> This results in the following when building the ocaml bindings:
> 
> Traceback (most recent call last):
>   File "genwrap.py", line 529, in <module>
>     ml.write(gen_ocaml_ml(ty, False))
>   File "genwrap.py", line 217, in gen_ocaml_ml
>     s += gen_struct(ty)
>   File "genwrap.py", line 119, in gen_struct
>     x = ocaml_instance_of_field(f)
>   File "genwrap.py", line 112, in ocaml_instance_of_field
>     return "%s : %s" % (munge_name(name), ocaml_type_of(f.type))
>   File "genwrap.py", line 90, in ocaml_type_of
>     return ty.rawname.capitalize() + ".t"
> AttributeError: 'NoneType' object has no attribute 'capitalize'
> make[7]: *** No rule to make target '_libxl_types.ml.in', needed by 'xenlight.ml'.  Stop.
> 
> I'll take a look.

I have a patch to genwrap.py which results in the following diff to the
generate ml files for the anonymous sub-struct added by the IDL change
above.

Dave/Euan/Rob, is that idiomatic ocaml or is it possible to have
anonymous structs in ocaml like it is in C?

If there is a better/more usual way to do this would you mind supplying
me with the ocaml I should be aiming for please?

Ian.

--- tools/ocaml/libs/xl/_libxl_BACKUP_types.ml.in	2015-07-08 11:22:35.000000000 +0100
+++ tools/ocaml/libs/xl/_libxl_types.ml.in	2015-07-08 12:25:56.000000000 +0100
@@ -508,6 +508,17 @@ module Vnode_info = struct
 	external default : ctx -> unit -> t = "stub_libxl_vnode_info_init"
 end
 
+(* libxl_gic_version implementation *)
+type gic_version = 
+	 | GIC_VERSION_DEFAULT
+	 | GIC_VERSION_V2
+	 | GIC_VERSION_V3
+
+let string_of_gic_version = function
+	| GIC_VERSION_DEFAULT -> "DEFAULT"
+	| GIC_VERSION_V2 -> "V2"
+	| GIC_VERSION_V3 -> "V3"
+
 (* libxl_domain_build_info implementation *)
 module Domain_build_info = struct
 
@@ -566,6 +577,10 @@ module Domain_build_info = struct
 	
 	type type__union = Hvm of type_hvm | Pv of type_pv | Invalid
 	
+	type arch_arm__anon = {
+		gic_version : gic_version;
+	}
+	
 	type t =
 	{
 		max_vcpus : int;
@@ -607,6 +622,7 @@ module Domain_build_info = struct
 		ramdisk : string option;
 		device_tree : string option;
 		xl_type : type__union;
+		arch_arm : arch_arm__anon;
 	}
 	external default : ctx -> ?xl_type:domain_type -> unit -> t = "stub_libxl_domain_build_info_init"
 end

Re: [PATCH v5 2/3] arm: Allow the user to specify the GIC version

[Rob Hoes]

> On 8 Jul 2015, at 12:37, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> ...
> 
> I have a patch to genwrap.py which results in the following diff to the
> generate ml files for the anonymous sub-struct added by the IDL change
> above.
> 
> Dave/Euan/Rob, is that idiomatic ocaml or is it possible to have
> anonymous structs in ocaml like it is in C?

I think that you have done the right thing. I’m not sure if you can define a nested record type without giving the inner one a name. But frankly, I don’t think there is much point to that either. In fact, I’d even drop the "__anon” and use the struct name as the type name directly.

Or for consistency, use "type_<name>”, which is similar to what happens in the keyed union case (“type_hvm", “type_pv”). Incidentally, those structs are also defined as "Struct(None, [...”, but are probably handled specially because the struct appears inside the union.

Cheers,
Rob

> If there is a better/more usual way to do this would you mind supplying
> me with the ocaml I should be aiming for please?
> 
> Ian.
> 
> --- tools/ocaml/libs/xl/_libxl_BACKUP_types.ml.in	2015-07-08 11:22:35.000000000 +0100
> +++ tools/ocaml/libs/xl/_libxl_types.ml.in	2015-07-08 12:25:56.000000000 +0100
> @@ -508,6 +508,17 @@ module Vnode_info = struct
> 	external default : ctx -> unit -> t = "stub_libxl_vnode_info_init"
> end
> 
> +(* libxl_gic_version implementation *)
> +type gic_version = 
> +	 | GIC_VERSION_DEFAULT
> +	 | GIC_VERSION_V2
> +	 | GIC_VERSION_V3
> +
> +let string_of_gic_version = function
> +	| GIC_VERSION_DEFAULT -> "DEFAULT"
> +	| GIC_VERSION_V2 -> "V2"
> +	| GIC_VERSION_V3 -> "V3"
> +
> (* libxl_domain_build_info implementation *)
> module Domain_build_info = struct
> 
> @@ -566,6 +577,10 @@ module Domain_build_info = struct
> 	
> 	type type__union = Hvm of type_hvm | Pv of type_pv | Invalid
> 	
> +	type arch_arm__anon = {
> +		gic_version : gic_version;
> +	}
> +	
> 	type t =
> 	{
> 		max_vcpus : int;
> @@ -607,6 +622,7 @@ module Domain_build_info = struct
> 		ramdisk : string option;
> 		device_tree : string option;
> 		xl_type : type__union;
> +		arch_arm : arch_arm__anon;
> 	}
> 	external default : ctx -> ?xl_type:domain_type -> unit -> t = "stub_libxl_domain_build_info_init"
> end
> 
> 
> 

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

Re: [PATCH v5 2/3] arm: Allow the user to specify the GIC version

[Ian Campbell]

On Wed, 2015-07-08 at 15:08 +0100, Rob Hoes wrote:
> 
> > On 8 Jul 2015, at 12:37, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > ...
> > 
> > I have a patch to genwrap.py which results in the following diff to the
> > generate ml files for the anonymous sub-struct added by the IDL change
> > above.
> > 
> > Dave/Euan/Rob, is that idiomatic ocaml or is it possible to have
> > anonymous structs in ocaml like it is in C?
> 
> I think that you have done the right thing. I’m not sure if you can
> define a nested record type without giving the inner one a name. But
> frankly, I don’t think there is much point to that either.

Thanks.

>  In fact, I’d even drop the "__anon” and use the struct name as the
> type name directly.

I did have that to start with, see below for how the keyed union stuff
worked which lead me to suffix it.

> Or for consistency, use "type_<name>”, which is similar to what
> happens in the keyed union case (“type_hvm", “type_pv”).

Actually in this case the "type" is the name of the KeyedUnion and the
hvm|pv are the potential values of the enum which is the key.

>  Incidentally, those structs are also defined as "Struct(None, [...”,
> but are probably handled specially because the struct appears inside
> the union.

Correct, they needed different special handling of the struct in order
to construct the keyedunion as an ocaml discriminated type thing, which
ends up with (for a keyedunion key field "type" of the domain type
enum):
	type type_hvm = { ... }
	type type_pv = { ... }
        type type__union = Hvm of type_hvm | Pv of type_pv | Invalid

then the field is "xl_type : type__union";

(the xl_ prefix is because type, the name of the C field, is an ocaml
keyword...)

I added the __anon suffix for consistency with the __union suffix here.

Ian.



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

Re: [PATCH v5 2/3] arm: Allow the user to specify the GIC version

[Rob Hoes]

> On 8 Jul 2015, at 15:19, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> 
> On Wed, 2015-07-08 at 15:08 +0100, Rob Hoes wrote:
>> 
>>> On 8 Jul 2015, at 12:37, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>>> ...
>>> 
>>> I have a patch to genwrap.py which results in the following diff to the
>>> generate ml files for the anonymous sub-struct added by the IDL change
>>> above.
>>> 
>>> Dave/Euan/Rob, is that idiomatic ocaml or is it possible to have
>>> anonymous structs in ocaml like it is in C?
>> 
>> I think that you have done the right thing. I’m not sure if you can
>> define a nested record type without giving the inner one a name. But
>> frankly, I don’t think there is much point to that either.
> 
> Thanks.
> 
>> In fact, I’d even drop the "__anon” and use the struct name as the
>> type name directly.
> 
> I did have that to start with, see below for how the keyed union stuff
> worked which lead me to suffix it.

It’s just that __anon doesn’t really mean much, because it isn’t anonymous in OCaml :) In hindsight, I would have probably dropped the __union as well...

Anyway, I don’t care too much about these names, because in practice you won’t normally use them directly.

Rob

>> Or for consistency, use "type_<name>”, which is similar to what
>> happens in the keyed union case (“type_hvm", “type_pv”).
> 
> Actually in this case the "type" is the name of the KeyedUnion and the
> hvm|pv are the potential values of the enum which is the key.
> 
>> Incidentally, those structs are also defined as "Struct(None, [...”,
>> but are probably handled specially because the struct appears inside
>> the union.
> 
> Correct, they needed different special handling of the struct in order
> to construct the keyedunion as an ocaml discriminated type thing, which
> ends up with (for a keyedunion key field "type" of the domain type
> enum):
> 	type type_hvm = { ... }
> 	type type_pv = { ... }
>        type type__union = Hvm of type_hvm | Pv of type_pv | Invalid
> 
> then the field is "xl_type : type__union";
> 
> (the xl_ prefix is because type, the name of the C field, is an ocaml
> keyword...)
> 
> I added the __anon suffix for consistency with the __union suffix here.
> 
> Ian.
> 
> 

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

Re: [PATCH v5 2/3] arm: Allow the user to specify the GIC version

[Ian Campbell]

On Wed, 2015-07-08 at 15:34 +0100, Rob Hoes wrote:
> 
> > On 8 Jul 2015, at 15:19, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > 
> > On Wed, 2015-07-08 at 15:08 +0100, Rob Hoes wrote:
> >> 
> >>> On 8 Jul 2015, at 12:37, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> >>> ...
> >>> 
> >>> I have a patch to genwrap.py which results in the following diff to the
> >>> generate ml files for the anonymous sub-struct added by the IDL change
> >>> above.
> >>> 
> >>> Dave/Euan/Rob, is that idiomatic ocaml or is it possible to have
> >>> anonymous structs in ocaml like it is in C?
> >> 
> >> I think that you have done the right thing. I’m not sure if you can
> >> define a nested record type without giving the inner one a name. But
> >> frankly, I don’t think there is much point to that either.
> > 
> > Thanks.
> > 
> >> In fact, I’d even drop the "__anon” and use the struct name as the
> >> type name directly.
> > 
> > I did have that to start with, see below for how the keyed union stuff
> > worked which lead me to suffix it.
> 
> It’s just that __anon doesn’t really mean much, because it isn’t
> anonymous in OCaml :) In hindsight, I would have probably dropped the
> __union as well...

Me too, now ;-)

> Anyway, I don’t care too much about these names, because in practice
> you won’t normally use them directly.

Right, thanks.

> 
> Rob
> 
> >> Or for consistency, use "type_<name>”, which is similar to what
> >> happens in the keyed union case (“type_hvm", “type_pv”).
> > 
> > Actually in this case the "type" is the name of the KeyedUnion and the
> > hvm|pv are the potential values of the enum which is the key.
> > 
> >> Incidentally, those structs are also defined as "Struct(None, [...”,
> >> but are probably handled specially because the struct appears inside
> >> the union.
> > 
> > Correct, they needed different special handling of the struct in order
> > to construct the keyedunion as an ocaml discriminated type thing, which
> > ends up with (for a keyedunion key field "type" of the domain type
> > enum):
> > 	type type_hvm = { ... }
> > 	type type_pv = { ... }
> >        type type__union = Hvm of type_hvm | Pv of type_pv | Invalid
> > 
> > then the field is "xl_type : type__union";
> > 
> > (the xl_ prefix is because type, the name of the C field, is an ocaml
> > keyword...)
> > 
> > I added the __anon suffix for consistency with the __union suffix here.
> > 
> > Ian.
> > 
> > 
> 



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

[PATCH v5 3/3] xen/arm: gic-v3: Add support of vGICv2 when available

[Julien Grall]

* Modify the GICv3 driver to recognize a such device. I wasn't able
  to find a register which tell if GICv2 is supported on GICv3. The only
  way to find it seems to check if the DT node provides GICC and GICV.

* Disable access to ICC_SRE_EL1 to guest using vGICv2

* The LR is slightly different for vGICv2. The interrupt is always
injected with group0.

* Add a comment explaining why Group1 is used for vGICv3.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

---
    I haven't address the request from Ian to not use the R/M/W idiom.
    Most of the usage of the idiom are for EL2 registers (i.e registers
    used by the hypervisor). They can be modified at any time by Xen,
    and not specific to the running domain. We would have to progate the
    change to each domain if we don't use the R/W/M.

    ICC_SRE_EL2 falls under the same umbrella. It would be preferable to
    stay with the same model as today i.e:
        - *_EL1: straight save/restore
        - *_EL2: R/M/W to necessary field

    Changes in v3:
        - Add Ian's ack
        - Remove the isb which was not strictly necessary

    Changes in v2:
        - Use vgic_v2_hw_setup to notify that GICv3 supports GICv2
        - Revert changes in comment
---
 xen/arch/arm/gic-v3.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 51 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 337fbb9..2033951 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -247,7 +247,7 @@ static void gicv3_enable_sre(void)
     uint32_t val;
 
     val = READ_SYSREG32(ICC_SRE_EL2);
-    val |= GICC_SRE_EL2_SRE | GICC_SRE_EL2_ENEL1;
+    val |= GICC_SRE_EL2_SRE;
 
     WRITE_SYSREG32(val, ICC_SRE_EL2);
     isb();
@@ -375,6 +375,19 @@ static void gicv3_save_state(struct vcpu *v)
 
 static void gicv3_restore_state(const struct vcpu *v)
 {
+    uint32_t val;
+
+    val = READ_SYSREG32(ICC_SRE_EL2);
+    /*
+     * Don't give access to system registers when the guest is using
+     * GICv2
+     */
+    if ( v->domain->arch.vgic.version == GIC_V2 )
+        val &= ~GICC_SRE_EL2_ENEL1;
+    else
+        val |= GICC_SRE_EL2_ENEL1;
+    WRITE_SYSREG32(val, ICC_SRE_EL2);
+
     WRITE_SYSREG32(v->arch.gic.v3.sre_el1, ICC_SRE_EL1);
     WRITE_SYSREG32(v->arch.gic.v3.vmcr, ICH_VMCR_EL2);
     restore_aprn_regs(&v->arch.gic);
@@ -866,13 +879,20 @@ static void gicv3_disable_interface(void)
 static void gicv3_update_lr(int lr, const struct pending_irq *p,
                             unsigned int state)
 {
-    uint64_t grp = GICH_LR_GRP1;
     uint64_t val = 0;
 
     BUG_ON(lr >= gicv3_info.nr_lrs);
     BUG_ON(lr < 0);
 
-    val =  (((uint64_t)state & 0x3) << GICH_LR_STATE_SHIFT) | grp;
+    val =  (((uint64_t)state & 0x3) << GICH_LR_STATE_SHIFT);
+
+    /*
+     * When the guest is GICv3, all guest IRQs are Group 1, as Group0
+     * would result in a FIQ in the guest, which it wouldn't expect
+     */
+    if ( current->domain->arch.vgic.version == GIC_V3 )
+        val |= GICH_LR_GRP1;
+
     val |= ((uint64_t)p->priority & 0xff) << GICH_LR_PRIORITY_SHIFT;
     val |= ((uint64_t)p->irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT;
 
@@ -1119,6 +1139,33 @@ static int __init cmp_rdist(const void *a, const void *b)
     return ( l->base < r->base) ? -1 : 0;
 }
 
+/* If the GICv3 supports GICv2, initialize it */
+static void __init gicv3_init_v2(const struct dt_device_node *node,
+                                 paddr_t dbase)
+{
+    int res;
+    paddr_t cbase, vbase;
+
+    /*
+     * For GICv3 supporting GICv2, GICC and GICV base address will be
+     * provided.
+     */
+    res = dt_device_get_address(node, 1 + gicv3.rdist_count,
+                                &cbase, NULL);
+    if ( res )
+        return;
+
+    res = dt_device_get_address(node, 1 + gicv3.rdist_count + 2,
+                                &vbase, NULL);
+    if ( res )
+        return;
+
+    printk("GICv3 compatible with GICv2 cbase %#"PRIpaddr" vbase %#"PRIpaddr"\n",
+           cbase, vbase);
+
+    vgic_v2_setup_hw(dbase, cbase, vbase);
+}
+
 /* Set up the GIC */
 static int __init gicv3_init(void)
 {
@@ -1216,6 +1263,7 @@ static int __init gicv3_init(void)
 
     vgic_v3_setup_hw(dbase, gicv3.rdist_count, gicv3.rdist_regions,
                      gicv3.rdist_stride);
+    gicv3_init_v2(node, dbase);
 
     spin_lock_init(&gicv3.lock);
 
-- 
2.1.4

Re: [PATCH v5 0/3] Add support for GICv2 on GICv3

[Ian Campbell]

On Tue, 2015-07-07 at 17:22 +0100, Julien Grall wrote:
> Julien Grall (3):
>   xen/arm: Rename XEN_DOMCTL_CONFIG_GIC_DEFAULT to
>     XEN_DOMCTL_CONFIG_GIC_NATIVE

Applied this one.

>   arm: Allow the user to specify the GIC version

But this one caused a build failure, I've replied to the patch.

>   xen/arm: gic-v3: Add support of vGICv2 when available

Re: [PATCH v5 0/3] Add support for GICv2 on GICv3

[Ian Campbell]

On Tue, 2015-07-07 at 17:22 +0100, Julien Grall wrote:
>   arm: Allow the user to specify the GIC version
>   xen/arm: gic-v3: Add support of vGICv2 when available

Applied following "tools: ocaml: Handle anonymous struct members of
structs in libxl IDL". thanks.

Ian.