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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Fri, 31 Oct 2014, Julien Grall wrote:
> Hi Stefano,
> 
> On 31/10/2014 17:45, Stefano Stabellini wrote:
> > >   static int make_timer_node(libxl__gc *gc, void *fdt, const struct
> > > arch_info *ainfo)
> > >   {
> > >       int res;
> > > @@ -454,6 +539,7 @@ out:
> > > 
> > >   int libxl__arch_domain_init_hw_description(libxl__gc *gc,
> > >                                              libxl_domain_build_info
> > > *info,
> > > +                                           libxl__domain_build_state
> > > *state,
> > >                                              struct xc_dom_image *dom)
> > >   {
> > >       void *fdt = NULL;
> > 
> > Can't you just call xc_configure_domain from here? Why do you need to
> > introduce libxl__arch_domain_create_pre?
> 
> The DOMCTL was initially created to defer the VGIC initialization which has to
> be done before the toolstack set the number of VCPUs.
> 
> Even though, the current implementation of the DOMCTL only returns the version
> of the VGIC, I think it's still better to call it in the right place for now.
> 
> I don't really want to justify on the latter patch why we have to move the
> call earlier.

Each patch needs to be able to stand on its own.
If you'll have valid reasons to introduce libxl__arch_domain_create_pre
in the future, you will, otherwise you won't. It doesn't look like you
have valid reasons here.

Moreover given that we are close to RC2, this patch needs to be as
simple as possible. Getting rid of libxl__arch_domain_create_pre would
certainly make it easier. You could also remove the change
to libxl__domain_build_state.


> > >   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_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 -EINVAL;
> > 
> > -EOPNOTSUPP is a better choice I think
> 
> I will use it in the next version.
> 
> > > 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);
> > 
> > I think that the comment is enough: GUEST_GICV3_RDIST_REGIONS is already
> > #define to be 1.
> 
> I would prefer to keep this compilation-time check.
> 
> If someone decides to bump the number of re-distributor without implementing
> the hyp side, it will be able to know quickly.
> 
> > > +        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/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> > > index 787e93c..4b81e70 100644
> > > --- a/xen/include/asm-arm/domain.h
> > > +++ b/xen/include/asm-arm/domain.h
> > > @@ -47,7 +47,6 @@ struct arch_domain
> > >   #ifdef CONFIG_ARM_64
> > >       enum domain_type type;
> > >   #endif
> > > -
> > >       /* Virtual MMU */
> > >       struct p2m_domain p2m;
> > >       uint64_t vttbr;
> > 
> > spurious change
> 
> Oops, it was a leftover after melting the 2 patches. I will drop it in the
> next version.
> 
> Regards,
> 
> -- 
> Julien Grall
> 

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