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

[Glenn Washburn]

Good day Patrick,

On Wed, 24 Feb 2021 07:46:48 +0100
Patrick Rudolph <patrick.rudolph@9elements.com> wrote:

> Hi Glenn,
> yes it's the same patch series, but has been cleaned and improved a
> lot. I've addressed most of the comments received earlier.

I see that I implied that you hadn't and I didn't mean to. I only meant
to suggest that there were possible unaddressed issues. Glad to hear
those were already taken care of.

> As stated in the commit message, USB 3.1 and USB 3.2 are not
> tested, as I lack hardware to test this. I'm not going to look into
> this any further into this as it works fine with USB 3.0 compliant
> xhci controllers.
> 
> I'l fix the remaining issues and publish a new patch series.

Great. Are you thinking of adding in some tests? If not, I would
appreciate knowing the qemu opts used to create the virtual machine (I'm
unsure how to turn on xHCI in qemu).

Glenn

> 
> On Fri, Feb 19, 2021 at 7:11 PM Glenn Washburn
> <development@efficientek.com> wrote:
> >
> > 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
> > >