On Thu, 23 Jul 2020 02:59:55 +0000 "Weitao Wang(BJ-RD)" <WeitaoWang@xxxxxxxxxxx> wrote: > On , Jul 22, 2020 at 02:44:14PM +0200, Alan wrote: > > On Wed, Jul 22, 2020 at 02:44:14PM +0200, Greg KH wrote: > > > On Wed, Jul 22, 2020 at 07:57:48PM +0800, WeitaoWangoc wrote: > > > > This bug is found in Zhaoxin platform, but it's a commom code bug. > > > > Fail sequence: > > > > step1: Unbind UHCI controller from native driver; > > > > step2: Bind UHCI controller to vfio-pci, which will put UHCI controller in one > > vfio > > > > group's device list and set UHCI's dev->driver_data to struct > > vfio-pci(for UHCI) > > > > > > Hah, that works? How do you do that properly? What code does that? > > > > Yeah, that can't possibly work. The USB core expects that any host > > controller device (or at least, any PCI host controller device) has its > > driver_data set to point to a struct usb_hcd. It doesn't expect a host > > controller to be bound to anything other than a host controller driver. > > > > Things could easily go very wrong here. For example, suppose at this > > point the ehci-hcd driver just happens to bind to the EHCI controller. > > When this happens, the EHCI controller hardware takes over all the USB > > connections that were routed to the UHCI controller. How will vfio-pci > > deal with that? Pretty ungracefully, I imagine. The issue I believe we're seeing here is not with vfio-pci trying to do anything with the device, the IOMMU grouping would prevent a user from opening any device within the group while other devices within the group are bound to host drivers. So it should be fine if the EHCI device takes over the other ports, but it's not ok that ehci-hcd assumes the driver private data for any other UHCI/OHCI/EHCI device in the same slot is something that it's free to modify. It really seems there should be some sort of companion device registration/opt-in rather than modifying unvalidated data. > > The only way to make this work at all is to unbind both uhci-hcd and > > ehci-hcd first. Then after both are finished you can safely bind > > vfio-pci to the EHCI controller and the UHCI controllers (in that > > order). > > > I'm agree with you, unbind both uhci-hcd and ehci-hcd first then bind to > vfio-pci is a more reasonable sequence. Our experiments prove that this > sequence is indeed good as expected. > However, I did not find a formal document to prescribe this order. > Unfortunately, some application software such as virt-manager/qemu assign > UHCI/EHCI to guest OS has the same bind/unbind sequence as test “by hand”. > Do we need to consider compatibility with this application scenario? Unbinding all functions first, before binding any to vfio-pci should indeed work, thanks to the for_each_companion() function at least testing for null private data before going further. I'd still argue though that these hcd drivers are overstepping their authority by walking the PCI bus and assuming any device in the same slot, matching a set of class codes, is making use of a driver with a known data structure that they're allowed to modify. Even if we claim that the user needs to know what they're doing when they change driver binding, that's a pretty subtle interaction with no validation. Thanks, Alex
