On Mon, 3 Mar 2025 at 11:17, Philippe Mathieu-Daudé <phi...@linaro.org> wrote:
>
> Hi Patrick,
>
> On 27/2/25 16:42, Patrick Venture wrote:
> > eth_hdr requires 2 byte alignment
> >
> > Signed-off-by: Patrick Venture <vent...@google.com>
> > ---
> > hw/net/ftgmac100.c | 15 ++++++++++++---
> > 1 file changed, 12 insertions(+), 3 deletions(-)
>
>
> > @@ -1028,6 +1032,7 @@ static ssize_t ftgmac100_receive(NetClientState *nc,
> > const uint8_t *buf,
> > {
> > FTGMAC100State *s = FTGMAC100(qemu_get_nic_opaque(nc));
> > FTGMAC100Desc bd;
> > + struct eth_header eth_hdr = {};
> > uint32_t flags = 0;
> > uint64_t addr;
> > uint32_t crc;
> > @@ -1036,7 +1041,11 @@ static ssize_t ftgmac100_receive(NetClientState *nc,
> > const uint8_t *buf,
> > uint32_t buf_len;
> > size_t size = len;
> > uint32_t first = FTGMAC100_RXDES0_FRS;
> > - uint16_t proto = be16_to_cpu(PKT_GET_ETH_HDR(buf)->h_proto);
>
> The LD/ST API deals with unaligned fields, would that help?
>
> uint16_t proto = lduw_be_p(&PKT_GET_ETH_HDR(buf)->h_proto);
No, it doesn't, unfortunately -- I forget the details, but
if the struct is unaligned but its definition says it is
aligned then you get UB even with our accessor functions.
This is why in commit f8b94b4c520126 I had to fix this for
struct ip_header by marking it as QEMU_PACKED, even though
the actual access there was a uint8_t*.
(See also the other thread where I suggested to Patrick that
the best approach here is to mark eth_hdr as QEMU_PACKED,
and fix up anything we need to do to make that change.)
thanks
-- PMM
