[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [RFC PATCH v2] xen/arm: Add support for GICv3 for domU



Hi Stefano,

On 28/10/2014 18:12, Stefano Stabellini wrote:
I have just realized that this patch didn't make RC1 last Friday.

I asked for few changes: documentation, way to implement the check... but Vijay never came back with a new version.

What should we do about it?

Without it, I am not sure we should really claim that Xen 4.5 has GICv3
support, given that we wouldn't be able to start any guests on a GICv3
platform.

I don't think is so risky it couldn't make RC2, but that's not entirely
up to me.

This patch series requires to defer the VGIC initialization later.

The patch [1] to implement this feature has been deferred to Xen 4.6 because it was only necessary to my non-pci passthrough series (which has not reached Xen 4.5 for various reasons).

I don't think this patch should go in Xen 4.5 at this stage of the release (RC1 went out last week), because it modify too much the way to initialize the domain (VGIC has been deferred). Furthermore it has been reworked in a private branch [2] to address the comments.

By side effect, the toolstack part for the GICv3 would have to be defer.

Regards,

[1] https://patches.linaro.org/34664/
[2] http://xenbits.xen.org/gitweb/?p=people/julieng/xen-unstable.git;a=commitdiff;h=002b216707ae90b669ffebf009042d5e42b26dc0;hp=30e15fa1f52aa790bf13b5f40dc62d832443846c

On Wed, 8 Oct 2014, Julien Grall wrote:
Hello Vijay,

On 10/06/2014 01:46 PM, vijay.kilari@xxxxxxxxx wrote:
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 7d9eec2..8f3f074 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -349,6 +349,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
      ("disable_migrate", libxl_defbool),
      ("cpuid",           libxl_cpuid_policy_list),
      ("blkdev_start",    string),
+    ("gic_version",     uint32),

How would you differentiate GICv2 from GICv2m with an integer? I think
an enum would be better to describe the GIC version.

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 2ec17ca..5fcb396 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1523,6 +1523,9 @@ skip_vfb:
      if (!xlu_cfg_get_long (config, "pci_seize", &l, 0))
          pci_seize = l;

+    if (!xlu_cfg_get_long (config, "gic_version", &l, 0))
+        b_info->gic_version = l;
+

You have to document this new option in docs/man/xl.cfg.pod.5

[..]

diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
index 370dd99..1bea026 100644
--- a/xen/arch/arm/domctl.c
+++ b/xen/arch/arm/domctl.c
@@ -10,6 +10,8 @@
  #include <xen/errno.h>
  #include <xen/sched.h>
  #include <xen/hypercall.h>
+#include <asm/gic.h>
+#include <xen/guest_access.h>
  #include <public/domctl.h>

  long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
@@ -39,9 +41,36 @@ long arch_do_domctl(struct xen_domctl *domctl, struct domain 
*d,
          if ( domctl->u.configuredomain.nr_spis > (gic_number_lines() - 32) )
              return -EINVAL;

-        return domain_configure_vgic(d, domctl->u.configuredomain.nr_spis);
-    }
+        if ( domain_configure_vgic(d, domctl->u.configuredomain.nr_spis) )
+            return -EINVAL;

domain_configure_vgic should be called after we check that current
version of GIC match. The user may want to chose to emulate a GICv2 on
GICv3 hardware.

diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index cebb349..6f80c99 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -363,6 +363,14 @@ typedef uint64_t xen_callback_t;
   * should instead use the FDT.
   */

+/* GICv3 address space */
+#define GUEST_GICV3_GICD_BASE      0x03001000ULL
+#define GUEST_GICV3_GICD_SIZE      0x10000ULL
+#define GUEST_GICV3_GICR_BASE      0x03020000ULL
+#define GUEST_GICV3_GICR_SIZE      0x200000ULL
+#define GUEST_GICV3_RDIST_STRIDE   0x20000ULL
+#define GUEST_GICV3_RDIST_REGIONS  0x1ULL
+

This should go after "/* Physical Address Space */

  /* Physical Address Space */
  #define GUEST_GICD_BASE   0x03001000ULL
  #define GUEST_GICD_SIZE   0x00001000ULL

Please modify those defines, along *GICC* to add GICV2 in the name.

diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 8adb8e2..502cfb6 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -73,6 +73,8 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_createdomain_t);
  struct xen_domctl_configuredomain {
      /* IN parameters */
      uint32_t nr_spis;
+    /* IN/OUT parameter */
+    uint32_t gic_version;

uint32_t sounds a bit too much for the gic_version. Maybe a uint8_t?
Also a better name would be vgic_version.

Futhermore, people reading the structure don't know what value should be
expected in this field. I would introduce define to specify the
different value.

Regards,

--
Julien Grall


--
Julien Grall

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.