Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220307224620.1933061-1-heiko@sntech.de> <YllWTN+15CoskNBt@Red>
 <70da24dd-2d03-fc49-151d-daabb315a5f6@sholland.org> <YlpxsYREWv/LQ+HY@Red>
 <849a3728-7e84-4f26-0c73-4d68eae9ae01@sholland.org> <YlsZxVjgt3ZNQ7Ub@Red>
 <CAJF2gTSNzLfon7yH3zvOJfYwQnVRvNWW7KygShLqcagRfbyAkg@mail.gmail.com>
 <YlvTkfIO9Oz3ib5i@Red> <CAJF2gTQ5rYATTHj2UtwxKw0dpvdHBUAih1RJf0XuGM8b6euZwQ@mail.gmail.com>
 <YlxP26dklDj2y+cP@Red>
In-Reply-To: <YlxP26dklDj2y+cP@Red>
From:   Philipp Tomsich <philipp.tomsich@vrull.eu>
Date:   Mon, 18 Apr 2022 17:29:10 +0200
Message-ID: <CAAeLtUC=eF6hp2V2d7wEWWX=vkEcP8NoPLLz=hUrE4x4Q7u4mw@mail.gmail.com>
Subject: Re: [PATCH 0/2] riscv: implement Zicbom-based CMO instructions + the
 t-head variant
To:     Corentin Labbe <clabbe.montjoie@gmail.com>
Cc:     Guo Ren <guoren@kernel.org>, Samuel Holland <samuel@sholland.org>,
        Heiko Stuebner <heiko@sntech.de>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        linux-riscv <linux-riscv@lists.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Wei Fu <wefu@redhat.com>, Atish Patra <atishp@atishpatra.org>,
        Anup Patel <anup@brainfault.org>,
        Nick Kossifidis <mick@ics.forth.gr>,
        Christoph Muellner <cmuellner@linux.com>,
        Herbert Xu <herbert@gondor.apana.org.au>,
        linux-crypto@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sun, 17 Apr 2022 at 19:35, Corentin Labbe <clabbe.montjoie@gmail.com> wr=
ote:
>
> Le Sun, Apr 17, 2022 at 04:49:34PM +0800, Guo Ren a =C3=A9crit :
> > On Sun, Apr 17, 2022 at 4:45 PM Corentin Labbe
> > <clabbe.montjoie@gmail.com> wrote:
> > >
> > > Le Sun, Apr 17, 2022 at 10:17:34AM +0800, Guo Ren a =C3=A9crit :
> > > > On Sun, Apr 17, 2022 at 3:32 AM Corentin Labbe
> > > > <clabbe.montjoie@gmail.com> wrote:
> > > > >
> > > > > Le Sat, Apr 16, 2022 at 12:47:29PM -0500, Samuel Holland a =C3=A9=
crit :
> > > > > > On 4/16/22 2:35 AM, Corentin Labbe wrote:
> > > > > > > Le Fri, Apr 15, 2022 at 09:19:23PM -0500, Samuel Holland a =
=C3=A9crit :
> > > > > > >> On 4/15/22 6:26 AM, Corentin Labbe wrote:
> > > > > > >>> Le Mon, Mar 07, 2022 at 11:46:18PM +0100, Heiko Stuebner a =
=C3=A9crit :
> > > > > > >>>> This series is based on the alternatives changes done in m=
y svpbmt series
> > > > > > >>>> and thus also depends on Atish's isa-extension parsing ser=
ies.
> > > > > > >>>>
> > > > > > >>>> It implements using the cache-management instructions from=
 the  Zicbom-
> > > > > > >>>> extension to handle cache flush, etc actions on platforms =
needing them.
> > > > > > >>>>
> > > > > > >>>> SoCs using cpu cores from T-Head like the Allwinne D1 impl=
ement a
> > > > > > >>>> different set of cache instructions. But while they are di=
fferent,
> > > > > > >>>> instructions they provide the same functionality, so a var=
iant can
> > > > > > >>>> easly hook into the existing alternatives mechanism on tho=
se.
> > > > > > >>>>
> > > > > > >>>>
> > > > > > >>>
> > > > > > >>> Hello
> > > > > > >>>
> > > > > > >>> I am testing https://github.com/smaeul/linux.git branch:ori=
gin/riscv/d1-wip which contain this serie.
> > > > > > >>>
> > > > > > >>> I am hitting a buffer corruption problem with DMA.
> > > > > > >>> The sun8i-ce crypto driver fail self tests due to "device o=
verran destination buffer".
> > > > > > >>> In fact the buffer is not overran by device but by dma_map_=
single() operation.
> > > > > > >>>
> > > > > > >>> The following small code show the problem:
> > > > > > >>>
> > > > > > >>> dma_addr_t dma;
> > > > > > >>> u8 *buf;
> > > > > > >>> #define BSIZE 2048
> > > > > > >>> #define DMASIZE 16
> > > > > > >>>
> > > > > > >>> buf =3D kmalloc(BSIZE, GFP_KERNEL | GFP_DMA);
> > > > > > >>> for (i =3D 0; i < BSIZE; i++)
> > > > > > >>>     buf[i] =3D 0xFE;
> > > > > > >>> print_hex_dump(KERN_INFO, "DMATEST1:", DUMP_PREFIX_NONE, 16=
, 4, buf, 256, false);
> > > > > > >>> dma =3D dma_map_single(ce->dev, buf, DMASIZE, DMA_FROM_DEVI=
CE);
> > > > > > >>
> > > > > > >> This function (through dma_direct_map_page()) ends up callin=
g
> > > > > > >> arch_sync_dma_for_device(..., ..., DMA_FROM_DEVICE), which i=
nvalidates the CPU's
> > > > > > >> cache. This is the same thing other architectures do (at lea=
st arm, arm64,
> > > > > > >> openrisc, and powerpc). So this appears to be working as int=
ended.
> > > > > > >
> > > > > > > This behavour is not present at least on ARM and ARM64.
> > > > > > > The sample code I provided does not corrupt the buffer on the=
m.
> > > > > >
> > > > > > That can be explained by the 0xFE bytes having been flushed to =
DRAM already in
> > > > > > your ARM/ARM64 tests, whereas in your riscv64 case, the 0xFE by=
tes were still in
> > > > > > a dirty cache line. The cache topology and implementation is to=
tally different
> > > > > > across the SoCs, so this is not too surprising.
> > > > > >
> > > > > > Semantically, dma_map_single(..., DMA_FROM_DEVICE) means you ar=
e doing a
> > > > > > unidirectional DMA transfer from the device into that buffer. S=
o the contents of
> > > > > > the buffer are "undefined" until the DMA transfer completes. If=
 you are also
> > > > > > writing data into the buffer from the CPU side, then you need D=
MA_BIDIRECTIONAL.
> > > > > >
> > > > > > Regards,
> > > > > > Samuel
> > > > >
> > > > > +CC crypto mailing list + maintainer
> > > > >
> > > > > My problem is that crypto selftest, for each buffer where I need =
to do a cipher operation,
> > > > > concat a poison buffer to check that device does write beyond buf=
fer.
> > > > >
> > > > > But the dma_map_sg(FROM_DEVICE) corrupts this poison buffer and c=
rypto selftests fails thinking my device did a buffer overrun.
> > > > >
> > > > > So you mean that on SoC D1, this crypto API check strategy is imp=
ossible ?
> > > >
> > > > I think you could try to replace all CLEAN & INVAL ops with FLUSH o=
ps
> > > > for the testing. (All cache block-aligned data from the device for =
the
> > > > CPU should be invalided.)
> > > >
> > >
> > > With:
> > > diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-nonc=
oherent.c
> > > index 2c124bcc1932..608483522e05 100644
> > > --- a/arch/riscv/mm/dma-noncoherent.c
> > > +++ b/arch/riscv/mm/dma-noncoherent.c
> > > @@ -21,7 +21,7 @@ void arch_sync_dma_for_device(phys_addr_t paddr, si=
ze_t size, enum dma_data_dire
> > >                 ALT_CMO_OP(CLEAN, (unsigned long)phys_to_virt(paddr),=
 size);
> > >                 break;
> > >         case DMA_FROM_DEVICE:
> > > -               ALT_CMO_OP(INVAL, (unsigned long)phys_to_virt(paddr),=
 size);
> > > +               ALT_CMO_OP(FLUSH, (unsigned long)phys_to_virt(paddr),=
 size);
> > >                 break;
> > >         case DMA_BIDIRECTIONAL:
> > >                 ALT_CMO_OP(FLUSH, (unsigned long)phys_to_virt(paddr),=
 size);
> > >
> > >
> > > The crypto self test works and I got no more buffer corruption.
> > No, No ... it's not a solution. That means your driver has a problem.
> > From device, we only need INVAL enough.
> >
>
> For me, my driver works fine, the problem came from dma_map_sg(), probabl=
y I didnt explain right, I restart.
>
> Example:
> crypto self test send to my driver an AES cipher operation of 16 bytes in=
side a SG, but the original buffer is greater (said 32 for the example).
> So the first 16 bytes are used by the SG and the last 16 bytes are a pois=
oned buffer (with value 0xFE) to check driver do not write beyong the norma=
l operation of 16 bytes (and beyond the SG length).
>
> Doing the dma_map_sg(FROM_DEVICE) on the SG corrupt the whole buffer.

Doesn't the DMA_FROM_DEVICE indicate that there are no expected writes
from the CPU to the buffer (and that any modifications to the
underlying cache line can be dropped via an invalidation)?
In other words: does the behavior change when mapping as
DMA_BIDIRECTIONAL =E2=80=94 and: should a map/unmap sequence be used where =
it
is first mapped as DMA_TO_DEVICE when poisoning the buffer and later
as DMA_FROM_DEVICE when in normal operation?

Philipp.

> My driver write normally via DMA the first 16 bytes.
> Crypto API check the last bytes, no more 0xFE, so it fail believing my dr=
iver wrote beyond the first 16 bytes.
>
> But even If I disable my hardware operation, the buffer is still corrupte=
d. (See my sample code which just do dma_map/dma_unmap)
>
> So the problem is the dma_map(FROM_DEVICE) which change buffer content.
>
> So if this behavour is normal on D1 SoC, how to fix the crypto self tests=
 ?