Re: [PATCH] hw/net: npcm7xx_emc: fix alignment to eth_hdr

[Peter Maydell]

On Thu, 27 Feb 2025 at 15:55, Patrick Venture <vent...@google.com> wrote:
>
>
>
> On Thu, Feb 27, 2025 at 7:52 AM Peter Maydell <peter.mayd...@linaro.org> 
> wrote:
>>
>> On Thu, 27 Feb 2025 at 15:40, Patrick Venture <vent...@google.com> wrote:
>> >
>> > 'const struct eth_header', which requires 2 byte alignment
>> >
>> > Signed-off-by: Patrick Venture <vent...@google.com>
>> > ---
>> >  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(&eth_hdr, PKT_GET_ETH_HDR(buf),
>> > +           (sizeof(eth_hdr) > len) ? len : sizeof(eth_hdr));
>> > +    pkt_type = get_eth_packet_type(&eth_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
-- PMM