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

[Julien Grall <julien.grall@xxxxxxxxxx>]

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