On Fri, Oct 7, 2011 at 12:48 AM, Benjamin Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx> wrote: > >> Are you saying that Power is intrinsically unable to support SR-IOV? > > The 'pseries' platform on power is, at least with the current > hypervisor, as the HV doesn't allocate resources for the IOV BAR and the > kernel is not allowed to mess around with PCI resources allocation. > > In the long run, they -may- fix that, but currently, those machines must > not enable SR-IOV or bad things happen (sriov_init enables VFs which > causes the PF driver to fail as well). > >> Or is it merely that we can't use SR-IOV on this particular >> machine/configuration because the BAR assignments are fixed and there >> isn't enough space for the VF BARs? >> >> I would expect the latter, and therefore it seems wrong to disable it >> across the board. Perhaps on another machine with different PCI >> resource assignments, there *would* be space for the VF BARs and >> SR-IOV could be used. > > No. We can never modify the resource allocation and we know that on all > current pseries platforms, the HV will not allocate the VF BAR. > > The patch is not ideal though. Ideally, what you'd need to do is: > > - Detect that the VF BAR hasn't been assigned > > - Combine that with the knowledge that you cannot re-assign resources > on the platform (this is a flag in asm-generic/pci-flags.h but that's > not available on all archs unfortunately). > > And based on that, don't try to mess around with VFs etc... in > sriov_init(). Every architecture has to deal with the case that an SR-IOV adapter appears, but we can't allocate space for the VF BARs. It might be because of pseries limitations, or it might simply be that there's no more space available. So I'd rather have a generic solution (don't enable VFs if we can't allocate resources for them) than an arch-specific one (don't enable VFs on pseries). >> I think this approach is a band-aid that happens to cover up the >> problem. I'd rather fix the PF enable path so it doesn't depend on VF >> BAR allocation. That seems like a much cleaner approach to me. I >> think your pci_select_bars() patch is acceptable for now, even though >> I would still like to see us move all the VF BAR stuff out of the >> pci_dev and into the pci_sriov. > > It's more than just the BAR allocation it seems. The code in > sriov_init() also unconditionally sets the VF count and change the > system page size setting, which appears to break some adapters when the > VF BAR hasn't been assigned. Either that or the attempt at changing the > page size breaks the firmware or the driver (Nish, have you tried just > preventing that part ?) It sounds like we might have a separate issue here that could be addressed with a separate patch series: don't do any configuration of the SR-IOV capability (including system page size) until we know that all the system-level constraints, e.g., having enough MMIO space, have already been satisfied. If we leave SR-IOV disabled for any reason (no resources, command-line parameter, etc.), I don't think we should be touching anything in the SR-IOV capability. Regarding a command-line parameter, I am not opposed to that, as long as the understanding is that it's only for debugging purposes. I don't want normal users to have to know or figure out that they need it. My preference would be something like "pci=nosriov" to bring it under the PCI umbrella rather than leaving it sort of disconnected. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
