* [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface [not found] <20171207222145.9769-1-Govinda.Tatti@Oracle.COM> @ 2017-12-07 22:21 ` Govinda Tatti 2017-12-08 20:24 ` Bjorn Helgaas [not found] ` <20171208202424.GC12367@bhelgaas-glaptop.roam.corp.google.com> 2017-12-07 22:21 ` [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute Govinda Tatti [not found] ` <20171207222145.9769-3-Govinda.Tatti@Oracle.COM> 2 siblings, 2 replies; 40+ messages in thread From: Govinda Tatti @ 2017-12-07 22:21 UTC (permalink / raw) To: xen-devel, linux-kernel, linux-pci, bhelgaas, boris.ostrovsky, jgross, JBeulich, roger.pau This patch exports pcie_has_flr() and it is being used by Xen pciback driver to reset (flr/slot/bus) PCI devices based on 'reset' SysFS attribute. Signed-off-by: Govinda Tatti <Govinda.Tatti@Oracle.COM> --- v3: -New drivers/pci/pci.c | 3 ++- include/linux/pci.h | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 6078dfc..499e922 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -3872,7 +3872,7 @@ static void pci_flr_wait(struct pci_dev *dev) * Returns true if the device advertises support for PCIe function level * resets. */ -static bool pcie_has_flr(struct pci_dev *dev) +bool pcie_has_flr(struct pci_dev *dev) { u32 cap; @@ -3882,6 +3882,7 @@ static bool pcie_has_flr(struct pci_dev *dev) pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap); return cap & PCI_EXP_DEVCAP_FLR; } +EXPORT_SYMBOL_GPL(pcie_has_flr); /** * pcie_flr - initiate a PCIe function level reset diff --git a/include/linux/pci.h b/include/linux/pci.h index d16a7c0..44bf2b5 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -1089,6 +1089,7 @@ int pcie_get_mps(struct pci_dev *dev); int pcie_set_mps(struct pci_dev *dev, int mps); int pcie_get_minimum_link(struct pci_dev *dev, enum pci_bus_speed *speed, enum pcie_link_width *width); +bool pcie_has_flr(struct pci_dev *dev); void pcie_flr(struct pci_dev *dev); int __pci_reset_function(struct pci_dev *dev); int __pci_reset_function_locked(struct pci_dev *dev); -- 2.9.5 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface 2017-12-07 22:21 ` [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface Govinda Tatti @ 2017-12-08 20:24 ` Bjorn Helgaas [not found] ` <20171208202424.GC12367@bhelgaas-glaptop.roam.corp.google.com> 1 sibling, 0 replies; 40+ messages in thread From: Bjorn Helgaas @ 2017-12-08 20:24 UTC (permalink / raw) To: Govinda Tatti Cc: jgross, linux-pci, linux-kernel, JBeulich, bhelgaas, xen-devel, boris.ostrovsky, roger.pau On Thu, Dec 07, 2017 at 05:21:44PM -0500, Govinda Tatti wrote: > This patch exports pcie_has_flr() and it is being used by Xen pciback > driver to reset (flr/slot/bus) PCI devices based on 'reset' SysFS > attribute. > > Signed-off-by: Govinda Tatti <Govinda.Tatti@Oracle.COM> > --- > v3: -New > > drivers/pci/pci.c | 3 ++- > include/linux/pci.h | 1 + > 2 files changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 6078dfc..499e922 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -3872,7 +3872,7 @@ static void pci_flr_wait(struct pci_dev *dev) > * Returns true if the device advertises support for PCIe function level > * resets. > */ > -static bool pcie_has_flr(struct pci_dev *dev) > +bool pcie_has_flr(struct pci_dev *dev) > { > u32 cap; > > @@ -3882,6 +3882,7 @@ static bool pcie_has_flr(struct pci_dev *dev) > pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap); > return cap & PCI_EXP_DEVCAP_FLR; > } > +EXPORT_SYMBOL_GPL(pcie_has_flr); I'd rather change pcie_flr() so you could *always* call it, and it would return 0, -ENOTTY, or whatever, based on whether FLR is supported. Is that feasible? I don't like the "Can I do this? Ok, do this" style of interfaces. It's racy (not really applicable in this case) and seems clunky. > /** > * pcie_flr - initiate a PCIe function level reset > diff --git a/include/linux/pci.h b/include/linux/pci.h > index d16a7c0..44bf2b5 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -1089,6 +1089,7 @@ int pcie_get_mps(struct pci_dev *dev); > int pcie_set_mps(struct pci_dev *dev, int mps); > int pcie_get_minimum_link(struct pci_dev *dev, enum pci_bus_speed *speed, > enum pcie_link_width *width); > +bool pcie_has_flr(struct pci_dev *dev); > void pcie_flr(struct pci_dev *dev); > int __pci_reset_function(struct pci_dev *dev); > int __pci_reset_function_locked(struct pci_dev *dev); > -- > 2.9.5 > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 40+ messages in thread
[parent not found: <20171208202424.GC12367@bhelgaas-glaptop.roam.corp.google.com>]
* Re: [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface [not found] ` <20171208202424.GC12367@bhelgaas-glaptop.roam.corp.google.com> @ 2017-12-12 0:29 ` Govinda Tatti [not found] ` <426eeeab-0dcd-8de3-9c5f-a166acf2c130@Oracle.COM> 2017-12-12 15:07 ` Christoph Hellwig 2 siblings, 0 replies; 40+ messages in thread From: Govinda Tatti @ 2017-12-12 0:29 UTC (permalink / raw) To: Bjorn Helgaas Cc: jgross, linux-pci, linux-kernel, JBeulich, bhelgaas, xen-devel, boris.ostrovsky, roger.pau Thanks Bjorn for your review comments. Please see below for my comments. On 12/8/2017 2:24 PM, Bjorn Helgaas wrote: > On Thu, Dec 07, 2017 at 05:21:44PM -0500, Govinda Tatti wrote: >> This patch exports pcie_has_flr() and it is being used by Xen pciback >> driver to reset (flr/slot/bus) PCI devices based on 'reset' SysFS >> attribute. >> >> Signed-off-by: Govinda Tatti <Govinda.Tatti@Oracle.COM> >> --- >> v3: -New >> >> drivers/pci/pci.c | 3 ++- >> include/linux/pci.h | 1 + >> 2 files changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c >> index 6078dfc..499e922 100644 >> --- a/drivers/pci/pci.c >> +++ b/drivers/pci/pci.c >> @@ -3872,7 +3872,7 @@ static void pci_flr_wait(struct pci_dev *dev) >> * Returns true if the device advertises support for PCIe function level >> * resets. >> */ >> -static bool pcie_has_flr(struct pci_dev *dev) >> +bool pcie_has_flr(struct pci_dev *dev) >> { >> u32 cap; >> >> @@ -3882,6 +3882,7 @@ static bool pcie_has_flr(struct pci_dev *dev) >> pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap); >> return cap & PCI_EXP_DEVCAP_FLR; >> } >> +EXPORT_SYMBOL_GPL(pcie_has_flr); > I'd rather change pcie_flr() so you could *always* call it, and it > would return 0, -ENOTTY, or whatever, based on whether FLR is > supported. Is that feasible? Sure, I will add pcie_has_flr() logic inside pcie_flr() and return appropriate values as suggested by you. Do we still want to retain pcie_has_flr() and its usage inside pci.c?.Otherwise, I will remove it and do required cleanup. Please let me know. Cheers GOVINDA _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 40+ messages in thread
[parent not found: <426eeeab-0dcd-8de3-9c5f-a166acf2c130@Oracle.COM>]
* Re: [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface [not found] ` <426eeeab-0dcd-8de3-9c5f-a166acf2c130@Oracle.COM> @ 2017-12-12 0:59 ` Bjorn Helgaas [not found] ` <20171212005919.GB30595@bhelgaas-glaptop.roam.corp.google.com> 1 sibling, 0 replies; 40+ messages in thread From: Bjorn Helgaas @ 2017-12-12 0:59 UTC (permalink / raw) To: Govinda Tatti Cc: jgross, linux-pci, linux-kernel, JBeulich, bhelgaas, xen-devel, boris.ostrovsky, roger.pau On Mon, Dec 11, 2017 at 06:29:29PM -0600, Govinda Tatti wrote: > > Thanks Bjorn for your review comments. Please see below for my comments. > > On 12/8/2017 2:24 PM, Bjorn Helgaas wrote: > >On Thu, Dec 07, 2017 at 05:21:44PM -0500, Govinda Tatti wrote: > >>This patch exports pcie_has_flr() and it is being used by Xen pciback > >>driver to reset (flr/slot/bus) PCI devices based on 'reset' SysFS > >>attribute. > >> > >>Signed-off-by: Govinda Tatti <Govinda.Tatti@Oracle.COM> > >>--- > >>v3: -New > >> > >> drivers/pci/pci.c | 3 ++- > >> include/linux/pci.h | 1 + > >> 2 files changed, 3 insertions(+), 1 deletion(-) > >> > >>diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > >>index 6078dfc..499e922 100644 > >>--- a/drivers/pci/pci.c > >>+++ b/drivers/pci/pci.c > >>@@ -3872,7 +3872,7 @@ static void pci_flr_wait(struct pci_dev *dev) > >> * Returns true if the device advertises support for PCIe function level > >> * resets. > >> */ > >>-static bool pcie_has_flr(struct pci_dev *dev) > >>+bool pcie_has_flr(struct pci_dev *dev) > >> { > >> u32 cap; > >>@@ -3882,6 +3882,7 @@ static bool pcie_has_flr(struct pci_dev *dev) > >> pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap); > >> return cap & PCI_EXP_DEVCAP_FLR; > >> } > >>+EXPORT_SYMBOL_GPL(pcie_has_flr); > >I'd rather change pcie_flr() so you could *always* call it, and it > >would return 0, -ENOTTY, or whatever, based on whether FLR is > >supported. Is that feasible? > Sure, I will add pcie_has_flr() logic inside pcie_flr() and return > appropriate > values as suggested by you. Do we still want to retain pcie_has_flr() and > its usage inside pci.c?.Otherwise, I will remove it and do required cleanup. If you can restructure the code and remove pcie_has_flr() while retaining the existing behavior of its callers, that would be great. Bjorn _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 40+ messages in thread
[parent not found: <20171212005919.GB30595@bhelgaas-glaptop.roam.corp.google.com>]
* Re: [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface [not found] ` <20171212005919.GB30595@bhelgaas-glaptop.roam.corp.google.com> @ 2017-12-13 20:46 ` Govinda Tatti [not found] ` <49956aaf-5fd5-939d-5fc7-231ffdb98b70@Oracle.COM> 1 sibling, 0 replies; 40+ messages in thread From: Govinda Tatti @ 2017-12-13 20:46 UTC (permalink / raw) To: Bjorn Helgaas Cc: jgross, linux-pci, linux-kernel, JBeulich, bhelgaas, xen-devel, boris.ostrovsky, roger.pau >>>> -static bool pcie_has_flr(struct pci_dev *dev) >>>> +bool pcie_has_flr(struct pci_dev *dev) >>>> { >>>> u32 cap; >>>> @@ -3882,6 +3882,7 @@ static bool pcie_has_flr(struct pci_dev *dev) >>>> pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap); >>>> return cap & PCI_EXP_DEVCAP_FLR; >>>> } >>>> +EXPORT_SYMBOL_GPL(pcie_has_flr); >>> I'd rather change pcie_flr() so you could *always* call it, and it >>> would return 0, -ENOTTY, or whatever, based on whether FLR is >>> supported. Is that feasible? >> Sure, I will add pcie_has_flr() logic inside pcie_flr() and return >> appropriate >> values as suggested by you. Do we still want to retain pcie_has_flr() and >> its usage inside pci.c?.Otherwise, I will remove it and do required cleanup. > If you can restructure the code and remove pcie_has_flr() while > retaining the existing behavior of its callers, that would be great. I checked the current usage of pcie_has_flr() and pcie_flr(). I have a couple of questions or need some clarification. 1. pcie_has_flr() usage inside pci_probe_reset_function(). This function is only calling pcie_has_flr() but not pcie_flr(). Rest of the code is trying to do specific type of reset except pcie_flr(). rc = pci_dev_specific_reset(dev, 1); if (rc != -ENOTTY) return rc; if (pcie_has_flr(dev)) return 0; rc = pci_af_flr(dev, 1); if (rc != -ENOTTY) return rc; In other-words, I can remove usage of pcie_has_flr() in all other places in pci.c except in above function. 2. W.r.t pcie_flr(), I am planning to return error code. Currently, the following file/modules are calling this function. My plan is to add a check for return code and print a WANRING message if return code is NON-ZERO. I hope this is sufficient for this patch. drivers/crypto/qat/qat_common/adf_aer.c drivers/infiniband/hw/hfi1/chip.c (2 places) drivers/net/ethernet/cavium/liquidio/lio_vf_main.c drivers/net/ethernet/intel/ixgbe/ixgbe_main.c (2 places) drivers/pci/quirks.c (2 places) Cheers GOVINDA _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 40+ messages in thread
[parent not found: <49956aaf-5fd5-939d-5fc7-231ffdb98b70@Oracle.COM>]
* Re: [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface [not found] ` <49956aaf-5fd5-939d-5fc7-231ffdb98b70@Oracle.COM> @ 2017-12-13 21:24 ` Bjorn Helgaas [not found] ` <20171213212420.GH30595@bhelgaas-glaptop.roam.corp.google.com> 1 sibling, 0 replies; 40+ messages in thread From: Bjorn Helgaas @ 2017-12-13 21:24 UTC (permalink / raw) To: Govinda Tatti Cc: jgross, linux-pci, linux-kernel, Christoph Hellwig, JBeulich, bhelgaas, xen-devel, boris.ostrovsky, roger.pau [+cc Christoph] On Wed, Dec 13, 2017 at 02:46:57PM -0600, Govinda Tatti wrote: > > >>>>-static bool pcie_has_flr(struct pci_dev *dev) > >>>>+bool pcie_has_flr(struct pci_dev *dev) > >>>> { > >>>> u32 cap; > >>>>@@ -3882,6 +3882,7 @@ static bool pcie_has_flr(struct pci_dev *dev) > >>>> pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap); > >>>> return cap & PCI_EXP_DEVCAP_FLR; > >>>> } > >>>>+EXPORT_SYMBOL_GPL(pcie_has_flr); > >>>I'd rather change pcie_flr() so you could *always* call it, and it > >>>would return 0, -ENOTTY, or whatever, based on whether FLR is > >>>supported. Is that feasible? > >>Sure, I will add pcie_has_flr() logic inside pcie_flr() and return > >>appropriate > >>values as suggested by you. Do we still want to retain pcie_has_flr() and > >>its usage inside pci.c?.Otherwise, I will remove it and do required cleanup. > >If you can restructure the code and remove pcie_has_flr() while > >retaining the existing behavior of its callers, that would be great. > I checked the current usage of pcie_has_flr() and pcie_flr(). I have > a couple > of questions or need some clarification. > > 1. pcie_has_flr() usage inside pci_probe_reset_function(). > > This function is only calling pcie_has_flr() but not pcie_flr(). > Rest of the code is trying to do specific type of reset except > pcie_flr(). > > rc = pci_dev_specific_reset(dev, 1); > if (rc != -ENOTTY) > return rc; > if (pcie_has_flr(dev)) > return 0; > rc = pci_af_flr(dev, 1); > if (rc != -ENOTTY) > return rc; > > In other-words, I can remove usage of pcie_has_flr() in all other places > in pci.c except in above function. I think we should keep the EXPORT_SYMBOL_GPL() part of a60a2b73ba69 ("PCI: Export pcie_flr()"), but revert the restructuring part. Prior to a60a2b73ba69, we had int pcie_flr(struct pci_dev *dev, int probe); like all the other reset methods. AFAICT, the addition of pcie_has_flr() was to optimize the path slightly because when drivers call pcie_flr(), they should already know that their hardware supports FLR. But I don't think that optimization is worth the extra code complexity. If we do need to optimize it, we can check this in the core during enumeration and set PCI_DEV_FLAGS_NO_FLR_RESET accordingly. Christoph, chime in if I'm missing something here. > 2. W.r.t pcie_flr(), I am planning to return error code. Currently, > the following > file/modules are calling this function. My plan is to add a check > for return > code and print a WANRING message if return code is NON-ZERO. I > hope this is > sufficient for this patch. > > drivers/crypto/qat/qat_common/adf_aer.c > drivers/infiniband/hw/hfi1/chip.c (2 places) > drivers/net/ethernet/cavium/liquidio/lio_vf_main.c > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c (2 places) > drivers/pci/quirks.c (2 places) Checking the return code is probably overkill, since pcie_flr() is void and returns no errors now. The only point of the return value is to tell whether the device supports FLR. If we call it with "probe == 0" there's no useful error to return. Bjorn _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 40+ messages in thread
[parent not found: <20171213212420.GH30595@bhelgaas-glaptop.roam.corp.google.com>]
* Re: [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface [not found] ` <20171213212420.GH30595@bhelgaas-glaptop.roam.corp.google.com> @ 2017-12-14 12:52 ` Christoph Hellwig [not found] ` <20171214125206.GA24958@infradead.org> 2017-12-15 15:48 ` Govinda Tatti 2 siblings, 0 replies; 40+ messages in thread From: Christoph Hellwig @ 2017-12-14 12:52 UTC (permalink / raw) To: Bjorn Helgaas Cc: jgross, Govinda Tatti, linux-pci, linux-kernel, Christoph Hellwig, JBeulich, bhelgaas, xen-devel, boris.ostrovsky, roger.pau On Wed, Dec 13, 2017 at 03:24:21PM -0600, Bjorn Helgaas wrote: > Prior to a60a2b73ba69, we had > > int pcie_flr(struct pci_dev *dev, int probe); > > like all the other reset methods. AFAICT, the addition of > pcie_has_flr() was to optimize the path slightly because when drivers > call pcie_flr(), they should already know that their hardware supports > FLR. But I don't think that optimization is worth the extra code > complexity. If we do need to optimize it, we can check this in the > core during enumeration and set PCI_DEV_FLAGS_NO_FLR_RESET > accordingly. > > Christoph, chime in if I'm missing something here. Didn't we just have that discussion in another thread a few days ago? I think that the pcie_has_flr was a mistake in retrospective but I think the bool probe API was an even bigger mistake. The only use of it is to hide the reset attribute in sysfs. I'd much rather always have it and have it return EOPNOTSUPP if no reset method is supported. I can send a patch for that if it sounds fine to you. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 40+ messages in thread
[parent not found: <20171214125206.GA24958@infradead.org>]
* Re: [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface [not found] ` <20171214125206.GA24958@infradead.org> @ 2017-12-15 0:24 ` Bjorn Helgaas 0 siblings, 0 replies; 40+ messages in thread From: Bjorn Helgaas @ 2017-12-15 0:24 UTC (permalink / raw) To: Christoph Hellwig Cc: jgross, Govinda Tatti, linux-pci, linux-kernel, JBeulich, bhelgaas, xen-devel, boris.ostrovsky, roger.pau On Thu, Dec 14, 2017 at 04:52:06AM -0800, Christoph Hellwig wrote: > On Wed, Dec 13, 2017 at 03:24:21PM -0600, Bjorn Helgaas wrote: > > Prior to a60a2b73ba69, we had > > > > int pcie_flr(struct pci_dev *dev, int probe); > > > > like all the other reset methods. AFAICT, the addition of > > pcie_has_flr() was to optimize the path slightly because when drivers > > call pcie_flr(), they should already know that their hardware supports > > FLR. But I don't think that optimization is worth the extra code > > complexity. If we do need to optimize it, we can check this in the > > core during enumeration and set PCI_DEV_FLAGS_NO_FLR_RESET > > accordingly. > > > > Christoph, chime in if I'm missing something here. > > Didn't we just have that discussion in another thread a few days > ago? Probably, but I didn't have a clear picture of what you were suggesting. > I think that the pcie_has_flr was a mistake in retrospective but I > think the bool probe API was an even bigger mistake. The only use > of it is to hide the reset attribute in sysfs. I'd much rather always > have it and have it return EOPNOTSUPP if no reset method is supported. > > I can send a patch for that if it sounds fine to you. If you can get rid of the whole probe infrastructure, that sounds good to me. Bjorn _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface [not found] ` <20171213212420.GH30595@bhelgaas-glaptop.roam.corp.google.com> 2017-12-14 12:52 ` Christoph Hellwig [not found] ` <20171214125206.GA24958@infradead.org> @ 2017-12-15 15:48 ` Govinda Tatti 2017-12-15 18:18 ` Bjorn Helgaas [not found] ` <20171215181801.GU30595@bhelgaas-glaptop.roam.corp.google.com> 2 siblings, 2 replies; 40+ messages in thread From: Govinda Tatti @ 2017-12-15 15:48 UTC (permalink / raw) To: Bjorn Helgaas, Christoph Hellwig Cc: jgross, linux-pci, linux-kernel, JBeulich, bhelgaas, xen-devel, boris.ostrovsky, roger.pau Thanks Bjorn and Christophfor your response. Please see below for my comments. On 12/13/2017 3:24 PM, Bjorn Helgaas wrote: > [+cc Christoph] > > On Wed, Dec 13, 2017 at 02:46:57PM -0600, Govinda Tatti wrote: >>>>>> -static bool pcie_has_flr(struct pci_dev *dev) >>>>>> +bool pcie_has_flr(struct pci_dev *dev) >>>>>> { >>>>>> u32 cap; >>>>>> @@ -3882,6 +3882,7 @@ static bool pcie_has_flr(struct pci_dev *dev) >>>>>> pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap); >>>>>> return cap & PCI_EXP_DEVCAP_FLR; >>>>>> } >>>>>> +EXPORT_SYMBOL_GPL(pcie_has_flr); >>>>> I'd rather change pcie_flr() so you could *always* call it, and it >>>>> would return 0, -ENOTTY, or whatever, based on whether FLR is >>>>> supported. Is that feasible? >>>> Sure, I will add pcie_has_flr() logic inside pcie_flr() and return >>>> appropriate >>>> values as suggested by you. Do we still want to retain pcie_has_flr() and >>>> its usage inside pci.c?.Otherwise, I will remove it and do required cleanup. >>> If you can restructure the code and remove pcie_has_flr() while >>> retaining the existing behavior of its callers, that would be great. >> I checked the current usage of pcie_has_flr() and pcie_flr(). I have >> a couple >> of questions or need some clarification. >> >> 1. pcie_has_flr() usage inside pci_probe_reset_function(). >> >> This function is only calling pcie_has_flr() but not pcie_flr(). >> Rest of the code is trying to do specific type of reset except >> pcie_flr(). >> >> rc = pci_dev_specific_reset(dev, 1); >> if (rc != -ENOTTY) >> return rc; >> if (pcie_has_flr(dev)) >> return 0; >> rc = pci_af_flr(dev, 1); >> if (rc != -ENOTTY) >> return rc; >> >> In other-words, I can remove usage of pcie_has_flr() in all other places >> in pci.c except in above function. > I think we should keep the EXPORT_SYMBOL_GPL() part of a60a2b73ba69 > ("PCI: Export pcie_flr()"), but revert the restructuring part. > > Prior to a60a2b73ba69, we had > > int pcie_flr(struct pci_dev *dev, int probe); > > like all the other reset methods. AFAICT, the addition of > pcie_has_flr() was to optimize the path slightly because when drivers > call pcie_flr(), they should already know that their hardware supports > FLR. But I don't think that optimization is worth the extra code > complexity. If we do need to optimize it, we can check this in the > core during enumeration and set PCI_DEV_FLAGS_NO_FLR_RESET > accordingly. > > Christoph, chime in if I'm missing something here. Not all code paths are aware of FLR capability and also, not using pcie_flr(). For example, arch/powerpc/platforms/powernv/eeh-powernv.c drivers/crypto/cavium/nitrox/nitrox_main.c drivers/net/ethernet/cavium/liquidio/octeon_mailbox.c So, we should consider one of these options. - set PCI_DEV_FLAGS_NO_FLR_RESET if it is not supported. - pcie_flr() should return if it is not supported If we modify pcie_flr() to return error codes, then we need to modify all existing modules that are calling this function. Please let me know your preference, so that I can move accordingly. Thanks. Cheers GOVINDA _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface 2017-12-15 15:48 ` Govinda Tatti @ 2017-12-15 18:18 ` Bjorn Helgaas [not found] ` <20171215181801.GU30595@bhelgaas-glaptop.roam.corp.google.com> 1 sibling, 0 replies; 40+ messages in thread From: Bjorn Helgaas @ 2017-12-15 18:18 UTC (permalink / raw) To: Govinda Tatti Cc: jgross, Sinan Kaya, Srikanth Jampala, Herbert Xu, Satanand Burla, linux-pci, Felix Manlunas, linux-kernel, Derek Chickles, Christoph Hellwig, JBeulich, Russell Currey, bhelgaas, xen-devel, boris.ostrovsky, Raghu Vatsavayi, roger.pau [+cc Russell, Sinan, Herbert, Srikanth, Derek, Satanand, Felix, Raghu] On Fri, Dec 15, 2017 at 09:48:02AM -0600, Govinda Tatti wrote: > On 12/13/2017 3:24 PM, Bjorn Helgaas wrote: > >On Wed, Dec 13, 2017 at 02:46:57PM -0600, Govinda Tatti wrote: > >>>>>>-static bool pcie_has_flr(struct pci_dev *dev) > >>>>>>+bool pcie_has_flr(struct pci_dev *dev) > >>>>>> { > >>>>>> u32 cap; > >>>>>>@@ -3882,6 +3882,7 @@ static bool pcie_has_flr(struct pci_dev *dev) > >>>>>> pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap); > >>>>>> return cap & PCI_EXP_DEVCAP_FLR; > >>>>>> } > >>>>>>+EXPORT_SYMBOL_GPL(pcie_has_flr); > >>>>>I'd rather change pcie_flr() so you could *always* call it, and > >>>>>it would return 0, -ENOTTY, or whatever, based on whether FLR > >>>>>is supported. Is that feasible? > >>>>Sure, I will add pcie_has_flr() logic inside pcie_flr() and > >>>>return appropriate values as suggested by you. Do we still want > >>>>to retain pcie_has_flr() and its usage inside pci.c?.Otherwise, > >>>>I will remove it and do required cleanup. > >>>If you can restructure the code and remove pcie_has_flr() while > >>>retaining the existing behavior of its callers, that would be > >>>great. > >>I checked the current usage of pcie_has_flr() and pcie_flr(). I > >>have a couple of questions or need some clarification. > >> > >>1. pcie_has_flr() usage inside pci_probe_reset_function(). > >> > >> This function is only calling pcie_has_flr() but not pcie_flr(). > >> Rest of the code is trying to do specific type of reset except > >> pcie_flr(). > >> > >> rc = pci_dev_specific_reset(dev, 1); > >> if (rc != -ENOTTY) > >> return rc; > >> if (pcie_has_flr(dev)) > >> return 0; > >> rc = pci_af_flr(dev, 1); > >> if (rc != -ENOTTY) > >> return rc; > >> > >> In other-words, I can remove usage of pcie_has_flr() in all > >> other places in pci.c except in above function. > >I think we should keep the EXPORT_SYMBOL_GPL() part of a60a2b73ba69 > >("PCI: Export pcie_flr()"), but revert the restructuring part. > > > >Prior to a60a2b73ba69, we had > > > > int pcie_flr(struct pci_dev *dev, int probe); > > > >like all the other reset methods. AFAICT, the addition of > >pcie_has_flr() was to optimize the path slightly because when > >drivers call pcie_flr(), they should already know that their > >hardware supports FLR. But I don't think that optimization is > >worth the extra code complexity. If we do need to optimize it, we > >can check this in the core during enumeration and set > >PCI_DEV_FLAGS_NO_FLR_RESET accordingly. > Not all code paths are aware of FLR capability and also, not > using pcie_flr(). For example, > > arch/powerpc/platforms/powernv/eeh-powernv.c I assume you're referring to pnv_eeh_do_flr() (which contains code similar to pcie_flr()) and pnv_eeh_do_af_flr() (which has code similar to pci_af_flr()). I agree that those are problematic and would ideally be unified with the PCI core implementations. Powerpc has quite a bit of this sort of special-case code for several reasons, some just historical and some more concrete, so I don't know how feasible this is. > drivers/crypto/cavium/nitrox/nitrox_main.c This has nitrox_reset_device(), which should definitely be replaced with a core interface. > drivers/net/ethernet/cavium/liquidio/octeon_mailbox.c And this has octeon_mbox_process_cmd() which also does a home-grown PCI_EXP_DEVCTL_BCR_FLR request and also should definitely use a core interface. > So, we should consider one of these options. > > - set PCI_DEV_FLAGS_NO_FLR_RESET if it is not supported. > - pcie_flr() should return if it is not supported > > If we modify pcie_flr() to return error codes, then we need to modify > all existing modules that are calling this function. Yes, of course. > Please let me know your preference, so that I can move accordingly. Thanks. I think Christoph volunteered to do some restructuring, but I don't know his timeframe. If you can, I would probably wait for that because there's so much overlap here. The other paths that use PCI_EXP_DEVCTL_BCR_FLR are definitely issues and should be fixed, but again should wait for the revised pcie_flr() interface. And if they're not actually required for your Xen issue, they sound like "nice to have" cleanups that will not gate your Xen fixes. I added this to my ever-growing list of cleanups to do. Bjorn _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 40+ messages in thread
[parent not found: <20171215181801.GU30595@bhelgaas-glaptop.roam.corp.google.com>]
* Re: [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface [not found] ` <20171215181801.GU30595@bhelgaas-glaptop.roam.corp.google.com> @ 2017-12-15 20:01 ` Govinda Tatti 2017-12-18 3:09 ` Alexey Kardashevskiy ` (2 subsequent siblings) 3 siblings, 0 replies; 40+ messages in thread From: Govinda Tatti @ 2017-12-15 20:01 UTC (permalink / raw) To: Bjorn Helgaas Cc: jgross, Satanand Burla, Srikanth Jampala, Herbert Xu, Christoph Hellwig, linux-pci, Russell Currey, linux-kernel, Derek Chickles, Sinan Kaya, JBeulich, Felix Manlunas, bhelgaas, xen-devel, boris.ostrovsky, Raghu Vatsavayi, roger.pau Thanks Bjorn for your response. Please see below for my comments. >> So, we should consider one of these options. >> >> - set PCI_DEV_FLAGS_NO_FLR_RESET if it is not supported. >> - pcie_flr() should return if it is not supported >> >> If we modify pcie_flr() to return error codes, then we need to modify >> all existing modules that are calling this function. > Yes, of course. > >> Please let me know your preference, so that I can move accordingly. Thanks. > I think Christoph volunteered to do some restructuring, but I don't > know his timeframe. If you can, I would probably wait for that > because there's so much overlap here. OK. > > The other paths that use PCI_EXP_DEVCTL_BCR_FLR are definitely issues > and should be fixed, but again should wait for the revised pcie_flr() > interface. And if they're not actually required for your Xen issue, > they sound like "nice to have" cleanups that will not gate your Xen > fixes. I added this to my ever-growing list of cleanups to do. For now, I am planning to use existing pcie_flr() after checking FLR capability inside Xenpciback driver (like other existing pcie_flr() usage). We will switch to revised pcie_flr() once it is available. Cheers GOVINDA _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface [not found] ` <20171215181801.GU30595@bhelgaas-glaptop.roam.corp.google.com> 2017-12-15 20:01 ` Govinda Tatti @ 2017-12-18 3:09 ` Alexey Kardashevskiy 2017-12-18 12:26 ` Christoph Hellwig [not found] ` <20171218122629.GA18423@infradead.org> 3 siblings, 0 replies; 40+ messages in thread From: Alexey Kardashevskiy @ 2017-12-18 3:09 UTC (permalink / raw) To: Bjorn Helgaas, Govinda Tatti Cc: jgross, Sinan Kaya, Srikanth Jampala, Herbert Xu, Satanand Burla, linux-pci, Felix Manlunas, linux-kernel, Derek Chickles, Christoph Hellwig, JBeulich, Russell Currey, bhelgaas, xen-devel, boris.ostrovsky, Raghu Vatsavayi, roger.pau On 16/12/17 05:18, Bjorn Helgaas wrote: > [+cc Russell, Sinan, Herbert, Srikanth, Derek, Satanand, Felix, Raghu] > > On Fri, Dec 15, 2017 at 09:48:02AM -0600, Govinda Tatti wrote: >> On 12/13/2017 3:24 PM, Bjorn Helgaas wrote: >>> On Wed, Dec 13, 2017 at 02:46:57PM -0600, Govinda Tatti wrote: > >>>>>>>> -static bool pcie_has_flr(struct pci_dev *dev) >>>>>>>> +bool pcie_has_flr(struct pci_dev *dev) >>>>>>>> { >>>>>>>> u32 cap; >>>>>>>> @@ -3882,6 +3882,7 @@ static bool pcie_has_flr(struct pci_dev *dev) >>>>>>>> pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap); >>>>>>>> return cap & PCI_EXP_DEVCAP_FLR; >>>>>>>> } >>>>>>>> +EXPORT_SYMBOL_GPL(pcie_has_flr); > >>>>>>> I'd rather change pcie_flr() so you could *always* call it, and >>>>>>> it would return 0, -ENOTTY, or whatever, based on whether FLR >>>>>>> is supported. Is that feasible? > >>>>>> Sure, I will add pcie_has_flr() logic inside pcie_flr() and >>>>>> return appropriate values as suggested by you. Do we still want >>>>>> to retain pcie_has_flr() and its usage inside pci.c?.Otherwise, >>>>>> I will remove it and do required cleanup. > >>>>> If you can restructure the code and remove pcie_has_flr() while >>>>> retaining the existing behavior of its callers, that would be >>>>> great. > >>>> I checked the current usage of pcie_has_flr() and pcie_flr(). I >>>> have a couple of questions or need some clarification. >>>> >>>> 1. pcie_has_flr() usage inside pci_probe_reset_function(). >>>> >>>> This function is only calling pcie_has_flr() but not pcie_flr(). >>>> Rest of the code is trying to do specific type of reset except >>>> pcie_flr(). >>>> >>>> rc = pci_dev_specific_reset(dev, 1); >>>> if (rc != -ENOTTY) >>>> return rc; >>>> if (pcie_has_flr(dev)) >>>> return 0; >>>> rc = pci_af_flr(dev, 1); >>>> if (rc != -ENOTTY) >>>> return rc; >>>> >>>> In other-words, I can remove usage of pcie_has_flr() in all >>>> other places in pci.c except in above function. > >>> I think we should keep the EXPORT_SYMBOL_GPL() part of a60a2b73ba69 >>> ("PCI: Export pcie_flr()"), but revert the restructuring part. >>> >>> Prior to a60a2b73ba69, we had >>> >>> int pcie_flr(struct pci_dev *dev, int probe); >>> >>> like all the other reset methods. AFAICT, the addition of >>> pcie_has_flr() was to optimize the path slightly because when >>> drivers call pcie_flr(), they should already know that their >>> hardware supports FLR. But I don't think that optimization is >>> worth the extra code complexity. If we do need to optimize it, we >>> can check this in the core during enumeration and set >>> PCI_DEV_FLAGS_NO_FLR_RESET accordingly. > >> Not all code paths are aware of FLR capability and also, not >> using pcie_flr(). For example, >> >> arch/powerpc/platforms/powernv/eeh-powernv.c > > I assume you're referring to pnv_eeh_do_flr() (which contains code similar > to pcie_flr()) and pnv_eeh_do_af_flr() (which has code similar to > pci_af_flr()). I agree that those are problematic and would ideally be > unified with the PCI core implementations. > > Powerpc has quite a bit of this sort of special-case code for several > reasons, some just historical and some more concrete, so I don't know how > feasible this is. It would be lovely if pnv-eeh code used pci_af_flr() but since pnv_eeh_do_flr() uses different config space accessors (not sure why exactly, probably to avoid freezing the entire PHB), it is harder than just trivial change. I'll try and have a deeper look though. -- Alexey _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface [not found] ` <20171215181801.GU30595@bhelgaas-glaptop.roam.corp.google.com> 2017-12-15 20:01 ` Govinda Tatti 2017-12-18 3:09 ` Alexey Kardashevskiy @ 2017-12-18 12:26 ` Christoph Hellwig [not found] ` <20171218122629.GA18423@infradead.org> 3 siblings, 0 replies; 40+ messages in thread From: Christoph Hellwig @ 2017-12-18 12:26 UTC (permalink / raw) To: Bjorn Helgaas Cc: jgross, Sinan Kaya, Govinda Tatti, Herbert Xu, Satanand Burla, linux-pci, Felix Manlunas, linux-kernel, Derek Chickles, Christoph Hellwig, Srikanth Jampala, JBeulich, Russell Currey, bhelgaas, xen-devel, boris.ostrovsky, Raghu Vatsavayi, roger.pau On Fri, Dec 15, 2017 at 12:18:02PM -0600, Bjorn Helgaas wrote: > I think Christoph volunteered to do some restructuring, but I don't > know his timeframe. If you can, I would probably wait for that > because there's so much overlap here. I'll have some time over the holidays. If you need it more urgent than that feel free to take over. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 40+ messages in thread
[parent not found: <20171218122629.GA18423@infradead.org>]
* Re: [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface [not found] ` <20171218122629.GA18423@infradead.org> @ 2017-12-18 17:22 ` Govinda Tatti 2018-09-09 18:59 ` Pasi Kärkkäinen [not found] ` <20180909185944.GC18222@reaktio.net> 2 siblings, 0 replies; 40+ messages in thread From: Govinda Tatti @ 2017-12-18 17:22 UTC (permalink / raw) To: Christoph Hellwig Cc: jgross, Satanand Burla, Srikanth Jampala, Herbert Xu, linux-pci, Felix Manlunas, linux-kernel, Derek Chickles, Sinan Kaya, Bjorn Helgaas, JBeulich, Russell Currey, bhelgaas, xen-devel, boris.ostrovsky, Raghu Vatsavayi, roger.pau On 12/18/2017 6:26 AM, Christoph Hellwig wrote: > On Fri, Dec 15, 2017 at 12:18:02PM -0600, Bjorn Helgaas wrote: >> I think Christoph volunteered to do some restructuring, but I don't >> know his timeframe. If you can, I would probably wait for that >> because there's so much overlap here. > I'll have some time over the holidays. If you need it more urgent > than that feel free to take over. We will wait for your changes. Thanks Christoph. Cheers GOVINDA _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface [not found] ` <20171218122629.GA18423@infradead.org> 2017-12-18 17:22 ` Govinda Tatti @ 2018-09-09 18:59 ` Pasi Kärkkäinen [not found] ` <20180909185944.GC18222@reaktio.net> 2 siblings, 0 replies; 40+ messages in thread From: Pasi Kärkkäinen @ 2018-09-09 18:59 UTC (permalink / raw) To: Christoph Hellwig Cc: jgross, Satanand Burla, Govinda Tatti, Herbert Xu, linux-pci, Russell Currey, linux-kernel, Derek Chickles, Sinan Kaya, Srikanth Jampala, Bjorn Helgaas, JBeulich, Felix Manlunas, bhelgaas, xen-devel, boris.ostrovsky, Raghu Vatsavayi, roger.pau On Mon, Dec 18, 2017 at 04:26:30AM -0800, Christoph Hellwig wrote: > On Fri, Dec 15, 2017 at 12:18:02PM -0600, Bjorn Helgaas wrote: > > I think Christoph volunteered to do some restructuring, but I don't > > know his timeframe. If you can, I would probably wait for that > > because there's so much overlap here. > > I'll have some time over the holidays. If you need it more urgent > than that feel free to take over. > Replying to an old thread.. I noticed pcie_has_flr() has been recently exported in upstream Linux: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2d2917f7747805a1f4188672f308d82a8ba01700 Are there more changes / cleanups planned to these interfaces, as mentioned last year? (context: xen-pciback reset/do_flr features upstreaming, which kind of stalled last year when pcie_has_flr() wasn't exported at the time) Thanks, -- Pasi _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 40+ messages in thread
[parent not found: <20180909185944.GC18222@reaktio.net>]
* Re: [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface [not found] ` <20180909185944.GC18222@reaktio.net> @ 2018-09-10 2:33 ` Sinan Kaya [not found] ` <9ffe43d2-a44b-974c-85c9-9923d71c5dba@kernel.org> 1 sibling, 0 replies; 40+ messages in thread From: Sinan Kaya @ 2018-09-10 2:33 UTC (permalink / raw) To: Pasi Kärkkäinen, Christoph Hellwig Cc: jgross, Satanand Burla, Govinda Tatti, Herbert Xu, linux-pci, Russell Currey, linux-kernel, Derek Chickles, Sinan Kaya, Srikanth Jampala, Bjorn Helgaas, JBeulich, Felix Manlunas, bhelgaas, xen-devel, boris.ostrovsky, Raghu Vatsavayi, roger.pau On 9/9/2018 2:59 PM, Pasi Kärkkäinen wrote: > I noticed pcie_has_flr() has been recently exported in upstream Linux: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2d2917f7747805a1f4188672f308d82a8ba01700 > > Are there more changes / cleanups planned to these interfaces, as mentioned last year? > > (context: xen-pciback reset/do_flr features upstreaming, which kind of stalled last year when pcie_has_flr() wasn't exported at the time) Exporting pcie_has_flr() is a very simple change which could have been done by the XEN porting effort. Maybe, the right question is what is so special about XEN reset? What feature PCI core is missing to support XEN FLR reset that caused the effort to stall? _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 40+ messages in thread
[parent not found: <9ffe43d2-a44b-974c-85c9-9923d71c5dba@kernel.org>]
* Re: [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface [not found] ` <9ffe43d2-a44b-974c-85c9-9923d71c5dba@kernel.org> @ 2018-09-10 9:52 ` Pasi Kärkkäinen [not found] ` <20180910095231.GD18222@reaktio.net> 1 sibling, 0 replies; 40+ messages in thread From: Pasi Kärkkäinen @ 2018-09-10 9:52 UTC (permalink / raw) To: Sinan Kaya Cc: jgross, Sinan Kaya, Govinda Tatti, Herbert Xu, linux-pci, Russell Currey, linux-kernel, Derek Chickles, Christoph Hellwig, xen-devel, Srikanth Jampala, Bjorn Helgaas, JBeulich, Felix Manlunas, bhelgaas, Satanand Burla, boris.ostrovsky, Raghu Vatsavayi, roger.pau Hi, On Sun, Sep 09, 2018 at 10:33:02PM -0400, Sinan Kaya wrote: > On 9/9/2018 2:59 PM, Pasi Kärkkäinen wrote: > >I noticed pcie_has_flr() has been recently exported in upstream Linux: > >https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2d2917f7747805a1f4188672f308d82a8ba01700 > > > >Are there more changes / cleanups planned to these interfaces, as mentioned last year? > > > >(context: xen-pciback reset/do_flr features upstreaming, which kind of stalled last year when pcie_has_flr() wasn't exported at the time) > > Exporting pcie_has_flr() is a very simple change which could have been done > by the XEN porting effort. > > Maybe, the right question is what is so special about XEN reset? > > What feature PCI core is missing to support XEN FLR reset that caused > the effort to stall? > Well one of the reasons probably was because Christoph was planning to deprecate the pcie_has_flr() functionality.. https://lists.xen.org/archives/html/xen-devel/2017-12/msg01057.html https://lists.xen.org/archives/html/xen-devel/2017-12/msg01252.html But now that pcie_has_flr() is exported and available I guess it's fine to continue using it also for xen-pciback :) Thanks, -- Pasi _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 40+ messages in thread
[parent not found: <20180910095231.GD18222@reaktio.net>]
* Re: [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface [not found] ` <20180910095231.GD18222@reaktio.net> @ 2018-09-10 17:04 ` Sinan Kaya 0 siblings, 0 replies; 40+ messages in thread From: Sinan Kaya @ 2018-09-10 17:04 UTC (permalink / raw) To: Pasi Kärkkäinen Cc: jgross, Sinan Kaya, Govinda Tatti, Herbert Xu, linux-pci, Russell Currey, linux-kernel, Derek Chickles, Christoph Hellwig, xen-devel, Srikanth Jampala, Bjorn Helgaas, JBeulich, Felix Manlunas, bhelgaas, Satanand Burla, boris.ostrovsky, Raghu Vatsavayi, roger.pau On 9/10/2018 5:52 AM, Pasi Kärkkäinen wrote: > Hi, > > On Sun, Sep 09, 2018 at 10:33:02PM -0400, Sinan Kaya wrote: >> On 9/9/2018 2:59 PM, Pasi Kärkkäinen wrote: >>> I noticed pcie_has_flr() has been recently exported in upstream Linux: >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2d2917f7747805a1f4188672f308d82a8ba01700 >>> >>> Are there more changes / cleanups planned to these interfaces, as mentioned last year? >>> >>> (context: xen-pciback reset/do_flr features upstreaming, which kind of stalled last year when pcie_has_flr() wasn't exported at the time) >> >> Exporting pcie_has_flr() is a very simple change which could have been done >> by the XEN porting effort. >> >> Maybe, the right question is what is so special about XEN reset? >> >> What feature PCI core is missing to support XEN FLR reset that caused >> the effort to stall? >> > > Well one of the reasons probably was because Christoph was planning to deprecate the pcie_has_flr() functionality.. > > https://lists.xen.org/archives/html/xen-devel/2017-12/msg01057.html > https://lists.xen.org/archives/html/xen-devel/2017-12/msg01252.html > > But now that pcie_has_flr() is exported and available I guess it's fine to continue using it also for xen-pciback :) > Yeah, I would go ahead with the implementation. Refactoring can be done independently. > > Thanks, > > -- Pasi > > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface [not found] ` <20171208202424.GC12367@bhelgaas-glaptop.roam.corp.google.com> 2017-12-12 0:29 ` Govinda Tatti [not found] ` <426eeeab-0dcd-8de3-9c5f-a166acf2c130@Oracle.COM> @ 2017-12-12 15:07 ` Christoph Hellwig 2 siblings, 0 replies; 40+ messages in thread From: Christoph Hellwig @ 2017-12-12 15:07 UTC (permalink / raw) To: Bjorn Helgaas Cc: jgross, Govinda Tatti, linux-pci, linux-kernel, JBeulich, bhelgaas, xen-devel, boris.ostrovsky, roger.pau On Fri, Dec 08, 2017 at 02:24:24PM -0600, Bjorn Helgaas wrote: > I'd rather change pcie_flr() so you could *always* call it, and it > would return 0, -ENOTTY, or whatever, based on whether FLR is > supported. Is that feasible? > > I don't like the "Can I do this? Ok, do this" style of interfaces. > It's racy (not really applicable in this case) and seems clunky. I was tempted to change all that for the whole reset sequence but didn't dare to do that because someone probably put some thought into the current scheme. I'd love to get rid of the "can I do this" interfaces entirely, and also change to EOPNOTSUPP for the not supported case. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute [not found] <20171207222145.9769-1-Govinda.Tatti@Oracle.COM> 2017-12-07 22:21 ` [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface Govinda Tatti @ 2017-12-07 22:21 ` Govinda Tatti [not found] ` <20171207222145.9769-3-Govinda.Tatti@Oracle.COM> 2 siblings, 0 replies; 40+ messages in thread From: Govinda Tatti @ 2017-12-07 22:21 UTC (permalink / raw) To: xen-devel, linux-kernel, linux-pci, bhelgaas, boris.ostrovsky, jgross, JBeulich, roger.pau The life-cycle of a PCI device in Xen pciback is complex and is constrained by the generic PCI locking mechanism. - It starts with the device being bound to us, for which we do a function reset (done via SysFS so the PCI lock is held). - If the device is unbound from us, we also do a function reset (done via SysFS so the PCI lock is held). - If the device is un-assigned from a guest - we do a function reset (no PCI lock is held). All reset operations are done on the individual PCI function level (so bus:device:function). The reset for an individual PCI function means device must support FLR (PCIe or AF), PM reset on D3hot->D0 device specific reset, or a secondary bus reset for a singleton device on a bus but FLR does not have widespread support or it is not reliable in some cases. So, we need to provide an alternate mechanism to users to perform a slot or bus level reset. Currently, a slot or bus reset is not exposed in SysFS as there is no good way of exposing a bus topology there. This is due to the complexity - we MUST know that the different functions of a PCIe device are not in use by other drivers, or if they are in use (say one of them is assigned to a guest and the other is idle) - it is still OK to reset the slot (assuming both of them are owned by Xen pciback). This patch does that providing an option to perform a flr/slot/bus reset when a PCI device is owned by Xen PCI backend. It will try to execute one of these reset method, starting with FLR if it is supported. Otherwise, it tries slot or bus reset method. For slot or bus reset method, it also checks to make sure that all of the devices under the bridge are owned by Xen PCI backend before applying those resets. Due to the complexity with the PCI lock we cannot do the reset when a device is bound ('echo $BDF > bind') or when unbound ('echo $BDF > unbind') as the pci_[slot|bus]_reset also takes the same lock resulting in a dead-lock. Putting the reset function in a work-queue or thread won't work either - as we have to do the reset function outside the 'unbind' context (it holds the PCI lock). But once you 'unbind' a device the device is no longer under the ownership of Xen pciback and the pci_set_drvdata has been reset, so we cannot use a thread for this. Instead of doing all this complex dance, we depend on the tool-stack doing the right thing. As such, we implement 'reset' SysFS attribute which 'xl' uses when a device is detached or attached from/to a guest. It bypasses the need to worry about the PCI lock. BTW, previously defined "do_flr" attribute has been renamed to "reset" since "do_flr" name doesn't represent all PCI reset methods and plus, currently it is not being used. So, there is no impact in renaming this sysfs attribute. To not inadvertently do a bus reset that would affect devices that are in use by other drivers (other than Xen pciback) prior to the reset, we check that all of the devices under the bridge are owned by Xen pciback. If they are not, we refrain from executing the bus (or slot) reset. Signed-off-by: Govinda Tatti <Govinda.Tatti@Oracle.COM> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> --- v1: - First posting v2: - struct pcistub_args: Changed docunt field as unsigned int - pcistub_reset_dev: initialization of "struct pcistub_args" - pcistub_reset_dev: combined multiple if-statements - pcistub_do_flr: removed goto statement v3: - Resynced with linux kernel 4.14.4 and latest pci_stub.c changes. - Renamed "do_flr" SysFS attribute to "reset". Plus, modified "reset" SysFS attribute code as per the latest changes in 4.14.4. - struct pcistub_args: added "const" to "struct pci_dev *dev" field - pcistub_device_search: Renamed found_dev to found - pcistub_device_search: Modified comments and return statements - pcistub_device_reset: introduced FLR reset code - pcistub_device_reset: Modified all dev_dbg messages Documentation/ABI/testing/sysfs-driver-pciback | 15 +++ drivers/xen/xen-pciback/pci_stub.c | 128 +++++++++++++++++++++++++ 2 files changed, 143 insertions(+) diff --git a/Documentation/ABI/testing/sysfs-driver-pciback b/Documentation/ABI/testing/sysfs-driver-pciback index 6a733bf..d295b42 100644 --- a/Documentation/ABI/testing/sysfs-driver-pciback +++ b/Documentation/ABI/testing/sysfs-driver-pciback @@ -11,3 +11,18 @@ Description: #echo 00:19.0-E0:2:FF > /sys/bus/pci/drivers/pciback/quirks will allow the guest to read and write to the configuration register 0x0E. + +What: /sys/bus/pci/drivers/pciback/reset +Date: Dec 2017 +KernelVersion: 4.15 +Contact: xen-devel@lists.xenproject.org +Description: + An option to perform a flr/slot/bus reset when a PCI device + is owned by Xen PCI backend. Writing a string of DDDD:BB:DD.F + will cause the pciback driver to perform a flr or slot or bus + reset if the device supports it. It will try to execute one + of these reset method, starting with FLR if it is supported. + Otherwise, it tries slot or bus reset methods. For slot or + bus reset method, it also checks to make sure that all of the + devices under the bridge are owned by Xen PCI backend before + performing those resets. diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c index 9e480fd..cad704e 100644 --- a/drivers/xen/xen-pciback/pci_stub.c +++ b/drivers/xen/xen-pciback/pci_stub.c @@ -313,6 +313,102 @@ void pcistub_put_pci_dev(struct pci_dev *dev) up_write(&pcistub_sem); } +struct pcistub_args { + const struct pci_dev *dev; + unsigned int dcount; +}; + +static int pcistub_device_search(struct pci_dev *dev, void *data) +{ + struct pcistub_device *psdev; + struct pcistub_args *arg = data; + bool found = false; + unsigned long flags; + + spin_lock_irqsave(&pcistub_devices_lock, flags); + + list_for_each_entry(psdev, &pcistub_devices, dev_list) { + if (psdev->dev == dev) { + found = true; + arg->dcount++; + break; + } + } + + spin_unlock_irqrestore(&pcistub_devices_lock, flags); + + /* Device not owned by pcistub. Abort the walk */ + if (!found) { + arg->dev = dev; + return 1; + } + + return 0; +} + +static int pcistub_device_reset(struct pci_dev *dev) +{ + struct xen_pcibk_dev_data *dev_data; + bool slot = false, bus = false; + struct pcistub_args arg = {}; + + if (!dev) + return -EINVAL; + + dev_dbg(&dev->dev, "[%s]\n", __func__); + + /* First check and try FLR */ + if (pcie_has_flr(dev)) { + dev_dbg(&dev->dev, "resetting %s device using FLR\n", + pci_name(dev)); + pcie_flr(dev); + return 0; + } + + if (!pci_probe_reset_slot(dev->slot)) + slot = true; + else if ((!pci_probe_reset_bus(dev->bus)) && + (!pci_is_root_bus(dev->bus))) + bus = true; + + if (!bus && !slot) + return -EOPNOTSUPP; + + /* + * Make sure all devices on this bus are owned by the + * PCI backend so that we can safely reset the whole bus. + */ + pci_walk_bus(dev->bus, pcistub_device_search, &arg); + + /* All devices under the bus should be part of pcistub! */ + if (arg.dev) { + dev_err(&dev->dev, + "%s on the same bus as %s and is not owned by " + DRV_NAME "\n", pci_name(arg.dev), pci_name(dev)); + + return -EBUSY; + } + + dev_dbg(&dev->dev, "pcistub owns %d devices on PCI Bus %04x:%02x", + arg.dcount, pci_domain_nr(dev->bus), dev->bus->number); + + dev_data = pci_get_drvdata(dev); + if (!pci_load_saved_state(dev, dev_data->pci_saved_state)) + pci_restore_state(dev); + + /* This disables the device. */ + xen_pcibk_reset_device(dev); + + /* Cleanup up any emulated fields */ + xen_pcibk_config_reset_dev(dev); + + dev_dbg(&dev->dev, "resetting %s device using %s reset\n", + pci_name(dev), slot ? "slot" : "bus"); + + return slot ? pci_try_reset_slot(dev->slot) : + pci_try_reset_bus(dev->bus); +} + static int pcistub_match_one(struct pci_dev *dev, struct pcistub_device_id *pdev_id) { @@ -1430,6 +1526,32 @@ static ssize_t permissive_show(struct device_driver *drv, char *buf) } static DRIVER_ATTR_RW(permissive); +static ssize_t reset_store(struct device_driver *drv, const char *buf, + size_t count) +{ + struct pcistub_device *psdev; + int domain, bus, slot, func; + int err; + + err = str_to_slot(buf, &domain, &bus, &slot, &func); + if (err) + return err; + + psdev = pcistub_device_find(domain, bus, slot, func); + if (psdev) { + err = pcistub_device_reset(psdev->dev); + pcistub_device_put(psdev); + } else { + err = -ENODEV; + } + + if (!err) + err = count; + + return err; +} +static DRIVER_ATTR_WO(reset); + static void pcistub_exit(void) { driver_remove_file(&xen_pcibk_pci_driver.driver, &driver_attr_new_slot); @@ -1443,6 +1565,8 @@ static void pcistub_exit(void) &driver_attr_irq_handlers); driver_remove_file(&xen_pcibk_pci_driver.driver, &driver_attr_irq_handler_state); + driver_remove_file(&xen_pcibk_pci_driver.driver, + &driver_attr_reset); pci_unregister_driver(&xen_pcibk_pci_driver); } @@ -1536,6 +1660,10 @@ static int __init pcistub_init(void) if (!err) err = driver_create_file(&xen_pcibk_pci_driver.driver, &driver_attr_irq_handler_state); + if (!err) + err = driver_create_file(&xen_pcibk_pci_driver.driver, + &driver_attr_reset); + if (err) pcistub_exit(); -- 2.9.5 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 40+ messages in thread
[parent not found: <20171207222145.9769-3-Govinda.Tatti@Oracle.COM>]
* Re: [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute [not found] ` <20171207222145.9769-3-Govinda.Tatti@Oracle.COM> @ 2017-12-08 9:34 ` Jan Beulich 2017-12-12 14:48 ` Govinda Tatti 2017-12-12 15:01 ` Govinda Tatti 0 siblings, 2 replies; 40+ messages in thread From: Jan Beulich @ 2017-12-08 9:34 UTC (permalink / raw) To: Govinda Tatti Cc: Juergen Gross, linux-pci, linux-kernel, bhelgaas, xen-devel, boris.ostrovsky, roger.pau >>> On 07.12.17 at 23:21, <Govinda.Tatti@Oracle.COM> wrote: > Due to the complexity with the PCI lock we cannot do the reset when a > device is bound ('echo $BDF > bind') or when unbound ('echo $BDF > unbind') > as the pci_[slot|bus]_reset also takes the same lock resulting in a > dead-lock. It took me a moment to figure that here you're referring to the process of (un)binding, not the state. To avoid that ambiguity in wording, how about "... we cannot do the reset while a device is being bound (...) or while it is being unbound ..."? > --- a/Documentation/ABI/testing/sysfs-driver-pciback > +++ b/Documentation/ABI/testing/sysfs-driver-pciback > @@ -11,3 +11,18 @@ Description: > #echo 00:19.0-E0:2:FF > /sys/bus/pci/drivers/pciback/quirks > will allow the guest to read and write to the configuration > register 0x0E. > + > +What: /sys/bus/pci/drivers/pciback/reset > +Date: Dec 2017 > +KernelVersion: 4.15 > +Contact: xen-devel@lists.xenproject.org > +Description: > + An option to perform a flr/slot/bus reset when a PCI device > + is owned by Xen PCI backend. Writing a string of DDDD:BB:DD.F SSSS:BB:DD.F (or else the D-s are ambiguous, the more that "domain" in Xen code is ambiguous anyway - I continue to be mislead by struct pcistub_device_id's domain field) Also I assume the SSSS part is optional (default zero), which probably can and should be expressed in some way. > --- a/drivers/xen/xen-pciback/pci_stub.c > +++ b/drivers/xen/xen-pciback/pci_stub.c > @@ -313,6 +313,102 @@ void pcistub_put_pci_dev(struct pci_dev *dev) > up_write(&pcistub_sem); > } > > +struct pcistub_args { > + const struct pci_dev *dev; > + unsigned int dcount; The sole use of this field is for a debug message. Why not drop it and make "dev" the "data" argument without further indirection? > +static int pcistub_device_search(struct pci_dev *dev, void *data) > +{ > + struct pcistub_device *psdev; > + struct pcistub_args *arg = data; > + bool found = false; > + unsigned long flags; > + > + spin_lock_irqsave(&pcistub_devices_lock, flags); > + > + list_for_each_entry(psdev, &pcistub_devices, dev_list) { > + if (psdev->dev == dev) { > + found = true; > + arg->dcount++; > + break; Neither here nor in the caller I can see a check whether the device is currently assigned to a guest. Ownership by pciback alone imo is not sufficient to allow a reset to be performed. > +static int pcistub_device_reset(struct pci_dev *dev) > +{ > + struct xen_pcibk_dev_data *dev_data; > + bool slot = false, bus = false; > + struct pcistub_args arg = {}; > + > + if (!dev) > + return -EINVAL; > + > + dev_dbg(&dev->dev, "[%s]\n", __func__); > + > + /* First check and try FLR */ > + if (pcie_has_flr(dev)) { > + dev_dbg(&dev->dev, "resetting %s device using FLR\n", > + pci_name(dev)); > + pcie_flr(dev); The lack of error check here puzzled me, but I see the function indeed returns void right now. I think the prereq patch should change this along with exporting the function - you really don't want the device to be handed to a guest when the FLR timed out. > + return 0; > + } > + > + if (!pci_probe_reset_slot(dev->slot)) > + slot = true; > + else if ((!pci_probe_reset_bus(dev->bus)) && > + (!pci_is_root_bus(dev->bus))) Too many parentheses for my taste. > +static ssize_t reset_store(struct device_driver *drv, const char *buf, > + size_t count) > +{ > + struct pcistub_device *psdev; > + int domain, bus, slot, func; > + int err; > + > + err = str_to_slot(buf, &domain, &bus, &slot, &func); > + if (err) > + return err; > + > + psdev = pcistub_device_find(domain, bus, slot, func); > + if (psdev) { > + err = pcistub_device_reset(psdev->dev); > + pcistub_device_put(psdev); > + } else { > + err = -ENODEV; > + } > + > + if (!err) > + err = count; > + > + return err; > +} > +static DRIVER_ATTR_WO(reset); Would it be worth for reads of the file to return whether the device can be reset this way (i.e. the result of the checks you do before actually doing the reset)? Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute 2017-12-08 9:34 ` Jan Beulich @ 2017-12-12 14:48 ` Govinda Tatti 2017-12-12 15:01 ` Jan Beulich ` (3 more replies) 2017-12-12 15:01 ` Govinda Tatti 1 sibling, 4 replies; 40+ messages in thread From: Govinda Tatti @ 2017-12-12 14:48 UTC (permalink / raw) To: Jan Beulich Cc: Juergen Gross, linux-pci, linux-kernel, bhelgaas, xen-devel, boris.ostrovsky, roger.pau [-- Attachment #1: Type: text/html, Size: 7647 bytes --] [-- Attachment #2: Type: text/plain, Size: 157 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute 2017-12-12 14:48 ` Govinda Tatti @ 2017-12-12 15:01 ` Jan Beulich [not found] ` <5A2FFD690200007800196DFB@prv-mh.provo.novell.com> ` (2 subsequent siblings) 3 siblings, 0 replies; 40+ messages in thread From: Jan Beulich @ 2017-12-12 15:01 UTC (permalink / raw) To: Govinda Tatti Cc: Juergen Gross, linux-pci, linux-kernel, bhelgaas, xen-devel, boris.ostrovsky, roger.pau >>> On 12.12.17 at 15:48, <Govinda.Tatti@Oracle.COM> wrote: > Thanks Jan for your review comments. Please see below for my comments. First of all - can you please do something about your reply style? HTML mail should be avoided. You'll see that the (plain text) reply as a result is rather hard to follow, too. > --- a/Documentation/ABI/testing/sysfs-driver-pciback > +++ b/Documentation/ABI/testing/sysfs-driver-pciback > @@ -11,3 +11,18 @@ Description: > #echo 00:19.0-E0:2:FF > /sys/bus/pci/drivers/pciback/quirks > will allow the guest to read and write to the configuration > register 0x0E. > + > +What: /sys/bus/pci/drivers/pciback/reset > +Date: Dec 2017 > +KernelVersion: 4.15 > +Contact: xen-devel@lists.xenproject.org > +Description: > + An option to perform a flr/slot/bus reset when a PCI device > + is owned by Xen PCI backend. Writing a string of DDDD:BB:DD.F > SSSS:BB:DD.F (or else the D-s are ambiguous, the more that "domain" > in Xen code is ambiguous anyway - I continue to be mislead by struct > pcistub_device_id's domain field) Thanks for catching this issue. I will > fix it. > > > Also I assume the SSSS part is optional (default zero), which > probably can and should be expressed in some way. SSSS can be 0 or > non-zero, subject to system configuration. The question isn't system configuration, but whether the field can be omitted on input, with zero being assumed in such a case. That's a common shorthand, considering that the vast majority of x86 (and maybe other) systems aren't using segments other than zero. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 40+ messages in thread
[parent not found: <5A2FFD690200007800196DFB@prv-mh.provo.novell.com>]
* Re: [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute [not found] ` <5A2FFD690200007800196DFB@prv-mh.provo.novell.com> @ 2017-12-12 15:14 ` Govinda Tatti 0 siblings, 0 replies; 40+ messages in thread From: Govinda Tatti @ 2017-12-12 15:14 UTC (permalink / raw) To: Jan Beulich Cc: Juergen Gross, linux-pci, linux-kernel, bhelgaas, xen-devel, boris.ostrovsky, roger.pau On 12/12/2017 9:01 AM, Jan Beulich wrote: >>>> On 12.12.17 at 15:48, <Govinda.Tatti@Oracle.COM> wrote: >> Thanks Jan for your review comments. Please see below for my comments. > First of all - can you please do something about your reply style? > HTML mail should be avoided. You'll see that the (plain text) reply > as a result is rather hard to follow, too. Sorry about it. I had an issue with my Thunderbird setting. > >> --- a/Documentation/ABI/testing/sysfs-driver-pciback >> +++ b/Documentation/ABI/testing/sysfs-driver-pciback >> @@ -11,3 +11,18 @@ Description: >> #echo 00:19.0-E0:2:FF > /sys/bus/pci/drivers/pciback/quirks >> will allow the guest to read and write to the configuration >> register 0x0E. >> + >> +What: /sys/bus/pci/drivers/pciback/reset >> +Date: Dec 2017 >> +KernelVersion: 4.15 >> +Contact: xen-devel@lists.xenproject.org >> +Description: >> + An option to perform a flr/slot/bus reset when a PCI device >> + is owned by Xen PCI backend. Writing a string of DDDD:BB:DD.F >> SSSS:BB:DD.F (or else the D-s are ambiguous, the more that "domain" >> in Xen code is ambiguous anyway - I continue to be mislead by struct >> pcistub_device_id's domain field) Thanks for catching this issue. I will >> fix it. >> >> >> Also I assume the SSSS part is optional (default zero), which >> probably can and should be expressed in some way. SSSS can be 0 or >> non-zero, subject to system configuration. > The question isn't system configuration, but whether the field can > be omitted on input, with zero being assumed in such a case. That's > a common shorthand, considering that the vast majority of x86 > (and maybe other) systems aren't using segments other than zero Yes, it can be omitted if SSSS is zero.I will add this information to above documentation file. Cheers GOVINDA _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute 2017-12-12 14:48 ` Govinda Tatti 2017-12-12 15:01 ` Jan Beulich [not found] ` <5A2FFD690200007800196DFB@prv-mh.provo.novell.com> @ 2017-12-15 19:52 ` Govinda Tatti [not found] ` <f19dbb09-ef22-2cf4-fb38-2a7c42b5dc48@Oracle.COM> 3 siblings, 0 replies; 40+ messages in thread From: Govinda Tatti @ 2017-12-15 19:52 UTC (permalink / raw) To: Jan Beulich Cc: Juergen Gross, linux-pci, linux-kernel, bhelgaas, xen-devel, boris.ostrovsky, roger.pau Jan, One quick update on pcie_flr() specific implementation. Please see below. >>> +static int pcistub_device_reset(struct pci_dev *dev) >>> +{ >>> + struct xen_pcibk_dev_data *dev_data; >>> + bool slot = false, bus = false; >>> + struct pcistub_args arg = {}; >>> + >>> + if (!dev) >>> + return -EINVAL; >>> + >>> + dev_dbg(&dev->dev, "[%s]\n", __func__); >>> + >>> + /* First check and try FLR */ >>> + if (pcie_has_flr(dev)) { >>> + dev_dbg(&dev->dev, "resetting %s device using FLR\n", >>> + pci_name(dev)); >>> + pcie_flr(dev); >> The lack of error check here puzzled me, but I see the function >> indeed returns void right now. I think the prereq patch should >> change this along with exporting the function - you really don't >> want the device to be handed to a guest when the FLR timed >> out. > We will change pcie_flr() to return error code. I will make this change > in the next version of this patch. I exchanged some emails with Bjorn/Christoph and it looks like Christoph as some planto restructure pcie flr specific functions but I don't know the exact time-frame. For now,I am planning to use existing pcie_flr() after checking FLR capability. We will switchto revised pcie_flr() once it is available. I hope you are fine with this approach. Please let me know. Thanks. Cheers GOVINDA _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 40+ messages in thread
[parent not found: <f19dbb09-ef22-2cf4-fb38-2a7c42b5dc48@Oracle.COM>]
* Re: [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute [not found] ` <f19dbb09-ef22-2cf4-fb38-2a7c42b5dc48@Oracle.COM> @ 2017-12-18 7:36 ` Jan Beulich [not found] ` <5A377E020200007800197FFA@prv-mh.provo.novell.com> 1 sibling, 0 replies; 40+ messages in thread From: Jan Beulich @ 2017-12-18 7:36 UTC (permalink / raw) To: Govinda Tatti Cc: Juergen Gross, linux-pci, linux-kernel, bhelgaas, xen-devel, boris.ostrovsky, roger.pau >>> On 15.12.17 at 20:52, <Govinda.Tatti@Oracle.COM> wrote: >>>> +static int pcistub_device_reset(struct pci_dev *dev) >>>> +{ >>>> + struct xen_pcibk_dev_data *dev_data; >>>> + bool slot = false, bus = false; >>>> + struct pcistub_args arg = {}; >>>> + >>>> + if (!dev) >>>> + return -EINVAL; >>>> + >>>> + dev_dbg(&dev->dev, "[%s]\n", __func__); >>>> + >>>> + /* First check and try FLR */ >>>> + if (pcie_has_flr(dev)) { >>>> + dev_dbg(&dev->dev, "resetting %s device using FLR\n", >>>> + pci_name(dev)); >>>> + pcie_flr(dev); >>> The lack of error check here puzzled me, but I see the function >>> indeed returns void right now. I think the prereq patch should >>> change this along with exporting the function - you really don't >>> want the device to be handed to a guest when the FLR timed >>> out. >> We will change pcie_flr() to return error code. I will make this change >> in the next version of this patch. > I exchanged some emails with Bjorn/Christoph and it looks like Christoph > as some planto restructure pcie flr specific functions but I don't know > the exact time-frame. For now,I am planning to use existing pcie_flr() > after checking FLR capability. We will switchto revised pcie_flr() once > it is available. > > I hope you are fine with this approach. Please let me know. Thanks. I've seen that other discussion. I don't think the change here should be done prior to the error reporting being put in place, for security reasons. But in the end it'll be Konrad as the maintainer to judge. Or wait, looks like there's some confusion in ./MAINTAINERS: Konrad is listed as maintainer for "XEN PCI SUBSYSTEM", but the list of files doesn't include pciback. So it would instead be Boris or Jürgen to give you a final word. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 40+ messages in thread
[parent not found: <5A377E020200007800197FFA@prv-mh.provo.novell.com>]
* Re: [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute [not found] ` <5A377E020200007800197FFA@prv-mh.provo.novell.com> @ 2017-12-18 17:32 ` Boris Ostrovsky [not found] ` <559ffd12-b541-8a69-60bd-fbe10e3dc159@oracle.com> 1 sibling, 0 replies; 40+ messages in thread From: Boris Ostrovsky @ 2017-12-18 17:32 UTC (permalink / raw) To: Jan Beulich, Govinda Tatti Cc: Juergen Gross, linux-pci, linux-kernel, bhelgaas, xen-devel, roger.pau On 12/18/2017 02:36 AM, Jan Beulich wrote: >>>> On 15.12.17 at 20:52, <Govinda.Tatti@Oracle.COM> wrote: >>>>> +static int pcistub_device_reset(struct pci_dev *dev) >>>>> +{ >>>>> + struct xen_pcibk_dev_data *dev_data; >>>>> + bool slot = false, bus = false; >>>>> + struct pcistub_args arg = {}; >>>>> + >>>>> + if (!dev) >>>>> + return -EINVAL; >>>>> + >>>>> + dev_dbg(&dev->dev, "[%s]\n", __func__); >>>>> + >>>>> + /* First check and try FLR */ >>>>> + if (pcie_has_flr(dev)) { >>>>> + dev_dbg(&dev->dev, "resetting %s device using FLR\n", >>>>> + pci_name(dev)); >>>>> + pcie_flr(dev); >>>> The lack of error check here puzzled me, but I see the function >>>> indeed returns void right now. I think the prereq patch should >>>> change this along with exporting the function - you really don't >>>> want the device to be handed to a guest when the FLR timed >>>> out. >>> We will change pcie_flr() to return error code. I will make this change >>> in the next version of this patch. >> I exchanged some emails with Bjorn/Christoph and it looks like Christoph >> as some planto restructure pcie flr specific functions but I don't know >> the exact time-frame. For now,I am planning to use existing pcie_flr() >> after checking FLR capability. We will switchto revised pcie_flr() once >> it is available. >> >> I hope you are fine with this approach. Please let me know. Thanks. > I've seen that other discussion. I don't think the change here > should be done prior to the error reporting being put in place, > for security reasons. But in the end it'll be Konrad as the > maintainer to judge. > > Or wait, looks like there's some confusion in ./MAINTAINERS: > Konrad is listed as maintainer for "XEN PCI SUBSYSTEM", but the > list of files doesn't include pciback. So it would instead be Boris > or Jürgen to give you a final word. This is now 4.16 material so we can at least wait until closer to opening of the merge window when we may have the PCI updates. (And I just noticed that you responded to Christoph.) Besides, we don't want to make kernel changes until the interface is settled (i.e the toolstack changes are accepted). -boris _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 40+ messages in thread
[parent not found: <559ffd12-b541-8a69-60bd-fbe10e3dc159@oracle.com>]
* Re: [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute [not found] ` <559ffd12-b541-8a69-60bd-fbe10e3dc159@oracle.com> @ 2018-09-16 11:43 ` Pasi Kärkkäinen [not found] ` <20180916114306.GF18222@reaktio.net> 1 sibling, 0 replies; 40+ messages in thread From: Pasi Kärkkäinen @ 2018-09-16 11:43 UTC (permalink / raw) To: Boris Ostrovsky Cc: Juergen Gross, Govinda Tatti, Konrad Rzeszutek Wilk, linux-pci, linux-kernel, Jan Beulich, bhelgaas, xen-devel, roger.pau Hi, On Mon, Dec 18, 2017 at 12:32:11PM -0500, Boris Ostrovsky wrote: > On 12/18/2017 02:36 AM, Jan Beulich wrote: > >>>> On 15.12.17 at 20:52, <Govinda.Tatti@Oracle.COM> wrote: > >>>>> +static int pcistub_device_reset(struct pci_dev *dev) > >>>>> +{ > >>>>> + struct xen_pcibk_dev_data *dev_data; > >>>>> + bool slot = false, bus = false; > >>>>> + struct pcistub_args arg = {}; > >>>>> + > >>>>> + if (!dev) > >>>>> + return -EINVAL; > >>>>> + > >>>>> + dev_dbg(&dev->dev, "[%s]\n", __func__); > >>>>> + > >>>>> + /* First check and try FLR */ > >>>>> + if (pcie_has_flr(dev)) { > >>>>> + dev_dbg(&dev->dev, "resetting %s device using FLR\n", > >>>>> + pci_name(dev)); > >>>>> + pcie_flr(dev); > >>>> The lack of error check here puzzled me, but I see the function > >>>> indeed returns void right now. I think the prereq patch should > >>>> change this along with exporting the function - you really don't > >>>> want the device to be handed to a guest when the FLR timed > >>>> out. > >>> We will change pcie_flr() to return error code. I will make this change > >>> in the next version of this patch. > >> I exchanged some emails with Bjorn/Christoph and it looks like Christoph > >> as some planto restructure pcie flr specific functions but I don't know > >> the exact time-frame. For now,I am planning to use existing pcie_flr() > >> after checking FLR capability. We will switchto revised pcie_flr() once > >> it is available. > >> > >> I hope you are fine with this approach. Please let me know. Thanks. > > I've seen that other discussion. I don't think the change here > > should be done prior to the error reporting being put in place, > > for security reasons. But in the end it'll be Konrad as the > > maintainer to judge. > > > > Or wait, looks like there's some confusion in ./MAINTAINERS: > > Konrad is listed as maintainer for "XEN PCI SUBSYSTEM", but the > > list of files doesn't include pciback. So it would instead be Boris > > or Jürgen to give you a final word. > > > This is now 4.16 material so we can at least wait until closer to > opening of the merge window when we may have the PCI updates. (And I > just noticed that you responded to Christoph.) > > Besides, we don't want to make kernel changes until the interface is > settled (i.e the toolstack changes are accepted). > It seems Govinda's email address is giving an error, so I assume someone else needs to pick up this pciback 'reset' feature. Is it likely someone else from Oracle can/will pick up and refresh this patch, with the review comments addressed? Meanwhile the pcie_has_flr() has been exported in upstream Linux kernel, so that's already available for use now. "PCI: Export pcie_has_flr()": https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2d2917f7747805a1f4188672f308d82a8ba01700 > -boris > Thanks, -- Pasi _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 40+ messages in thread
[parent not found: <20180916114306.GF18222@reaktio.net>]
* Re: [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute [not found] ` <20180916114306.GF18222@reaktio.net> @ 2018-09-17 18:06 ` Boris Ostrovsky [not found] ` <a726840b-8a5c-0890-73c6-3a95a7205153@oracle.com> 1 sibling, 0 replies; 40+ messages in thread From: Boris Ostrovsky @ 2018-09-17 18:06 UTC (permalink / raw) To: Pasi Kärkkäinen Cc: Juergen Gross, Govinda Tatti, Konrad Rzeszutek Wilk, linux-pci, linux-kernel, Jan Beulich, bhelgaas, xen-devel, roger.pau On 9/16/18 7:43 AM, Pasi Kärkkäinen wrote: > Hi, > > On Mon, Dec 18, 2017 at 12:32:11PM -0500, Boris Ostrovsky wrote: >> On 12/18/2017 02:36 AM, Jan Beulich wrote: >>>>>> On 15.12.17 at 20:52, <Govinda.Tatti@Oracle.COM> wrote: >>>>>>> +static int pcistub_device_reset(struct pci_dev *dev) >>>>>>> +{ >>>>>>> + struct xen_pcibk_dev_data *dev_data; >>>>>>> + bool slot = false, bus = false; >>>>>>> + struct pcistub_args arg = {}; >>>>>>> + >>>>>>> + if (!dev) >>>>>>> + return -EINVAL; >>>>>>> + >>>>>>> + dev_dbg(&dev->dev, "[%s]\n", __func__); >>>>>>> + >>>>>>> + /* First check and try FLR */ >>>>>>> + if (pcie_has_flr(dev)) { >>>>>>> + dev_dbg(&dev->dev, "resetting %s device using FLR\n", >>>>>>> + pci_name(dev)); >>>>>>> + pcie_flr(dev); >>>>>> The lack of error check here puzzled me, but I see the function >>>>>> indeed returns void right now. I think the prereq patch should >>>>>> change this along with exporting the function - you really don't >>>>>> want the device to be handed to a guest when the FLR timed >>>>>> out. >>>>> We will change pcie_flr() to return error code. I will make this change >>>>> in the next version of this patch. >>>> I exchanged some emails with Bjorn/Christoph and it looks like Christoph >>>> as some planto restructure pcie flr specific functions but I don't know >>>> the exact time-frame. For now,I am planning to use existing pcie_flr() >>>> after checking FLR capability. We will switchto revised pcie_flr() once >>>> it is available. >>>> >>>> I hope you are fine with this approach. Please let me know. Thanks. >>> I've seen that other discussion. I don't think the change here >>> should be done prior to the error reporting being put in place, >>> for security reasons. But in the end it'll be Konrad as the >>> maintainer to judge. >>> >>> Or wait, looks like there's some confusion in ./MAINTAINERS: >>> Konrad is listed as maintainer for "XEN PCI SUBSYSTEM", but the >>> list of files doesn't include pciback. So it would instead be Boris >>> or Jürgen to give you a final word. >> >> This is now 4.16 material so we can at least wait until closer to >> opening of the merge window when we may have the PCI updates. (And I >> just noticed that you responded to Christoph.) >> >> Besides, we don't want to make kernel changes until the interface is >> settled (i.e the toolstack changes are accepted). >> > It seems Govinda's email address is giving an error, so I assume someone else needs to pick up this pciback 'reset' feature. > Is it likely someone else from Oracle can/will pick up and refresh this patch, with the review comments addressed? Govinda is no longer at Oracle. What about the toolstack changes? Have they been accepted? I vaguely recall there was a discussion about those changes but don't remember how it ended. -boris > > > Meanwhile the pcie_has_flr() has been exported in upstream Linux kernel, so that's already available for use now. > > "PCI: Export pcie_has_flr()": > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2d2917f7747805a1f4188672f308d82a8ba01700 > > > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 40+ messages in thread
[parent not found: <a726840b-8a5c-0890-73c6-3a95a7205153@oracle.com>]
* Re: [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute [not found] ` <a726840b-8a5c-0890-73c6-3a95a7205153@oracle.com> @ 2018-09-18 7:15 ` Pasi Kärkkäinen [not found] ` <20180918071519.GG18222@reaktio.net> 1 sibling, 0 replies; 40+ messages in thread From: Pasi Kärkkäinen @ 2018-09-18 7:15 UTC (permalink / raw) To: Boris Ostrovsky Cc: Juergen Gross, Konrad Rzeszutek Wilk, George Dunlap, linux-pci, linux-kernel, Jan Beulich, bhelgaas, xen-devel, roger.pau Hi, On Mon, Sep 17, 2018 at 02:06:02PM -0400, Boris Ostrovsky wrote: > On 9/16/18 7:43 AM, Pasi Kärkkäinen wrote: > > Hi, > > > > On Mon, Dec 18, 2017 at 12:32:11PM -0500, Boris Ostrovsky wrote: > >> On 12/18/2017 02:36 AM, Jan Beulich wrote: > >>>>>> On 15.12.17 at 20:52, <Govinda.Tatti@Oracle.COM> wrote: > >>>>>>> +static int pcistub_device_reset(struct pci_dev *dev) > >>>>>>> +{ > >>>>>>> + struct xen_pcibk_dev_data *dev_data; > >>>>>>> + bool slot = false, bus = false; > >>>>>>> + struct pcistub_args arg = {}; > >>>>>>> + > >>>>>>> + if (!dev) > >>>>>>> + return -EINVAL; > >>>>>>> + > >>>>>>> + dev_dbg(&dev->dev, "[%s]\n", __func__); > >>>>>>> + > >>>>>>> + /* First check and try FLR */ > >>>>>>> + if (pcie_has_flr(dev)) { > >>>>>>> + dev_dbg(&dev->dev, "resetting %s device using FLR\n", > >>>>>>> + pci_name(dev)); > >>>>>>> + pcie_flr(dev); > >>>>>> The lack of error check here puzzled me, but I see the function > >>>>>> indeed returns void right now. I think the prereq patch should > >>>>>> change this along with exporting the function - you really don't > >>>>>> want the device to be handed to a guest when the FLR timed > >>>>>> out. > >>>>> We will change pcie_flr() to return error code. I will make this change > >>>>> in the next version of this patch. > >>>> I exchanged some emails with Bjorn/Christoph and it looks like Christoph > >>>> as some planto restructure pcie flr specific functions but I don't know > >>>> the exact time-frame. For now,I am planning to use existing pcie_flr() > >>>> after checking FLR capability. We will switchto revised pcie_flr() once > >>>> it is available. > >>>> > >>>> I hope you are fine with this approach. Please let me know. Thanks. > >>> I've seen that other discussion. I don't think the change here > >>> should be done prior to the error reporting being put in place, > >>> for security reasons. But in the end it'll be Konrad as the > >>> maintainer to judge. > >>> > >>> Or wait, looks like there's some confusion in ./MAINTAINERS: > >>> Konrad is listed as maintainer for "XEN PCI SUBSYSTEM", but the > >>> list of files doesn't include pciback. So it would instead be Boris > >>> or Jürgen to give you a final word. > >> > >> This is now 4.16 material so we can at least wait until closer to > >> opening of the merge window when we may have the PCI updates. (And I > >> just noticed that you responded to Christoph.) > >> > >> Besides, we don't want to make kernel changes until the interface is > >> settled (i.e the toolstack changes are accepted). > >> > > It seems Govinda's email address is giving an error, so I assume someone else needs to pick up this pciback 'reset' feature. > > Is it likely someone else from Oracle can/will pick up and refresh this patch, with the review comments addressed? > > > Govinda is no longer at Oracle. > Yep, thought so. Removed from CC list. > What about the toolstack changes? Have they been accepted? I vaguely > recall there was a discussion about those changes but don't remember how > it ended. > I don't think toolstack/libxl patch has been applied yet either. "[PATCH V1 0/1] Xen/Tools: PCI reset using 'reset' SysFS attribute": https://lists.xen.org/archives/html/xen-devel/2017-12/msg00664.html "[PATCH V1 1/1] Xen/libxl: Perform PCI reset using 'reset' SysFS attribute": https://lists.xen.org/archives/html/xen-devel/2017-12/msg00663.html George asked for some clarifications: https://lists.xen.org/archives/html/xen-devel/2017-12/msg01044.html https://lists.xen.org/archives/html/xen-devel/2017-12/msg01116.html > > -boris > Thanks, -- Pasi > > > > > > > Meanwhile the pcie_has_flr() has been exported in upstream Linux kernel, so that's already available for use now. > > > > "PCI: Export pcie_has_flr()": > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2d2917f7747805a1f4188672f308d82a8ba01700 > > > > > > > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 40+ messages in thread
[parent not found: <20180918071519.GG18222@reaktio.net>]
* Re: [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute [not found] ` <20180918071519.GG18222@reaktio.net> @ 2018-09-18 9:32 ` George Dunlap [not found] ` <5E7DDB68-4E68-48A5-AEEC-EE1B21A50E9E@citrix.com> 1 sibling, 0 replies; 40+ messages in thread From: George Dunlap @ 2018-09-18 9:32 UTC (permalink / raw) To: Pasi Kärkkäinen Cc: Juergen Gross, Konrad Rzeszutek Wilk, linux-pci, linux-kernel, George Dunlap, Jan Beulich, bhelgaas, xen-devel, Boris Ostrovsky, Roger Pau Monne > On Sep 18, 2018, at 8:15 AM, Pasi Kärkkäinen <pasik@iki.fi> wrote: > > Hi, > > On Mon, Sep 17, 2018 at 02:06:02PM -0400, Boris Ostrovsky wrote: >> On 9/16/18 7:43 AM, Pasi Kärkkäinen wrote: >>> Hi, >>> >>> On Mon, Dec 18, 2017 at 12:32:11PM -0500, Boris Ostrovsky wrote: >>>> On 12/18/2017 02:36 AM, Jan Beulich wrote: >>>>>>>> On 15.12.17 at 20:52, <Govinda.Tatti@Oracle.COM> wrote: >>>>>>>>> +static int pcistub_device_reset(struct pci_dev *dev) >>>>>>>>> +{ >>>>>>>>> + struct xen_pcibk_dev_data *dev_data; >>>>>>>>> + bool slot = false, bus = false; >>>>>>>>> + struct pcistub_args arg = {}; >>>>>>>>> + >>>>>>>>> + if (!dev) >>>>>>>>> + return -EINVAL; >>>>>>>>> + >>>>>>>>> + dev_dbg(&dev->dev, "[%s]\n", __func__); >>>>>>>>> + >>>>>>>>> + /* First check and try FLR */ >>>>>>>>> + if (pcie_has_flr(dev)) { >>>>>>>>> + dev_dbg(&dev->dev, "resetting %s device using FLR\n", >>>>>>>>> + pci_name(dev)); >>>>>>>>> + pcie_flr(dev); >>>>>>>> The lack of error check here puzzled me, but I see the function >>>>>>>> indeed returns void right now. I think the prereq patch should >>>>>>>> change this along with exporting the function - you really don't >>>>>>>> want the device to be handed to a guest when the FLR timed >>>>>>>> out. >>>>>>> We will change pcie_flr() to return error code. I will make this change >>>>>>> in the next version of this patch. >>>>>> I exchanged some emails with Bjorn/Christoph and it looks like Christoph >>>>>> as some planto restructure pcie flr specific functions but I don't know >>>>>> the exact time-frame. For now,I am planning to use existing pcie_flr() >>>>>> after checking FLR capability. We will switchto revised pcie_flr() once >>>>>> it is available. >>>>>> >>>>>> I hope you are fine with this approach. Please let me know. Thanks. >>>>> I've seen that other discussion. I don't think the change here >>>>> should be done prior to the error reporting being put in place, >>>>> for security reasons. But in the end it'll be Konrad as the >>>>> maintainer to judge. >>>>> >>>>> Or wait, looks like there's some confusion in ./MAINTAINERS: >>>>> Konrad is listed as maintainer for "XEN PCI SUBSYSTEM", but the >>>>> list of files doesn't include pciback. So it would instead be Boris >>>>> or Jürgen to give you a final word. >>>> >>>> This is now 4.16 material so we can at least wait until closer to >>>> opening of the merge window when we may have the PCI updates. (And I >>>> just noticed that you responded to Christoph.) >>>> >>>> Besides, we don't want to make kernel changes until the interface is >>>> settled (i.e the toolstack changes are accepted). >>>> >>> It seems Govinda's email address is giving an error, so I assume someone else needs to pick up this pciback 'reset' feature. >>> Is it likely someone else from Oracle can/will pick up and refresh this patch, with the review comments addressed? >> >> >> Govinda is no longer at Oracle. >> > > Yep, thought so. Removed from CC list. > > >> What about the toolstack changes? Have they been accepted? I vaguely >> recall there was a discussion about those changes but don't remember how >> it ended. >> > > I don't think toolstack/libxl patch has been applied yet either. > > > "[PATCH V1 0/1] Xen/Tools: PCI reset using 'reset' SysFS attribute": > https://lists.xen.org/archives/html/xen-devel/2017-12/msg00664.html > > "[PATCH V1 1/1] Xen/libxl: Perform PCI reset using 'reset' SysFS attribute": > https://lists.xen.org/archives/html/xen-devel/2017-12/msg00663.html > > George asked for some clarifications: > https://lists.xen.org/archives/html/xen-devel/2017-12/msg01044.html > https://lists.xen.org/archives/html/xen-devel/2017-12/msg01116.html Right, the description of the patch didn’t actually tell you what was going on. It should have said something like, “xl currently attempts to reset a device using X; but that’s never been implemented in Linux. Instead, use Y, which [is better for whatever reason]”. -George _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 40+ messages in thread
[parent not found: <5E7DDB68-4E68-48A5-AEEC-EE1B21A50E9E@citrix.com>]
* Re: [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute [not found] ` <5E7DDB68-4E68-48A5-AEEC-EE1B21A50E9E@citrix.com> @ 2018-09-18 18:09 ` Boris Ostrovsky [not found] ` <352310b3-ec9b-2ceb-83f0-4550718120c3@oracle.com> 1 sibling, 0 replies; 40+ messages in thread From: Boris Ostrovsky @ 2018-09-18 18:09 UTC (permalink / raw) To: George Dunlap, Pasi Kärkkäinen, Roger Pau Monne Cc: Juergen Gross, Konrad Rzeszutek Wilk, linux-pci, linux-kernel, Jan Beulich, bhelgaas, xen-devel On 9/18/18 5:32 AM, George Dunlap wrote: > >> On Sep 18, 2018, at 8:15 AM, Pasi Kärkkäinen <pasik@iki.fi> wrote: >> >> Hi, >> >> On Mon, Sep 17, 2018 at 02:06:02PM -0400, Boris Ostrovsky wrote: >>> What about the toolstack changes? Have they been accepted? I vaguely >>> recall there was a discussion about those changes but don't remember how >>> it ended. >>> >> I don't think toolstack/libxl patch has been applied yet either. >> >> >> "[PATCH V1 0/1] Xen/Tools: PCI reset using 'reset' SysFS attribute": >> https://lists.xen.org/archives/html/xen-devel/2017-12/msg00664.html >> >> "[PATCH V1 1/1] Xen/libxl: Perform PCI reset using 'reset' SysFS attribute": >> https://lists.xen.org/archives/html/xen-devel/2017-12/msg00663.html Will this patch work for *BSD? Roger? >> >> George asked for some clarifications: >> https://lists.xen.org/archives/html/xen-devel/2017-12/msg01044.html >> https://lists.xen.org/archives/html/xen-devel/2017-12/msg01116.html > Right, the description of the patch didn’t actually tell you what was going on. It should have said something like, “xl currently attempts to reset a device using X; but that’s never been implemented in Linux. Instead, use Y, which [is better for whatever reason]”. Yes, the description can be tightened a bit ;-) -boris _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 40+ messages in thread
[parent not found: <352310b3-ec9b-2ceb-83f0-4550718120c3@oracle.com>]
* Re: [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute [not found] ` <352310b3-ec9b-2ceb-83f0-4550718120c3@oracle.com> @ 2018-09-19 9:05 ` Roger Pau Monné [not found] ` <20180919090526.s3ahnemrt2ik2tx3@mac.bytemobile.com> 1 sibling, 0 replies; 40+ messages in thread From: Roger Pau Monné @ 2018-09-19 9:05 UTC (permalink / raw) To: Boris Ostrovsky Cc: Juergen Gross, Konrad Rzeszutek Wilk, linux-pci, George Dunlap, linux-kernel, Jan Beulich, bhelgaas, xen-devel On Tue, Sep 18, 2018 at 02:09:53PM -0400, Boris Ostrovsky wrote: > On 9/18/18 5:32 AM, George Dunlap wrote: > > > >> On Sep 18, 2018, at 8:15 AM, Pasi Kärkkäinen <pasik@iki.fi> wrote: > >> > >> Hi, > >> > >> On Mon, Sep 17, 2018 at 02:06:02PM -0400, Boris Ostrovsky wrote: > >>> What about the toolstack changes? Have they been accepted? I vaguely > >>> recall there was a discussion about those changes but don't remember how > >>> it ended. > >>> > >> I don't think toolstack/libxl patch has been applied yet either. > >> > >> > >> "[PATCH V1 0/1] Xen/Tools: PCI reset using 'reset' SysFS attribute": > >> https://lists.xen.org/archives/html/xen-devel/2017-12/msg00664.html > >> > >> "[PATCH V1 1/1] Xen/libxl: Perform PCI reset using 'reset' SysFS attribute": > >> https://lists.xen.org/archives/html/xen-devel/2017-12/msg00663.html > > > Will this patch work for *BSD? Roger? At least FreeBSD don't support pci-passthrough, so none of this works ATM. There's no sysfs on BSD, so much of what's in libxl_pci.c will have to be moved to libxl_linux.c when BSD support is added. Thanks, Roger. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 40+ messages in thread
[parent not found: <20180919090526.s3ahnemrt2ik2tx3@mac.bytemobile.com>]
* Re: [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute [not found] ` <20180919090526.s3ahnemrt2ik2tx3@mac.bytemobile.com> @ 2018-10-03 15:51 ` Pasi Kärkkäinen [not found] ` <20181003155104.GH5318@reaktio.net> 1 sibling, 0 replies; 40+ messages in thread From: Pasi Kärkkäinen @ 2018-10-03 15:51 UTC (permalink / raw) To: Roger Pau Monné Cc: Juergen Gross, Konrad Rzeszutek Wilk, linux-pci, George Dunlap, linux-kernel, Jan Beulich, bhelgaas, xen-devel, Boris Ostrovsky On Wed, Sep 19, 2018 at 11:05:26AM +0200, Roger Pau Monné wrote: > On Tue, Sep 18, 2018 at 02:09:53PM -0400, Boris Ostrovsky wrote: > > On 9/18/18 5:32 AM, George Dunlap wrote: > > > > > >> On Sep 18, 2018, at 8:15 AM, Pasi Kärkkäinen <pasik@iki.fi> wrote: > > >> > > >> Hi, > > >> > > >> On Mon, Sep 17, 2018 at 02:06:02PM -0400, Boris Ostrovsky wrote: > > >>> What about the toolstack changes? Have they been accepted? I vaguely > > >>> recall there was a discussion about those changes but don't remember how > > >>> it ended. > > >>> > > >> I don't think toolstack/libxl patch has been applied yet either. > > >> > > >> > > >> "[PATCH V1 0/1] Xen/Tools: PCI reset using 'reset' SysFS attribute": > > >> https://lists.xen.org/archives/html/xen-devel/2017-12/msg00664.html > > >> > > >> "[PATCH V1 1/1] Xen/libxl: Perform PCI reset using 'reset' SysFS attribute": > > >> https://lists.xen.org/archives/html/xen-devel/2017-12/msg00663.html > > > > > > Will this patch work for *BSD? Roger? > > At least FreeBSD don't support pci-passthrough, so none of this works > ATM. There's no sysfs on BSD, so much of what's in libxl_pci.c will > have to be moved to libxl_linux.c when BSD support is added. > Ok. That sounds like it's OK for the initial pci 'reset' implementation in xl/libxl to be linux-only.. Thanks, -- Pasi > Thanks, Roger. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 40+ messages in thread
[parent not found: <20181003155104.GH5318@reaktio.net>]
* Re: [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute [not found] ` <20181003155104.GH5318@reaktio.net> @ 2018-10-08 14:32 ` Boris Ostrovsky [not found] ` <f6b8e055-7afc-b4de-af88-425d799dcd28@oracle.com> 1 sibling, 0 replies; 40+ messages in thread From: Boris Ostrovsky @ 2018-10-08 14:32 UTC (permalink / raw) To: Pasi Kärkkäinen, Roger Pau Monné Cc: Juergen Gross, Konrad Rzeszutek Wilk, linux-pci, George Dunlap, linux-kernel, Jan Beulich, bhelgaas, xen-devel On 10/3/18 11:51 AM, Pasi Kärkkäinen wrote: > On Wed, Sep 19, 2018 at 11:05:26AM +0200, Roger Pau Monné wrote: >> On Tue, Sep 18, 2018 at 02:09:53PM -0400, Boris Ostrovsky wrote: >>> On 9/18/18 5:32 AM, George Dunlap wrote: >>>>> On Sep 18, 2018, at 8:15 AM, Pasi Kärkkäinen <pasik@iki.fi> wrote: >>>>> >>>>> Hi, >>>>> >>>>> On Mon, Sep 17, 2018 at 02:06:02PM -0400, Boris Ostrovsky wrote: >>>>>> What about the toolstack changes? Have they been accepted? I vaguely >>>>>> recall there was a discussion about those changes but don't remember how >>>>>> it ended. >>>>>> >>>>> I don't think toolstack/libxl patch has been applied yet either. >>>>> >>>>> >>>>> "[PATCH V1 0/1] Xen/Tools: PCI reset using 'reset' SysFS attribute": >>>>> https://lists.xen.org/archives/html/xen-devel/2017-12/msg00664.html >>>>> >>>>> "[PATCH V1 1/1] Xen/libxl: Perform PCI reset using 'reset' SysFS attribute": >>>>> https://lists.xen.org/archives/html/xen-devel/2017-12/msg00663.html >>> >>> Will this patch work for *BSD? Roger? >> At least FreeBSD don't support pci-passthrough, so none of this works >> ATM. There's no sysfs on BSD, so much of what's in libxl_pci.c will >> have to be moved to libxl_linux.c when BSD support is added. >> > Ok. That sounds like it's OK for the initial pci 'reset' implementation in xl/libxl to be linux-only.. > Are these two patches still needed? ISTR they were written originally to deal with guest trying to use device that was previously assigned to another guest. But pcistub_put_pci_dev() calls __pci_reset_function_locked() which first tries FLR, and it looks like it was added relatively recently. -boris _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 40+ messages in thread
[parent not found: <f6b8e055-7afc-b4de-af88-425d799dcd28@oracle.com>]
* Re: [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute [not found] ` <f6b8e055-7afc-b4de-af88-425d799dcd28@oracle.com> @ 2018-10-23 18:40 ` Håkon Alstadheim 2018-10-29 15:30 ` Pasi Kärkkäinen 2018-11-14 14:24 ` [PATCH cargo-cult-version] For linux-4.19.x . " Håkon Alstadheim 2019-08-26 21:05 ` [Xen-devel] [PATCH V3 2/2] " Pasi Kärkkäinen 1 sibling, 2 replies; 40+ messages in thread From: Håkon Alstadheim @ 2018-10-23 18:40 UTC (permalink / raw) To: xen-devel Den 08. okt. 2018 16:32, skrev Boris Ostrovsky: > > Are these two patches still needed? ISTR they were written originally > to deal with guest trying to use device that was previously assigned to > another guest. But pcistub_put_pci_dev() calls > __pci_reset_function_locked() which first tries FLR, and it looks like > it was added relatively recently. > > Sorry for the late reply, but I just now booted xen staging-4.11 (94fba9f438a2c36ad9bf3a481a6013ddc7cf8cd9), with gentoo-sources-4.19.0 as dom0. Shut down and started again a VM that has a secondary GPU passed through, and the whole machine hung. I haven't had time to look more closely into this, other than that my old "do_flr" patch no longer applies to gentoo-sources (i.e. the linux kernel sources) . "do_flr" worked for me on linux-4.18.? , with appropriate patch to the linux kernel. So, something is definitely needed. No "reset" , or "do_flr" is present in linux-4.19.0, viz: <code> $ cd /usr/src/linux/drivers/xen/xen-pciback $ grep DRIVER * pci_stub.c:#define PCISTUB_DRIVER_NAME "pciback" pci_stub.c: !strcmp(dev->driver_override, PCISTUB_DRIVER_NAME)) || pci_stub.c: .name = PCISTUB_DRIVER_NAME, pci_stub.c:static DRIVER_ATTR_WO(new_slot); pci_stub.c:static DRIVER_ATTR_WO(remove_slot); pci_stub.c:static DRIVER_ATTR_RO(slots); pci_stub.c:static DRIVER_ATTR_RO(irq_handlers); pci_stub.c:static DRIVER_ATTR_WO(irq_handler_state); pci_stub.c:static DRIVER_ATTR_RW(quirks); pci_stub.c:static DRIVER_ATTR_RW(permissive); pci_stub.c: if (action != BUS_NOTIFY_UNBIND_DRIVER) $ </code> I'd be happy to test patches. Seems I only got one corrupt file from my test this morning :-D . _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute 2018-10-23 18:40 ` Håkon Alstadheim @ 2018-10-29 15:30 ` Pasi Kärkkäinen 2018-11-14 14:24 ` [PATCH cargo-cult-version] For linux-4.19.x . " Håkon Alstadheim 1 sibling, 0 replies; 40+ messages in thread From: Pasi Kärkkäinen @ 2018-10-29 15:30 UTC (permalink / raw) To: Håkon Alstadheim; +Cc: xen-devel Hi, On Tue, Oct 23, 2018 at 08:40:29PM +0200, Håkon Alstadheim wrote: > > > Den 08. okt. 2018 16:32, skrev Boris Ostrovsky: > > > > Are these two patches still needed? ISTR they were written originally > > to deal with guest trying to use device that was previously assigned to > > another guest. But pcistub_put_pci_dev() calls > > __pci_reset_function_locked() which first tries FLR, and it looks like > > it was added relatively recently. > > > > > Sorry for the late reply, but I just now booted xen staging-4.11 > (94fba9f438a2c36ad9bf3a481a6013ddc7cf8cd9), with gentoo-sources-4.19.0 > as dom0. Shut down and started again a VM that has a secondary GPU > passed through, and the whole machine hung. I haven't had time to look > more closely into this, other than that my old "do_flr" patch no longer > applies to gentoo-sources (i.e. the linux kernel sources) . "do_flr" > worked for me on linux-4.18.? , with appropriate patch to the linux kernel. > > So, something is definitely needed. No "reset" , or "do_flr" is present > in linux-4.19.0, viz: > <code> > $ cd /usr/src/linux/drivers/xen/xen-pciback > $ grep DRIVER * > pci_stub.c:#define PCISTUB_DRIVER_NAME "pciback" > pci_stub.c: !strcmp(dev->driver_override, PCISTUB_DRIVER_NAME)) || > pci_stub.c: .name = PCISTUB_DRIVER_NAME, > pci_stub.c:static DRIVER_ATTR_WO(new_slot); > pci_stub.c:static DRIVER_ATTR_WO(remove_slot); > pci_stub.c:static DRIVER_ATTR_RO(slots); > pci_stub.c:static DRIVER_ATTR_RO(irq_handlers); > pci_stub.c:static DRIVER_ATTR_WO(irq_handler_state); > pci_stub.c:static DRIVER_ATTR_RW(quirks); > pci_stub.c:static DRIVER_ATTR_RW(permissive); > pci_stub.c: if (action != BUS_NOTIFY_UNBIND_DRIVER) > $ > </code> > > I'd be happy to test patches. Seems I only got one corrupt file from my > test this morning :-D . > Håkon: Please do test the patches and report how it works for you! Here are the links: Linux kernel xen-pciback 'reset' patches: "[PATCH V3 0/2] Xen/PCIback: PCI reset using 'reset' SysFS attribute": https://lists.xen.org/archives/html/xen-devel/2017-12/msg00659.html (Patch 1/2 is not needed anymore in upstream Linux kernel, as pcie_has_flr() is already exported meanwhile) "[PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute": https://lists.xen.org/archives/html/xen-devel/2017-12/msg00661.html Xen libxl 'reset' patches: "[PATCH V1 0/1] Xen/Tools: PCI reset using 'reset' SysFS attribute": https://lists.xen.org/archives/html/xen-devel/2017-12/msg00664.html "[PATCH V1 1/1] Xen/libxl: Perform PCI reset using 'reset' SysFS attribute": https://lists.xen.org/archives/html/xen-devel/2017-12/msg00663.html Thanks, -- Pasi _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH cargo-cult-version] For linux-4.19.x . Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute 2018-10-23 18:40 ` Håkon Alstadheim 2018-10-29 15:30 ` Pasi Kärkkäinen @ 2018-11-14 14:24 ` Håkon Alstadheim 1 sibling, 0 replies; 40+ messages in thread From: Håkon Alstadheim @ 2018-11-14 14:24 UTC (permalink / raw) To: xen-devel [-- Attachment #1: Type: text/plain, Size: 1608 bytes --] Den 23.10.2018 20:40, skrev Håkon Alstadheim: > > Den 08. okt. 2018 16:32, skrev Boris Ostrovsky: >> Are these two patches still needed? ISTR they were written originally >> to deal with guest trying to use device that was previously assigned to >> another guest. But pcistub_put_pci_dev() calls >> __pci_reset_function_locked() which first tries FLR, and it looks like >> it was added relatively recently. >> >> > Sorry for the late reply, but I just now booted xen staging-4.11 > (94fba9f438a2c36ad9bf3a481a6013ddc7cf8cd9), with gentoo-sources-4.19.0 > as dom0. Shut down and started again a VM that has a secondary GPU > passed through, and the whole machine hung. I haven't had time to look > more closely into this, other than that my old "do_flr" patch no longer > applies to gentoo-sources (i.e. the linux kernel sources) . "do_flr" > worked for me on linux-4.18.? , with appropriate patch to the linux kernel. Without some kind of fix, my whole server (dom0) goes down whenever a domu with pci passed through is re-started. NOTE: I am not a programmer. I have no idea what I am doing. The patch I have as a starting-point does not compile correctly when applied to kernel version 4.19.x. I get implicit declarations of pci_try_reset_slot() and pci_try_reset_bus(). Replacing those with pci_reset_bus(dev) gives me the attached patch which applies cleanly to gentoo-sources-4.19.2, compiles without warnings, and works to let me restart a domU with pci-passthrough (modulo changing do_flr to reset in xen libxl). I hope a dev will adopt these and give them proper care :-) . [-- Attachment #2: pci_brute_reset-home-hack.patch --] [-- Type: text/x-patch, Size: 4434 bytes --] --- a/drivers/xen/xen-pciback/pci_stub.c 2018-10-22 08:37:37.000000000 +0200 +++ b/drivers/xen/xen-pciback/pci_stub.c 2018-11-14 12:45:21.926468126 +0100 @@ -244,6 +244,90 @@ return found_dev; } +struct pcistub_args { + struct pci_dev *dev; + unsigned int dcount; +}; + +static int pcistub_search_dev(struct pci_dev *dev, void *data) +{ + struct pcistub_device *psdev; + struct pcistub_args *arg = data; + bool found_dev = false; + unsigned long flags; + + spin_lock_irqsave(&pcistub_devices_lock, flags); + + list_for_each_entry(psdev, &pcistub_devices, dev_list) { + if (psdev->dev == dev) { + found_dev = true; + arg->dcount++; + break; + } + } + + spin_unlock_irqrestore(&pcistub_devices_lock, flags); + + /* Device not owned by pcistub, someone owns it. Abort the walk */ + if (!found_dev) + arg->dev = dev; + + return found_dev ? 0 : 1; +} + +static int pcistub_reset_dev(struct pci_dev *dev) +{ + struct xen_pcibk_dev_data *dev_data; + bool slot = false, bus = false; + struct pcistub_args arg = {}; + + if (!dev) + return -EINVAL; + + dev_dbg(&dev->dev, "[%s]\n", __func__); + + if (!pci_probe_reset_slot(dev->slot)) + slot = true; + else if ((!pci_probe_reset_bus(dev->bus)) && + (!pci_is_root_bus(dev->bus))) + bus = true; + + if (!bus && !slot) + return -EOPNOTSUPP; + + /* + * Make sure all devices on this bus are owned by the + * PCI backend so that we can safely reset the whole bus. + */ + pci_walk_bus(dev->bus, pcistub_search_dev, &arg); + + /* All devices under the bus should be part of pcistub! */ + if (arg.dev) { + dev_err(&dev->dev, "%s device on bus 0x%x is not owned by pcistub\n", + pci_name(arg.dev), dev->bus->number); + + return -EBUSY; + } + + dev_dbg(&dev->dev, "pcistub owns %d devices on bus 0x%x\n", + arg.dcount, dev->bus->number); + + dev_data = pci_get_drvdata(dev); + if (!pci_load_saved_state(dev, dev_data->pci_saved_state)) + pci_restore_state(dev); + + /* This disables the device. */ + xen_pcibk_reset_device(dev); + + /* Cleanup up any emulated fields */ + xen_pcibk_config_reset_dev(dev); + + dev_dbg(&dev->dev, "resetting %s device using %s reset\n", + pci_name(dev), slot ? "slot" : "bus"); + + return pci_reset_bus(dev); +} + /* * Called when: * - XenBus state has been reconfigure (pci unplug). See xen_pcibk_remove_device @@ -1430,6 +1514,33 @@ } static DRIVER_ATTR_RW(permissive); +static ssize_t reset_store(struct device_driver *drv, const char *buf, + size_t count) +{ + struct pcistub_device *psdev; + int domain, bus, slot, func; + int err; + + err = str_to_slot(buf, &domain, &bus, &slot, &func); + if (err) + return err; + + psdev = pcistub_device_find(domain, bus, slot, func); + if (psdev) { + err = pcistub_reset_dev(psdev->dev); + pcistub_device_put(psdev); + } else { + err = -ENODEV; + } + + if (!err) + err = count; + + return err; +} + +static DRIVER_ATTR_WO(reset); + static void pcistub_exit(void) { driver_remove_file(&xen_pcibk_pci_driver.driver, &driver_attr_new_slot); @@ -1443,6 +1554,8 @@ &driver_attr_irq_handlers); driver_remove_file(&xen_pcibk_pci_driver.driver, &driver_attr_irq_handler_state); + driver_remove_file(&xen_pcibk_pci_driver.driver, + &driver_attr_reset); pci_unregister_driver(&xen_pcibk_pci_driver); } @@ -1536,6 +1649,11 @@ if (!err) err = driver_create_file(&xen_pcibk_pci_driver.driver, &driver_attr_irq_handler_state); + + if (!err) + err = driver_create_file(&xen_pcibk_pci_driver.driver, + &driver_attr_reset); + if (err) pcistub_exit(); --- a/Documentation/ABI/testing/sysfs-driver-pciback 2017-11-12 19:46:13.000000000 +0100 +++ b/Documentation/ABI/testing/sysfs-driver-pciback 2017-11-25 21:37:35.235738190 +0100 @@ -11,3 +11,15 @@ #echo 00:19.0-E0:2:FF > /sys/bus/pci/drivers/pciback/quirks will allow the guest to read and write to the configuration register 0x0E. + +What: /sys/bus/pci/drivers/pciback/reset +Date: Nov 2017 +KernelVersion: 4.15 +Contact: xen-devel@lists.xenproject.org +Description: + An option to perform a slot or bus reset when a PCI device + is owned by Xen PCI backend. Writing a string of DDDD:BB:DD.F + will cause the pciback driver to perform a slot or bus reset + if the device supports it. It also checks to make sure that + all of the devices under the bridge are owned by Xen PCI + backend. [-- Attachment #3: do_flr-to-reset.patch --] [-- Type: text/x-patch, Size: 426 bytes --] --- a/tools/libxl/libxl_pci.c 2018-10-24 15:57:14.384810336 +0200 +++ b/tools/libxl/libxl_pci.c 2018-10-24 15:58:32.759342602 +0200 @@ -1119,7 +1119,7 @@ char *reset; int fd, rc; - reset = GCSPRINTF("%s/do_flr", SYSFS_PCIBACK_DRIVER); + reset = GCSPRINTF("%s/reset", SYSFS_PCIBACK_DRIVER); fd = open(reset, O_WRONLY); if (fd >= 0) { char *buf = GCSPRINTF(PCI_BDF, domain, bus, dev, func); [-- Attachment #4: Type: text/plain, Size: 157 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Xen-devel] [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute [not found] ` <f6b8e055-7afc-b4de-af88-425d799dcd28@oracle.com> 2018-10-23 18:40 ` Håkon Alstadheim @ 2019-08-26 21:05 ` Pasi Kärkkäinen 1 sibling, 0 replies; 40+ messages in thread From: Pasi Kärkkäinen @ 2019-08-26 21:05 UTC (permalink / raw) To: Boris Ostrovsky Cc: Juergen Gross, Konrad Rzeszutek Wilk, linux-pci, linux-kernel, George Dunlap, Jan Beulich, bhelgaas, xen-devel, Håkon Alstadheim, Roger Pau Monné Hi, On Mon, Oct 08, 2018 at 10:32:45AM -0400, Boris Ostrovsky wrote: > On 10/3/18 11:51 AM, Pasi Kärkkäinen wrote: > > On Wed, Sep 19, 2018 at 11:05:26AM +0200, Roger Pau Monné wrote: > >> On Tue, Sep 18, 2018 at 02:09:53PM -0400, Boris Ostrovsky wrote: > >>> On 9/18/18 5:32 AM, George Dunlap wrote: > >>>>> On Sep 18, 2018, at 8:15 AM, Pasi Kärkkäinen <pasik@iki.fi> wrote: > >>>>> > >>>>> Hi, > >>>>> > >>>>> On Mon, Sep 17, 2018 at 02:06:02PM -0400, Boris Ostrovsky wrote: > >>>>>> What about the toolstack changes? Have they been accepted? I vaguely > >>>>>> recall there was a discussion about those changes but don't remember how > >>>>>> it ended. > >>>>>> > >>>>> I don't think toolstack/libxl patch has been applied yet either. > >>>>> > >>>>> > >>>>> "[PATCH V1 0/1] Xen/Tools: PCI reset using 'reset' SysFS attribute": > >>>>> https://lists.xen.org/archives/html/xen-devel/2017-12/msg00664.html > >>>>> > >>>>> "[PATCH V1 1/1] Xen/libxl: Perform PCI reset using 'reset' SysFS attribute": > >>>>> https://lists.xen.org/archives/html/xen-devel/2017-12/msg00663.html > >>> > >>> Will this patch work for *BSD? Roger? > >> At least FreeBSD don't support pci-passthrough, so none of this works > >> ATM. There's no sysfs on BSD, so much of what's in libxl_pci.c will > >> have to be moved to libxl_linux.c when BSD support is added. > >> > > Ok. That sounds like it's OK for the initial pci 'reset' implementation in xl/libxl to be linux-only.. > > > > Are these two patches still needed? ISTR they were written originally > to deal with guest trying to use device that was previously assigned to > another guest. But pcistub_put_pci_dev() calls > __pci_reset_function_locked() which first tries FLR, and it looks like > it was added relatively recently. > Replying to an old thread.. I only now realized I forgot to reply to this message earlier. afaik these patches are still needed. Håkon (CC'd) wrote to me in private that he gets a (dom0) Linux kernel crash if he doesn't have these patches applied. Here are the links to both the linux kernel and libxl patches: "[Xen-devel] [PATCH V3 0/2] Xen/PCIback: PCI reset using 'reset' SysFS attribute": https://lists.xen.org/archives/html/xen-devel/2017-12/msg00659.html [Note that PATCH V3 1/2 "Drivers/PCI: Export pcie_has_flr() interface" is already applied in upstream linux kernel, so it's not needed anymore] "[Xen-devel] [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute": https://lists.xen.org/archives/html/xen-devel/2017-12/msg00661.html "[Xen-devel] [PATCH V1 0/1] Xen/Tools: PCI reset using 'reset' SysFS attribute": https://lists.xen.org/archives/html/xen-devel/2017-12/msg00664.html "[Xen-devel] [PATCH V1 1/1] Xen/libxl: Perform PCI reset using 'reset' SysFS attribute": https://lists.xen.org/archives/html/xen-devel/2017-12/msg00663.html > > -boris Thanks, -- Pasi _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute 2017-12-08 9:34 ` Jan Beulich 2017-12-12 14:48 ` Govinda Tatti @ 2017-12-12 15:01 ` Govinda Tatti 1 sibling, 0 replies; 40+ messages in thread From: Govinda Tatti @ 2017-12-12 15:01 UTC (permalink / raw) To: Jan Beulich Cc: Juergen Gross, linux-pci, linux-kernel, bhelgaas, xen-devel, boris.ostrovsky, roger.pau Thanks Jan for your review comments. Please see below for my comments. On 12/8/2017 3:34 AM, Jan Beulich wrote: >>>> On 07.12.17 at 23:21,<Govinda.Tatti@Oracle.COM> wrote: >> Due to the complexity with the PCI lock we cannot do the reset when a >> device is bound ('echo $BDF > bind') or when unbound ('echo $BDF > unbind') >> as the pci_[slot|bus]_reset also takes the same lock resulting in a >> dead-lock. > It took me a moment to figure that here you're referring to the > process of (un)binding, not the state. To avoid that ambiguity in > wording, how about "... we cannot do the reset while a device is > being bound (...) or while it is being unbound ..."? Sure, I will fix it. >> --- a/Documentation/ABI/testing/sysfs-driver-pciback >> +++ b/Documentation/ABI/testing/sysfs-driver-pciback >> @@ -11,3 +11,18 @@ Description: >> #echo 00:19.0-E0:2:FF > /sys/bus/pci/drivers/pciback/quirks >> will allow the guest to read and write to the configuration >> register 0x0E. >> + >> +What: /sys/bus/pci/drivers/pciback/reset >> +Date: Dec 2017 >> +KernelVersion: 4.15 >> +Contact:xen-devel@lists.xenproject.org >> +Description: >> + An option to perform a flr/slot/bus reset when a PCI device >> + is owned by Xen PCI backend. Writing a string of DDDD:BB:DD.F > SSSS:BB:DD.F (or else the D-s are ambiguous, the more that "domain" > in Xen code is ambiguous anyway - I continue to be mislead by struct > pcistub_device_id's domain field) Thanks for catching this issue. I will fix it. > Also I assume the SSSS part is optional (default zero), which > probably can and should be expressed in some way. SSSS can be 0 or non-zero, subject to system configuration. >> --- a/drivers/xen/xen-pciback/pci_stub.c >> +++ b/drivers/xen/xen-pciback/pci_stub.c >> @@ -313,6 +313,102 @@ void pcistub_put_pci_dev(struct pci_dev *dev) >> up_write(&pcistub_sem); >> } >> >> +struct pcistub_args { >> + const struct pci_dev *dev; >> + unsigned int dcount; > The sole use of this field is for a debug message. Why not drop it > and make "dev" the "data" argument without further indirection? I prefer to keep this data structure since it will be helpful to debug any issues orfor future enhancements. >> +static int pcistub_device_search(struct pci_dev *dev, void *data) >> +{ >> + struct pcistub_device *psdev; >> + struct pcistub_args *arg = data; >> + bool found = false; >> + unsigned long flags; >> + >> + spin_lock_irqsave(&pcistub_devices_lock, flags); >> + >> + list_for_each_entry(psdev, &pcistub_devices, dev_list) { >> + if (psdev->dev == dev) { >> + found = true; >> + arg->dcount++; >> + break; > Neither here nor in the caller I can see a check whether the device > is currently assigned to a guest. Ownership by pciback alone imo is > not sufficient to allow a reset to be performed. I can add the following check if ((psdev->dev == dev) && (pci_is_dev_assigned(dev))) >> +static int pcistub_device_reset(struct pci_dev *dev) >> +{ >> + struct xen_pcibk_dev_data *dev_data; >> + bool slot = false, bus = false; >> + struct pcistub_args arg = {}; >> + >> + if (!dev) >> + return -EINVAL; >> + >> + dev_dbg(&dev->dev, "[%s]\n", __func__); >> + >> + /* First check and try FLR */ >> + if (pcie_has_flr(dev)) { >> + dev_dbg(&dev->dev, "resetting %s device using FLR\n", >> + pci_name(dev)); >> + pcie_flr(dev); > The lack of error check here puzzled me, but I see the function > indeed returns void right now. I think the prereq patch should > change this along with exporting the function - you really don't > want the device to be handed to a guest when the FLR timed > out. We will change pcie_flr() to return error code. I will make this change in the next version of this patch. >> + return 0; >> + } >> + >> + if (!pci_probe_reset_slot(dev->slot)) >> + slot = true; >> + else if ((!pci_probe_reset_bus(dev->bus)) && >> + (!pci_is_root_bus(dev->bus))) > Too many parentheses for my taste. I will fix it. >> +static ssize_t reset_store(struct device_driver *drv, const char *buf, >> + size_t count) >> +{ >> + struct pcistub_device *psdev; >> + int domain, bus, slot, func; >> + int err; >> + >> + err = str_to_slot(buf, &domain, &bus, &slot, &func); >> + if (err) >> + return err; >> + >> + psdev = pcistub_device_find(domain, bus, slot, func); >> + if (psdev) { >> + err = pcistub_device_reset(psdev->dev); >> + pcistub_device_put(psdev); >> + } else { >> + err = -ENODEV; >> + } >> + >> + if (!err) >> + err = count; >> + >> + return err; >> +} >> +static DRIVER_ATTR_WO(reset); > Would it be worth for reads of the file to return whether the device > can be reset this way (i.e. the result of the checks you do before > actually doing the reset)? I don't think so. Plus, it makes this interface and its usage more complicated. Cheers GOVINDA _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 40+ messages in thread
end of thread, other threads:[~2019-08-26 21:06 UTC | newest] Thread overview: 40+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20171207222145.9769-1-Govinda.Tatti@Oracle.COM> 2017-12-07 22:21 ` [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface Govinda Tatti 2017-12-08 20:24 ` Bjorn Helgaas [not found] ` <20171208202424.GC12367@bhelgaas-glaptop.roam.corp.google.com> 2017-12-12 0:29 ` Govinda Tatti [not found] ` <426eeeab-0dcd-8de3-9c5f-a166acf2c130@Oracle.COM> 2017-12-12 0:59 ` Bjorn Helgaas [not found] ` <20171212005919.GB30595@bhelgaas-glaptop.roam.corp.google.com> 2017-12-13 20:46 ` Govinda Tatti [not found] ` <49956aaf-5fd5-939d-5fc7-231ffdb98b70@Oracle.COM> 2017-12-13 21:24 ` Bjorn Helgaas [not found] ` <20171213212420.GH30595@bhelgaas-glaptop.roam.corp.google.com> 2017-12-14 12:52 ` Christoph Hellwig [not found] ` <20171214125206.GA24958@infradead.org> 2017-12-15 0:24 ` Bjorn Helgaas 2017-12-15 15:48 ` Govinda Tatti 2017-12-15 18:18 ` Bjorn Helgaas [not found] ` <20171215181801.GU30595@bhelgaas-glaptop.roam.corp.google.com> 2017-12-15 20:01 ` Govinda Tatti 2017-12-18 3:09 ` Alexey Kardashevskiy 2017-12-18 12:26 ` Christoph Hellwig [not found] ` <20171218122629.GA18423@infradead.org> 2017-12-18 17:22 ` Govinda Tatti 2018-09-09 18:59 ` Pasi Kärkkäinen [not found] ` <20180909185944.GC18222@reaktio.net> 2018-09-10 2:33 ` Sinan Kaya [not found] ` <9ffe43d2-a44b-974c-85c9-9923d71c5dba@kernel.org> 2018-09-10 9:52 ` Pasi Kärkkäinen [not found] ` <20180910095231.GD18222@reaktio.net> 2018-09-10 17:04 ` Sinan Kaya 2017-12-12 15:07 ` Christoph Hellwig 2017-12-07 22:21 ` [PATCH V3 2/2] Xen/PCIback: Implement PCI flr/slot/bus reset with 'reset' SysFS attribute Govinda Tatti [not found] ` <20171207222145.9769-3-Govinda.Tatti@Oracle.COM> 2017-12-08 9:34 ` Jan Beulich 2017-12-12 14:48 ` Govinda Tatti 2017-12-12 15:01 ` Jan Beulich [not found] ` <5A2FFD690200007800196DFB@prv-mh.provo.novell.com> 2017-12-12 15:14 ` Govinda Tatti 2017-12-15 19:52 ` Govinda Tatti [not found] ` <f19dbb09-ef22-2cf4-fb38-2a7c42b5dc48@Oracle.COM> 2017-12-18 7:36 ` Jan Beulich [not found] ` <5A377E020200007800197FFA@prv-mh.provo.novell.com> 2017-12-18 17:32 ` Boris Ostrovsky [not found] ` <559ffd12-b541-8a69-60bd-fbe10e3dc159@oracle.com> 2018-09-16 11:43 ` Pasi Kärkkäinen [not found] ` <20180916114306.GF18222@reaktio.net> 2018-09-17 18:06 ` Boris Ostrovsky [not found] ` <a726840b-8a5c-0890-73c6-3a95a7205153@oracle.com> 2018-09-18 7:15 ` Pasi Kärkkäinen [not found] ` <20180918071519.GG18222@reaktio.net> 2018-09-18 9:32 ` George Dunlap [not found] ` <5E7DDB68-4E68-48A5-AEEC-EE1B21A50E9E@citrix.com> 2018-09-18 18:09 ` Boris Ostrovsky [not found] ` <352310b3-ec9b-2ceb-83f0-4550718120c3@oracle.com> 2018-09-19 9:05 ` Roger Pau Monné [not found] ` <20180919090526.s3ahnemrt2ik2tx3@mac.bytemobile.com> 2018-10-03 15:51 ` Pasi Kärkkäinen [not found] ` <20181003155104.GH5318@reaktio.net> 2018-10-08 14:32 ` Boris Ostrovsky [not found] ` <f6b8e055-7afc-b4de-af88-425d799dcd28@oracle.com> 2018-10-23 18:40 ` Håkon Alstadheim 2018-10-29 15:30 ` Pasi Kärkkäinen 2018-11-14 14:24 ` [PATCH cargo-cult-version] For linux-4.19.x . " Håkon Alstadheim 2019-08-26 21:05 ` [Xen-devel] [PATCH V3 2/2] " Pasi Kärkkäinen 2017-12-12 15:01 ` Govinda Tatti
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).