Re: [Xen-devel] [PATCH v6 11/36] ARM: GICv3: introduce separate pending_irq structs for LPIs

[Julien Grall <julien.grall@xxxxxxx>]

Hi Stefano,

On 04/07/2017 07:49 PM, Stefano Stabellini wrote:
> On Fri, 7 Apr 2017, Andre Przywara wrote:
> > For the same reason that allocating a struct irq_desc for each
> > possible LPI is not an option, having a struct pending_irq for each LPI
> > is also not feasible. We only care about mapped LPIs, so we can get away
> > with having struct pending_irq's only for them.
> > Maintain a radix tree per domain where we drop the pointer to the
> > respective pending_irq. The index used is the virtual LPI number.
> > The memory for the actual structures has been allocated already per
> > device at device mapping time.
> > Teach the existing VGIC functions to find the right pointer when being
> > given a virtual LPI number.
> >
> > Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> > ---
> >  xen/arch/arm/vgic-v2.c       |  8 ++++++++
> >  xen/arch/arm/vgic-v3.c       | 23 +++++++++++++++++++++++
> >  xen/arch/arm/vgic.c          |  2 ++
> >  xen/include/asm-arm/domain.h |  2 ++
> >  xen/include/asm-arm/vgic.h   |  2 ++
> >  5 files changed, 37 insertions(+)
> >
> > diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> > index dc9f95b..0587569 100644
> > --- a/xen/arch/arm/vgic-v2.c
> > +++ b/xen/arch/arm/vgic-v2.c
> > @@ -702,10 +702,18 @@ static void vgic_v2_domain_free(struct domain *d)
> >      /* Nothing to be cleanup for this driver */
> >  }
> >
> > +static struct pending_irq *vgic_v2_lpi_to_pending(struct domain *d,
> > +                                                  unsigned int vlpi)
> > +{
> > +    /* Dummy function, no LPIs on a VGICv2. */
> > +    BUG();
> > +}
> > +
> >  static const struct vgic_ops vgic_v2_ops = {
> >      .vcpu_init   = vgic_v2_vcpu_init,
> >      .domain_init = vgic_v2_domain_init,
> >      .domain_free = vgic_v2_domain_free,
> > +    .lpi_to_pending = vgic_v2_lpi_to_pending,
> >      .max_vcpus = 8,
> >  };
> >
> > diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> > index d10757a..f25125e 100644
> > --- a/xen/arch/arm/vgic-v3.c
> > +++ b/xen/arch/arm/vgic-v3.c
> > @@ -1451,6 +1451,9 @@ static int vgic_v3_domain_init(struct domain *d)
> >      d->arch.vgic.nr_regions = rdist_count;
> >      d->arch.vgic.rdist_regions = rdist_regions;
> >
> > +    rwlock_init(&d->arch.vgic.pend_lpi_tree_lock);
> > +    radix_tree_init(&d->arch.vgic.pend_lpi_tree);
> > +
> >      /*
> >       * Domain 0 gets the hardware address.
> >       * Guests get the virtual platform layout.
> > @@ -1528,14 +1531,34 @@ static int vgic_v3_domain_init(struct domain *d)
> >  static void vgic_v3_domain_free(struct domain *d)
> >  {
> >      vgic_v3_its_free_domain(d);
> > +    radix_tree_destroy(&d->arch.vgic.pend_lpi_tree, NULL);
> >      xfree(d->arch.vgic.rdist_regions);
> Who is freeing the pend_irqs array? Do we expect
> gicv3_its_map_guest_device(!valid) to be called for each assigned device
> at domain destroy time somehow? Today gicv3_its_map_guest_device is only
> called in response of a MAPD command, which comes from the guest, while
> we need to guarantee that we free data structures appropriately
> independently from guest behavior.
I will expect gicv3_its_map_guest_device(!valid) to be called by the 
passthrough subsystem. There are an ASSERT(...) in 
vgic_v3_its_domain_destroy (see patch #9) to check that assumption.
Cheers,

--
Julien Grall

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