Re: [PATCH v1 5/5] dma/xlnx-zdma: Reorg to fix CUR_DSCR

From: Peter Maydell <peter.maydell@linaro.org>
To: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
Cc: figlesia@xilinx.com, "Edgar Iglesias" <edgar.iglesias@xilinx.com>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"Sai Pavan Boddu" <sai.pavan.boddu@xilinx.com>,
	"Francisco Iglesias" <frasse.iglesias@gmail.com>,
	"Alistair Francis" <alistair@alistair23.me>,
	"Richard Henderson" <richard.henderson@linaro.org>,
	"QEMU Developers" <qemu-devel@nongnu.org>,
	"KONRAD Frederic" <frederic.konrad@adacore.com>,
	qemu-arm <qemu-arm@nongnu.org>,
	"Philippe Mathieu-Daudé" <philmd@redhat.com>,
	"Luc Michel" <luc.michel@greensocs.com>
Subject: Re: [PATCH v1 5/5] dma/xlnx-zdma: Reorg to fix CUR_DSCR
Date: Fri, 3 Apr 2020 19:39:52 +0100	[thread overview]
Message-ID: <CAFEAcA99fjZwYJzhypCxyqDMN9NR-BEd8Q21R-5Ge3tv1EwEkQ@mail.gmail.com> (raw)
In-Reply-To: <20200402134721.27863-6-edgar.iglesias@gmail.com>

On Thu, 2 Apr 2020 at 14:46, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
>
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Reorganize the descriptor handling so that CUR_DSCR always
> points to the next descriptor to be processed.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---

This is just moved-code in this patch so I think it's
a separate issue, but it looks like you have an endianness
bug here:

> +static void zdma_update_descr_addr(XlnxZDMA *s, bool type,
> +                                   unsigned int basereg)
> +{
> +    uint64_t addr, next;
> +
> +    if (type == DTYPE_LINEAR) {
> +        addr = zdma_get_regaddr64(s, basereg);
> +        next = addr + sizeof(s->dsc_dst);
> +    } else {
> +        addr = zdma_get_regaddr64(s, basereg);
> +        addr += sizeof(s->dsc_dst);
> +        address_space_read(s->dma_as, addr, s->attr, (void *) &next, 8);

This reads 8 bytes into the uint64_t 'next', which means
that the value from C's point of view will be different
for big-endian and little-endian hosts. You probably wanted
address_space_ldq_le(), assuming the h/w always does
little-endian reads and that these get_regaddr64 and
put_regaddr64 functions work with host-endian integers.

There's a similar problem elsewhere in the device where
it does this:
    address_space_read(s->dma_as, addr, s->attr, buf, sizeof(XlnxZDMADescr));
and assumes the guest structure is the same layout and
endianness as the host struct XlnxDMADescr.

I'm not a huge fan of defining host C structs to match
guest data structures, but if you want to go that way
you ought to (a) be byteswapping the contents appropriately
and (b) have a compile-time assert that the size of the
struct is what you think it is and the compiler hasn't
inserted any helpful extra padding.

thanks
-- PMM