[1/7] phb4/capp: Update and re-factor phb4_set_capi_mode()

Presently phb4_set_capi_mode() performs certain CAPP checks
like, checking of CAPP ucode loaded or checks if CAPP is still in
recovery, even when the requested mode is to switch to PCI mode.

Hence this patch updates and re-factors phb4_set_capi_mode() to make
sure CAPP related checks are only performed when request to enable
CAPP is made by mode==OPAL_PHB_CAPI_MODE_CAPI/DMA_TVT1. We also update
other possible modes requests to return a more appropriate status code
based on if CAPP is activated or not.

Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
---
 hw/phb4.c | 93 +++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 59 insertions(+), 34 deletions(-)

On 16/9/18 12:39 am, Vaibhav Jain wrote:
> Presently phb4_set_capi_mode() performs certain CAPP checks
> like, checking of CAPP ucode loaded or checks if CAPP is still in
> recovery, even when the requested mode is to switch to PCI mode.
> 
> Hence this patch updates and re-factors phb4_set_capi_mode() to make
> sure CAPP related checks are only performed when request to enable
> CAPP is made by mode==OPAL_PHB_CAPI_MODE_CAPI/DMA_TVT1. We also update
> other possible modes requests to return a more appropriate status code
> based on if CAPP is activated or not.
> 
> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> ---
>   hw/phb4.c | 93 +++++++++++++++++++++++++++++++++++--------------------
>   1 file changed, 59 insertions(+), 34 deletions(-)
> 
> diff --git a/hw/phb4.c b/hw/phb4.c
> index 94e741e0..4284c467 100644
> --- a/hw/phb4.c
> +++ b/hw/phb4.c
> @@ -4322,54 +4322,79 @@ static int64_t phb4_set_capi_mode(struct phb *phb, uint64_t mode,
>   {
>   	struct phb4 *p = phb_to_phb4(phb);
>   	struct proc_chip *chip = get_chip(p->chip_id);
> +	uint32_t offset = PHB4_CAPP_REG_OFFSET(p);
>   	uint64_t reg, ret;
> -	uint32_t offset;
> -
> -
> -	if (!capp_ucode_loaded(chip, p->index)) {
> -		PHBERR(p, "CAPP: ucode not loaded\n");
> -		return OPAL_RESOURCE;
> -	}
> +	bool attached;
>   
> +	/* No one else fiddle with capp while we modify its state */
>   	lock(&capi_lock);
> -	chip->capp_phb4_attached_mask |= 1 << p->index;
> -	unlock(&capi_lock);

Is there any way to release the lock earlier, like we currently do here? 
As is, we could be holding capi_lock for quite a while (some of the 
timeouts in enable_capi_mode() are up to 5 seconds) and we only need to 
hold capi_lock while we're touching the attached_mask, AIUI.

>   
> -	offset = PHB4_CAPP_REG_OFFSET(p);
> -	xscom_read(p->chip_id, CAPP_ERR_STATUS_CTRL + offset, &reg);
> -	if ((reg & PPC_BIT(5))) {
> -		PHBERR(p, "CAPP: recovery failed (%016llx)\n", reg);
> -		return OPAL_HARDWARE;
> -	} else if ((reg & PPC_BIT(0)) && (!(reg & PPC_BIT(1)))) {
> -		PHBDBG(p, "CAPP: recovery in progress\n");
> -		return OPAL_BUSY;
> -	}
> +	/* Check if CAPP are already attached to PHB */
> +	attached = chip->capp_phb4_attached_mask & (1 << p->index);
>   
>   	switch (mode) {
> -	case OPAL_PHB_CAPI_MODE_CAPI:
> -		ret = enable_capi_mode(p, pe_number,
> -					CAPP_MAX_STQ_ENGINES |
> -					CAPP_MIN_DMA_READ_ENGINES);
> -		disable_fast_reboot("CAPP being enabled");
> +
> +	case OPAL_PHB_CAPI_MODE_DMA: /* Enabled by default on p9 */
> +	case OPAL_PHB_CAPI_MODE_SNOOP_ON:
> +		/* nothing to do on P9 if CAPP is alreay enabled */
> +		ret = attached ? OPAL_SUCCESS : OPAL_UNSUPPORTED;
>   		break;
> +
> +	case OPAL_PHB_CAPI_MODE_SNOOP_OFF:
> +	case OPAL_PHB_CAPI_MODE_PCIE: /* Not supported at the moment */
> +		ret = !attached ? OPAL_SUCCESS : OPAL_UNSUPPORTED;
> +		break;
> +
> +	case OPAL_PHB_CAPI_MODE_CAPI: /* Fall Through */
>   	case OPAL_PHB_CAPI_MODE_DMA_TVT1:
> +		/* Check if ucode is available */
> +		if (!capp_ucode_loaded(chip, p->index)) {
> +			PHBERR(p, "CAPP: ucode not loaded\n");
> +			ret = OPAL_RESOURCE;
> +			break;
> +		}
> +
> +		xscom_read(p->chip_id, CAPP_ERR_STATUS_CTRL + offset, &reg);
> +		if ((reg & PPC_BIT(5))) {
> +			PHBERR(p, "CAPP: recovery failed (%016llx)\n", reg);
> +			ret = OPAL_HARDWARE;
> +			break;
> +		} else if ((reg & PPC_BIT(0)) && (!(reg & PPC_BIT(1)))) {
> +			PHBDBG(p, "CAPP: recovery in progress\n");
> +			ret = OPAL_BUSY;
> +			break;
> +		}
> +
> +		/*
> +		 * Mark the CAPP attached to the PHB right away so that
> +		 * if a MCE happens during CAPP init we can handle it.
> +		 * In case of an error in CAPP init we remove the PHB
> +		 * from the attached_mask later.
> +		 */
> +		chip->capp_phb4_attached_mask |= 1 << p->index;
>   		ret = enable_capi_mode(p, pe_number,
> -					CAPP_MIN_STQ_ENGINES |
> -					CAPP_MAX_DMA_READ_ENGINES);
> -		disable_fast_reboot("CAPP being enabled");
> -		break;
> -	case OPAL_PHB_CAPI_MODE_SNOOP_ON:
> -		/* nothing to do P9 if CAPP is alreay enabled */
> -		ret = OPAL_SUCCESS;
> +				       (mode == OPAL_PHB_CAPI_MODE_DMA_TVT1 ?
> +					/* Max perf for DMA */
> +					(CAPP_MIN_STQ_ENGINES |
> +					 CAPP_MAX_DMA_READ_ENGINES) :
> +					/* Max perf for CAPP STQ */
> +					(CAPP_MAX_STQ_ENGINES |
> +					 CAPP_MIN_DMA_READ_ENGINES)));

It would be easier to understand if you split this out into a separate 
variable rather than use the ternary operator inside the function call.

> +
> +		if (ret == OPAL_SUCCESS)
> +			/* Disable fast reboot for CAPP */
> +			disable_fast_reboot("CAPP being enabled");
> +		else
> +			/* In case of an error mark the PHB detached */
> +			chip->capp_phb4_attached_mask ^= 1 << p->index;
>   		break;
>   
> -	case OPAL_PHB_CAPI_MODE_PCIE: /* shouldn't be called on p9*/
> -	case OPAL_PHB_CAPI_MODE_DMA: /* Enabled by default on p9 */
> -	case OPAL_PHB_CAPI_MODE_SNOOP_OFF: /* shouldn't be called on p9*/
>   	default:
>   		ret = OPAL_UNSUPPORTED;
> -	}
> +		break;
> +	};
>   
> +	unlock(&capi_lock);
>   	return ret;
>   }
>   
>

Thanks for reviewing this patch Andrew.

Andrew Donnellan <andrew.donnellan@au1.ibm.com> writes:

>> +	/* No one else fiddle with capp while we modify its state */
>>   	lock(&capi_lock);
>> -	chip->capp_phb4_attached_mask |= 1 << p->index;
>> -	unlock(&capi_lock);
>
> Is there any way to release the lock earlier, like we currently do here? 
> As is, we could be holding capi_lock for quite a while (some of the 
> timeouts in enable_capi_mode() are up to 5 seconds) and we only need to 
> hold capi_lock while we're touching the attached_mask, AIUI.

Good point though we will need to hold the lock to prevent someone from
calling phb4_set_capi_mode() with a conflicting value and stomping on
us. I can probably replace lock with try_lock and return OPAL_BUSY in
that case leaving the caller to take recovery steps.


>> +				       (mode == OPAL_PHB_CAPI_MODE_DMA_TVT1 ?
>> +					/* Max perf for DMA */
>> +					(CAPP_MIN_STQ_ENGINES |
>> +					 CAPP_MAX_DMA_READ_ENGINES) :
>> +					/* Max perf for CAPP STQ */
>> +					(CAPP_MAX_STQ_ENGINES |
>> +					 CAPP_MIN_DMA_READ_ENGINES)));
>
Fair point will fix this in next iteration of this patchset.

Vaibhav Jain <vaibhav@linux.ibm.com> writes:
> Thanks for reviewing this patch Andrew.
>
> Andrew Donnellan <andrew.donnellan@au1.ibm.com> writes:
>
>>> +	/* No one else fiddle with capp while we modify its state */
>>>   	lock(&capi_lock);
>>> -	chip->capp_phb4_attached_mask |= 1 << p->index;
>>> -	unlock(&capi_lock);
>>
>> Is there any way to release the lock earlier, like we currently do here? 
>> As is, we could be holding capi_lock for quite a while (some of the 
>> timeouts in enable_capi_mode() are up to 5 seconds) and we only need to 
>> hold capi_lock while we're touching the attached_mask, AIUI.
>
> Good point though we will need to hold the lock to prevent someone from
> calling phb4_set_capi_mode() with a conflicting value and stomping on
> us. I can probably replace lock with try_lock and return OPAL_BUSY in
> that case leaving the caller to take recovery steps.

Instead of using a mutex for this, what about using a flag variable?

If it really can take seconds, then it should be an async OPAL call
anyway so as not to hold a CPU spinning in OPAL doing nothing.

Thanks for reviewing this patch Stewart

Stewart Smith <stewart@linux.ibm.com> writes:

> Instead of using a mutex for this, what about using a flag variable?
Yes good idea. I am working on v2 that will uses a flag in a newly
introduced 'struct capp *' stored in the struct phb. That should get rid
of the capi mutex lock as the call to set_capi_mode is already done in
context of phb_lock.

> If it really can take seconds, then it should be an async OPAL call
> anyway so as not to hold a CPU spinning in OPAL doing nothing.
At present capp init doesnt take that much time and is usually
completed within ~0.2ms. However I am working on a feature that would
require CAPP init to go through a phb creset which will be considerably
longer. But that will be driven by pci reset state machine and wouldnt
hold the execution thread in opal for long.