From mboxrd@z Thu Jan  1 00:00:00 1970
From: Julien Grall <julien.grall@citrix.com>
Subject: Re: [PATCH v4 11/17] xen/arm: ITS: Add GICR register
 emulation
Date: Thu, 16 Jul 2015 16:41:46 +0200
Message-ID: <55A7C2AA.8090805@citrix.com>
References: <1436514172-3263-1-git-send-email-vijay.kilari@gmail.com>
	<1436514172-3263-12-git-send-email-vijay.kilari@gmail.com>
	<55A69925.9070408@citrix.com>
	<CALicx6vp0gjzE5vy5R_i8FqHGTFEq+kydA=e4L+bxMYtpw2rQg@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <CALicx6vp0gjzE5vy5R_i8FqHGTFEq+kydA=e4L+bxMYtpw2rQg@mail.gmail.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Vijay Kilari <vijay.kilari@gmail.com>
Cc: Ian Campbell <Ian.Campbell@citrix.com>, Stefano Stabellini <stefano.stabellini@eu.citrix.com>, Prasun Kapoor <Prasun.Kapoor@caviumnetworks.com>, Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>, Tim Deegan <tim@xen.org>, "xen-devel@lists.xen.org" <xen-devel@lists.xen.org>, Stefano Stabellini <stefano.stabellini@citrix.com>, manish.jaggi@caviumnetworks.com
List-Id: xen-devel@lists.xenproject.org

Hi Vijay,

On 16/07/2015 16:15, Vijay Kilari wrote:
> On Wed, Jul 15, 2015 at 11:02 PM, Julien Grall <julien.grall@citrix.com> wrote:
>> On 10/07/2015 09:42, vijay.kilari@gmail.com wrote:
>>>
>>> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>>>
>>> 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