Re: [Xen-devel] [PATCH v7 21/28] xen/arm: ITS: Add GICR register emulation

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

On Mon, Sep 28, 2015 at 4:33 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> On 28/09/15 11:37, Vijay Kilari wrote:
>> On Mon, Sep 28, 2015 at 3:23 PM, Ian Campbell <ian.campbell@xxxxxxxxxx> 
>> wrote:
>>> On Sat, 2015-09-26 at 21:38 +0530, Vijay Kilari wrote:
>>>
>>>>  LPI property table is accessed in interrupt context. So interrupts
>>>> are disabled.
>>>
>>> Interrupts a reenabled while handling an interrupt, so a higher priority
>>> interrupt can still interrupt things.
>>>
>>> See the use of local_irq_enable in gic_interrupt.
>>
>> true, for this reason, I have taken spin lock with disabled
>> interrrupts when reading guest LPI table.
>>
>> Question is to avoid disabling interrupts for a  long time for about 8K/16K 
>> lpis
>> state update.
>
> Why only 8K/16K LPIs? From the ITS spec, the number of LPIs is encoded
> on 32bit so it would be possible to have 500K LPIs...
>
>> The LPI state update from guest LPI property table is done when guest
>> updates GICR_CTLR.EnableLPIs to 1.
>> (I am doing this in next revision).
>>
>> So we can make a check for guest's GICR_CTLR.EnableLPIs field before
>> injecting LPIs to the domain. As per design
>> GICR_CTLR.EnableLPIs should be 0 when updating guest property table.
>> i.e updating GICR_PROPBASER.
>
> GICR_CTLR is per vCPU and the property table is per domain. How would
> you know when to parse the property table?

property table is shared across all vCPUs. So we can parse the property table
on update of GICR_CTLR for vCPU0.

>
>> So that we don't need to disable interrupts(LPIs are already disabled)
>> while reading guest LPI table.
>
> That won't help. As already said on a previous mail, it was for command
> emulation, the vCPU will monopolize the physical CPU for a very long
> time. That means that no other vCPU can run this physical CPU.
>
> I think the best solution would be tasklet which will divide the parsing
> in small chunk. This would avoid to get Xen stuck on the physical CPU
> for a long time.
>
> A tasklet would be created when GICR_PROPBASER is written. Any vCPU
> sending an INV command would get block until the tasklet is finished.

Below is the psuedo code for handling LPI property table. Please
let me know if the locking mechanism is ok or not.

On GICR_CTLR update:
----------------------------------
case GICR_CTLR:
...

if ( v->vcpu_id == 0 && v->arch.vgic.gicr_ctlr && propbase == NULL )
{

    spin_lock_irqsave(&vgic.lpi_enable_lock, flags);
    vgic.enableLPIs = 0;
    spin_unlock_irqrestore(&vgic.lpi_enable_lock, flags);

    spin_lock_irqsave(&vgic.prop_lock, flags);

    // Allocate memory for property table
    // Copy guest LPI prop table to Xen table.
    // Unmap guest table.
    // Register mmio handlers.

    spin_unlock_irqrestore(&vgic.prop_lock, flags);

    schedule_tasklet();
    return 1;
}

....

In Tasklet function
--------------------------

spin_lock(&vgic.prop_lock);

    for ( i = 0 ; i < nr_lpis; i++ )
    {
        enable = prop_table[i] & LPI_PROP_ENABLED;
...
        if ( !enable )
            vgic_v3_enable_lpi(v, i + 8192);
        else
            vgic_v3_disable_lpi(v, i + 8192);
...
    }

spin_unlock(&vgic.prop_lock);

spin_lock_irqsave(&vgic.lpi_enable_lock, flags);
vgic.enableLPIs = 1;
spin_unlock_irqrestore(&vgic.lpi_enable_lock, flags);

In vits_process_inv()
------------------------------

done = 0;

do {
    spin_lock_irqsave(&vgic.lpi_enable_lock, flags);
    if ( vgic.enableLPIs )
        done = 1;
    spin_unlock_irqrestore(&vgic.lpi_enable_lock, flags);
    cpu_relax();
} while ( !done );

In vgic_vcpu_inject_irq()
----------------------------------
...

spin_lock_irqsave(&vgic.lpi_enable_lock, flags);
lpi_state = vgic.enableLPIs;
spin_unlock_irqrestore(&vgic.lpi_enable_lock, flags);

/*
 * This function is called from interrupt context. For lpis,
get_irq_priority() callback
 * takes prop_lock to read priority from lpi property table.
 * So we take default priority if property table is under update by
tasklet (vgic.enableLPIs == 0).
 */

if ( vgic_is_domain_lpi(d, lpi) && !lpi_state )
{
    /*
     * LPI is received for this domain, while LPIs property table
     * is not updated, set irq with default priority of LPI. Priority
is needed to enqueue this irq.
     */

    priority = DEFAULT_LPI_PRIORITY; //0xa2?
}
else

    priority = vgic_ops->get_irq_priority();

if ( vgic_is_domain_lpi(d, lpi) )
{
   if ( vgic.enableLPIs && test_bit(GIC_IRQ_GUEST_ENABLED, &n->status) )
       gic_raise_guest_irq(v, virq, priority);
}
else
{
   if ( test_bit(GIC_IRQ_GUEST_ENABLED, &n->status) )
       gic_raise_guest_irq(v, virq, priority);
}

...

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