On Thu, 27 Feb 2025 at 18:12, Patrick Venture <[email protected]> wrote: > > > > On Thu, Feb 27, 2025 at 8:08 AM Patrick Venture <[email protected]> wrote: >> >> >> >> 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. > > > Ok, so digging, and I see that vlanhdr is used similarly in the > rocker_of_dpa.c code, so, without trying to bit off the yak shave of fixing > all ethernet headers, but in reality ethernet packets are packed structures, > should we just make them all packed and bite that bullet?
If you want to do all of them that's probably the long term right thing. But the patchset structure would be a series of "fix X that assumes struct A is not packed", "fix Y that assumes struct A is not packed", "mark struct A packed", "fix Z that assumes struct B is not packed", "mark struct B packed", etc -- so I don't mind if you stop partway through without doing all of them. (After all, that's exactly what I did with only doing ip_header :-)) thanks -- PMM
