| Message ID | 20230919112827.1001484-3-volodymyr_babchuk@epam.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM: ITS: implement TODO and fix issues with cache | expand |
Hi Volodymyr, On 19/09/2023 12:28, Volodymyr Babchuk wrote: > There is no need to invalidate cache entry because we just wrote into a > memory region. Writing itself guarantees that cache entry is valid. The goal of invalidate is to remove the line from the cache. So I don't quite understand the reasoning here. So I am a little hesitant to remove the invalidate without some justification based on the specification. Cheers,
Hi Julien, Julien Grall <julien@xen.org> writes: > Hi Volodymyr, > > On 19/09/2023 12:28, Volodymyr Babchuk wrote: >> There is no need to invalidate cache entry because we just wrote into a >> memory region. Writing itself guarantees that cache entry is valid. > > The goal of invalidate is to remove the line from the cache. So I > don't quite understand the reasoning here. > Well, I may be wrong, but what is the goal in removing line from the cache? As I see this, we want to be sure that ITS sees data written in the memory, so we should flush a cache line. But why do we need to remove it from CPU's cache?
On 19/09/2023 15:36, Volodymyr Babchuk wrote: > Julien Grall <julien@xen.org> writes: >> On 19/09/2023 12:28, Volodymyr Babchuk wrote: >>> There is no need to invalidate cache entry because we just wrote into a >>> memory region. Writing itself guarantees that cache entry is valid. >> >> The goal of invalidate is to remove the line from the cache. So I >> don't quite understand the reasoning here. >> > > Well, I may be wrong, but what is the goal in removing line from the > cache? As I see this, we want to be sure that ITS sees data written in > the memory, so we should flush a cache line. But why do we need to > remove it from CPU's cache? I don't exactly know. From a brief look I agree with you. However, our driver is based on Linux where the clean & invalidate is also used. So I am a little be cautious to remove it. The way forward would be to ask the Marc Zyngier (GICv3 maintainer) why it was added in Linux. Then we can decide whether this can be removed in Xen. Cheers,
Hi Mark, I am writing to you, because you are GICv3 maintainer in Linux. We are updating ITS driver in Xen and we have a question about cache maintenance WRT memory shared with ITS. As I can see, the Linux ITS driver uses gic_flush_dcache_to_poc() all over the code. This boils down to "dc civac" instruction which does both clean and invalidate. But do we really need to invalidate a cache when we are sending an ITS command? In my understanding it is sufficient to clean a cache only and Linux uses clean&invalidate just out of convenience. Is this correct? Below you can find our discussion about this. Julien Grall <julien@xen.org> writes: > On 19/09/2023 15:36, Volodymyr Babchuk wrote: >> Julien Grall <julien@xen.org> writes: >>> On 19/09/2023 12:28, Volodymyr Babchuk wrote: >>>> There is no need to invalidate cache entry because we just wrote into a >>>> memory region. Writing itself guarantees that cache entry is valid. >>> >>> The goal of invalidate is to remove the line from the cache. So I >>> don't quite understand the reasoning here. >>> >> Well, I may be wrong, but what is the goal in removing line from the >> cache? As I see this, we want to be sure that ITS sees data written in >> the memory, so we should flush a cache line. But why do we need to >> remove it from CPU's cache? > > I don't exactly know. From a brief look I agree with you. However, our > driver is based on Linux where the clean & invalidate is also used. So > I am a little be cautious to remove it. > > The way forward would be to ask the Marc Zyngier (GICv3 maintainer) > why it was added in Linux. Then we can decide whether this can be > removed in Xen. > > Cheers,
Volodymyr, On Fri, 22 Sep 2023 01:22:11 +0100, Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> wrote: > > > Hi Mark, s/k/c/ > > I am writing to you, because you are GICv3 maintainer in Linux. We are > updating ITS driver in Xen and we have a question about cache > maintenance WRT memory shared with ITS. As I can see, the Linux ITS > driver uses gic_flush_dcache_to_poc() all over the code. This boils down > to "dc civac" instruction which does both clean and invalidate. But do > we really need to invalidate a cache when we are sending an ITS command? > In my understanding it is sufficient to clean a cache only and Linux > uses clean&invalidate just out of convenience. Is this correct? It really depends how you look at it. We use DC CIVA as the standard way to give a buffer to a device, as that's what the DMA API does. Switching to a simple clean is possible, but I don't really see what it brings you. ITS commands are usually written as a single command followed by a SYNC/VSYNC. That's a total a 8 64bit words, which makes a cache line on 99.999% of the implementations. What do you gain by keeping the cache line around? Not much. By the time you go around the command queue and need the same data again, it will have been evicted from your L1 already. So while I don't see a problem with what you are suggesting, I also think the change is pretty much irrelevant. HTH, M.
Hello Marc, Marc Zyngier <maz@kernel.org> writes: > Volodymyr, > > On Fri, 22 Sep 2023 01:22:11 +0100, > Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> wrote: >> >> >> Hi Mark, > > s/k/c/ Oh, I'm sorry. >> >> I am writing to you, because you are GICv3 maintainer in Linux. We are >> updating ITS driver in Xen and we have a question about cache >> maintenance WRT memory shared with ITS. As I can see, the Linux ITS >> driver uses gic_flush_dcache_to_poc() all over the code. This boils down >> to "dc civac" instruction which does both clean and invalidate. But do >> we really need to invalidate a cache when we are sending an ITS command? >> In my understanding it is sufficient to clean a cache only and Linux >> uses clean&invalidate just out of convenience. Is this correct? > > It really depends how you look at it. We use DC CIVA as the standard > way to give a buffer to a device, as that's what the DMA API > does. Switching to a simple clean is possible, but I don't really see > what it brings you. > > ITS commands are usually written as a single command followed by a > SYNC/VSYNC. That's a total a 8 64bit words, which makes a cache line > on 99.999% of the implementations. > > What do you gain by keeping the cache line around? Not much. By the > time you go around the command queue and need the same data again, it > will have been evicted from your L1 already. This is a great point. Thank you.
diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c index a9c971a55f..72cf318810 100644 --- a/xen/arch/arm/gic-v3-its.c +++ b/xen/arch/arm/gic-v3-its.c @@ -108,8 +108,7 @@ static int its_send_command(struct host_its *hw_its, const void *its_cmd) memcpy(hw_its->cmd_buf + writep, its_cmd, ITS_CMD_SIZE); if ( hw_its->flags & HOST_ITS_FLUSH_CMD_QUEUE ) - clean_and_invalidate_dcache_va_range(hw_its->cmd_buf + writep, - ITS_CMD_SIZE); + clean_dcache_va_range(hw_its->cmd_buf + writep, ITS_CMD_SIZE); else dsb(ishst);
There is no need to invalidate cache entry because we just wrote into a memory region. Writing itself guarantees that cache entry is valid. But we still need to flush cache line to be sure that ITS sees a command written into a queue. Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> --- xen/arch/arm/gic-v3-its.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)