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

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Wed, Oct 29, 2014 at 04:37:00PM +0000, Julien Grall wrote:
> On 10/29/2014 04:34 PM, Vijay Kilari wrote:
> > Hi Stefano,
> > 
> > On Wed, Oct 29, 2014 at 9:43 PM, Stefano Stabellini
> > <stefano.stabellini@xxxxxxxxxxxxx> wrote:
> >> FYI we talked f2f about this and we reached the conclusion that we
> >> should rework this patch to be non-intrusive and acceptable for 4.5.
> >> After all without it we cannot boot guests on GICv3 systems.
> > 
> >    I am on travel for rest of the week. Is it ok if I look at it next week?
> > Can we plan for 4.5 RC2?
> 
> That's ok. I can rework the patch for you.

RC2 is Nov 12.
> 
> Regards,
> 
> >>
> >> On Tue, 28 Oct 2014, Julien Grall wrote:
> >>> 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
> >>>
> 
> 
> -- 
> Julien Grall

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