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

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

On Wed, Jul 15, 2015 at 11:02 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> Hi Vijay,
>
> 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

>
> 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.
>
>> +    if ( !list_empty(&p->inflight) &&
>> +         !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
>> +        gic_raise_guest_irq(v, irq_to_virq(p->desc), p->priority);
>> +
>> +    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
>> +}
>> +

[...]

>
>> +    {
>> +        DPRINTK("%pv: vITS: LPI Table read offset 0x%x\n", v, offset);
>> +        spin_lock(&v->domain->arch.vits->prop_lock);
>> +        *r = *((u8*)v->domain->arch.vits->prop_page + offset);
>
>
> You didn't answer my question on v3... I.e "what about other access? a
> 64/32/16 bits access are valid and will return the wrong value."

  One byte represents one LPI in the configuration table. This table being in
memory, guest can access 16/32/64. I will make a check on access size and
handle accordingly.

>> +
>> +    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

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