[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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |