linux-kernel - Re: [RFC 1/5] RISC-V: Implement arch_sync_dma* functions

Open Source and information security mailing list archives

Message-ID: <CAOnJCUJFOZsqC2F9sqJqYw5bLJK0fQS+w=Rp6PeM2ih=75Cxag@mail.gmail.com>
Date:   Mon, 26 Jul 2021 14:52:59 -0700
From:   Atish Patra <atishp@...shpatra.org>
To:     Christoph Hellwig <hch@....de>
Cc:     Atish Patra <atish.patra@....com>,
        devicetree <devicetree@...r.kernel.org>,
        Albert Ou <aou@...s.berkeley.edu>,
        Tobias Klauser <tklauser@...tanz.ch>,
        Robin Murphy <robin.murphy@....com>,
        "linux-kernel@...r.kernel.org List" <linux-kernel@...r.kernel.org>,
        Rob Herring <robh+dt@...nel.org>,
        iommu@...ts.linux-foundation.org,
        Guo Ren <guoren@...ux.alibaba.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        linux-riscv <linux-riscv@...ts.infradead.org>,
        Frank Rowand <frowand.list@...il.com>,
        Dmitry Vyukov <dvyukov@...gle.com>
Subject: Re: [RFC 1/5] RISC-V: Implement arch_sync_dma* functions

On Sun, Jul 25, 2021 at 11:57 PM Christoph Hellwig <hch@....de> wrote:
>
> > +#ifdef CONFIG_RISCV_DMA_NONCOHERENT
> > +struct riscv_dma_cache_sync {
> > +     void (*cache_invalidate)(phys_addr_t paddr, size_t size);
> > +     void (*cache_clean)(phys_addr_t paddr, size_t size);
> > +     void (*cache_flush)(phys_addr_t paddr, size_t size);
> > +};
> > +
> > +void riscv_dma_cache_sync_set(struct riscv_dma_cache_sync *ops);
> > +#endif
>
> As told a bunch of times before: doing indirect calls here is a
> performance nightmare.  Use something that actually does perform
> horribly like alternatives.  Or even delay implementing that until
> we need it and do a plain direct call for now.
>

I was initially planning to replace this with alternatives in the
future versions. But there is no point in doing it
until we have CMO spec finalized. We also don't have any other
platform using these apart from sifive l2
cache controllers for now.

I will change these to direct for now.

> static void __dma_sync(phys_addr_t paddr, size_t size, enum dma_data_direction dir)
> > +{
> > +     if ((dir == DMA_FROM_DEVICE) && (dma_cache_sync->cache_invalidate))
> > +             dma_cache_sync->cache_invalidate(paddr, size);
> > +     else if ((dir == DMA_TO_DEVICE) && (dma_cache_sync->cache_clean))
> > +             dma_cache_sync->cache_clean(paddr, size);
> > +     else if ((dir == DMA_BIDIRECTIONAL) && dma_cache_sync->cache_flush)
> > +             dma_cache_sync->cache_flush(paddr, size);
> > +}
>
> The seletion of flush types is completely broken.  Take a look at the
> comment in arch/arc/mm/dma.c above arch_sync_dma_for_device for a good
> explanation.
>

Thanks. I will fix it.

> > +void arch_dma_prep_coherent(struct page *page, size_t size)
> > +{
> > +     void *flush_addr = page_address(page);
> > +
> > +     memset(flush_addr, 0, size);
>
> arch_dma_prep_coherent is not supposed to modify the content of
> the data.

Sorry. This was a leftover from some experimental code. It shouldn't
have been included.

> _______________________________________________
> iommu mailing list
> iommu@...ts.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu

-- 
Regards,
Atish

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives