On Thu, Feb 27, 2025 at 8:01 AM Peter Maydell <[email protected]> wrote:
> On Thu, 27 Feb 2025 at 15:55, Patrick Venture <[email protected]> wrote: > > > > > > > > On Thu, Feb 27, 2025 at 7:52 AM Peter Maydell <[email protected]> > wrote: > >> > >> On Thu, 27 Feb 2025 at 15:40, Patrick Venture <[email protected]> > wrote: > >> > > >> > 'const struct eth_header', which requires 2 byte alignment > >> > > >> > Signed-off-by: Patrick Venture <[email protected]> > >> > --- > >> > hw/net/npcm7xx_emc.c | 7 ++++++- > >> > 1 file changed, 6 insertions(+), 1 deletion(-) > >> > > >> > diff --git a/hw/net/npcm7xx_emc.c b/hw/net/npcm7xx_emc.c > >> > index e06f652629..11ed4a9e6a 100644 > >> > --- a/hw/net/npcm7xx_emc.c > >> > +++ b/hw/net/npcm7xx_emc.c > >> > @@ -424,7 +424,12 @@ static bool emc_can_receive(NetClientState *nc) > >> > static bool emc_receive_filter1(NPCM7xxEMCState *emc, const uint8_t > *buf, > >> > size_t len, const char **fail_reason) > >> > { > >> > - eth_pkt_types_e pkt_type = > get_eth_packet_type(PKT_GET_ETH_HDR(buf)); > >> > + struct eth_header eth_hdr = {}; > >> > + eth_pkt_types_e pkt_type; > >> > + > >> > + memcpy(ð_hdr, PKT_GET_ETH_HDR(buf), > >> > + (sizeof(eth_hdr) > len) ? len : sizeof(eth_hdr)); > >> > + pkt_type = get_eth_packet_type(ð_hdr); > >> > >> Maybe better to mark struct eth_header as QEMU_PACKED? > >> Compare commit f8b94b4c5201 ("net: mark struct ip_header as > >> QEMU_PACKED"). The handling of these header structs in eth.h > >> is in general pretty suspect IMHO. We do the same > >> "get_eth_packet_type(PKT_GET_ETH_HDR(buf))" in other devices, > >> so this isn't just this device's bug. > > > Roger that. We saw this in the two NICs we happened to be testing that > day, and yeah, I grepped and just figured that those other NICs were doing > something with their buffer allocations that we didn't. I'll give > QEMU_PACKED whirl. > > You might find you need to make some fixes to other > devices to get the QEMU_PACKED change to compile (do an > all-targets build to test that). For instance for the > ip_header change I had to first fix virtio-net.c in commit > 5814c0846793715. The kind of thing that will need fixing is > if there are places where code takes the address of the > h_proto field and puts it into a uint16_t* : the compiler > will complain about that. A quick grep suggests that the > rocker_of_dpa.c code might be doing something like this, but > hopefully that's it. > Thanks for the head's up. > > thanks > -- PMM >
