[PATCH] pci: Save and restore VFs as a part of a reset

[PATCH] pci: Save and restore VFs as a part of a reset

[Alexander Duyck]

This fixes an issue I found in which triggering a reset via the PCI sysfs
reset while SR-IOV was enabled would leave the VFs in a state in which the
BME and MSI-X enable bits were all cleared.

To correct that I have added code so that the VF state is saved and restored
as a part of the PF save and restore state functions.  By doing this the VF
state is restored as well as the IOV state allowing the VFs to resume function
following a reset.

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---
 drivers/pci/iov.c |   48 ++++++++++++++++++++++++++++++++++++++++++++++--
 drivers/pci/pci.c |    2 ++
 drivers/pci/pci.h |    5 +++++
 3 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index de7a747..645ed71 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -521,13 +521,57 @@ resource_size_t pci_sriov_resource_alignment(struct pci_dev *dev, int resno)
 }
 
 /**
+ * pci_save_iov_state - Save the state of the VF configurations
+ * @dev: the PCI device
+ */
+int pci_save_iov_state(struct pci_dev *dev)
+{
+	struct pci_dev *vfdev = NULL;
+	unsigned short dev_id;
+
+	/* only search if we are a PF */
+	if (!dev->is_physfn)
+		return 0;
+
+	/* retrieve VF device ID */
+	pci_read_config_word(dev, dev->sriov->pos + PCI_SRIOV_VF_DID, &dev_id);
+
+	/* loop through all the VFs and save their state information */
+	while ((vfdev = pci_get_device(dev->vendor, dev_id, vfdev))) {
+		if (vfdev->is_virtfn && (vfdev->physfn == dev)) {
+			int err = pci_save_state(vfdev);
+			if (err)
+				return err;
+		}
+	}
+
+	return 0;
+}
+
+/**
  * pci_restore_iov_state - restore the state of the IOV capability
  * @dev: the PCI device
  */
 void pci_restore_iov_state(struct pci_dev *dev)
 {
-	if (dev->is_physfn)
-		sriov_restore_state(dev);
+	struct pci_dev *vfdev = NULL;
+	unsigned short dev_id;
+
+	if (!dev->is_physfn)
+		return;
+
+	sriov_restore_state(dev);
+
+	/* retrieve VF device ID */
+	pci_read_config_word(dev, dev->sriov->pos + PCI_SRIOV_VF_DID, &dev_id);
+
+	/* loop through all VFs and restore state for any of our VFs */
+	while ((vfdev = pci_get_device(dev->vendor, dev_id, vfdev))) {
+		if (vfdev->is_virtfn && (vfdev->physfn == dev))
+			pci_restore_state(vfdev);
+	}
+
+	return 0;
 }
 
 /**
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 7325d43..120a432 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1015,6 +1015,8 @@ pci_save_state(struct pci_dev *dev)
 		return i;
 	if ((i = pci_save_vc_state(dev)) != 0)
 		return i;
+	if ((i = pci_save_iov_state(dev)) != 0)
+		return i;
 	return 0;
 }
 
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 6bd0822..9185edd 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -254,6 +254,7 @@ void pci_iov_release(struct pci_dev *dev);
 int pci_iov_resource_bar(struct pci_dev *dev, int resno,
 			 enum pci_bar_type *type);
 resource_size_t pci_sriov_resource_alignment(struct pci_dev *dev, int resno);
+int pci_save_iov_state(struct pci_dev *dev);
 void pci_restore_iov_state(struct pci_dev *dev);
 int pci_iov_bus_range(struct pci_bus *bus);
 
@@ -271,6 +272,10 @@ static inline int pci_iov_resource_bar(struct pci_dev *dev, int resno,
 {
 	return 0;
 }
+static inline void pci_save_iov_state(struct pci_dev *dev)
+{
+	return 0;
+}
 static inline void pci_restore_iov_state(struct pci_dev *dev)
 {
 }

Re: [PATCH] pci: Save and restore VFs as a part of a reset

[Bjorn Helgaas]

On Mon, May 05, 2014 at 02:25:17PM -0700, Alexander Duyck wrote:
> This fixes an issue I found in which triggering a reset via the PCI sysfs
> reset while SR-IOV was enabled would leave the VFs in a state in which the
> BME and MSI-X enable bits were all cleared.
> 
> To correct that I have added code so that the VF state is saved and restored
> as a part of the PF save and restore state functions.  By doing this the VF
> state is restored as well as the IOV state allowing the VFs to resume function
> following a reset.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> ---
>  drivers/pci/iov.c |   48 ++++++++++++++++++++++++++++++++++++++++++++++--
>  drivers/pci/pci.c |    2 ++
>  drivers/pci/pci.h |    5 +++++
>  3 files changed, 53 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index de7a747..645ed71 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -521,13 +521,57 @@ resource_size_t pci_sriov_resource_alignment(struct pci_dev *dev, int resno)
>  }
>  
>  /**
> + * pci_save_iov_state - Save the state of the VF configurations
> + * @dev: the PCI device
> + */
> +int pci_save_iov_state(struct pci_dev *dev)
> +{
> +	struct pci_dev *vfdev = NULL;
> +	unsigned short dev_id;
> +
> +	/* only search if we are a PF */
> +	if (!dev->is_physfn)
> +		return 0;
> +
> +	/* retrieve VF device ID */
> +	pci_read_config_word(dev, dev->sriov->pos + PCI_SRIOV_VF_DID, &dev_id);

This is just an optimization to reduce the number of things you get
back from pci_get_device(), and hence reduce the number of times the
"vfdev->is_virtfn && (vfdev->physfn == dev)" condition is false, right?

> +	/* loop through all the VFs and save their state information */
> +	while ((vfdev = pci_get_device(dev->vendor, dev_id, vfdev))) {
> +		if (vfdev->is_virtfn && (vfdev->physfn == dev)) {
> +			int err = pci_save_state(vfdev);

It makes me uneasy to operate on another device (we're resetting A, and
here we save state for B).  I know B is dependent on A, since B is a VF
related to PF A, but what synchronization is there to serialize this
against any other save/restore operations that may be in progress by B's
driver or by a sysfs operation on B?

Is there anything in the reset path that pays attention to whether
resetting this PF will clobber VFs?  Do we care whether those VFs are in
use?  I assume they might be in use by guests?

> +			if (err)
> +				return err;
> +		}
> +	}

pci_get_device() acquires a reference on each device it returns, so this
strategy would require a pci_dev_put().

But I'm not really keen on pci_get_device() in the first place.  It works
by iterating over all PCI devices in the system, which seems like a
sledgehammer approach.  It *is* widely used, but mostly in quirk-type code
from which I avert my eyes.

Maybe you could do something based on pci_walk_bus()?  If you did that, I
think the PCI_SRIOV_VF_DID would become superfluous.

> +
> +	return 0;
> +}
> +
> +/**
>   * pci_restore_iov_state - restore the state of the IOV capability
>   * @dev: the PCI device
>   */
>  void pci_restore_iov_state(struct pci_dev *dev)
>  {
> -	if (dev->is_physfn)
> -		sriov_restore_state(dev);
> +	struct pci_dev *vfdev = NULL;
> +	unsigned short dev_id;
> +
> +	if (!dev->is_physfn)
> +		return;
> +
> +	sriov_restore_state(dev);
> +
> +	/* retrieve VF device ID */
> +	pci_read_config_word(dev, dev->sriov->pos + PCI_SRIOV_VF_DID, &dev_id);
> +
> +	/* loop through all VFs and restore state for any of our VFs */
> +	while ((vfdev = pci_get_device(dev->vendor, dev_id, vfdev))) {
> +		if (vfdev->is_virtfn && (vfdev->physfn == dev))
> +			pci_restore_state(vfdev);
> +	}
> +
> +	return 0;
>  }
>  
>  /**
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 7325d43..120a432 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1015,6 +1015,8 @@ pci_save_state(struct pci_dev *dev)
>  		return i;
>  	if ((i = pci_save_vc_state(dev)) != 0)
>  		return i;
> +	if ((i = pci_save_iov_state(dev)) != 0)
> +		return i;
>  	return 0;
>  }
>  
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 6bd0822..9185edd 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -254,6 +254,7 @@ void pci_iov_release(struct pci_dev *dev);
>  int pci_iov_resource_bar(struct pci_dev *dev, int resno,
>  			 enum pci_bar_type *type);
>  resource_size_t pci_sriov_resource_alignment(struct pci_dev *dev, int resno);
> +int pci_save_iov_state(struct pci_dev *dev);
>  void pci_restore_iov_state(struct pci_dev *dev);
>  int pci_iov_bus_range(struct pci_bus *bus);
>  
> @@ -271,6 +272,10 @@ static inline int pci_iov_resource_bar(struct pci_dev *dev, int resno,
>  {
>  	return 0;
>  }
> +static inline void pci_save_iov_state(struct pci_dev *dev)
> +{
> +	return 0;
> +}
>  static inline void pci_restore_iov_state(struct pci_dev *dev)
>  {
>  }
> 

Re: [PATCH] pci: Save and restore VFs as a part of a reset

[Alexander Duyck]

On 05/27/2014 03:22 PM, Bjorn Helgaas wrote:
> On Mon, May 05, 2014 at 02:25:17PM -0700, Alexander Duyck wrote:
>> This fixes an issue I found in which triggering a reset via the PCI sysfs
>> reset while SR-IOV was enabled would leave the VFs in a state in which the
>> BME and MSI-X enable bits were all cleared.
>>
>> To correct that I have added code so that the VF state is saved and restored
>> as a part of the PF save and restore state functions.  By doing this the VF
>> state is restored as well as the IOV state allowing the VFs to resume function
>> following a reset.
>>
>> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
>> ---
>>  drivers/pci/iov.c |   48 ++++++++++++++++++++++++++++++++++++++++++++++--
>>  drivers/pci/pci.c |    2 ++
>>  drivers/pci/pci.h |    5 +++++
>>  3 files changed, 53 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>> index de7a747..645ed71 100644
>> --- a/drivers/pci/iov.c
>> +++ b/drivers/pci/iov.c
>> @@ -521,13 +521,57 @@ resource_size_t pci_sriov_resource_alignment(struct pci_dev *dev, int resno)
>>  }
>>  
>>  /**
>> + * pci_save_iov_state - Save the state of the VF configurations
>> + * @dev: the PCI device
>> + */
>> +int pci_save_iov_state(struct pci_dev *dev)
>> +{
>> +	struct pci_dev *vfdev = NULL;
>> +	unsigned short dev_id;
>> +
>> +	/* only search if we are a PF */
>> +	if (!dev->is_physfn)
>> +		return 0;
>> +
>> +	/* retrieve VF device ID */
>> +	pci_read_config_word(dev, dev->sriov->pos + PCI_SRIOV_VF_DID, &dev_id);
> 
> This is just an optimization to reduce the number of things you get
> back from pci_get_device(), and hence reduce the number of times the
> "vfdev->is_virtfn && (vfdev->physfn == dev)" condition is false, right?
> 

Yes, this is just a mechanism to reduce the number of items we have to test.

>> +	/* loop through all the VFs and save their state information */
>> +	while ((vfdev = pci_get_device(dev->vendor, dev_id, vfdev))) {
>> +		if (vfdev->is_virtfn && (vfdev->physfn == dev)) {
>> +			int err = pci_save_state(vfdev);
> 
> It makes me uneasy to operate on another device (we're resetting A, and
> here we save state for B).  I know B is dependent on A, since B is a VF
> related to PF A, but what synchronization is there to serialize this
> against any other save/restore operations that may be in progress by B's
> driver or by a sysfs operation on B?

I don't believe there is any synchronization mechanism in place
currently.  I can look into that as well.  Odds are we probably need to
have the VFs check the parent lock before they take any independent action.

> 
> Is there anything in the reset path that pays attention to whether
> resetting this PF will clobber VFs?  Do we care whether those VFs are in
> use?  I assume they might be in use by guests?

The problem I found was that the sysfs reset call doesn't bother to
check with the PF driver at all.  It just clobbers the PF and any VFs on
it without talking to the PF driver.

> 
>> +			if (err)
>> +				return err;
>> +		}
>> +	}
> 
> pci_get_device() acquires a reference on each device it returns, so this
> strategy would require a pci_dev_put().

Yes, if I am not mistaken the pci_dev_put is called as a part of
pci_get_dev_by_id which is what pci_get_device ends up being.

> 
> But I'm not really keen on pci_get_device() in the first place.  It works
> by iterating over all PCI devices in the system, which seems like a
> sledgehammer approach.  It *is* widely used, but mostly in quirk-type code
> from which I avert my eyes.
> 
> Maybe you could do something based on pci_walk_bus()?  If you did that, I
> think the PCI_SRIOV_VF_DID would become superfluous.
> 

I can look into that, I'm not familiar with the interface.  I'll have to
see what the relationship is between the PF and VF in terms of busses as
I don't recall it off of the top of my head.

If it is simple enough I might submit a second patch to update the
pci_vfs_assigned since it is using similar logic.

Thanks,

Alex

Re: [PATCH] pci: Save and restore VFs as a part of a reset

[Bjorn Helgaas]

[+cc Alex, Don]

On Tue, May 27, 2014 at 5:53 PM, Alexander Duyck
<alexander.h.duyck@intel.com> wrote:
> On 05/27/2014 03:22 PM, Bjorn Helgaas wrote:
>> On Mon, May 05, 2014 at 02:25:17PM -0700, Alexander Duyck wrote:
>>> This fixes an issue I found in which triggering a reset via the PCI sysfs
>>> reset while SR-IOV was enabled would leave the VFs in a state in which the
>>> BME and MSI-X enable bits were all cleared.
>>>
>>> To correct that I have added code so that the VF state is saved and restored
>>> as a part of the PF save and restore state functions.  By doing this the VF
>>> state is restored as well as the IOV state allowing the VFs to resume function
>>> following a reset.
>>>
>>> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
>>> ---
>>>  drivers/pci/iov.c |   48 ++++++++++++++++++++++++++++++++++++++++++++++--
>>>  drivers/pci/pci.c |    2 ++
>>>  drivers/pci/pci.h |    5 +++++
>>>  3 files changed, 53 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>>> index de7a747..645ed71 100644
>>> --- a/drivers/pci/iov.c
>>> +++ b/drivers/pci/iov.c
>>> @@ -521,13 +521,57 @@ resource_size_t pci_sriov_resource_alignment(struct pci_dev *dev, int resno)
>>>  }
>>>
>>>  /**
>>> + * pci_save_iov_state - Save the state of the VF configurations
>>> + * @dev: the PCI device
>>> + */
>>> +int pci_save_iov_state(struct pci_dev *dev)
>>> +{
>>> +    struct pci_dev *vfdev = NULL;
>>> +    unsigned short dev_id;
>>> +
>>> +    /* only search if we are a PF */
>>> +    if (!dev->is_physfn)
>>> +            return 0;
>>> +
>>> +    /* retrieve VF device ID */
>>> +    pci_read_config_word(dev, dev->sriov->pos + PCI_SRIOV_VF_DID, &dev_id);
...

>>> +    /* loop through all the VFs and save their state information */
>>> +    while ((vfdev = pci_get_device(dev->vendor, dev_id, vfdev))) {
>>> +            if (vfdev->is_virtfn && (vfdev->physfn == dev)) {
>>> +                    int err = pci_save_state(vfdev);
>>
>> It makes me uneasy to operate on another device (we're resetting A, and
>> here we save state for B).  I know B is dependent on A, since B is a VF
>> related to PF A, but what synchronization is there to serialize this
>> against any other save/restore operations that may be in progress by B's
>> driver or by a sysfs operation on B?
>
> I don't believe there is any synchronization mechanism in place
> currently.  I can look into that as well.  Odds are we probably need to
> have the VFs check the parent lock before they take any independent action.

It's just the whole question of how we manage the single "saved-state"
area.  Right now, I think almost all use of it is under control of the
driver that owns the device, in suspend/resume methods.  The
exceptions are the PM suspend/freeze/etc. routines in
pci/pci-driver.c, which I assume prevent the driver from running and
are therefore safe, and the reset path.  I don't know how the

>> Is there anything in the reset path that pays attention to whether
>> resetting this PF will clobber VFs?  Do we care whether those VFs are in
>> use?  I assume they might be in use by guests?
>
> The problem I found was that the sysfs reset call doesn't bother to
> check with the PF driver at all.  It just clobbers the PF and any VFs on
> it without talking to the PF driver.

There is Keith Busch's recent patch:
http://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h=pci/hotplug&id=3ebe7f9f7e4a4fd1f6461ecd01ff2961317a483a
.  I dunno if that's useful to you or not.

And I'm not sure there's actually a requirement to *have* a PF driver.
 Obviously there has to be a way to enable the VFs, but once they're
enabled, it might be possible to keep using them via VF drivers even
without a PF driver in the picture.

Maybe resetting the PF should just fail if there's an active VF.  If
you need to reset the PF, you'd have to unbind the VFs first.

>>> +                    if (err)
>>> +                            return err;
>>> +            }
>>> +    }
>>
>> pci_get_device() acquires a reference on each device it returns, so this
>> strategy would require a pci_dev_put().
>
> Yes, if I am not mistaken the pci_dev_put is called as a part of
> pci_get_dev_by_id which is what pci_get_device ends up being.

Oh, yeah, you're right.  I forgot about that.  Since you call it in a
loop until you get NULL back, you're OK.  It's only when you stop
before you get NULL that you have to deal with the extra reference.

>> But I'm not really keen on pci_get_device() in the first place.  It works
>> by iterating over all PCI devices in the system, which seems like a
>> sledgehammer approach.  It *is* widely used, but mostly in quirk-type code
>> from which I avert my eyes.
>>
>> Maybe you could do something based on pci_walk_bus()?  If you did that, I
>> think the PCI_SRIOV_VF_DID would become superfluous.
>>
>
> I can look into that, I'm not familiar with the interface.  I'll have to
> see what the relationship is between the PF and VF in terms of busses as
> I don't recall it off of the top of my head.

This reminds me about an open problem: VFs can be on "virtual" buses,
which aren't really connected in the hierarchy, and I don't think we
have a nice way to iterate over them.  So probably pci_get_device() is
the best we can do now.

Bjorn

Re: [PATCH] pci: Save and restore VFs as a part of a reset

[Alex Williamson]

On Tue, 2014-05-27 at 19:19 -0600, Bjorn Helgaas wrote:
> [+cc Alex, Don]
> 
> On Tue, May 27, 2014 at 5:53 PM, Alexander Duyck
> <alexander.h.duyck@intel.com> wrote:
> > On 05/27/2014 03:22 PM, Bjorn Helgaas wrote:
> >> On Mon, May 05, 2014 at 02:25:17PM -0700, Alexander Duyck wrote:
> >>> This fixes an issue I found in which triggering a reset via the PCI sysfs
> >>> reset while SR-IOV was enabled would leave the VFs in a state in which the
> >>> BME and MSI-X enable bits were all cleared.
> >>>
> >>> To correct that I have added code so that the VF state is saved and restored
> >>> as a part of the PF save and restore state functions.  By doing this the VF
> >>> state is restored as well as the IOV state allowing the VFs to resume function
> >>> following a reset.
> >>>
> >>> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> >>> ---
> >>>  drivers/pci/iov.c |   48 ++++++++++++++++++++++++++++++++++++++++++++++--
> >>>  drivers/pci/pci.c |    2 ++
> >>>  drivers/pci/pci.h |    5 +++++
> >>>  3 files changed, 53 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> >>> index de7a747..645ed71 100644
> >>> --- a/drivers/pci/iov.c
> >>> +++ b/drivers/pci/iov.c
> >>> @@ -521,13 +521,57 @@ resource_size_t pci_sriov_resource_alignment(struct pci_dev *dev, int resno)
> >>>  }
> >>>
> >>>  /**
> >>> + * pci_save_iov_state - Save the state of the VF configurations
> >>> + * @dev: the PCI device
> >>> + */
> >>> +int pci_save_iov_state(struct pci_dev *dev)
> >>> +{
> >>> +    struct pci_dev *vfdev = NULL;
> >>> +    unsigned short dev_id;
> >>> +
> >>> +    /* only search if we are a PF */
> >>> +    if (!dev->is_physfn)
> >>> +            return 0;
> >>> +
> >>> +    /* retrieve VF device ID */
> >>> +    pci_read_config_word(dev, dev->sriov->pos + PCI_SRIOV_VF_DID, &dev_id);
> ...
> 
> >>> +    /* loop through all the VFs and save their state information */
> >>> +    while ((vfdev = pci_get_device(dev->vendor, dev_id, vfdev))) {
> >>> +            if (vfdev->is_virtfn && (vfdev->physfn == dev)) {
> >>> +                    int err = pci_save_state(vfdev);
> >>
> >> It makes me uneasy to operate on another device (we're resetting A, and
> >> here we save state for B).  I know B is dependent on A, since B is a VF
> >> related to PF A, but what synchronization is there to serialize this
> >> against any other save/restore operations that may be in progress by B's
> >> driver or by a sysfs operation on B?
> >
> > I don't believe there is any synchronization mechanism in place
> > currently.  I can look into that as well.  Odds are we probably need to
> > have the VFs check the parent lock before they take any independent action.
> 
> It's just the whole question of how we manage the single "saved-state"
> area.  Right now, I think almost all use of it is under control of the
> driver that owns the device, in suspend/resume methods.  The
> exceptions are the PM suspend/freeze/etc. routines in
> pci/pci-driver.c, which I assume prevent the driver from running and
> are therefore safe, and the reset path.  I don't know how the

Makes me a little uneasy too, what happens to a transaction headed
to/from the VF while the PF is in a reset state?  I suspect not good
things.  OTOH, the reset interface and a good bit of pci-sysfs have
always been at-your-own-risk interfaces and this restores some bits that
might get us closer to it being survivable.

We do have a way for drivers to get a long-term save state that they can
keep on their own, pci_save_state(); pci_store_saved_state() along with
pci_load_saved_state(); pci_restore_state().  Both KVM and VFIO use this
for assigning a device so we can attempt to re-load the pre-assigned
saved state.

> >> Is there anything in the reset path that pays attention to whether
> >> resetting this PF will clobber VFs?  Do we care whether those VFs are in
> >> use?  I assume they might be in use by guests?
> >
> > The problem I found was that the sysfs reset call doesn't bother to
> > check with the PF driver at all.  It just clobbers the PF and any VFs on
> > it without talking to the PF driver.
> 
> There is Keith Busch's recent patch:
> http://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h=pci/hotplug&id=3ebe7f9f7e4a4fd1f6461ecd01ff2961317a483a
> .  I dunno if that's useful to you or not.
> 
> And I'm not sure there's actually a requirement to *have* a PF driver.
>  Obviously there has to be a way to enable the VFs, but once they're
> enabled, it might be possible to keep using them via VF drivers even
> without a PF driver in the picture.
> 
> Maybe resetting the PF should just fail if there's an active VF.  If
> you need to reset the PF, you'd have to unbind the VFs first.

The use case is certainly questionable, personally I'm not going to
expect VFs to continue working after the PF is reset.  Driver binding
gets complicated, especially when KVM doesn't actually bind devices to
use them.  Hopefully we'll get that out of the tree some day though.  I
suppose we could -EBUSY the PF reset as long as VFs are enabled.

> >>> +                    if (err)
> >>> +                            return err;
> >>> +            }
> >>> +    }
> >>
> >> pci_get_device() acquires a reference on each device it returns, so this
> >> strategy would require a pci_dev_put().
> >
> > Yes, if I am not mistaken the pci_dev_put is called as a part of
> > pci_get_dev_by_id which is what pci_get_device ends up being.
> 
> Oh, yeah, you're right.  I forgot about that.  Since you call it in a
> loop until you get NULL back, you're OK.  It's only when you stop
> before you get NULL that you have to deal with the extra reference.
> 
> >> But I'm not really keen on pci_get_device() in the first place.  It works
> >> by iterating over all PCI devices in the system, which seems like a
> >> sledgehammer approach.  It *is* widely used, but mostly in quirk-type code
> >> from which I avert my eyes.
> >>
> >> Maybe you could do something based on pci_walk_bus()?  If you did that, I
> >> think the PCI_SRIOV_VF_DID would become superfluous.
> >>
> >
> > I can look into that, I'm not familiar with the interface.  I'll have to
> > see what the relationship is between the PF and VF in terms of busses as
> > I don't recall it off of the top of my head.
> 
> This reminds me about an open problem: VFs can be on "virtual" buses,
> which aren't really connected in the hierarchy, and I don't think we
> have a nice way to iterate over them.  So probably pci_get_device() is
> the best we can do now.

Yeah, those virtual buses don't have a bus->self, we just have to skip
to bus->parent->self.  pci_walk_bus() goes in the opposite direction,
but without an actual device hosting the bus, I don't see how it finds
it.  Thanks,

Alex

Re: [PATCH] pci: Save and restore VFs as a part of a reset

[Alexander Duyck]

On 05/27/2014 09:12 PM, Alex Williamson wrote:
> On Tue, 2014-05-27 at 19:19 -0600, Bjorn Helgaas wrote:
>> [+cc Alex, Don]
>>
>> On Tue, May 27, 2014 at 5:53 PM, Alexander Duyck
>> <alexander.h.duyck@intel.com> wrote:
>>> On 05/27/2014 03:22 PM, Bjorn Helgaas wrote:
>>>> On Mon, May 05, 2014 at 02:25:17PM -0700, Alexander Duyck wrote:
>>>>> This fixes an issue I found in which triggering a reset via the PCI sysfs
>>>>> reset while SR-IOV was enabled would leave the VFs in a state in which the
>>>>> BME and MSI-X enable bits were all cleared.
>>>>>
>>>>> To correct that I have added code so that the VF state is saved and restored
>>>>> as a part of the PF save and restore state functions.  By doing this the VF
>>>>> state is restored as well as the IOV state allowing the VFs to resume function
>>>>> following a reset.
>>>>>
>>>>> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
>>>>> ---
>>>>>  drivers/pci/iov.c |   48 ++++++++++++++++++++++++++++++++++++++++++++++--
>>>>>  drivers/pci/pci.c |    2 ++
>>>>>  drivers/pci/pci.h |    5 +++++
>>>>>  3 files changed, 53 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>>>>> index de7a747..645ed71 100644
>>>>> --- a/drivers/pci/iov.c
>>>>> +++ b/drivers/pci/iov.c
>>>>> @@ -521,13 +521,57 @@ resource_size_t pci_sriov_resource_alignment(struct pci_dev *dev, int resno)
>>>>>  }
>>>>>
>>>>>  /**
>>>>> + * pci_save_iov_state - Save the state of the VF configurations
>>>>> + * @dev: the PCI device
>>>>> + */
>>>>> +int pci_save_iov_state(struct pci_dev *dev)
>>>>> +{
>>>>> +    struct pci_dev *vfdev = NULL;
>>>>> +    unsigned short dev_id;
>>>>> +
>>>>> +    /* only search if we are a PF */
>>>>> +    if (!dev->is_physfn)
>>>>> +            return 0;
>>>>> +
>>>>> +    /* retrieve VF device ID */
>>>>> +    pci_read_config_word(dev, dev->sriov->pos + PCI_SRIOV_VF_DID, &dev_id);
>> ...
>>
>>>>> +    /* loop through all the VFs and save their state information */
>>>>> +    while ((vfdev = pci_get_device(dev->vendor, dev_id, vfdev))) {
>>>>> +            if (vfdev->is_virtfn && (vfdev->physfn == dev)) {
>>>>> +                    int err = pci_save_state(vfdev);
>>>>
>>>> It makes me uneasy to operate on another device (we're resetting A, and
>>>> here we save state for B).  I know B is dependent on A, since B is a VF
>>>> related to PF A, but what synchronization is there to serialize this
>>>> against any other save/restore operations that may be in progress by B's
>>>> driver or by a sysfs operation on B?
>>>
>>> I don't believe there is any synchronization mechanism in place
>>> currently.  I can look into that as well.  Odds are we probably need to
>>> have the VFs check the parent lock before they take any independent action.
>>
>> It's just the whole question of how we manage the single "saved-state"
>> area.  Right now, I think almost all use of it is under control of the
>> driver that owns the device, in suspend/resume methods.  The
>> exceptions are the PM suspend/freeze/etc. routines in
>> pci/pci-driver.c, which I assume prevent the driver from running and
>> are therefore safe, and the reset path.  I don't know how the
> 
> Makes me a little uneasy too, what happens to a transaction headed
> to/from the VF while the PF is in a reset state?  I suspect not good
> things.  OTOH, the reset interface and a good bit of pci-sysfs have
> always been at-your-own-risk interfaces and this restores some bits that
> might get us closer to it being survivable.
> 
> We do have a way for drivers to get a long-term save state that they can
> keep on their own, pci_save_state(); pci_store_saved_state() along with
> pci_load_saved_state(); pci_restore_state().  Both KVM and VFIO use this
> for assigning a device so we can attempt to re-load the pre-assigned
> saved state.

So it sounds like this patch would interfere with the functioning of KVM
and VFIO then.  So perhaps I should just not take the direct approach of
saving it myself then.  Perhaps it would be better to just use the
notifier provided below to notify the VFs that an event has occurred.

>>>> Is there anything in the reset path that pays attention to whether
>>>> resetting this PF will clobber VFs?  Do we care whether those VFs are in
>>>> use?  I assume they might be in use by guests?
>>>
>>> The problem I found was that the sysfs reset call doesn't bother to
>>> check with the PF driver at all.  It just clobbers the PF and any VFs on
>>> it without talking to the PF driver.
>>
>> There is Keith Busch's recent patch:
>> http://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h=pci/hotplug&id=3ebe7f9f7e4a4fd1f6461ecd01ff2961317a483a
>> .  I dunno if that's useful to you or not.
>>
>> And I'm not sure there's actually a requirement to *have* a PF driver.
>>  Obviously there has to be a way to enable the VFs, but once they're
>> enabled, it might be possible to keep using them via VF drivers even
>> without a PF driver in the picture.
>>
>> Maybe resetting the PF should just fail if there's an active VF.  If
>> you need to reset the PF, you'd have to unbind the VFs first.
> 
> The use case is certainly questionable, personally I'm not going to
> expect VFs to continue working after the PF is reset.  Driver binding
> gets complicated, especially when KVM doesn't actually bind devices to
> use them.  Hopefully we'll get that out of the tree some day though.  I
> suppose we could -EBUSY the PF reset as long as VFs are enabled.

What I could do is go through and notify the VFs that they are about to
get hit by a reset.  What they do with that information would be up to them.

So if the VFs are loaded on the host I could then at least allow them to
recover by saving and restoring the config space within the driver
themselves.

>>>> But I'm not really keen on pci_get_device() in the first place.  It works
>>>> by iterating over all PCI devices in the system, which seems like a
>>>> sledgehammer approach.  It *is* widely used, but mostly in quirk-type code
>>>> from which I avert my eyes.
>>>>
>>>> Maybe you could do something based on pci_walk_bus()?  If you did that, I
>>>> think the PCI_SRIOV_VF_DID would become superfluous.
>>>>
>>>
>>> I can look into that, I'm not familiar with the interface.  I'll have to
>>> see what the relationship is between the PF and VF in terms of busses as
>>> I don't recall it off of the top of my head.
>>
>> This reminds me about an open problem: VFs can be on "virtual" buses,
>> which aren't really connected in the hierarchy, and I don't think we
>> have a nice way to iterate over them.  So probably pci_get_device() is
>> the best we can do now.
> 
> Yeah, those virtual buses don't have a bus->self, we just have to skip
> to bus->parent->self.  pci_walk_bus() goes in the opposite direction,
> but without an actual device hosting the bus, I don't see how it finds
> it.  Thanks,
> 
> Alex

It seems like we should be able to come up with something like
pci_walk_vbus() though or something similar.  All we would need to do is
search the VFs on the bus of the PF and all child busses to that bus if
I am not mistaken.

Thanks,

Alex

Re: [PATCH] pci: Save and restore VFs as a part of a reset

[Alex Williamson]

On Wed, 2014-05-28 at 09:39 -0700, Alexander Duyck wrote:
> On 05/27/2014 09:12 PM, Alex Williamson wrote:
> > On Tue, 2014-05-27 at 19:19 -0600, Bjorn Helgaas wrote:
> >> [+cc Alex, Don]
> >>
> >> On Tue, May 27, 2014 at 5:53 PM, Alexander Duyck
> >> <alexander.h.duyck@intel.com> wrote:
> >>> On 05/27/2014 03:22 PM, Bjorn Helgaas wrote:
> >>>> On Mon, May 05, 2014 at 02:25:17PM -0700, Alexander Duyck wrote:
> >>>>> This fixes an issue I found in which triggering a reset via the PCI sysfs
> >>>>> reset while SR-IOV was enabled would leave the VFs in a state in which the
> >>>>> BME and MSI-X enable bits were all cleared.
> >>>>>
> >>>>> To correct that I have added code so that the VF state is saved and restored
> >>>>> as a part of the PF save and restore state functions.  By doing this the VF
> >>>>> state is restored as well as the IOV state allowing the VFs to resume function
> >>>>> following a reset.
> >>>>>
> >>>>> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> >>>>> ---
> >>>>>  drivers/pci/iov.c |   48 ++++++++++++++++++++++++++++++++++++++++++++++--
> >>>>>  drivers/pci/pci.c |    2 ++
> >>>>>  drivers/pci/pci.h |    5 +++++
> >>>>>  3 files changed, 53 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> >>>>> index de7a747..645ed71 100644
> >>>>> --- a/drivers/pci/iov.c
> >>>>> +++ b/drivers/pci/iov.c
> >>>>> @@ -521,13 +521,57 @@ resource_size_t pci_sriov_resource_alignment(struct pci_dev *dev, int resno)
> >>>>>  }
> >>>>>
> >>>>>  /**
> >>>>> + * pci_save_iov_state - Save the state of the VF configurations
> >>>>> + * @dev: the PCI device
> >>>>> + */
> >>>>> +int pci_save_iov_state(struct pci_dev *dev)
> >>>>> +{
> >>>>> +    struct pci_dev *vfdev = NULL;
> >>>>> +    unsigned short dev_id;
> >>>>> +
> >>>>> +    /* only search if we are a PF */
> >>>>> +    if (!dev->is_physfn)
> >>>>> +            return 0;
> >>>>> +
> >>>>> +    /* retrieve VF device ID */
> >>>>> +    pci_read_config_word(dev, dev->sriov->pos + PCI_SRIOV_VF_DID, &dev_id);
> >> ...
> >>
> >>>>> +    /* loop through all the VFs and save their state information */
> >>>>> +    while ((vfdev = pci_get_device(dev->vendor, dev_id, vfdev))) {
> >>>>> +            if (vfdev->is_virtfn && (vfdev->physfn == dev)) {
> >>>>> +                    int err = pci_save_state(vfdev);
> >>>>
> >>>> It makes me uneasy to operate on another device (we're resetting A, and
> >>>> here we save state for B).  I know B is dependent on A, since B is a VF
> >>>> related to PF A, but what synchronization is there to serialize this
> >>>> against any other save/restore operations that may be in progress by B's
> >>>> driver or by a sysfs operation on B?
> >>>
> >>> I don't believe there is any synchronization mechanism in place
> >>> currently.  I can look into that as well.  Odds are we probably need to
> >>> have the VFs check the parent lock before they take any independent action.
> >>
> >> It's just the whole question of how we manage the single "saved-state"
> >> area.  Right now, I think almost all use of it is under control of the
> >> driver that owns the device, in suspend/resume methods.  The
> >> exceptions are the PM suspend/freeze/etc. routines in
> >> pci/pci-driver.c, which I assume prevent the driver from running and
> >> are therefore safe, and the reset path.  I don't know how the
> > 
> > Makes me a little uneasy too, what happens to a transaction headed
> > to/from the VF while the PF is in a reset state?  I suspect not good
> > things.  OTOH, the reset interface and a good bit of pci-sysfs have
> > always been at-your-own-risk interfaces and this restores some bits that
> > might get us closer to it being survivable.
> > 
> > We do have a way for drivers to get a long-term save state that they can
> > keep on their own, pci_save_state(); pci_store_saved_state() along with
> > pci_load_saved_state(); pci_restore_state().  Both KVM and VFIO use this
> > for assigning a device so we can attempt to re-load the pre-assigned
> > saved state.
> 
> So it sounds like this patch would interfere with the functioning of KVM
> and VFIO then.  So perhaps I should just not take the direct approach of
> saving it myself then.  Perhaps it would be better to just use the
> notifier provided below to notify the VFs that an event has occurred.

No, I don't think it interferes with KVM/VFIO any more than it does any
other VF driver.  KVM/VFIO will save off the pre-assigned device state
into their own buffer to be restored later, so use of the save buffer
here should be ok unless there's a direct race.  I think you're still
making the VF more likely to be usable after a PF reset, but maybe the
question is still whether we should allow it or pretend that we can
handle continued VF operation after a PF reset.

> >>>> Is there anything in the reset path that pays attention to whether
> >>>> resetting this PF will clobber VFs?  Do we care whether those VFs are in
> >>>> use?  I assume they might be in use by guests?
> >>>
> >>> The problem I found was that the sysfs reset call doesn't bother to
> >>> check with the PF driver at all.  It just clobbers the PF and any VFs on
> >>> it without talking to the PF driver.
> >>
> >> There is Keith Busch's recent patch:
> >> http://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h=pci/hotplug&id=3ebe7f9f7e4a4fd1f6461ecd01ff2961317a483a
> >> .  I dunno if that's useful to you or not.
> >>
> >> And I'm not sure there's actually a requirement to *have* a PF driver.
> >>  Obviously there has to be a way to enable the VFs, but once they're
> >> enabled, it might be possible to keep using them via VF drivers even
> >> without a PF driver in the picture.
> >>
> >> Maybe resetting the PF should just fail if there's an active VF.  If
> >> you need to reset the PF, you'd have to unbind the VFs first.
> > 
> > The use case is certainly questionable, personally I'm not going to
> > expect VFs to continue working after the PF is reset.  Driver binding
> > gets complicated, especially when KVM doesn't actually bind devices to
> > use them.  Hopefully we'll get that out of the tree some day though.  I
> > suppose we could -EBUSY the PF reset as long as VFs are enabled.
> 
> What I could do is go through and notify the VFs that they are about to
> get hit by a reset.  What they do with that information would be up to them.
> 
> So if the VFs are loaded on the host I could then at least allow them to
> recover by saving and restoring the config space within the driver
> themselves.

Maybe if the notifier list had to return some affirmative result for the
PF reset to proceed.  Thanks,

Alex

> >>>> But I'm not really keen on pci_get_device() in the first place.  It works
> >>>> by iterating over all PCI devices in the system, which seems like a
> >>>> sledgehammer approach.  It *is* widely used, but mostly in quirk-type code
> >>>> from which I avert my eyes.
> >>>>
> >>>> Maybe you could do something based on pci_walk_bus()?  If you did that, I
> >>>> think the PCI_SRIOV_VF_DID would become superfluous.
> >>>>
> >>>
> >>> I can look into that, I'm not familiar with the interface.  I'll have to
> >>> see what the relationship is between the PF and VF in terms of busses as
> >>> I don't recall it off of the top of my head.
> >>
> >> This reminds me about an open problem: VFs can be on "virtual" buses,
> >> which aren't really connected in the hierarchy, and I don't think we
> >> have a nice way to iterate over them.  So probably pci_get_device() is
> >> the best we can do now.
> > 
> > Yeah, those virtual buses don't have a bus->self, we just have to skip
> > to bus->parent->self.  pci_walk_bus() goes in the opposite direction,
> > but without an actual device hosting the bus, I don't see how it finds
> > it.  Thanks,
> > 
> > Alex
> 
> It seems like we should be able to come up with something like
> pci_walk_vbus() though or something similar.  All we would need to do is
> search the VFs on the bus of the PF and all child busses to that bus if
> I am not mistaken.
> 
> Thanks,
> 
> Alex
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pci: Save and restore VFs as a part of a reset

[Bjorn Helgaas]

On Wed, May 28, 2014 at 10:39 AM, Alexander Duyck
<alexander.h.duyck@intel.com> wrote:
> On 05/27/2014 09:12 PM, Alex Williamson wrote:
>> On Tue, 2014-05-27 at 19:19 -0600, Bjorn Helgaas wrote:

>>> Maybe resetting the PF should just fail if there's an active VF.  If
>>> you need to reset the PF, you'd have to unbind the VFs first.
>>
>> The use case is certainly questionable, personally I'm not going to
>> expect VFs to continue working after the PF is reset.  Driver binding
>> gets complicated, especially when KVM doesn't actually bind devices to
>> use them.  Hopefully we'll get that out of the tree some day though.  I
>> suppose we could -EBUSY the PF reset as long as VFs are enabled.
>
> What I could do is go through and notify the VFs that they are about to
> get hit by a reset.  What they do with that information would be up to them.
>
> So if the VFs are loaded on the host I could then at least allow them to
> recover by saving and restoring the config space within the driver
> themselves.

I really like the idea of punting by failing the PF reset if there are
any active VFs.  That's a really easy way of making sure we aren't
going to blow up any guests.  What problems would it cause if we went
this route?

>>> This reminds me about an open problem: VFs can be on "virtual" buses,
>>> which aren't really connected in the hierarchy, and I don't think we
>>> have a nice way to iterate over them.  So probably pci_get_device() is
>>> the best we can do now.
>>
>> Yeah, those virtual buses don't have a bus->self, we just have to skip
>> to bus->parent->self.  pci_walk_bus() goes in the opposite direction,
>> but without an actual device hosting the bus, I don't see how it finds
>> it.  Thanks,
>
> It seems like we should be able to come up with something like
> pci_walk_vbus() though or something similar.  All we would need to do is
> search the VFs on the bus of the PF and all child busses to that bus if
> I am not mistaken.

I don't think that's going to work because the virtual buses don't
appear as the child bus of anything.

Bjorn

Re: [PATCH] pci: Save and restore VFs as a part of a reset

[Don Dutile]

On 05/28/2014 04:14 PM, Bjorn Helgaas wrote:
> On Wed, May 28, 2014 at 10:39 AM, Alexander Duyck
> <alexander.h.duyck@intel.com> wrote:
>> On 05/27/2014 09:12 PM, Alex Williamson wrote:
>>> On Tue, 2014-05-27 at 19:19 -0600, Bjorn Helgaas wrote:
>
>>>> Maybe resetting the PF should just fail if there's an active VF.  If
>>>> you need to reset the PF, you'd have to unbind the VFs first.
>>>
>>> The use case is certainly questionable, personally I'm not going to
>>> expect VFs to continue working after the PF is reset.  Driver binding
>>> gets complicated, especially when KVM doesn't actually bind devices to
>>> use them.  Hopefully we'll get that out of the tree some day though.  I
>>> suppose we could -EBUSY the PF reset as long as VFs are enabled.
>>
>> What I could do is go through and notify the VFs that they are about to
>> get hit by a reset.  What they do with that information would be up to them.
>>
>> So if the VFs are loaded on the host I could then at least allow them to
>> recover by saving and restoring the config space within the driver
>> themselves.
>
> I really like the idea of punting by failing the PF reset if there are
> any active VFs.  That's a really easy way of making sure we aren't
> going to blow up any guests.  What problems would it cause if we went
> this route?
>
I think this is the safest route.  PF<->VF interaction isn't architected,
and resetting the PF with active VFs will probably hang a number of SRIOV
implementations, requiring a system-level reset to correct the compounded problem.

>>>> This reminds me about an open problem: VFs can be on "virtual" buses,
>>>> which aren't really connected in the hierarchy, and I don't think we
>>>> have a nice way to iterate over them.  So probably pci_get_device() is
>>>> the best we can do now.
>>>
>>> Yeah, those virtual buses don't have a bus->self, we just have to skip
>>> to bus->parent->self.  pci_walk_bus() goes in the opposite direction,
>>> but without an actual device hosting the bus, I don't see how it finds
>>> it.  Thanks,
>>
>> It seems like we should be able to come up with something like
>> pci_walk_vbus() though or something similar.  All we would need to do is
>> search the VFs on the bus of the PF and all child busses to that bus if
>> I am not mistaken.
>
> I don't think that's going to work because the virtual buses don't
> appear as the child bus of anything.
>
+1.

> Bjorn
>

Re: [PATCH] pci: Save and restore VFs as a part of a reset

[Alexander Duyck]

On 05/28/2014 01:34 PM, Don Dutile wrote:
> On 05/28/2014 04:14 PM, Bjorn Helgaas wrote:
>> On Wed, May 28, 2014 at 10:39 AM, Alexander Duyck
>> <alexander.h.duyck@intel.com> wrote:
>>> On 05/27/2014 09:12 PM, Alex Williamson wrote:
>>>> On Tue, 2014-05-27 at 19:19 -0600, Bjorn Helgaas wrote:
>>
>>>>> Maybe resetting the PF should just fail if there's an active VF.  If
>>>>> you need to reset the PF, you'd have to unbind the VFs first.
>>>>
>>>> The use case is certainly questionable, personally I'm not going to
>>>> expect VFs to continue working after the PF is reset.  Driver binding
>>>> gets complicated, especially when KVM doesn't actually bind devices to
>>>> use them.  Hopefully we'll get that out of the tree some day though.  I
>>>> suppose we could -EBUSY the PF reset as long as VFs are enabled.
>>>
>>> What I could do is go through and notify the VFs that they are about to
>>> get hit by a reset.  What they do with that information would be up
>>> to them.
>>>
>>> So if the VFs are loaded on the host I could then at least allow them to
>>> recover by saving and restoring the config space within the driver
>>> themselves.
>>
>> I really like the idea of punting by failing the PF reset if there are
>> any active VFs.  That's a really easy way of making sure we aren't
>> going to blow up any guests.  What problems would it cause if we went
>> this route?
>>
> I think this is the safest route.  PF<->VF interaction isn't architected,
> and resetting the PF with active VFs will probably hang a number of SRIOV
> implementations, requiring a system-level reset to correct the
> compounded problem.

Well it still might be worth while to allow a full PCIe reset in cases
where the hardware has gotten into a bad state.  It seems like it might
be worthwhile to update the newly added reset notifier to allow for the
device to indicate if it ready for a reset or not, with the default
being to return -ENOTTY if the function is not implemented.

> 
>>>>> This reminds me about an open problem: VFs can be on "virtual" buses,
>>>>> which aren't really connected in the hierarchy, and I don't think we
>>>>> have a nice way to iterate over them.  So probably pci_get_device() is
>>>>> the best we can do now.
>>>>
>>>> Yeah, those virtual buses don't have a bus->self, we just have to skip
>>>> to bus->parent->self.  pci_walk_bus() goes in the opposite direction,
>>>> but without an actual device hosting the bus, I don't see how it finds
>>>> it.  Thanks,
>>>
>>> It seems like we should be able to come up with something like
>>> pci_walk_vbus() though or something similar.  All we would need to do is
>>> search the VFs on the bus of the PF and all child busses to that bus if
>>> I am not mistaken.
>>
>> I don't think that's going to work because the virtual buses don't
>> appear as the child bus of anything.
>>
> +1.
> 

Maybe I don't understand something but I have a function that I am
already testing that seems to work for what I need.  Is there any reason
I couldn't use the bus->children list to navigate through the bus list
and get all of the children of a given bus?

I'll submit a couple patches for feedback on those bits.

Thanks,

Alex