Re: [Xen-devel] [PATCH v4 11/17] xen/arm: ITS: Add GICR register emulation

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Vijay,

On 16/07/2015 16:15, Vijay Kilari wrote:
> On Wed, Jul 15, 2015 at 11:02 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> > On 10/07/2015 09:42, vijay.kilari@xxxxxxxxx wrote:
> > > From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
> > >
> > > Emulate LPI related changes to GICR registers
> [..]
> > > +    p = irq_to_pending(v, vlpi);
> > > +
> > > +    set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
> > > +
> > > +    spin_lock_irqsave(&v->arch.vgic.lock, flags);
> > > +
> > > +    /*XXX: raise on right vcpu */
> >
> > /* XXX: ... */
> >
> > I guess you added this comment because I mentioned possible locking problem
> > here?
>     I meant to find out Collection (VCPU) for this vlpi and inject on that VCPU
I'm nearly sure there is locking problem if you don't do it know.

> > If so, did you think how this would affect the vLPI handling to not do the
> > correct locking for Xen 4.6?
> >
> > Note for the others: AFAICT the locking of the pending_irq structure is done
> > using the lock of the vCPU where the IRQ is routed. Stefano, please confirm
> > if it's true.
[...]


> > > +
> > > +    spin_lock(&v->domain->arch.vits->prop_lock);
> >
> > I don't understand why you need to take the prop_lock here. You only use
> > them in the handler which you don't have yet registered.
>      lock is taken as property table can be changed. However as I am
> not handling change in property table in this series, It can be dropped.
>
> > > +    if ( id_bits > gic_nr_id_bits() )
> > > +        id_bits = gic_nr_id_bits();
> >
> > What happen if the property configuration table is smaller?
> Spec does not say anything about it
So you have to handle it properly to avoid the helper reading out of the 
LPI configuration table.
Regards,

--
Julien Grall

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