search

openvizsla / ov_ftdi

FT2232H-based USB sniffer
345 stars 102 forks source link

Use LINKTYPE_USB_2_0 pcap format #30

Closed desowin closed 5 years ago

desowin commented 5 years ago

Use the registered LINKTYPE_USB_2_0 (288) in pcap output handler.

Capture timebase is set to whatever the UNIX timestamp was at the time when capture was started. The FPGA 60 MHz clock counter is used to advance the timestamp. In order to not lose any information, the nanosecond pcap magic is used (though the real time resolution is 1 / 60 MHz = 16.(6) ns).

If you are interested in dissector development progress, you can CC yourself to https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=15908 I will be referencing linked bug in the dissector commits. The dissector will be submitted directly to Wireshark gerrit.

desowin commented 5 years ago

@tmbinc, @laf0rge, @matwey, @cyphunk Could you please review/comment on this pull request?

matwey commented 5 years ago

Reference:

somehdlguy commented 5 years ago

LGTM. I'll try and find my OV to test it tonight. I don't see a problem with merging it.

matwey commented 5 years ago

Hello,

I am not sure if you are following this format https://github.com/matwey/libopenvizsla/wiki/OpenVizsla-protocol-description or you are skipping the size and timestamp fields?

desowin commented 5 years ago

I am not sure if you are following this format https://github.com/matwey/libopenvizsla/wiki/OpenVizsla-protocol-description or you are skipping the size and timestamp fields?

I am skipping the size and timestamp fields as they are already in the pcap header itself.

matwey commented 5 years ago

Can we please discuss the format?

I mean, currently, comments in pcap/dlt.h bring people to the full format description. And it may be confusing if we will have inconsistent docs and implementations. My libpcap (https://github.com/matwey/libpcap/commits/openvizsla) also uses full format.

So, if we want to change the format, we have to begin with pcap/dlt.h header.

desowin commented 5 years ago

Can we please discuss the format?

Yes, that's the intended purpose of this pull request.

I mean, currently, comments in pcap-common.h bring the people to the full format description. And it may be confusing if we will have inconsistent docs and implementations. My libpcap (https://github.com/matwey/libpcap/commits/openvizsla) also uses full format.

So, if we want to change the format, we have to begin with pcap-common.h header.

I wasn't aware of your libpcap fork, sorry! I just checked the https://github.com/matwey/libopenvizsla, noticed that there's no pcap code there and thus assumed you simply have documented the frame format as sent on the FTDI serial link.

Do you know if there are already some OpenVizsla pcap files in the wild using the full OpenVizsla frame? If that's the case, then for sure this pull request has to be changed.

matwey commented 5 years ago

Fortunately, I think there is likely no existing pcap files in the wild.

More references:

matwey commented 5 years ago

I think we should collect pros and cons for the formats. I'll try to start:

Comments:

somehdlguy commented 5 years ago

I'd encourage you not to pin your format directly to what the OV emits. Several of the design choices were based on what we could do quickly in hardware. These tradeoffs may change if someone adds features to the bitstream.

Collaborating with some of the other USB recorders like the greatfet rhododendron project, colin o'flynn's unnamed USB tool, and any others that you can think of might be helpful. It might make sense to have a LINKTYPE_USBLL and then have individual analyzers convert to that frame format.

Then again, I'm not the one actually writing any of that code, so you can completely ignore that second paragraph if its inconvenient.

matwey commented 5 years ago

I'd encourage you not to pin your format directly to what the OV emits. Several of the design choices were based on what we could do quickly in hardware. These tradeoffs may change if someone adds features to the bitstream.

Is it guaranteed that OpenVizsla FPGA firmware has backward compatibility? How do you imagine further Host-Device protocol updates?

I'd encourage collaborating with some of the other USB recorders like the greatfet rhododendron project, colin o'flynn's unnamed USB tool, and any others that you can think of. It might make sense to have a LINKTYPE_USBLL and then have individual analyzers convert to that frame format.

Then again, I'm not the one actually writing any of that code, so you can completely ignore that second paragraph if its inconvenient.

desowin commented 5 years ago

Please take a look at the https://code.wireshark.org/review/#/c/34006/ which is the initial Wireshark dissector code (that works with the "reduced format" full format). It currently only shows the low level USB data. The OpenVizsla specific part is clearly separated from the actual USB payload. It works best if you filter out SOFs (usbll.pid != 0xA5).

I understand the @matwey approach of simply encapsulating whatever OpenVizsla itself sends - and I am perfectly fine with adding these fields to the code in this PR. You could easily dissect more data in Wireshark (and in fact, the dissector has future capability in place as it'll just mark data as undissected and to be submitted to bugzilla).

desowin commented 5 years ago

One potential performance improvement would be to make it possible to generate pcap stream directly in the OpenVizsla FPGA. Then the application could simply passthrough to the reader that understands pcap format. Such example application could be Wireshark extcap.

For example in https://github.com/desowin/usbpcap USBPcapDriver generates pcap stream in kernel buffer and USBPcapCMD does control the driver (start/stop capture, set device filters) and simply pass through the data to Wireshark, without analyzing it at all (not even knowing what are the packet boundaries).

somehdlguy commented 5 years ago

Is it guaranteed that OpenVizsla FPGA firmware has backward compatibility? How do you imagine further Host-Device protocol updates?

Historically, we've codeveloped the bitstream and host tool. A single repository allowed us to make synchronized changes at a protocol level, and the zip-based firmware package kept internal details (ex: names to autogenerated register addresses) in sync. Register addresses are explicitly unstable; so any tools accessing the hardware should be parsing "map.txt" rather than hardcoding register addresses.

FPGA RTL backwards compatibility isn't really possible. (In particular, register I/O for init sequences are inherently tied to deep implementation details).

I'm not sure we've thought about how backwards compatibility / protocol compatibility would work. Now that there are external tools, we would probably have to think a bit more about how we would make changes. On the other hand, its not like we've made any changes in the past several years; so that day may never come.

desowin commented 5 years ago

Is it guaranteed that OpenVizsla FPGA firmware has backward compatibility? How do you imagine further Host-Device protocol updates?

Historically, we've codeveloped the bitstream and host tool. A single repository allowed us to make synchronized changes at a protocol level, and the zip-based firmware package kept internal details (ex: names to autogenerated register addresses) in sync. Register addresses are explicitly unstable; so any tools accessing the hardware should be parsing "map.txt" rather than hardcoding register addresses.

FPGA RTL backwards compatibility isn't really possible. (In particular, register I/O for init sequences are inherently tied to deep implementation details).

I think the only backwards compatibility of the FPGA that matters is the data format that gets sent to the host (from FPGA to FTDI). If that format includes some registers, then as long as they are marked as possible to change - and thus that user should not hardcode them, then it's fine in my opinion.

somehdlguy commented 5 years ago

I think the only backwards compatibility of the FPGA that matters is the data format that gets sent to the host (from FPGA to FTDI). If that format includes some registers, then as long as they are marked as possible to change - and thus that user should not hardcode them, then it's fine in my opinion.

I disagree. To be useful, a client side tool must:

  1. initialize hardware (requires internal details)
  2. start a capture (requires internal register details to reset/start HW)
  3. parse capture (potentially fixed frame format)
  4. end a capture (requires internal details)

Since steps 1, 2 and 4 will vary with FPGA impl details and potentially force changes to user tools on FPGA internal changes, I'm not sure there is a ton of value in fixing the format of 3.

In any case, this requires some more thought. I'm not sure what the best approach would be (maybe a small softcore on the OV to handle register details?)

desowin commented 5 years ago

I have updated the pcap format to match @matwey libpcap implementation. After consideration this seems to be the only sane approach. As there are some unused FPGA pins available on the connector, someone might want to use that to capture some other traffic. It should be relatively easy to hook this up to Wireshark by simply using some new magic byte.

desowin commented 5 years ago

Since steps 1, 2 and 4 will vary with FPGA impl details and potentially force changes to user tools on FPGA internal changes, I'm not sure there is a ton of value in fixing the format of 3.

If we opt to go the Wireshark extcap route, then the steps 1, 2 and 4 are perfectly fine to be done in the extcap itself - that is completely separate from the Wireshark. Wireshark needs to be only aware of 3. The FPGA binary and extcap can be distributed as one package.

matwey commented 5 years ago

Hi,

I think I understand @somehdlguy point that FPGA firmware is loaded by the host software and hence there is no compatibility in FPGA required. The host software always loads FPGA with compatible firmware version. But I am not sure that we will have any alternative firmware not based on the current one... So, sticking to A0-format seems to be not so bad.

desowin commented 5 years ago

Recently the LINKTYPE_USB_2_0 was assigned. Description of this is "USB 2.0, 1.1, or 1.0 packet, beginning with a PID, as described by Chapter 8 "Protocol Layer" of the the Universal Serial Bus Specification Revision 2.0.". The initial LINKTYPE_USB_2_0 dissector ("usbll") is now merged in Wireshark master.

The LINKTYPE_OPENVIZSLA dissector is developed and available at Wireshark gerrit and is marked as WorkInProgress until we arrive with conclusion here. The LINKTYPE_OPENVIZSLA dissector handles the OpenVizsla-specific fields. In case of packet starting with 0xA0 it dissects the flags, size, timestamp, and hands the USB data over to the "usbll" dissector. The use of OpenVizsla-specific header makes it possible to extend the OpenVizsla dissector in the future. It is possible to extend it with non-USB protocols (on the unused FPGA pins that are available at the header).

matwey commented 5 years ago

I've just managed to build wireshark with patches from https://code.wireshark.org/review/#/c/34061/

Screenshot_20190825_132449

desowin commented 5 years ago

@matwey Thanks to your screenshot, I noticed the typo in the flags dissection (the order in which the flags bits appear is wrong due to this typo). It is now fixed in the patch set 3 of the linked change.

laf0rge commented 5 years ago

Sorry for being late to the party.

I'd encourage you not to pin your format directly to what the OV emits. Several of the design choices were based on what we could do quickly in hardware. These tradeoffs may change if someone adds features to the bitstream.

I fully agree with that. A more or less "generic" format for "phy level" USB protocol traces as a pcap DLT shouldn't really have anything related to OpenVizsla in the pcap file. How the hardware works, or how the USB (trace) format of OV works should be independent of the pcap/DLT format.

Collaborating with some of the other USB recorders like the greatfet rhododendron project, colin o'flynn's unnamed USB tool, and any others that you can think of might be helpful. It might make sense to have a LINKTYPE_USBLL and then have individual analyzers convert to that frame format.

Full ACK. Meanwhile, there's also new hardware like @lambdaconcept as well as their software https://github.com/lambdaconcept/usb2sniffer-qt around.

Coordinating with the various folks may be difficult and time-consuming. But at the very least, a pcap/DLT format should IMHO be designed hardware-independent and not modelled specifically for OV or any other hardware.

We've done something similar with the GSMTAP format, which was developed inside Osmocom but from the very beginning was intended to cover GSM protocol messages no matter whcih software has captured/generated them, and it's highly successfully with virtually all (at least FOSS) software supporting it.

laf0rge commented 5 years ago

Recently the LINKTYPE_USB_2_0 was assigned. Description of this is "USB 2.0, 1.1, or 1.0 packet, beginning with a PID, as described by Chapter 8 "Protocol Layer" of the the Universal Serial Bus Specification Revision 2.0.". The initial LINKTYPE_USB_2_0 dissector ("usbll") is now merged in Wireshark master.

Great. I would - irrespective to the LINKTYPE_OPENVIZSLA - love to see a patch for ovctl.py to emit that format directly, as an option. Sure, some of the OV specific fields are missing, but at least a that point one could generate a format that's parse-able by mainline wireshark.

The LINKTYPE_OPENVIZSLA dissector is developed and available at Wireshark gerrit and is marked as WorkInProgress until we arrive with conclusion here. The LINKTYPE_OPENVIZSLA dissector handles the OpenVizsla-specific fields. In case of packet starting with 0xA0 it dissects the flags, size, timestamp, and hands the USB data over to the "usbll" dissector. The use of OpenVizsla-specific header makes it possible to extend the OpenVizsla dissector in the future. It is possible to extend it with non-USB protocols (on the unused FPGA pins that are available at the header).

Looks sane to me. The "standard format" as suggested by @somehdlguy and me is implemented already in mainline wireshark, and the openvizsla specific one is just a thin wrapper around it. I think this pull request should be merged so the wireshark dissector can be merged.

desowin commented 5 years ago

Great. I would - irrespective to the LINKTYPE_OPENVIZSLA - love to see a patch for ovctl.py to emit that format directly, as an option. Sure, some of the OV specific fields are missing, but at least a that point one could generate a format that's parse-able by mainline wireshark.

Alternative option would be to simply change this pull request to only use the LINKTYPE_USB_2_0 and leave what to do with the LINKTYPE_OPENVIZSLA for the future.

desowin commented 5 years ago

FYI: The https://github.com/matwey/libopenvizsla/pull/6 contains minimal single executable Wireshark extcap that is confirmed to work on both Linux and Windows.

desowin commented 5 years ago

I have changed this pull request to contain the LINKTYPE_USB_2_0 as this format is essentially defined by USB specification itself and multiple tools can use it (e.g. sigrok exporter).

The biggest reason behind the change, is that the current LINKTYPE_OPENVIZSLA is not mature enough in my opinion. Fixing on the LINKTYPE_OPENVIZSLA would either restrain the changes to the FPGA gateware and/or lead to abandoning it later on when the gateware changes (if it eventually does). Current issue I see with the 0xA0 magic is that when there is too big packet, only the bit in flags is set and there's no way to tell what was the complete length on the wire. The 800 bytes capture limit is arbitrary as there can be bigger packets on the wire (see #29).

desowin commented 5 years ago

I'd encourage you not to pin your format directly to what the OV emits. Several of the design choices were based on what we could do quickly in hardware. These tradeoffs may change if someone adds features to the bitstream.

The more I have spent time on OpenVizsla software, the more I agree with this. Basically the current "FPGA to host" protocol is pretty much some protocol and not the protocol.

I would really like to see #31 implemented in the FPGA and not in the host side (as I did). I am pretty sure the current "FPGA to host" protocol would need to be changed (to accomodate for the 24-bit overflows as they will be very likely on filtered data). As I have very limited understanding on HDL languages (the 101 level) I don't really know what would be the easiest change in the bitstream at the FPGA side (ultimately the person that does implement it in HDL will decide). What I do know, is that pretty much any bitstream change will be easy to handle in the host software.

Considering "FPGA + host software" as a one common package has the advantage of being able to completely shift the functionality from one place to another without having to worry about external tools. I think the LINKTYPE_USB_2_0 is pretty much all that is really needed in high-level decoding software (and using that linktype does not put any arbitrary constrains on the the sniffer HDL to host SW communication).