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

[Julien Grall <julien.grall@citrix.com>]

Hi Vijay,

On 04/05/2015 14:27, Vijay Kilari wrote:
> On Mon, May 4, 2015 at 6:34 PM, Julien Grall <julien.grall@citrix.com> wrote:
>>
>>
>> On 04/05/2015 13:58, Vijay Kilari wrote:
>>>
>>> On Thu, Apr 30, 2015 at 7:59 PM, Julien Grall <julien.grall@citrix.com>
>>> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 30/04/15 14:47, Stefano Stabellini wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> If the devid is not within this range, the ITS won't recognize the
>>>>>>>> value and
>>>>>>>> won't be able to send the interrupt.
>>>>>>>>
>>>>>>>> So this is clearly not the right value.
>>>>>>>
>>>>>>>
>>>>>>> Sure, in that case the maximum value allowed by GITS_TYPER.Devbits.
>>>>>>> Vijay, what is the value of GITS_TYPER.Devbits on your platform?
>>>>>>
>>>>>>
>>>>>> It is 21 bits
>>>>>
>>>>>
>>>>> I would imagine that 21 bits would be plenty to find an unused devid.
>>>>>
>>>>> Alternatively we could use an inexistent function of a real device, such
>>>>> as 00:00.1 (function 1 of the host bridge).
>>>>
>>>>
>>>> As discussed IRL, this idea sounds good to me.
>>>>
>>>> Although I would be happy with any other way which ensure the devid is
>>>> free.
>>>
>>>
>>> Has prototyped with 00.00.1 as device id. But I see that Dom0 boot is
>>> slow compared to polling mode. This could be because Dom0 has to keep
>>> trapping on creader to check if creader is updated or not.
>>
>>
>> How did you implement the interrupt mode? Could it be improve?
>
>     1) In physical ITS driver its_device is created with devID 00:00.1 with
> 256 MSI-x are reserved and is named as completion_dev, which is global.

That's a lot of MSI-x reserved... Can't you use only one per domain?

>     2) In Domain init,
>           - one irq (called completion_irq) is allocated per domain for
> this device

So only 256 domain can run on your platform at the same time?

>             and irq desc is allocated to this domain. This way we can get vITS
>             context when interrupt is received.
>           - An array of 32 requests(its_requests) is allocated which stores all
>             the information about the ITS commands that are converted and written
>             to physical ITS queue and each request info contains
>             [CReader vITS] [CWriter vITS] [Physical Q Index] [Number of commands]
>              [Completion irq] [ status ]

Why 32 requests?

Also some of the fields don't make much sense to me such as "Number of 
Commands" , "Completion IRQ"  and "status" I guess I will find out when 
you will send the new series.

>     3) When vITS received ITS command. This command is converted to physical
>        command  and written to physical queue, INT command is appended with
>        completion_irq and entry is made in its_requests.
>     4) On receiving completion_irq, vITS structure and its_requests
> info is parsed
>         and Creader of vITS is upstated with [ Cwriter vITS ] stored
> for this request
>         and this request is removed from its_requests.

You complicate the code by allowing 32 batch of command per domain 
(looping is very slow). You should only allow one batch of command per 
domain. When the batch is finished you can send another one.

>   I am adding one INT per command. This can be improved to add one INT
> cmd for all
>   the pending commands. Existing Linux driver sends 2 commands at a time.

You should not assume that other OS will send 2 commands at the same 
time... It could be more or less.

Although, having a INT per command is rather slow. One INT command per 
batch would improve the boot time.

Regards,

-- 
Julien Grall