[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.