hw/net/msf2-emac.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)
The msf2-emac ethernet controller has functions emac_load_desc() and
emac_store_desc() which read and write the in-memory descriptor
blocks and handle conversion between guest and host endianness.
As currently written, emac_store_desc() does the endianness
conversion in-place; this means that it effectively consumes the
input EmacDesc struct, because on a big-endian host the fields will
be overwritten with the little-endian versions of their values.
Unfortunately, in all the callsites the code continues to access
fields in the EmacDesc struct after it has called emac_store_desc()
-- specifically, it looks at the d.next field.
The effect of this is that on a big-endian host networking doesn't
work because the address of the next descriptor is corrupted.
We could fix this by making the callsite avoid using the struct; but
it's more robust to have emac_store_desc() leave its input alone.
(emac_load_desc() also does an in-place conversion, but here this is
fine, because the function is supposed to be initializing the
struct.)
Cc: qemu-stable@nongnu.org
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
This is one of a number of issues that prevent 'make check-avocado'
working for arm targets on a big-endian host...
hw/net/msf2-emac.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/hw/net/msf2-emac.c b/hw/net/msf2-emac.c
index 7ccd3e51427..34c1f768db0 100644
--- a/hw/net/msf2-emac.c
+++ b/hw/net/msf2-emac.c
@@ -120,12 +120,16 @@ static void emac_load_desc(MSF2EmacState *s, EmacDesc *d, hwaddr desc)
static void emac_store_desc(MSF2EmacState *s, EmacDesc *d, hwaddr desc)
{
- /* Convert from host endianness into LE. */
- d->pktaddr = cpu_to_le32(d->pktaddr);
- d->pktsize = cpu_to_le32(d->pktsize);
- d->next = cpu_to_le32(d->next);
+ EmacDesc outd;
+ /*
+ * Convert from host endianness into LE. We use a local struct because
+ * calling code may still want to look at the fields afterwards.
+ */
+ outd.pktaddr = cpu_to_le32(d->pktaddr);
+ outd.pktsize = cpu_to_le32(d->pktsize);
+ outd.next = cpu_to_le32(d->next);
- address_space_write(&s->dma_as, desc, MEMTXATTRS_UNSPECIFIED, d, sizeof *d);
+ address_space_write(&s->dma_as, desc, MEMTXATTRS_UNSPECIFIED, &outd, sizeof outd);
}
static void msf2_dma_tx(MSF2EmacState *s)
--
2.34.1
On 24/04/2023 17.19, Peter Maydell wrote: > The msf2-emac ethernet controller has functions emac_load_desc() and > emac_store_desc() which read and write the in-memory descriptor > blocks and handle conversion between guest and host endianness. > > As currently written, emac_store_desc() does the endianness > conversion in-place; this means that it effectively consumes the > input EmacDesc struct, because on a big-endian host the fields will > be overwritten with the little-endian versions of their values. > Unfortunately, in all the callsites the code continues to access > fields in the EmacDesc struct after it has called emac_store_desc() > -- specifically, it looks at the d.next field. > > The effect of this is that on a big-endian host networking doesn't > work because the address of the next descriptor is corrupted. > > We could fix this by making the callsite avoid using the struct; but > it's more robust to have emac_store_desc() leave its input alone. > > (emac_load_desc() also does an in-place conversion, but here this is > fine, because the function is supposed to be initializing the > struct.) > > Cc: qemu-stable@nongnu.org > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > This is one of a number of issues that prevent 'make check-avocado' > working for arm targets on a big-endian host... > > hw/net/msf2-emac.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/hw/net/msf2-emac.c b/hw/net/msf2-emac.c > index 7ccd3e51427..34c1f768db0 100644 > --- a/hw/net/msf2-emac.c > +++ b/hw/net/msf2-emac.c > @@ -120,12 +120,16 @@ static void emac_load_desc(MSF2EmacState *s, EmacDesc *d, hwaddr desc) > > static void emac_store_desc(MSF2EmacState *s, EmacDesc *d, hwaddr desc) You could likely also add a "const" to "EmacDesc *d" now. > { > - /* Convert from host endianness into LE. */ > - d->pktaddr = cpu_to_le32(d->pktaddr); > - d->pktsize = cpu_to_le32(d->pktsize); > - d->next = cpu_to_le32(d->next); > + EmacDesc outd; > + /* > + * Convert from host endianness into LE. We use a local struct because > + * calling code may still want to look at the fields afterwards. > + */ > + outd.pktaddr = cpu_to_le32(d->pktaddr); > + outd.pktsize = cpu_to_le32(d->pktsize); > + outd.next = cpu_to_le32(d->next); > > - address_space_write(&s->dma_as, desc, MEMTXATTRS_UNSPECIFIED, d, sizeof *d); > + address_space_write(&s->dma_as, desc, MEMTXATTRS_UNSPECIFIED, &outd, sizeof outd); > } Reviewed-by: Thomas Huth <thuth@redhat.com>
On Mon, 24 Apr 2023 at 17:27, Thomas Huth <thuth@redhat.com> wrote: > > On 24/04/2023 17.19, Peter Maydell wrote: > > The msf2-emac ethernet controller has functions emac_load_desc() and > > emac_store_desc() which read and write the in-memory descriptor > > blocks and handle conversion between guest and host endianness. > > > > As currently written, emac_store_desc() does the endianness > > conversion in-place; this means that it effectively consumes the > > input EmacDesc struct, because on a big-endian host the fields will > > be overwritten with the little-endian versions of their values. > > Unfortunately, in all the callsites the code continues to access > > fields in the EmacDesc struct after it has called emac_store_desc() > > -- specifically, it looks at the d.next field. > > > > The effect of this is that on a big-endian host networking doesn't > > work because the address of the next descriptor is corrupted. > > > > We could fix this by making the callsite avoid using the struct; but > > it's more robust to have emac_store_desc() leave its input alone. > > > > (emac_load_desc() also does an in-place conversion, but here this is > > fine, because the function is supposed to be initializing the > > struct.) > > > > Cc: qemu-stable@nongnu.org > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > --- > > This is one of a number of issues that prevent 'make check-avocado' > > working for arm targets on a big-endian host... > > > > hw/net/msf2-emac.c | 14 +++++++++----- > > 1 file changed, 9 insertions(+), 5 deletions(-) > > > > diff --git a/hw/net/msf2-emac.c b/hw/net/msf2-emac.c > > index 7ccd3e51427..34c1f768db0 100644 > > --- a/hw/net/msf2-emac.c > > +++ b/hw/net/msf2-emac.c > > @@ -120,12 +120,16 @@ static void emac_load_desc(MSF2EmacState *s, EmacDesc *d, hwaddr desc) > > > > static void emac_store_desc(MSF2EmacState *s, EmacDesc *d, hwaddr desc) > > You could likely also add a "const" to "EmacDesc *d" now. Yep; applied to target-arm.next with that change added. -- PMM
© 2016 - 2024 Red Hat, Inc.