Re: [PATCH v2 for 4.5] xen/arm: Add support for GICv3 for domU

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Mon, Nov 03, 2014 at 06:07:33PM +0530, Vijay Kilari wrote:
> On Mon, Nov 3, 2014 at 4:31 PM, Stefano Stabellini
> <stefano.stabellini@eu.citrix.com> wrote:
> > On Sat, 1 Nov 2014, Julien Grall wrote:
> >> The vGIC will emulate the same version as the hardware. The toolstack has
> >> to retrieve the version of the vGIC in order to be able to create the
> >> corresponding device tree node.
> >>
> >> A new DOMCTL has been introduced for ARM to configure the domain. For now
> >> it only allow the toolstack to retrieve the version of vGIC.
> >> This DOMCTL will be extend later to let the user choose the version of the
> >> emulated GIC.
> >>
> >> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> >> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> >> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> >> Cc: Jan Beulich <jbeulich@suse.com>
> >> Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> >> Cc: Wei Liu <wei.liu2@citrix.com>
> >
> > Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> Tested with GICv3 platform.

Thank you for doing that.

It also needs Acks from Daniel and Jan.

> 
> >
> >
> >>     Changes in v2:
> >>         - Use memcpu in xc_domain_configure
> >>         - Rename the DOMCTL into domctl_arm_configuredomain
> >>         - Drop arch_domain_create_pre. Will be reintroduced in Xen 4.6
> >>         and fold the code in arch_domain_init_hw_description
> >>         - Return -EOPNOTSUPP when the value of gic_version is not
> >>         supported
> >>
> >> This patch is based on Vijay's GICv3 guest support patch [1] and my patch to
> >> defer the initialization of the vGIC [2].
> >>
> >> Xen 4.5 has already support for the hardware GICv3. While it's possible to
> >> boot and use DOM0, there is no guest support. The first version of this patch
> >> has been sent a couple of months ago, but has never reached unstable for
> >> various reasons (based on deferred series, lake of review at that time...).
> >> Without it, platform with GICv3 support won't be able to boot any guest.
> >>
> >> The patch has been reworked to make it acceptable for Xen 4.5. Except the new
> >> DOMCTL to retrieve the GIC version and one check. The code is purely GICv3.
> >>
> >> Features such as adding a new config option to let the user choose the GIC
> >> version are deferred to Xen 4.6.
> >>
> >> It has been tested on both GICv2/GICv3 platform. And build tested for x86.
> >>
> >> [1] http://lists.xen.org/archives/html/xen-devel/2014-10/msg00583.html
> >> [2] https://patches.linaro.org/34664/
> >> ---
> >>  tools/flask/policy/policy/modules/xen/xen.if |    2 +-
> >>  tools/libxc/include/xenctrl.h                |    6 ++
> >>  tools/libxc/xc_domain.c                      |   20 ++++++
> >>  tools/libxl/libxl_arm.c                      |   97 ++++++++++++++++++++++++--
> >>  xen/arch/arm/domctl.c                        |   35 ++++++++++
> >>  xen/arch/arm/gic-v3.c                        |   16 ++++-
> >>  xen/include/public/arch-arm.h                |   16 +++++
> >>  xen/include/public/domctl.h                  |   17 +++++
> >>  xen/xsm/flask/hooks.c                        |    3 +
> >>  xen/xsm/flask/policy/access_vectors          |    2 +
> >>  10 files changed, 206 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/tools/flask/policy/policy/modules/xen/xen.if b/tools/flask/policy/policy/modules/xen/xen.if
> >> index 641c797..fa69c9d 100644
> >> --- a/tools/flask/policy/policy/modules/xen/xen.if
> >> +++ b/tools/flask/policy/policy/modules/xen/xen.if
> >> @@ -49,7 +49,7 @@ define(`create_domain_common', `
> >>                       getdomaininfo hypercall setvcpucontext setextvcpucontext
> >>                       getscheduler getvcpuinfo getvcpuextstate getaddrsize
> >>                       getaffinity setaffinity };
> >> -     allow $1 $2:domain2 { set_cpuid settsc setscheduler setclaim set_max_evtchn set_vnumainfo get_vnumainfo psr_cmt_op };
> >> +     allow $1 $2:domain2 { set_cpuid settsc setscheduler setclaim set_max_evtchn set_vnumainfo get_vnumainfo psr_cmt_op configure_domain };
> >>       allow $1 $2:security check_context;
> >>       allow $1 $2:shadow enable;
> >>       allow $1 $2:mmu { map_read map_write adjust memorymap physmap pinpage mmuext_op };
> >> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> >> index 564e187..45e282c 100644
> >> --- a/tools/libxc/include/xenctrl.h
> >> +++ b/tools/libxc/include/xenctrl.h
> >> @@ -483,6 +483,12 @@ int xc_domain_create(xc_interface *xch,
> >>                       uint32_t flags,
> >>                       uint32_t *pdomid);
> >>
> >> +#if defined(__arm__) || defined(__aarch64__)
> >> +typedef xen_domctl_arm_configuredomain_t xc_domain_configuration_t;
> >> +
> >> +int xc_domain_configure(xc_interface *xch, uint32_t domid,
> >> +                        xc_domain_configuration_t *config);
> >> +#endif
> >>
> >>  /* Functions to produce a dump of a given domain
> >>   *  xc_domain_dumpcore - produces a dump to a specified file
> >> diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
> >> index a9bcd4a..1aa1f45 100644
> >> --- a/tools/libxc/xc_domain.c
> >> +++ b/tools/libxc/xc_domain.c
> >> @@ -48,6 +48,26 @@ int xc_domain_create(xc_interface *xch,
> >>      return 0;
> >>  }
> >>
> >> +#if defined(__arm__) || defined(__aarch64__)
> >> +int xc_domain_configure(xc_interface *xch, uint32_t domid,
> >> +                        xc_domain_configuration_t *config)
> >> +{
> >> +    int rc;
> >> +    DECLARE_DOMCTL;
> >> +
> >> +    domctl.cmd = XEN_DOMCTL_arm_configure_domain;
> >> +    domctl.domain = (domid_t)domid;
> >> +    /* xc_domain_configure_t is an alias to xen_domctl_arm_configuredomain */
> >> +    memcpy(&domctl.u.configuredomain, config, sizeof(*config));
> >> +
> >> +    rc = do_domctl(xch, &domctl);
> >> +    if ( !rc )
> >> +        memcpy(config, &domctl.u.configuredomain, sizeof(*config));
> >> +
> >> +    return rc;
> >> +}
> >> +#endif
> >> +
> >>  int xc_domain_cacheflush(xc_interface *xch, uint32_t domid,
> >>                           xen_pfn_t start_pfn, xen_pfn_t nr_pfns)
> >>  {
> >> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> >> index a122e4a..3dd6e7c 100644
> >> --- a/tools/libxl/libxl_arm.c
> >> +++ b/tools/libxl/libxl_arm.c
> >> @@ -23,6 +23,18 @@
> >>  #define DT_IRQ_TYPE_LEVEL_HIGH     0x00000004
> >>  #define DT_IRQ_TYPE_LEVEL_LOW      0x00000008
> >>
> >> +static const char *gicv_to_string(uint8_t gic_version)
> >> +{
> >> +    switch (gic_version) {
> >> +    case XEN_DOMCTL_CONFIG_GIC_V2:
> >> +        return "V2";
> >> +    case XEN_DOMCTL_CONFIG_GIC_V3:
> >> +        return "V3";
> >> +    default:
> >> +        return "unknown";
> >> +    }
> >> +}
> >> +
> >>  int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
> >>                                uint32_t domid)
> >>  {
> >> @@ -307,9 +319,9 @@ static int make_memory_nodes(libxl__gc *gc, void *fdt,
> >>      return 0;
> >>  }
> >>
> >> -static int make_intc_node(libxl__gc *gc, void *fdt,
> >> -                          uint64_t gicd_base, uint64_t gicd_size,
> >> -                          uint64_t gicc_base, uint64_t gicc_size)
> >> +static int make_gicv2_node(libxl__gc *gc, void *fdt,
> >> +                           uint64_t gicd_base, uint64_t gicd_size,
> >> +                           uint64_t gicc_base, uint64_t gicc_size)
> >>  {
> >>      int res;
> >>      const char *name = GCSPRINTF("interrupt-controller@%"PRIx64, gicd_base);
> >> @@ -350,6 +362,57 @@ static int make_intc_node(libxl__gc *gc, void *fdt,
> >>      return 0;
> >>  }
> >>
> >> +static int make_gicv3_node(libxl__gc *gc, void *fdt)
> >> +{
> >> +    int res;
> >> +    const uint64_t gicd_base = GUEST_GICV3_GICD_BASE;
> >> +    const uint64_t gicd_size = GUEST_GICV3_GICD_SIZE;
> >> +    const uint64_t gicr0_base = GUEST_GICV3_GICR0_BASE;
> >> +    const uint64_t gicr0_size = GUEST_GICV3_GICR0_SIZE;
> >> +    const char *name = GCSPRINTF("interrupt-controller@%"PRIx64, gicd_base);
> >> +
> >> +    res = fdt_begin_node(fdt, name);
> >> +    if (res) return res;
> >> +
> >> +    res = fdt_property_compat(gc, fdt, 1,
> >> +                              "arm,gic-v3");
> >> +    if (res) return res;
> >> +
> >> +    res = fdt_property_cell(fdt, "#interrupt-cells", 3);
> >> +    if (res) return res;
> >> +
> >> +    res = fdt_property_cell(fdt, "#address-cells", 0);
> >> +    if (res) return res;
> >> +
> >> +    res = fdt_property(fdt, "interrupt-controller", NULL, 0);
> >> +    if (res) return res;
> >> +
> >> +    res = fdt_property_cell(fdt, "redistributor-stride",
> >> +                            GUEST_GICV3_RDIST_STRIDE);
> >> +    if (res) return res;
> >> +
> >> +    res = fdt_property_cell(fdt, "#redistributor-regions",
> >> +                            GUEST_GICV3_RDIST_REGIONS);
> >> +    if (res) return res;
> >> +
> >> +    res = fdt_property_regs(gc, fdt, ROOT_ADDRESS_CELLS, ROOT_SIZE_CELLS,
> >> +                            2,
> >> +                            gicd_base, gicd_size,
> >> +                            gicr0_base, gicr0_size);
> >> +    if (res) return res;
> >> +
> >> +    res = fdt_property_cell(fdt, "linux,phandle", PHANDLE_GIC);
> >> +    if (res) return res;
> >> +
> >> +    res = fdt_property_cell(fdt, "phandle", PHANDLE_GIC);
> >> +    if (res) return res;
> >> +
> >> +    res = fdt_end_node(fdt);
> >> +    if (res) return res;
> >> +
> >> +    return 0;
> >> +}
> >> +
> >>  static int make_timer_node(libxl__gc *gc, void *fdt, const struct arch_info *ainfo)
> >>  {
> >>      int res;
> >> @@ -456,6 +519,7 @@ int libxl__arch_domain_init_hw_description(libxl__gc *gc,
> >>                                             libxl_domain_build_info *info,
> >>                                             struct xc_dom_image *dom)
> >>  {
> >> +    xc_domain_configuration_t config;
> >>      void *fdt = NULL;
> >>      int rc, res;
> >>      size_t fdt_size = 0;
> >> @@ -471,8 +535,16 @@ int libxl__arch_domain_init_hw_description(libxl__gc *gc,
> >>      ainfo = get_arch_info(gc, dom);
> >>      if (ainfo == NULL) return ERROR_FAIL;
> >>
> >> +    LOG(DEBUG, "configure the domain");
> >> +    config.gic_version = XEN_DOMCTL_CONFIG_GIC_DEFAULT;
> >> +    if (xc_domain_configure(CTX->xch, dom->guest_domid, &config) != 0) {
> >> +        LOG(ERROR, "counldn't configure the domain");
> >> +        return ERROR_FAIL;
> >> +    }
> >> +
> >>      LOG(DEBUG, "constructing DTB for Xen version %d.%d guest",
> >>          vers->xen_version_major, vers->xen_version_minor);
> >> +    LOG(DEBUG, "  - vGIC version: %s", gicv_to_string(config.gic_version));
> >>
> >>  /*
> >>   * Call "call" handling FDR_ERR_*. Will either:
> >> @@ -520,9 +592,22 @@ next_resize:
> >>          FDT( make_psci_node(gc, fdt) );
> >>
> >>          FDT( make_memory_nodes(gc, fdt, dom) );
> >> -        FDT( make_intc_node(gc, fdt,
> >> -                            GUEST_GICD_BASE, GUEST_GICD_SIZE,
> >> -                            GUEST_GICC_BASE, GUEST_GICD_SIZE) );
> >> +
> >> +        switch (config.gic_version) {
> >> +        case XEN_DOMCTL_CONFIG_GIC_V2:
> >> +            FDT( make_gicv2_node(gc, fdt,
> >> +                                 GUEST_GICD_BASE, GUEST_GICD_SIZE,
> >> +                                 GUEST_GICC_BASE, GUEST_GICC_SIZE) );
> >> +            break;
> >> +        case XEN_DOMCTL_CONFIG_GIC_V3:
> >> +            FDT( make_gicv3_node(gc, fdt) );
> >> +            break;
> >> +        default:
> >> +            LOG(ERROR, "Unknown how to create the DT node for gic_version = %d",
> >> +                config.gic_version);
> >> +            rc = ERROR_FAIL;
> >> +            goto out;
> >> +        }
> >>
> >>          FDT( make_timer_node(gc, fdt, ainfo) );
> >>          FDT( make_hypervisor_node(gc, fdt, vers) );
> >> diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
> >> index 45974e7..5b8ff57 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,
> >> @@ -30,6 +32,39 @@ long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
> >>
> >>          return p2m_cache_flush(d, s, e);
> >>      }
> >> +    case XEN_DOMCTL_arm_configure_domain:
> >> +    {
> >> +        uint8_t gic_version;
> >> +
> >> +        /*
> >> +         * Xen 4.5: The vGIC is emulating the same version of the
> >> +         * hardware GIC. Only the value XEN_DOMCTL_CONFIG_GIC_DEFAULT
> >> +         * is allowed. The DOMCTL will return the actual version of the
> >> +         * GIC.
> >> +         */
> >> +        if ( domctl->u.configuredomain.gic_version != XEN_DOMCTL_CONFIG_GIC_DEFAULT )
> >> +            return -EOPNOTSUPP;
> >> +
> >> +        switch ( gic_hw_version() )
> >> +        {
> >> +        case GIC_V3:
> >> +            gic_version = XEN_DOMCTL_CONFIG_GIC_V3;
> >> +            break;
> >> +        case GIC_V2:
> >> +            gic_version = XEN_DOMCTL_CONFIG_GIC_V2;
> >> +            break;
> >> +        default:
> >> +            BUG();
> >> +        }
> >> +
> >> +        domctl->u.configuredomain.gic_version = gic_version;
> >> +
> >> +        /* TODO: Make the copy generic for all ARCH domctl */
> >> +        if ( __copy_to_guest(u_domctl, domctl, 1) )
> >> +            return -EFAULT;
> >> +
> >> +        return 0;
> >> +    }
> >>
> >>      default:
> >>          return subarch_do_domctl(domctl, d, u_domctl);
> >> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> >> index 91161a2..076aa62 100644
> >> --- a/xen/arch/arm/gic-v3.c
> >> +++ b/xen/arch/arm/gic-v3.c
> >> @@ -906,7 +906,21 @@ static int gicv_v3_init(struct domain *d)
> >>          d->arch.vgic.rdist_count = gicv3.rdist_count;
> >>      }
> >>      else
> >> -        d->arch.vgic.dbase = GUEST_GICD_BASE;
> >> +    {
> >> +        d->arch.vgic.dbase = GUEST_GICV3_GICD_BASE;
> >> +        d->arch.vgic.dbase_size = GUEST_GICV3_GICD_SIZE;
> >> +
> >> +        /* XXX: Only one Re-distributor region mapped for the guest */
> >> +        BUILD_BUG_ON(GUEST_GICV3_RDIST_REGIONS != 1);
> >> +
> >> +        d->arch.vgic.rdist_count = GUEST_GICV3_RDIST_REGIONS;
> >> +        d->arch.vgic.rdist_stride = GUEST_GICV3_RDIST_STRIDE;
> >> +
> >> +        /* The first redistributor should contain enough space for all CPUs */
> >> +        BUILD_BUG_ON((GUEST_GICV3_GICR0_SIZE / GUEST_GICV3_RDIST_STRIDE) < MAX_VIRT_CPUS);
> >> +        d->arch.vgic.rbase[0] = GUEST_GICV3_GICR0_BASE;
> >> +        d->arch.vgic.rbase_size[0] = GUEST_GICV3_GICR0_SIZE;
> >> +    }
> >>
> >>      d->arch.vgic.nr_lines = 0;
> >>
> >> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> >> index eecc561..d7df274 100644
> >> --- a/xen/include/public/arch-arm.h
> >> +++ b/xen/include/public/arch-arm.h
> >> @@ -373,11 +373,27 @@ typedef uint64_t xen_callback_t;
> >>   */
> >>
> >>  /* Physical Address Space */
> >> +
> >> +/* vGIC mappings: Only one set of mapping is used by the guest.
> >> + * Therefore they can overlap.
> >> + */
> >> +
> >> +/* vGIC v2 mappings */
> >>  #define GUEST_GICD_BASE   0x03001000ULL
> >>  #define GUEST_GICD_SIZE   0x00001000ULL
> >>  #define GUEST_GICC_BASE   0x03002000ULL
> >>  #define GUEST_GICC_SIZE   0x00000100ULL
> >>
> >> +/* vGIC v3 mappings */
> >> +#define GUEST_GICV3_GICD_BASE      0x03001000ULL
> >> +#define GUEST_GICV3_GICD_SIZE      0x00010000ULL
> >> +
> >> +#define GUEST_GICV3_RDIST_STRIDE   0x20000ULL
> >> +#define GUEST_GICV3_RDIST_REGIONS  1
> >> +
> >> +#define GUEST_GICV3_GICR0_BASE     0x03020000ULL    /* vCPU0 - vCPU15 */
> >> +#define GUEST_GICV3_GICR0_SIZE     0x00200000ULL
> >> +
> >>  /* 16MB == 4096 pages reserved for guest to use as a region to map its
> >>   * grant table in.
> >>   */
> >> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> >> index 58b19e7..8da204e 100644
> >> --- a/xen/include/public/domctl.h
> >> +++ b/xen/include/public/domctl.h
> >> @@ -68,6 +68,19 @@ struct xen_domctl_createdomain {
> >>  typedef struct xen_domctl_createdomain xen_domctl_createdomain_t;
> >>  DEFINE_XEN_GUEST_HANDLE(xen_domctl_createdomain_t);
> >>
> >> +#if defined(__arm__) || defined(__aarch64__)
> >> +#define XEN_DOMCTL_CONFIG_GIC_DEFAULT   0
> >> +#define XEN_DOMCTL_CONFIG_GIC_V2        1
> >> +#define XEN_DOMCTL_CONFIG_GIC_V3        2
> >> +/* XEN_DOMCTL_configure_domain */
> >> +struct xen_domctl_arm_configuredomain {
> >> +    /* IN/OUT parameters */
> >> +    uint8_t gic_version;
> >> +};
> >> +typedef struct xen_domctl_arm_configuredomain xen_domctl_arm_configuredomain_t;
> >> +DEFINE_XEN_GUEST_HANDLE(xen_domctl_arm_configuredomain_t);
> >> +#endif
> >> +
> >>  /* XEN_DOMCTL_getdomaininfo */
> >>  struct xen_domctl_getdomaininfo {
> >>      /* OUT variables. */
> >> @@ -1056,6 +1069,7 @@ struct xen_domctl {
> >>  #define XEN_DOMCTL_set_vcpu_msrs                 73
> >>  #define XEN_DOMCTL_setvnumainfo                  74
> >>  #define XEN_DOMCTL_psr_cmt_op                    75
> >> +#define XEN_DOMCTL_arm_configure_domain          76
> >>  #define XEN_DOMCTL_gdbsx_guestmemio            1000
> >>  #define XEN_DOMCTL_gdbsx_pausevcpu             1001
> >>  #define XEN_DOMCTL_gdbsx_unpausevcpu           1002
> >> @@ -1064,6 +1078,9 @@ struct xen_domctl {
> >>      domid_t  domain;
> >>      union {
> >>          struct xen_domctl_createdomain      createdomain;
> >> +#if defined(__arm__) || defined(__aarch64__)
> >> +        struct xen_domctl_arm_configuredomain configuredomain;
> >> +#endif
> >>          struct xen_domctl_getdomaininfo     getdomaininfo;
> >>          struct xen_domctl_getmemlist        getmemlist;
> >>          struct xen_domctl_getpageframeinfo  getpageframeinfo;
> >> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
> >> index 6d0fe72..846cf88 100644
> >> --- a/xen/xsm/flask/hooks.c
> >> +++ b/xen/xsm/flask/hooks.c
> >> @@ -727,6 +727,9 @@ static int flask_domctl(struct domain *d, int cmd)
> >>      case XEN_DOMCTL_psr_cmt_op:
> >>          return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__PSR_CMT_OP);
> >>
> >> +    case XEN_DOMCTL_configure_domain:
> >> +        return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__CONFIGURE_DOMAIN);
> >> +
> >>      default:
> >>          printk("flask_domctl: Unknown op %d\n", cmd);
> >>          return -EPERM;
> >> diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors
> >> index de0c707..bfe2fa5 100644
> >> --- a/xen/xsm/flask/policy/access_vectors
> >> +++ b/xen/xsm/flask/policy/access_vectors
> >> @@ -216,6 +216,8 @@ class domain2
> >>      get_vnumainfo
> >>  # XEN_DOMCTL_psr_cmt_op
> >>      psr_cmt_op
> >> +# XEN_DOMCTL_configure_domain
> >> +    configure_domain
> >>  }
> >>
> >>  # Similar to class domain, but primarily contains domctls related to HVM domains
> >> --
> >> 1.7.10.4
> >>
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel