Re: [PATCH 0/4] Support registering specific reset handler

[Bjorn Helgaas <bhelgaas@google.com>]

On Fri, Feb 20, 2015 at 04:53:08PM +1100, Gavin Shan wrote:
> On Thu, Feb 19, 2015 at 04:57:47PM -0200, cascardo@linux.vnet.ibm.com wrote:
> >On Tue, Feb 17, 2015 at 09:36:47AM +1100, Gavin Shan wrote:
> >> On Mon, Feb 16, 2015 at 11:14:27AM -0200, cascardo@linux.vnet.ibm.com wrote:
> >> >On Fri, Feb 13, 2015 at 03:54:55PM +1100, Gavin Shan wrote:
> >> >> VFIO PCI infrastructure depends on pci_reset_function() to do reset on
> >> >> PCI devices so that they would be in clean state when host or guest grabs
> >> >> them. Unfortunately, the function doesn't work (or not well) on some PCI
> >> >> devices that require EEH PE reset.
> >> >> 
> >> >> The patchset extends the quirk for PCI device speicific reset methods to
> >> >> allow dynamically registration. With it, we can translate reset requests
> >> >> for those special PCI devcies to EEH PE reset, which is only avaialble on
> >> >> 64-bits PowerPC platforms.
> >> >> 
> >> >
> >> >Hi, Gavin.
> >> >
> >> >I like your approach overall. That allows us to confine these quirks to
> >> >the platforms where they are relevant. I would make the quirks more
> >> >specific, though, instead of doing them for all IBM and Mellanox
> >> >devices.
> >> >
> >> 
> >> Yeah, we need have more specific vendor/device IDs for PATCH[4/4]. Could
> >> you please take a look on PATCH[4/4] and then suggest the specific devices
> >> that requries the platform-dependent reset quirk? Especially the device IDs
> >> for IBM/Mellanox we need put add quirks for.
> >> 
> >> >I wonder if we should not have some form of domain reset, where we would
> >> >reset all the devices on the same group, and use that on vfio. Grouping
> >> >the devices would then be made platform-dependent, as well as the reset
> >> >method. On powernv, we would group by IOMMU group and issue a
> >> >fundamental reset.
> >> >
> >> 
> >> I'm assuming "domain reset" is PE reset, which is the specific reset handler
> >> tries to do on PowerNV platform. The reason why we need platform specific
> >> reset handler is some adapters can't support function level reset methods
> >> (except pci_dev_specific_reset()) in __pci_dev_reset().
> >> 
> >
> >Well, in the case of Power servers, this would be the PE reset, I am not
> >sure what this would be on other platforms. No other platform implements
> >pci_set_pcie_reset_state, which I think would be one possible to
> >implement such a reset.
> >
> >What I am saying is that we should consider doing this on all platforms
> >and for all adapters, because this is not specific to Power and this is
> >not specific to this particular set of adapters. Otherwise, one can
> >simply program one adapter in the guest to write to host memory,
> >shutdown, and since no reset will take place, the card will simply write
> >to host memory when the guest is finished with and IOMMU is off.
> >
> 
> Yes, I believe your way will make things simpler and we don't need the
> code to support registering reset handler dynamically at atll. Please
> confirm if following idea is what you're suggesting: 
> 
> - Introduce function drivers/pci/quirks.c::pci_dev_specific_reset(), which
>   will be added to pci_dev_reset_methods[] for various vendor/device IDs.
> - pci_dev_specific_reset() routes the reset (or probe) request to
>   pci_set_pcie_reset_state(), which needs one more syntax for reset probing.
>   In turn, pci_set_pcie_reset_state() calls pcibios_set_pcie_reset_state(),
>   which returns -ETTY by default.
> 
> For PowerPC, pcibios_set_pcie_reset_state() will do PE reset, which can't
> be ported to other platforms as it depends on PowerPC unique EEH feature.
> So other platforms have to override this function and do things similar to
> PowerPC PE reset there.

Dropping for now because it sounds like you are considering reworking based
on Cascardo's ideas.  If not, please re-post these so they show up in
patchwork again.

Bjorn