[PATCH v2 1/3] powerpc/eeh: Ignore error handlers in eeh_pe_reset_and_recover()

[PATCH v2 1/3] powerpc/eeh: Ignore error handlers in eeh_pe_reset_and_recover()

[Gavin Shan]

The function eeh_pe_reset_and_recover() is used to recover EEH
error when the passthrough device are transferred to guest and
backwards, meaning the device's driver is vfio-pci or none.
When the driver is vfio-pci that provides error_detected() error
handler only, the handler simply stops the guest and it's not
expected behaviour. On the other hand, no error handlers will
be called if we don't have a bound driver.

This ignores all error handlers provided by device driver in
eeh_pe_reset_and_recover() to avoid the exceptional behaviour.

Fixes: 5cfb20b9 ("powerpc/eeh: Emulate EEH recovery for VFIO devices")
Cc: stable@vger.kernel.org #v3.18+
Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
Reviewed-by: Russell Currey <ruscur@russell.cc>
---
 arch/powerpc/kernel/eeh_driver.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/arch/powerpc/kernel/eeh_driver.c b/arch/powerpc/kernel/eeh_driver.c
index fb6207d..1c7d703 100644
--- a/arch/powerpc/kernel/eeh_driver.c
+++ b/arch/powerpc/kernel/eeh_driver.c
@@ -552,7 +552,7 @@ static int eeh_clear_pe_frozen_state(struct eeh_pe *pe,
 
 int eeh_pe_reset_and_recover(struct eeh_pe *pe)
 {
-	int result, ret;
+	int ret;
 
 	/* Bail if the PE is being recovered */
 	if (pe->state & EEH_PE_RECOVERING)
@@ -564,9 +564,6 @@ int eeh_pe_reset_and_recover(struct eeh_pe *pe)
 	/* Save states */
 	eeh_pe_dev_traverse(pe, eeh_dev_save_state, NULL);
 
-	/* Report error */
-	eeh_pe_dev_traverse(pe, eeh_report_error, &result);
-
 	/* Issue reset */
 	ret = eeh_reset_pe(pe);
 	if (ret) {
@@ -581,15 +578,9 @@ int eeh_pe_reset_and_recover(struct eeh_pe *pe)
 		return ret;
 	}
 
-	/* Notify completion of reset */
-	eeh_pe_dev_traverse(pe, eeh_report_reset, &result);
-
 	/* Restore device state */
 	eeh_pe_dev_traverse(pe, eeh_dev_restore_state, NULL);
 
-	/* Resume */
-	eeh_pe_dev_traverse(pe, eeh_report_resume, NULL);
-
 	/* Clear recovery mode */
 	eeh_pe_state_clear(pe, EEH_PE_RECOVERING);
 
-- 
2.1.0

[PATCH v2 2/3] powerpc/eeh: Restore config from edev in eeh_pe_reset_and_recover()

[Gavin Shan]

The function eeh_pe_reset_and_recover() is used to recover EEH
error when the passthrou device are transferred to guest and
backwards. The content in the device's config space will be lost
on PE reset issued in the middle of the recovery. The function
saves/restores it before/after the reset. However, config access
to some adapters like Broadcom BCM5719 at this point will causes
fenced PHB. The config space is always blocked and we save 0xFF's
that are restored at late point. The memory BARs are totally
corrupted, causing another EEH error upon access to one of the
memory BARs.

This restores the config space from the content saved to the
EEH device when it's populated, to resolve above issue.

Fixes: 5cfb20b9 ("powerpc/eeh: Emulate EEH recovery for VFIO devices")
Cc: stable@vger.kernel.org #v3.18+
Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
Reviewed-by: Russell Currey <ruscur@russell.cc>
---
 arch/powerpc/kernel/eeh_driver.c | 39 ++-------------------------------------
 1 file changed, 2 insertions(+), 37 deletions(-)

diff --git a/arch/powerpc/kernel/eeh_driver.c b/arch/powerpc/kernel/eeh_driver.c
index 1c7d703..ec6e889 100644
--- a/arch/powerpc/kernel/eeh_driver.c
+++ b/arch/powerpc/kernel/eeh_driver.c
@@ -163,22 +163,6 @@ static bool eeh_dev_removed(struct eeh_dev *edev)
 	return false;
 }
 
-static void *eeh_dev_save_state(void *data, void *userdata)
-{
-	struct eeh_dev *edev = data;
-	struct pci_dev *pdev;
-
-	if (!edev)
-		return NULL;
-
-	pdev = eeh_dev_to_pci_dev(edev);
-	if (!pdev)
-		return NULL;
-
-	pci_save_state(pdev);
-	return NULL;
-}
-
 /**
  * eeh_report_error - Report pci error to each device driver
  * @data: eeh device
@@ -304,22 +288,6 @@ static void *eeh_report_reset(void *data, void *userdata)
 	return NULL;
 }
 
-static void *eeh_dev_restore_state(void *data, void *userdata)
-{
-	struct eeh_dev *edev = data;
-	struct pci_dev *pdev;
-
-	if (!edev)
-		return NULL;
-
-	pdev = eeh_dev_to_pci_dev(edev);
-	if (!pdev)
-		return NULL;
-
-	pci_restore_state(pdev);
-	return NULL;
-}
-
 /**
  * eeh_report_resume - Tell device to resume normal operations
  * @data: eeh device
@@ -561,9 +529,6 @@ int eeh_pe_reset_and_recover(struct eeh_pe *pe)
 	/* Put the PE into recovery mode */
 	eeh_pe_state_mark(pe, EEH_PE_RECOVERING);
 
-	/* Save states */
-	eeh_pe_dev_traverse(pe, eeh_dev_save_state, NULL);
-
 	/* Issue reset */
 	ret = eeh_reset_pe(pe);
 	if (ret) {
@@ -578,8 +543,8 @@ int eeh_pe_reset_and_recover(struct eeh_pe *pe)
 		return ret;
 	}
 
-	/* Restore device state */
-	eeh_pe_dev_traverse(pe, eeh_dev_restore_state, NULL);
+	/* Restore device's config space */
+	eeh_pe_restore_bars(pe);
 
 	/* Clear recovery mode */
 	eeh_pe_state_clear(pe, EEH_PE_RECOVERING);
-- 
2.1.0

Re: [PATCH v2 2/3] powerpc/eeh: Restore config from edev in eeh_pe_reset_and_recover()

[Gavin Shan]

On Fri, Apr 22, 2016 at 11:28:03PM +1000, Gavin Shan wrote:
>The function eeh_pe_reset_and_recover() is used to recover EEH
>error when the passthrou device are transferred to guest and
>backwards. The content in the device's config space will be lost
>on PE reset issued in the middle of the recovery. The function
>saves/restores it before/after the reset. However, config access
>to some adapters like Broadcom BCM5719 at this point will causes
>fenced PHB. The config space is always blocked and we save 0xFF's
>that are restored at late point. The memory BARs are totally
>corrupted, causing another EEH error upon access to one of the
>memory BARs.
>
>This restores the config space from the content saved to the
>EEH device when it's populated, to resolve above issue.
>
>Fixes: 5cfb20b9 ("powerpc/eeh: Emulate EEH recovery for VFIO devices")
>Cc: stable@vger.kernel.org #v3.18+
>Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>Reviewed-by: Russell Currey <ruscur@russell.cc>
>---
> arch/powerpc/kernel/eeh_driver.c | 39 ++-------------------------------------
> 1 file changed, 2 insertions(+), 37 deletions(-)
>
>diff --git a/arch/powerpc/kernel/eeh_driver.c b/arch/powerpc/kernel/eeh_driver.c
>index 1c7d703..ec6e889 100644
>--- a/arch/powerpc/kernel/eeh_driver.c
>+++ b/arch/powerpc/kernel/eeh_driver.c
>@@ -163,22 +163,6 @@ static bool eeh_dev_removed(struct eeh_dev *edev)
> 	return false;
> }
>
>-static void *eeh_dev_save_state(void *data, void *userdata)
>-{
>-	struct eeh_dev *edev = data;
>-	struct pci_dev *pdev;
>-
>-	if (!edev)
>-		return NULL;
>-
>-	pdev = eeh_dev_to_pci_dev(edev);
>-	if (!pdev)
>-		return NULL;
>-
>-	pci_save_state(pdev);
>-	return NULL;
>-}
>-
> /**
>  * eeh_report_error - Report pci error to each device driver
>  * @data: eeh device
>@@ -304,22 +288,6 @@ static void *eeh_report_reset(void *data, void *userdata)
> 	return NULL;
> }
>
>-static void *eeh_dev_restore_state(void *data, void *userdata)
>-{
>-	struct eeh_dev *edev = data;
>-	struct pci_dev *pdev;
>-
>-	if (!edev)
>-		return NULL;
>-
>-	pdev = eeh_dev_to_pci_dev(edev);
>-	if (!pdev)
>-		return NULL;
>-
>-	pci_restore_state(pdev);
>-	return NULL;
>-}
>-
> /**
>  * eeh_report_resume - Tell device to resume normal operations
>  * @data: eeh device
>@@ -561,9 +529,6 @@ int eeh_pe_reset_and_recover(struct eeh_pe *pe)
> 	/* Put the PE into recovery mode */
> 	eeh_pe_state_mark(pe, EEH_PE_RECOVERING);
>
>-	/* Save states */
>-	eeh_pe_dev_traverse(pe, eeh_dev_save_state, NULL);
>-
> 	/* Issue reset */
> 	ret = eeh_reset_pe(pe);
> 	if (ret) {
>@@ -578,8 +543,8 @@ int eeh_pe_reset_and_recover(struct eeh_pe *pe)
> 		return ret;
> 	}
>
>-	/* Restore device state */
>-	eeh_pe_dev_traverse(pe, eeh_dev_restore_state, NULL);
>+	/* Restore device's config space */
>+	eeh_pe_restore_bars(pe);

As talked with David Gibson offline, it's not good enough to pick
the initial config space if the PE doesn't have blocked config property.
For that case, we still need rely on pci_{save,restore}_state() to
pick config space in last site. Thanks to David for checking the code
carefully. I will respin to fix it in next revision.

>
> 	/* Clear recovery mode */
> 	eeh_pe_state_clear(pe, EEH_PE_RECOVERING);
>-- 
>2.1.0
>

[PATCH v2 3/3] powerpc/eeh: Drop unnecessary label in eeh_pe_change_owner()

[Gavin Shan]

The label "reset" in eeh_pe_change_owner() is used only for once.
No need to keep it and just drop it. No logical changes introduced.

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Russell Currey <ruscur@russell.cc>
---
 arch/powerpc/kernel/eeh.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index 6544017..4b40d56 100644
--- a/arch/powerpc/kernel/eeh.c
+++ b/arch/powerpc/kernel/eeh.c
@@ -1336,14 +1336,11 @@ static int eeh_pe_change_owner(struct eeh_pe *pe)
 			    id->subdevice != pdev->subsystem_device)
 				continue;
 
-			goto reset;
+			return eeh_pe_reset_and_recover(pe);
 		}
 	}
 
 	return eeh_unfreeze_pe(pe, true);
-
-reset:
-	return eeh_pe_reset_and_recover(pe);
 }
 
 /**
-- 
2.1.0

Re: [PATCH v2 1/3] powerpc/eeh: Ignore error handlers in eeh_pe_reset_and_recover()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3060 bytes --]

On Fri, Apr 22, 2016 at 11:28:02PM +1000, Gavin Shan wrote:
> The function eeh_pe_reset_and_recover() is used to recover EEH
> error when the passthrough device are transferred to guest and
> backwards, meaning the device's driver is vfio-pci or none.
> When the driver is vfio-pci that provides error_detected() error
> handler only, the handler simply stops the guest and it's not
> expected behaviour. On the other hand, no error handlers will
> be called if we don't have a bound driver.
> 
> This ignores all error handlers provided by device driver in
> eeh_pe_reset_and_recover() to avoid the exceptional behaviour.
> 
> Fixes: 5cfb20b9 ("powerpc/eeh: Emulate EEH recovery for VFIO devices")
> Cc: stable@vger.kernel.org #v3.18+
> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> Reviewed-by: Russell Currey <ruscur@russell.cc>
> ---
>  arch/powerpc/kernel/eeh_driver.c | 11 +----------
>  1 file changed, 1 insertion(+), 10 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/eeh_driver.c b/arch/powerpc/kernel/eeh_driver.c
> index fb6207d..1c7d703 100644
> --- a/arch/powerpc/kernel/eeh_driver.c
> +++ b/arch/powerpc/kernel/eeh_driver.c
> @@ -552,7 +552,7 @@ static int eeh_clear_pe_frozen_state(struct eeh_pe *pe,
>  
>  int eeh_pe_reset_and_recover(struct eeh_pe *pe)
>  {
> -	int result, ret;
> +	int ret;
>  
>  	/* Bail if the PE is being recovered */
>  	if (pe->state & EEH_PE_RECOVERING)
> @@ -564,9 +564,6 @@ int eeh_pe_reset_and_recover(struct eeh_pe *pe)
>  	/* Save states */
>  	eeh_pe_dev_traverse(pe, eeh_dev_save_state, NULL);
>  
> -	/* Report error */
> -	eeh_pe_dev_traverse(pe, eeh_report_error, &result);

Ok, so after chatting to Gavin, I've made sense of this.  The basic
thing here is that eeh_pe_reset_and_recover() should be discarding any
errors from before the reset, not reporting them - the whole point is
that we know things have gone bad, and we want to clear back to a good
state.

>  	/* Issue reset */
>  	ret = eeh_reset_pe(pe);
>  	if (ret) {
> @@ -581,15 +578,9 @@ int eeh_pe_reset_and_recover(struct eeh_pe *pe)
>  		return ret;
>  	}
>  
> -	/* Notify completion of reset */
> -	eeh_pe_dev_traverse(pe, eeh_report_reset, &result);

However, it's not clear if removing the report of a reset makes sense.
There are no current users of reset notification IIUC, but if we're
going to remove the reset reporting, we should put that in a separate
patch with its own justification, and remove the other caller as well.

>  	/* Restore device state */
>  	eeh_pe_dev_traverse(pe, eeh_dev_restore_state, NULL);
>  
> -	/* Resume */
> -	eeh_pe_dev_traverse(pe, eeh_report_resume, NULL);

And I'm not sure if it makes sense to remove the resume notification either.

>  	/* Clear recovery mode */
>  	eeh_pe_state_clear(pe, EEH_PE_RECOVERING);
>  

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 1/3] powerpc/eeh: Ignore error handlers in eeh_pe_reset_and_recover()

[Gavin Shan]

On Tue, Apr 26, 2016 at 03:29:59PM +1000, David Gibson wrote:
>On Fri, Apr 22, 2016 at 11:28:02PM +1000, Gavin Shan wrote:
>> The function eeh_pe_reset_and_recover() is used to recover EEH
>> error when the passthrough device are transferred to guest and
>> backwards, meaning the device's driver is vfio-pci or none.
>> When the driver is vfio-pci that provides error_detected() error
>> handler only, the handler simply stops the guest and it's not
>> expected behaviour. On the other hand, no error handlers will
>> be called if we don't have a bound driver.
>> 
>> This ignores all error handlers provided by device driver in
>> eeh_pe_reset_and_recover() to avoid the exceptional behaviour.
>> 
>> Fixes: 5cfb20b9 ("powerpc/eeh: Emulate EEH recovery for VFIO devices")
>> Cc: stable@vger.kernel.org #v3.18+
>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>> Reviewed-by: Russell Currey <ruscur@russell.cc>
>> ---
>>  arch/powerpc/kernel/eeh_driver.c | 11 +----------
>>  1 file changed, 1 insertion(+), 10 deletions(-)
>> 
>> diff --git a/arch/powerpc/kernel/eeh_driver.c b/arch/powerpc/kernel/eeh_driver.c
>> index fb6207d..1c7d703 100644
>> --- a/arch/powerpc/kernel/eeh_driver.c
>> +++ b/arch/powerpc/kernel/eeh_driver.c
>> @@ -552,7 +552,7 @@ static int eeh_clear_pe_frozen_state(struct eeh_pe *pe,
>>  
>>  int eeh_pe_reset_and_recover(struct eeh_pe *pe)
>>  {
>> -	int result, ret;
>> +	int ret;
>>  
>>  	/* Bail if the PE is being recovered */
>>  	if (pe->state & EEH_PE_RECOVERING)
>> @@ -564,9 +564,6 @@ int eeh_pe_reset_and_recover(struct eeh_pe *pe)
>>  	/* Save states */
>>  	eeh_pe_dev_traverse(pe, eeh_dev_save_state, NULL);
>>  
>> -	/* Report error */
>> -	eeh_pe_dev_traverse(pe, eeh_report_error, &result);
>
>Ok, so after chatting to Gavin, I've made sense of this.  The basic
>thing here is that eeh_pe_reset_and_recover() should be discarding any
>errors from before the reset, not reporting them - the whole point is
>that we know things have gone bad, and we want to clear back to a good
>state.
>
>>  	/* Issue reset */
>>  	ret = eeh_reset_pe(pe);
>>  	if (ret) {
>> @@ -581,15 +578,9 @@ int eeh_pe_reset_and_recover(struct eeh_pe *pe)
>>  		return ret;
>>  	}
>>  
>> -	/* Notify completion of reset */
>> -	eeh_pe_dev_traverse(pe, eeh_report_reset, &result);
>
>However, it's not clear if removing the report of a reset makes sense.
>There are no current users of reset notification IIUC, but if we're
>going to remove the reset reporting, we should put that in a separate
>patch with its own justification, and remove the other caller as well.
>

Thanks, David. It makes sense to me. I will split it into two: one removes
eeh_report_error notification and another removes the left notification
handlers.

>>  	/* Restore device state */
>>  	eeh_pe_dev_traverse(pe, eeh_dev_restore_state, NULL);
>>  
>> -	/* Resume */
>> -	eeh_pe_dev_traverse(pe, eeh_report_resume, NULL);
>
>And I'm not sure if it makes sense to remove the resume notification either.
>

Based on the offline talk, we either keep all notification handlers or remove
all of them. As we can't keep eeh_report_error, we have to remove all of them.

>>  	/* Clear recovery mode */
>>  	eeh_pe_state_clear(pe, EEH_PE_RECOVERING);
>>  
>
>-- 
>David Gibson			| I'll have my music baroque, and my code
>david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
>				| _way_ _around_!
>http://www.ozlabs.org/~dgibson

Re: [PATCH v2 1/3] powerpc/eeh: Ignore error handlers in eeh_pe_reset_and_recover()

[Gavin Shan]

On Tue, Apr 26, 2016 at 08:17:31PM +1000, Gavin Shan wrote:
>On Tue, Apr 26, 2016 at 03:29:59PM +1000, David Gibson wrote:
>>On Fri, Apr 22, 2016 at 11:28:02PM +1000, Gavin Shan wrote:
>>> The function eeh_pe_reset_and_recover() is used to recover EEH
>>> error when the passthrough device are transferred to guest and
>>> backwards, meaning the device's driver is vfio-pci or none.
>>> When the driver is vfio-pci that provides error_detected() error
>>> handler only, the handler simply stops the guest and it's not
>>> expected behaviour. On the other hand, no error handlers will
>>> be called if we don't have a bound driver.
>>> 
>>> This ignores all error handlers provided by device driver in
>>> eeh_pe_reset_and_recover() to avoid the exceptional behaviour.
>>> 
>>> Fixes: 5cfb20b9 ("powerpc/eeh: Emulate EEH recovery for VFIO devices")
>>> Cc: stable@vger.kernel.org #v3.18+
>>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>>> Reviewed-by: Russell Currey <ruscur@russell.cc>
>>> ---
>>>  arch/powerpc/kernel/eeh_driver.c | 11 +----------
>>>  1 file changed, 1 insertion(+), 10 deletions(-)
>>> 
>>> diff --git a/arch/powerpc/kernel/eeh_driver.c b/arch/powerpc/kernel/eeh_driver.c
>>> index fb6207d..1c7d703 100644
>>> --- a/arch/powerpc/kernel/eeh_driver.c
>>> +++ b/arch/powerpc/kernel/eeh_driver.c
>>> @@ -552,7 +552,7 @@ static int eeh_clear_pe_frozen_state(struct eeh_pe *pe,
>>>  
>>>  int eeh_pe_reset_and_recover(struct eeh_pe *pe)
>>>  {
>>> -	int result, ret;
>>> +	int ret;
>>>  
>>>  	/* Bail if the PE is being recovered */
>>>  	if (pe->state & EEH_PE_RECOVERING)
>>> @@ -564,9 +564,6 @@ int eeh_pe_reset_and_recover(struct eeh_pe *pe)
>>>  	/* Save states */
>>>  	eeh_pe_dev_traverse(pe, eeh_dev_save_state, NULL);
>>>  
>>> -	/* Report error */
>>> -	eeh_pe_dev_traverse(pe, eeh_report_error, &result);
>>
>>Ok, so after chatting to Gavin, I've made sense of this.  The basic
>>thing here is that eeh_pe_reset_and_recover() should be discarding any
>>errors from before the reset, not reporting them - the whole point is
>>that we know things have gone bad, and we want to clear back to a good
>>state.
>>
>>>  	/* Issue reset */
>>>  	ret = eeh_reset_pe(pe);
>>>  	if (ret) {
>>> @@ -581,15 +578,9 @@ int eeh_pe_reset_and_recover(struct eeh_pe *pe)
>>>  		return ret;
>>>  	}
>>>  
>>> -	/* Notify completion of reset */
>>> -	eeh_pe_dev_traverse(pe, eeh_report_reset, &result);
>>
>>However, it's not clear if removing the report of a reset makes sense.
>>There are no current users of reset notification IIUC, but if we're
>>going to remove the reset reporting, we should put that in a separate
>>patch with its own justification, and remove the other caller as well.
>>
>
>Thanks, David. It makes sense to me. I will split it into two: one removes
>eeh_report_error notification and another removes the left notification
>handlers.
>
>>>  	/* Restore device state */
>>>  	eeh_pe_dev_traverse(pe, eeh_dev_restore_state, NULL);
>>>  
>>> -	/* Resume */
>>> -	eeh_pe_dev_traverse(pe, eeh_report_resume, NULL);
>>
>>And I'm not sure if it makes sense to remove the resume notification either.
>>
>
>Based on the offline talk, we either keep all notification handlers or remove
>all of them. As we can't keep eeh_report_error, we have to remove all of them.
>

v3 was posted for further review. Please ignore this series.

>>>  	/* Clear recovery mode */
>>>  	eeh_pe_state_clear(pe, EEH_PE_RECOVERING);
>>>  
>>
>>-- 
>>David Gibson			| I'll have my music baroque, and my code
>>david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
>>				| _way_ _around_!
>>http://www.ozlabs.org/~dgibson
>
>