Re: [RFC PATCH v2 13/22] xen/arm: its: Add virtual ITS command support

[Julien Grall <julien.grall@linaro.org>]

On 30/03/15 16:02, Vijay Kilari wrote:
> Hi Julien,

Hello Vijay,

> On Tue, Mar 24, 2015 at 5:18 PM, Julien Grall <julien.grall@linaro.org> wrote:
>> On 19/03/2015 14:38, vijay.kilari@gmail.com wrote:
>>>
>>> +int vgic_its_process_cmd(struct vcpu *v, struct vgic_its *vits)
>>> +{
>>> +    struct its_cmd_block virt_cmd;
>>> +
>>> +    /* XXX: Currently we are processing one cmd at a time */
>>> +    ASSERT(spin_is_locked(&vits->lock));
>>> +
>>> +    do {
>>> +        if ( vgic_its_read_virt_cmd(v, vits, &virt_cmd) )
>>> +            goto err;
>>> +        if ( vgic_its_parse_its_command(v, vits, &virt_cmd) )
>>> +            goto err;
>>> +    } while ( vits->cmd_write != vits->cmd_write_save );
>>> +
>>> +    vits->cmd_write_save = vits->cmd_write;
>>> +    DPRINTK("vITS: write_save 0x%lx write 0x%lx \n",
>>> +            vits->cmd_write_save,
>>> +            vits->cmd_write);
>>> +    /* XXX: Currently we are processing one cmd at a time */
>>> +    vgic_its_update_read_ptr(v, vits);
>>
>>
>> From the spec the GITS_CREADR should be updated at every command processing.
>> That would make cmd_write_save pointless.
> 
> See notes under section 4.9.9 Adding New Commands to the Queue
> Multiple commands can be written to a queue at once.

You didn't understand my point.

The steps to process a command are:

1)   read command
2)   handle command
3)   increment CREADR
4)   loop to 1 if another command to process

Currently, you only do the step 3 when all commands are processed.

>>
>> Also, you are taking the VITS lock for the whole process. This process can
>> be very long. How will it affect the other vCPUs of the domain?
>>
> 
> Yes, lock is taken on first command trap and holds until all commands
> are processed.
> In any case ITS commands are processed in synchronously. So any VCPU that
> send ITS commands is blocked.

This is wrong. The command processing is an asynchronous process and can
be long.

A VCPU may want to do other things (like handling interrupt) while the
ITS is processing.

With your implementation you rule out this possibility.

> Also ITS commands are sent while setting up device/irq and while
> releasing device/irq.
> So there should not be any overhead when device is under use.

Ok.

>> Finally, in environment with multiple guests using ITS, the ITS command send
>> to the physical ITS may be interleaved (i.e DOM1 cmd, DOM2 cmd, DOM1 cmd
>> ...). Is there any possible side-effect?
> 
> Each command is independent.  Generally SYNC/INV is followed after some
> commands. But it should not be a problem if they are interleaved.

What happen if the guest decide to not send the SYNC/INV? What is the
state of the ITS in this case? Would it be possible to receive a wrong LPIs?

Regards,

-- 
Julien Grall