Date:   Tue, 1 Feb 2022 18:41:45 -0600
From:   Alex Elder <elder@...aro.org>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Jakub Kicinski <kuba@...nel.org>,
        Network Development <netdev@...r.kernel.org>,
        "bjorn.andersson@...aro.org" <bjorn.andersson@...aro.org>,
        Florian Fainelli <f.fainelli@...il.com>
Subject: Re: IPA monitor (Final RFC)

On 2/1/22 6:19 PM, Andrew Lunn wrote:
> Hi Alex
> 
> This looks good in general.
> 
> 
>>      - If any monitor packet received from hardware is bad, it--along
>>
>>        with everything beyond it in its page--will be discarded.
>>
>>          - The received data must be big enough to hold a status
>>
>>            header.
>>
>>          - The received data must contain the packet data, meaning
>>
>>            packet length in the status header lies within range.
>   
> So bad in just the sense that capturing the packet and passing it to
> the application processor somehow went wrong.

All I'm saying is that the driver will take a little responsibility
for ensuring the stuff delivered to user space isn't complete crap.
To a certain extent that's to protect itself.  It would be easiest
to pass exactly what was received up to user space, without doing
any interpretation of it whatsoever.  But I think the kernel can
add this value at very little net cost.

The reality is, this should not happen.

> What about packets with bad CRC? Since the application processor is
> not involved, i assume something in the APA architecture is validating
> L2 and L3 CRCs. Do they get dropped, or can they be seen in the

The hardware can offload things like CRC calculation, but in that case
it's up to the receiving code to be told "this has a bad CRC" and
then decide to drop the received packet.  I think the replication
occurs early--possibly before hardware CRC calculations, so the
replica just gets delivered out this special endpoint just the way
it arrived.

> monitor stream? Does the header contain any indication of CRC errors,
> since if the packet has been truncated, it won't be possible to
> validate them. And you said L2 headers are not present anyway.

 From what I can tell, CRC errors are not indicated in the status
header.  The status seems to be more oriented toward "this is
the processing that the IPA hardware performed on this packet."
Including "it entered IPA on this 'port' and matched this
filter rule and got routed out this other 'port'."  But to be
honest my focus has been more on providing the feature than
what exactly those bits represent...

> Do you look at various libpcap-ng implementations? Since this is
> debugfs you are not defining a stable ABI, you can change it any time
> you want and break userspace. But maybe there could be small changes
> in the API which make it easier to feed to wireshark via libpcap.

I considered that.  That was really the interface I was envisioning
at first.  Those things don't really align perfectly with the
information that's made available here though.  This is more like
"what is the hardware doing to each packet" (so we can maybe
understand behavior, or identify a bug).  Rather than "what is
the content of packets flowing through?"  It might be useful
to use the powerful capabilities of things like wireshark for
analysis, but I kind of concluded the purpose of exposing this
information is a little different.

I've got most of the code buffering received data and making
it available through a file interface done.  But I have some
fine tuning to do before I'll be ready to post it for review.

Yes, the non-stable API means I can tweak it a bit even after
it's merged.  But we'll see.  It might be reasonable as-is.

Thanks a lot.

					-Alex

> 
> 	Andrew

Powered by blists - more mailing lists