* [PATCH] PCI: vmd: Fix secondary bus reset for Intel bridges
@ 2022-09-23 20:37 francisco.munoz.ruiz
2022-09-26 21:07 ` Jonathan Derrick
2022-10-24 20:45 ` Bjorn Helgaas
0 siblings, 2 replies; 9+ messages in thread
From: francisco.munoz.ruiz @ 2022-09-23 20:37 UTC (permalink / raw)
To: helgaas, lorenzo.pieralisi
Cc: jonathan.derrick, linux-pci, Francisco Munoz, Nirmal Patel
From: Francisco Munoz <francisco.munoz.ruiz@linux.intel.com>
The reset was never applied in the current implementation because Intel
Bridges owned by VMD are parentless. Internally, the reset API applies
a reset to the parent of the pci device supplied as argument, but in this
case it failed because there wasn't a parent. This change feeds a child
device of an Intel Bridge to the reset API and internally the reset is
applied to its parent.
Signed-off-by: Francisco Munoz <francisco.munoz.ruiz@linux.intel.com>
Reviewed-by: Nirmal Patel <nirmal.patel@linux.intel.com>
---
drivers/pci/controller/vmd.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index e06e9f4fc50f..34d6ba675440 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -859,8 +859,16 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
pci_scan_child_bus(vmd->bus);
vmd_domain_reset(vmd);
- list_for_each_entry(child, &vmd->bus->children, node)
- pci_reset_bus(child->self);
+
+ list_for_each_entry(child, &vmd->bus->children, node) {
+ if (!list_empty(&child->devices)) {
+ pci_reset_bus(list_first_entry(&child->devices,
+ struct pci_dev,
+ bus_list));
+ break;
+ }
+ }
+
pci_assign_unassigned_bus_resources(vmd->bus);
/*
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] PCI: vmd: Fix secondary bus reset for Intel bridges
2022-09-23 20:37 [PATCH] PCI: vmd: Fix secondary bus reset for Intel bridges francisco.munoz.ruiz
@ 2022-09-26 21:07 ` Jonathan Derrick
2022-10-06 18:26 ` Munoz Ruiz, Francisco
2022-10-24 20:45 ` Bjorn Helgaas
1 sibling, 1 reply; 9+ messages in thread
From: Jonathan Derrick @ 2022-09-26 21:07 UTC (permalink / raw)
To: francisco.munoz.ruiz, helgaas, lorenzo.pieralisi; +Cc: linux-pci, Nirmal Patel
On 9/23/2022 2:37 PM, francisco.munoz.ruiz@linux.intel.com wrote:
> From: Francisco Munoz <francisco.munoz.ruiz@linux.intel.com>
>
> The reset was never applied in the current implementation because Intel
> Bridges owned by VMD are parentless. Internally, the reset API applies
> a reset to the parent of the pci device supplied as argument, but in this
> case it failed because there wasn't a parent. This change feeds a child
> device of an Intel Bridge to the reset API and internally the reset is
> applied to its parent.
>
> Signed-off-by: Francisco Munoz <francisco.munoz.ruiz@linux.intel.com>
> Reviewed-by: Nirmal Patel <nirmal.patel@linux.intel.com>
Reviewed-by: Jonathan Derrick <jonathan.derrick@linux.dev>
> ---
> drivers/pci/controller/vmd.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index e06e9f4fc50f..34d6ba675440 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -859,8 +859,16 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
>
> pci_scan_child_bus(vmd->bus);
> vmd_domain_reset(vmd);
> - list_for_each_entry(child, &vmd->bus->children, node)
> - pci_reset_bus(child->self);
> +
> + list_for_each_entry(child, &vmd->bus->children, node) {
> + if (!list_empty(&child->devices)) {
> + pci_reset_bus(list_first_entry(&child->devices,
> + struct pci_dev,
> + bus_list));
> + break;
> + }
> + }
> +
> pci_assign_unassigned_bus_resources(vmd->bus);
>
> /*
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] PCI: vmd: Fix secondary bus reset for Intel bridges
2022-09-26 21:07 ` Jonathan Derrick
@ 2022-10-06 18:26 ` Munoz Ruiz, Francisco
2022-10-06 19:21 ` Bjorn Helgaas
0 siblings, 1 reply; 9+ messages in thread
From: Munoz Ruiz, Francisco @ 2022-10-06 18:26 UTC (permalink / raw)
To: helgaas, lorenzo.pieralisi; +Cc: linux-pci, Nirmal Patel, Jonathan Derrick
Hi,
Please let me know if something else is needed.
Thanks,
Francisco.
On 9/26/2022 2:07 PM, Jonathan Derrick wrote:
>
>
> On 9/23/2022 2:37 PM, francisco.munoz.ruiz@linux.intel.com wrote:
>> From: Francisco Munoz <francisco.munoz.ruiz@linux.intel.com>
>>
>> The reset was never applied in the current implementation because Intel
>> Bridges owned by VMD are parentless. Internally, the reset API applies
>> a reset to the parent of the pci device supplied as argument, but in this
>> case it failed because there wasn't a parent. This change feeds a child
>> device of an Intel Bridge to the reset API and internally the reset is
>> applied to its parent.
>>
>> Signed-off-by: Francisco Munoz <francisco.munoz.ruiz@linux.intel.com>
>> Reviewed-by: Nirmal Patel <nirmal.patel@linux.intel.com>
> Reviewed-by: Jonathan Derrick <jonathan.derrick@linux.dev>
>
>> ---
>> drivers/pci/controller/vmd.c | 12 ++++++++++--
>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
>> index e06e9f4fc50f..34d6ba675440 100644
>> --- a/drivers/pci/controller/vmd.c
>> +++ b/drivers/pci/controller/vmd.c
>> @@ -859,8 +859,16 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
>>
>> pci_scan_child_bus(vmd->bus);
>> vmd_domain_reset(vmd);
>> - list_for_each_entry(child, &vmd->bus->children, node)
>> - pci_reset_bus(child->self);
>> +
>> + list_for_each_entry(child, &vmd->bus->children, node) {
>> + if (!list_empty(&child->devices)) {
>> + pci_reset_bus(list_first_entry(&child->devices,
>> + struct pci_dev,
>> + bus_list));
>> + break;
>> + }
>> + }
>> +
>> pci_assign_unassigned_bus_resources(vmd->bus);
>>
>> /*
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] PCI: vmd: Fix secondary bus reset for Intel bridges
2022-10-06 18:26 ` Munoz Ruiz, Francisco
@ 2022-10-06 19:21 ` Bjorn Helgaas
0 siblings, 0 replies; 9+ messages in thread
From: Bjorn Helgaas @ 2022-10-06 19:21 UTC (permalink / raw)
To: Munoz Ruiz, Francisco
Cc: lorenzo.pieralisi, linux-pci, Nirmal Patel, Jonathan Derrick
On Thu, Oct 06, 2022 at 11:26:08AM -0700, Munoz Ruiz, Francisco wrote:
> Hi,
>
> Please let me know if something else is needed.
We're in the merge window now, so it's too late for v6.1, but we'll
start merging v6.2 changes about Oct 17.
> On 9/26/2022 2:07 PM, Jonathan Derrick wrote:
> > On 9/23/2022 2:37 PM, francisco.munoz.ruiz@linux.intel.com wrote:
> > > From: Francisco Munoz <francisco.munoz.ruiz@linux.intel.com>
> > >
> > > The reset was never applied in the current implementation because Intel
> > > Bridges owned by VMD are parentless. Internally, the reset API applies
> > > a reset to the parent of the pci device supplied as argument, but in this
> > > case it failed because there wasn't a parent. This change feeds a child
> > > device of an Intel Bridge to the reset API and internally the reset is
> > > applied to its parent.
> > >
> > > Signed-off-by: Francisco Munoz <francisco.munoz.ruiz@linux.intel.com>
> > > Reviewed-by: Nirmal Patel <nirmal.patel@linux.intel.com>
> > Reviewed-by: Jonathan Derrick <jonathan.derrick@linux.dev>
> >
> > > ---
> > > drivers/pci/controller/vmd.c | 12 ++++++++++--
> > > 1 file changed, 10 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> > > index e06e9f4fc50f..34d6ba675440 100644
> > > --- a/drivers/pci/controller/vmd.c
> > > +++ b/drivers/pci/controller/vmd.c
> > > @@ -859,8 +859,16 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
> > > pci_scan_child_bus(vmd->bus);
> > > vmd_domain_reset(vmd);
> > > - list_for_each_entry(child, &vmd->bus->children, node)
> > > - pci_reset_bus(child->self);
> > > +
> > > + list_for_each_entry(child, &vmd->bus->children, node) {
> > > + if (!list_empty(&child->devices)) {
> > > + pci_reset_bus(list_first_entry(&child->devices,
> > > + struct pci_dev,
> > > + bus_list));
> > > + break;
> > > + }
> > > + }
> > > +
> > > pci_assign_unassigned_bus_resources(vmd->bus);
> > > /*
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] PCI: vmd: Fix secondary bus reset for Intel bridges
2022-09-23 20:37 [PATCH] PCI: vmd: Fix secondary bus reset for Intel bridges francisco.munoz.ruiz
2022-09-26 21:07 ` Jonathan Derrick
@ 2022-10-24 20:45 ` Bjorn Helgaas
2022-10-31 21:45 ` [PATCH V2] " francisco.munoz.ruiz
1 sibling, 1 reply; 9+ messages in thread
From: Bjorn Helgaas @ 2022-10-24 20:45 UTC (permalink / raw)
To: francisco.munoz.ruiz
Cc: lorenzo.pieralisi, jonathan.derrick, linux-pci, Nirmal Patel
On Fri, Sep 23, 2022 at 01:37:57PM -0700, francisco.munoz.ruiz@linux.intel.com wrote:
> From: Francisco Munoz <francisco.munoz.ruiz@linux.intel.com>
>
> The reset was never applied in the current implementation because Intel
> Bridges owned by VMD are parentless. Internally, the reset API applies
> a reset to the parent of the pci device supplied as argument, but in this
> case it failed because there wasn't a parent. This change feeds a child
> device of an Intel Bridge to the reset API and internally the reset is
> applied to its parent.
What kind of problem does this fix? I guess some devices below a VMD
need to be reset before we can use them?
As a rule, Linux doesn't reset PCI devices at boot, so I'm just
wondering what's different about these.
If this fixes a problem, it's also nice if we can include a symptom in
the commit log so if people are seeing the problem, they can find the
solution, or distros can tell whether they need to include it.
> Signed-off-by: Francisco Munoz <francisco.munoz.ruiz@linux.intel.com>
> Reviewed-by: Nirmal Patel <nirmal.patel@linux.intel.com>
> ---
> drivers/pci/controller/vmd.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index e06e9f4fc50f..34d6ba675440 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -859,8 +859,16 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
>
> pci_scan_child_bus(vmd->bus);
> vmd_domain_reset(vmd);
> - list_for_each_entry(child, &vmd->bus->children, node)
> - pci_reset_bus(child->self);
> +
> + list_for_each_entry(child, &vmd->bus->children, node) {
> + if (!list_empty(&child->devices)) {
> + pci_reset_bus(list_first_entry(&child->devices,
> + struct pci_dev,
> + bus_list));
> + break;
> + }
> + }
> +
> pci_assign_unassigned_bus_resources(vmd->bus);
>
> /*
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH V2] PCI: vmd: Fix secondary bus reset for Intel bridges
2022-10-24 20:45 ` Bjorn Helgaas
@ 2022-10-31 21:45 ` francisco.munoz.ruiz
2022-11-02 23:42 ` Bjorn Helgaas
0 siblings, 1 reply; 9+ messages in thread
From: francisco.munoz.ruiz @ 2022-10-31 21:45 UTC (permalink / raw)
To: helgaas
Cc: lorenzo.pieralisi, jonathan.derrick, linux-pci, Francisco Munoz,
Nirmal Patel
From: Francisco Munoz <francisco.munoz.ruiz@linux.intel.com>
The reset was never applied in the current implementation because Intel
Bridges owned by VMD are parentless. Internally, pci_reset_bus applies
a reset to the parent of the pci device supplied as argument, but in this
case it failed because there wasn't a parent.
In more detail, this change allows the VMD driver to enumerate NVMe devices
in pass-through configurations when host reboots are performed. Commit id
“6aab5622296b990024ee67dd7efa7d143e7558d0” attempted to fix this, but
later we discovered that the code inside pci_reset_bus wasn’t triggering
secondary bus resets. Therefore, we updated the parameters passed to
it, and now NVMe SSDs attached to VMD bridges are properly enumerated in
VT-d pass-through scenarios.
Signed-off-by: Francisco Munoz <francisco.munoz.ruiz@linux.intel.com>
Reviewed-by: Nirmal Patel <nirmal.patel@linux.intel.com>
---
drivers/pci/controller/vmd.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index e06e9f4fc50f..34d6ba675440 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -859,8 +859,16 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
pci_scan_child_bus(vmd->bus);
vmd_domain_reset(vmd);
- list_for_each_entry(child, &vmd->bus->children, node)
- pci_reset_bus(child->self);
+
+ list_for_each_entry(child, &vmd->bus->children, node) {
+ if (!list_empty(&child->devices)) {
+ pci_reset_bus(list_first_entry(&child->devices,
+ struct pci_dev,
+ bus_list));
+ break;
+ }
+ }
+
pci_assign_unassigned_bus_resources(vmd->bus);
/*
--
2.25.1
Hi Bjorn,
I updated the commit message with more details. Hopefully, this will
clarify its purpose.
Thanks,
Francisco.
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH V2] PCI: vmd: Fix secondary bus reset for Intel bridges
2022-10-31 21:45 ` [PATCH V2] " francisco.munoz.ruiz
@ 2022-11-02 23:42 ` Bjorn Helgaas
2022-11-03 3:58 ` Munoz Ruiz, Francisco
2022-11-03 17:15 ` Alex Williamson
0 siblings, 2 replies; 9+ messages in thread
From: Bjorn Helgaas @ 2022-11-02 23:42 UTC (permalink / raw)
To: francisco.munoz.ruiz
Cc: lorenzo.pieralisi, jonathan.derrick, linux-pci, Nirmal Patel,
Alex Williamson, Myron Stowe
[+cc Alex, Myron]
On Mon, Oct 31, 2022 at 02:45:01PM -0700, francisco.munoz.ruiz@linux.intel.com wrote:
> From: Francisco Munoz <francisco.munoz.ruiz@linux.intel.com>
>
> The reset was never applied in the current implementation because Intel
> Bridges owned by VMD are parentless. Internally, pci_reset_bus applies
> a reset to the parent of the pci device supplied as argument, but in this
> case it failed because there wasn't a parent.
>
> In more detail, this change allows the VMD driver to enumerate NVMe devices
> in pass-through configurations when host reboots are performed. Commit id
> “6aab5622296b990024ee67dd7efa7d143e7558d0” attempted to fix this, but
> later we discovered that the code inside pci_reset_bus wasn’t triggering
> secondary bus resets. Therefore, we updated the parameters passed to
> it, and now NVMe SSDs attached to VMD bridges are properly enumerated in
> VT-d pass-through scenarios.
Did you mean "guest reboots" above? If the *host* reboots, I assume
everybody (host and guests) starts over, so a reset wouldn't really
apply.
Is the scenario that the VMD device is passed through to a guest, and
the guest OS is running vmd_probe() and vmd_enable_domain()?
I thought VFIO already had something to reset devices between guests.
But maybe this is different because from the point of view of VFIO,
the pass-through happens only once, and during that single session,
the guest OS reboots several times, so you want vmd_probe() to reset
the downstream devices?
Should this have a Fixes: tag for 6aab5622296b?
s/pci/PCI/ above in English text.
Also add "()" after function names.
Use the typical 12-char SHA1 + subject citation, e.g., 6aab5622296b
("PCI: vmd: Clean up domain before enumeration").
> Signed-off-by: Francisco Munoz <francisco.munoz.ruiz@linux.intel.com>
> Reviewed-by: Nirmal Patel <nirmal.patel@linux.intel.com>
> ---
> drivers/pci/controller/vmd.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index e06e9f4fc50f..34d6ba675440 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -859,8 +859,16 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
>
> pci_scan_child_bus(vmd->bus);
> vmd_domain_reset(vmd);
> - list_for_each_entry(child, &vmd->bus->children, node)
> - pci_reset_bus(child->self);
> +
> + list_for_each_entry(child, &vmd->bus->children, node) {
> + if (!list_empty(&child->devices)) {
> + pci_reset_bus(list_first_entry(&child->devices,
> + struct pci_dev,
> + bus_list));
> + break;
> + }
> + }
> +
> pci_assign_unassigned_bus_resources(vmd->bus);
>
> /*
> --
> 2.25.1
>
> Hi Bjorn,
>
> I updated the commit message with more details. Hopefully, this will
> clarify its purpose.
>
> Thanks,
> Francisco.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V2] PCI: vmd: Fix secondary bus reset for Intel bridges
2022-11-02 23:42 ` Bjorn Helgaas
@ 2022-11-03 3:58 ` Munoz Ruiz, Francisco
2022-11-03 17:15 ` Alex Williamson
1 sibling, 0 replies; 9+ messages in thread
From: Munoz Ruiz, Francisco @ 2022-11-03 3:58 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: lorenzo.pieralisi, jonathan.derrick, linux-pci, Nirmal Patel,
Alex Williamson, Myron Stowe
Thanks for including Alex and Myron.
On 11/2/2022 4:42 PM, Bjorn Helgaas wrote:
> [+cc Alex, Myron]
>
> On Mon, Oct 31, 2022 at 02:45:01PM -0700, francisco.munoz.ruiz@linux.intel.com wrote:
>> From: Francisco Munoz <francisco.munoz.ruiz@linux.intel.com>
>>
>> The reset was never applied in the current implementation because Intel
>> Bridges owned by VMD are parentless. Internally, pci_reset_bus applies
>> a reset to the parent of the pci device supplied as argument, but in this
>> case it failed because there wasn't a parent.
>>
>> In more detail, this change allows the VMD driver to enumerate NVMe devices
>> in pass-through configurations when host reboots are performed. Commit id
>> “6aab5622296b990024ee67dd7efa7d143e7558d0” attempted to fix this, but
>> later we discovered that the code inside pci_reset_bus wasn’t triggering
>> secondary bus resets. Therefore, we updated the parameters passed to
>> it, and now NVMe SSDs attached to VMD bridges are properly enumerated in
>> VT-d pass-through scenarios.
>
> Did you mean "guest reboots" above? If the *host* reboots, I assume
> everybody (host and guests) starts over, so a reset wouldn't really
> apply.
>
Correct, I meant guest reboots.
> Is the scenario that the VMD device is passed through to a guest, and
> the guest OS is running vmd_probe() and vmd_enable_domain()?
>
> I thought VFIO already had something to reset devices between guests.
> But maybe this is different because from the point of view of VFIO,
> the pass-through happens only once, and during that single session,
> the guest OS reboots several times, so you want vmd_probe() to reset
> the downstream devices?
Right, this change just makes sure pci_bus_reset(), introduced in
6aab5622296b ("PCI: vmd: Clean up domain before enumeration"), is sending
pci_bridge_secondary_bus_reset().
I'll issue a v3 with the suggestions below.
>
> Should this have a Fixes: tag for 6aab5622296b?
>
> s/pci/PCI/ above in English text.
>
> Also add "()" after function names.
>
> Use the typical 12-char SHA1 + subject citation, e.g., 6aab5622296b
> ("PCI: vmd: Clean up domain before enumeration").
>
>> Signed-off-by: Francisco Munoz <francisco.munoz.ruiz@linux.intel.com>
>> Reviewed-by: Nirmal Patel <nirmal.patel@linux.intel.com>
>> ---
>> drivers/pci/controller/vmd.c | 12 ++++++++++--
>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
>> index e06e9f4fc50f..34d6ba675440 100644
>> --- a/drivers/pci/controller/vmd.c
>> +++ b/drivers/pci/controller/vmd.c
>> @@ -859,8 +859,16 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
>>
>> pci_scan_child_bus(vmd->bus);
>> vmd_domain_reset(vmd);
>> - list_for_each_entry(child, &vmd->bus->children, node)
>> - pci_reset_bus(child->self);
>> +
>> + list_for_each_entry(child, &vmd->bus->children, node) {
>> + if (!list_empty(&child->devices)) {
>> + pci_reset_bus(list_first_entry(&child->devices,
>> + struct pci_dev,
>> + bus_list));
>> + break;
>> + }
>> + }
>> +
>> pci_assign_unassigned_bus_resources(vmd->bus);
>>
>> /*
>> --
>> 2.25.1
>>
>> Hi Bjorn,
>>
>> I updated the commit message with more details. Hopefully, this will
>> clarify its purpose.
>>
>> Thanks,
>> Francisco.
Thanks!
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V2] PCI: vmd: Fix secondary bus reset for Intel bridges
2022-11-02 23:42 ` Bjorn Helgaas
2022-11-03 3:58 ` Munoz Ruiz, Francisco
@ 2022-11-03 17:15 ` Alex Williamson
1 sibling, 0 replies; 9+ messages in thread
From: Alex Williamson @ 2022-11-03 17:15 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: francisco.munoz.ruiz, lorenzo.pieralisi, jonathan.derrick,
linux-pci, Nirmal Patel, Myron Stowe
On Wed, 2 Nov 2022 18:42:21 -0500
Bjorn Helgaas <helgaas@kernel.org> wrote:
> [+cc Alex, Myron]
>
> On Mon, Oct 31, 2022 at 02:45:01PM -0700, francisco.munoz.ruiz@linux.intel.com wrote:
> > From: Francisco Munoz <francisco.munoz.ruiz@linux.intel.com>
> >
> > The reset was never applied in the current implementation because Intel
> > Bridges owned by VMD are parentless. Internally, pci_reset_bus applies
> > a reset to the parent of the pci device supplied as argument, but in this
> > case it failed because there wasn't a parent.
> >
> > In more detail, this change allows the VMD driver to enumerate NVMe devices
> > in pass-through configurations when host reboots are performed. Commit id
> > “6aab5622296b990024ee67dd7efa7d143e7558d0” attempted to fix this, but
> > later we discovered that the code inside pci_reset_bus wasn’t triggering
> > secondary bus resets. Therefore, we updated the parameters passed to
> > it, and now NVMe SSDs attached to VMD bridges are properly enumerated in
> > VT-d pass-through scenarios.
>
> Did you mean "guest reboots" above? If the *host* reboots, I assume
> everybody (host and guests) starts over, so a reset wouldn't really
> apply.
>
> Is the scenario that the VMD device is passed through to a guest, and
> the guest OS is running vmd_probe() and vmd_enable_domain()?
>
> I thought VFIO already had something to reset devices between guests.
> But maybe this is different because from the point of view of VFIO,
> the pass-through happens only once, and during that single session,
> the guest OS reboots several times, so you want vmd_probe() to reset
> the downstream devices?
>
> Should this have a Fixes: tag for 6aab5622296b?
>
> s/pci/PCI/ above in English text.
>
> Also add "()" after function names.
>
> Use the typical 12-char SHA1 + subject citation, e.g., 6aab5622296b
> ("PCI: vmd: Clean up domain before enumeration").
>
> > Signed-off-by: Francisco Munoz <francisco.munoz.ruiz@linux.intel.com>
> > Reviewed-by: Nirmal Patel <nirmal.patel@linux.intel.com>
> > ---
> > drivers/pci/controller/vmd.c | 12 ++++++++++--
> > 1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> > index e06e9f4fc50f..34d6ba675440 100644
> > --- a/drivers/pci/controller/vmd.c
> > +++ b/drivers/pci/controller/vmd.c
> > @@ -859,8 +859,16 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
> >
> > pci_scan_child_bus(vmd->bus);
> > vmd_domain_reset(vmd);
> > - list_for_each_entry(child, &vmd->bus->children, node)
> > - pci_reset_bus(child->self);
> > +
> > + list_for_each_entry(child, &vmd->bus->children, node) {
> > + if (!list_empty(&child->devices)) {
> > + pci_reset_bus(list_first_entry(&child->devices,
> > + struct pci_dev,
> > + bus_list));
Do you want to test the return value here to avoid another case of not
actually doing what we expect it to do? WARN_ON perhaps? Thanks,
Alex
> > + break;
> > + }
> > + }
> > +
> > pci_assign_unassigned_bus_resources(vmd->bus);
> >
> > /*
> > --
> > 2.25.1
> >
> > Hi Bjorn,
> >
> > I updated the commit message with more details. Hopefully, this will
> > clarify its purpose.
> >
> > Thanks,
> > Francisco.
>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-11-03 17:16 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-23 20:37 [PATCH] PCI: vmd: Fix secondary bus reset for Intel bridges francisco.munoz.ruiz
2022-09-26 21:07 ` Jonathan Derrick
2022-10-06 18:26 ` Munoz Ruiz, Francisco
2022-10-06 19:21 ` Bjorn Helgaas
2022-10-24 20:45 ` Bjorn Helgaas
2022-10-31 21:45 ` [PATCH V2] " francisco.munoz.ruiz
2022-11-02 23:42 ` Bjorn Helgaas
2022-11-03 3:58 ` Munoz Ruiz, Francisco
2022-11-03 17:15 ` Alex Williamson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.