Re: [PATCH v2 0/7] Add xHCI USB support

[Glenn Washburn]

Hi Patrick,

Thanks for the contribution. I think this would be a great addition to
GRUB. However, there are a few issues I see at the moment.

On Mon,  7 Dec 2020 08:41:20 +0100
Patrick Rudolph <patrick.rudolph@9elements.com> wrote:

> Add basic support for xHCI USB controllers and non root xHCI hubs.
> The motivation is to use this code on platforms that do not provide
> user input by runtime services (like BIOS or UEFI platform) do.
> This is the case when GRUB is used as coreboot payload for example.
> 
> The code is based on seabios implementation, but has been heavily
> modified to match grubs internals.

Are these the same changes as in https://github.com/Nitrokey/grub.git
referenced in a previous email? I'm asking because there were some
comments that were unaddressed. So, if it is, could you please address
Thomas's email (I haven't checked to see if they are still relevant if
so)? (see: https://marc.info/?l=grub-devel&m=159769164332293)

Searching in the mailing list, I found this attempt in early 2017.
Having not looked at the code at all, could this help in addressing
some of Thomas's concerns?
https://github.com/bjornfor/grub/tree/add-coreboot-xhci-driver-2nd-attempt-v2

I've run your code through the GitLab CI I've configured, and there was
a build failure on x86_64-efi. Looks like some implicit type casting
issues. I believe it failed for both gcc 8.1 and 10.1. Here is the
error log when using gcc 10.1:
https://gitlab.com/gwashburn/grub/-/jobs/1025317154#L594

Also, I think that any xhci support should be accompanied by passing
make check tests.  See uhci_test for an example of how this might be
done. Since you've already tested on qemu, I think you're 90% of the
way there in making some tests. Both of the ones listed before would be
nice. If you'd like some guidance or help, let me know.

Glenn

> Changes done in version 2:
>  * Code cleanup
>  * Code style fixes
>  * Don't leak memory buffers
>  * Compile without warnings
>  * Add more defines
>  * Add more helper functions
>  * Don't assume a 1:1 virtual to physical mapping
>  * Flush cachelines after writing buffers
>  * Don't use hardcoded page size
>  * Proper scratchpad register setup
>  * xHCI bios ownership handoff
> 
> Changes done in version 3:
>  * Several bug fixes for real hardware
>  * Fixed a race condition detecting events, which doesn't appear on
>    qemu based xHCI controllers
>  * Don't accidently disable USB3.0 devices after first command
>  * Support arbitrary protocol speed IDs
>  * Coding style cleanup
>  * Support for non root SuperSpeed hubs
> 
> Changes Tested:
>  * Qemu system x86_64
>    * virtual USB HID keyboard (usb-kbd)
>    * virtual USB HID mass storage (usb-storage)
>  * Intel C246 integrated xHCI (Supermicro X11SSH-F)
>    * iKVM HID keyboard
>    * USB3 HID mass storage (controller root port)
>    * External USB HID keyboard
> 
> TODO:
>  * Test on more hardware
>  * Test on USB3 hubs
>  * Support for USB 3.1 and USB 3.2 controllers
> 
> Patrick Rudolph (7):
>   grub-core/bus/usb: Parse SuperSpeed companion descriptors
>   usb: Add enum for xHCI
>   usbtrans: Set default maximum packet size
>   grub-core/bus/usb: Add function pointer for attach/detach events
>   grub-core/bus/usb/usbhub: Add new private fields for xHCI controller
>   grub-core/bus/usb: Add xhci support
>   grub-core/bus/usb/usbhub: Add xHCI non root hub support
> 
>  Makefile.am                       |    2 +-
>  grub-core/Makefile.core.def       |    7 +
>  grub-core/bus/usb/ehci.c          |    2 +
>  grub-core/bus/usb/ohci.c          |    2 +
>  grub-core/bus/usb/serial/common.c |    2 +-
>  grub-core/bus/usb/uhci.c          |    2 +
>  grub-core/bus/usb/usb.c           |   44 +-
>  grub-core/bus/usb/usbhub.c        |   95 +-
>  grub-core/bus/usb/usbtrans.c      |    6 +-
>  grub-core/bus/usb/xhci-pci.c      |  195 +++
>  grub-core/bus/usb/xhci.c          | 2496
> +++++++++++++++++++++++++++++ grub-core/commands/usbtest.c      |
> 2 +- grub-core/disk/usbms.c            |    2 +-
>  grub-core/term/usb_keyboard.c     |    2 +-
>  include/grub/usb.h                |   18 +-
>  include/grub/usbdesc.h            |   12 +-
>  include/grub/usbtrans.h           |    4 +
>  17 files changed, 2852 insertions(+), 41 deletions(-)
>  create mode 100644 grub-core/bus/usb/xhci-pci.c
>  create mode 100644 grub-core/bus/usb/xhci.c
>