Re: [PATCH v2 05/27] ARM: GICv3 ITS: introduce ITS command handling

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

Hi Andre,

On 03/04/17 11:58, Andre Przywara wrote:
>>> +#define BUFPTR_MASK                     GENMASK(19, 5)
>>> +static int its_send_command(struct host_its *hw_its, const void
>>> *its_cmd)
>>> +{
>>> +    s_time_t deadline = NOW() + MILLISECS(1);
>>
>> It would be nice to explain where does this value comes from.
>
> In lack of any real data on this, one millisecond is just a guess.
> In v3 I added some comment stating that this is a "short" period to wait
> for. We don't want to stall for a long time for a single command, but
> also don't want to give up too easily. The command queue is handled by
> the ITS, which is hardware, so the only thing it does is handling
> commands. If we are unlucky enough to hit a full command queue due to a
> sudden spike in host commands, chances are that we get a free slot very
> soon afterwards. If not, we are screwed anyway.

It is not the first time I am saying that. If it is a guess, then this 
should be spelled out. A reader cannot know and we would have a question 
asking why "1 ms in Xen and 1 sec in Linux"...

>>> +/* Wait for an ITS to finish processing all commands. */
>>> +static int gicv3_its_wait_commands(struct host_its *hw_its)
>>> +{
>>> +    s_time_t deadline = NOW() + MILLISECS(100);
>>
>> Same remark as above. Why 1ms and 100ms here?
>
> I made this period longer, because it covers multiple commands and we
> are somewhat expected to wait here. Still it should be shorter than the
> Linux timeout, which is one second.

See above.

[...]

>>
>>> +/*
>>> + * Before an ITS gets initialized, it should be in a quiescent state,
>>> where
>>> + * all outstanding commands and transactions have finished.
>>> + * So if the ITS is already enabled, turn it off and wait for all
>>> outstanding
>>> + * operations to get processed by polling the QUIESCENT bit.
>>> + */
>>> +static int gicv3_disable_its(struct host_its *hw_its)
>>> +{
>>> +    uint32_t reg;
>>> +    s_time_t deadline = NOW() + MILLISECS(100);
>>
>> Why 100ms?
>
> Same rationale as above: We are dealing with multiple commands here,
> also are expected to take a longer time.

See above.

[...]

>>> +
>>> +                    /*
>>> +                     * The ITS refers to redistributors either by
>>> their physical
>>> +                     * address or by their ID. Determine those two
>>> values and
>>> +                     * let the ITS code store them in per host CPU
>>> variables to
>>> +                     * later be able to address those redistributors.
>>> +                     */
>>
>> Again, this comment does not look useful and is misleading as the code
>> to get/set the redistributor information is living in gic-v3-lpi.c and
>> not gic-v3-its.c.
>
> But we need to prepare those values here (and only here) and it does
> make a lot of sense to explain this, since the ability to deal with both
> ways of representing a redistributor is not obvious.
> And this is one of the connections between the ITS and the (GICv3)
> redistributors, where the line between them is not easy to draw.
> So in v4 I now reworded the comment to say that we hand this over to the
> ITS (instead of spoiling ITS implementation details).
> Hope you are OK with this.

Well, it makes sense to initialize the redistributor address in here and 
not in another place.

This kind of comments is more useful on the place where you initialize 
the ITS to explain why it is done there and not before.

Cheers,

-- 
Julien Grall

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