Re: [PATCH v10 11/32] ARM: GICv3: forward pending LPIs to guests

From: Stefano Stabellini <sstabellini@kernel.org>
To: Julien Grall <julien.grall@arm.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Vijay Kilari <vijay.kilari@gmail.com>,
	Andre Przywara <andre.przywara@arm.com>,
	Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>,
	xen-devel@lists.xenproject.org,
	Shanker Donthineni <shankerd@codeaurora.org>
Subject: Re: [PATCH v10 11/32] ARM: GICv3: forward pending LPIs to guests
Date: Wed, 31 May 2017 10:56:41 -0700 (PDT)	[thread overview]
Message-ID: <alpine.DEB.2.10.1705311054040.18759@sstabellini-ThinkPad-X260> (raw)
In-Reply-To: <dc411b80-7e35-ee24-003f-5bad5bdd9723@arm.com>

On Wed, 31 May 2017, Julien Grall wrote:
> Hi Stefano,
> 
> On 30/05/17 23:07, Stefano Stabellini wrote:
> > On Tue, 30 May 2017, Julien Grall wrote:
> > > >      };
> > > >  };
> > > > 
> > > > @@ -136,6 +135,80 @@ uint64_t gicv3_get_redist_address(unsigned int cpu,
> > > > bool use_pta)
> > > >          return per_cpu(lpi_redist, cpu).redist_id << 16;
> > > >  }
> > > > 
> > > > +static void vgic_vcpu_inject_lpi(struct domain *d, unsigned int virq)
> > > > +{
> > > > +    struct pending_irq *p = irq_to_pending(d->vcpu[0], virq);
> > > > +    struct vcpu *v = NULL;
> > > > +
> > > > +    if ( !p )
> > > > +        return;
> > > > +
> > > > +    if ( p->lpi_vcpu_id < d->max_vcpus )
> > > > +        v = d->vcpu[read_atomic(&p->lpi_vcpu_id)];
> > > 
> > > Hmmm, what does prevent lpi_vcpu_id to change between the check and the
> > > read?
> > 
> > Supposedly we are going to set lpi_vcpu_id only to good values? Meaning
> > that we are going to do the lpi_vcpu_id checks at the time of setting
> > lpi_vcpu_id. Thus, even if lpi_vcpu_id changes, it is not a problem. In
> > fact, if that is true, can we even drop the if ( p->lpi_vcpu_id <
> > d->max_vcpus ) test here?
> 
> I am not too confident to say lpi_vcpu_id will always be valid with the
> current locking in the vGIC.

Fair enough, but I wouldn't solve a bug with unnecessary code soon to be
removed. The problem lies elsewhere and should be fixed elsewhere. We
could even add an ASSERT(p->lpi_vcpu_id < d->max_vcpus) here, would
that work for you?

> There is a potential race between its_discard_event and this function. The
> former may reset pending_irq whilst reading lpi_vcpu_id as we cannot take the
> vCPU lock yet.
> 
> But all of this is racy anyway because of the locking. This will get solved by
> the vGIC rework after the merge.
> 
> So For the time being I would keep the check. We can revisit it later if
> necessary.

This is one of those things that are unimportant because they work
either way.

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