[Skiboot] [PATCH v3 06/13] hiomap: Enable Async IPMI messaging

Sat Dec 14 00:01:56 AEDT 2019

Comments below:

On 11/25/19 3:10 AM, Oliver O'Halloran wrote:
> On Wed, 2019-11-06 at 11:22 -0600, Deb McLemore wrote:
>> To provide higher layer async operations to access
>> the target flash, enable hiomap to perform async
>> ipmi messaging for call paths thru opal_flash_op.
>>
>> Special considerations and constraints are to prevent
>> recursive locking and/or polling inside OPAL calls.
>>
>> Objective is to remove the hiomap_queue_msg_sync for
>> moving windows (which already uses pollers) to allow
>> async requests to perform their functions.
>>
>> Call paths thru opal_flash_op will determine if the
>> requested operation needs to be re-queued, to allow
>> skiboot to jump back out to Linux to prevent RCU or
>> scheduler issues.
>>
>> For example, if a flash window move operation
>> is needed to erase a very large flash segment,
>> the time spent in opal firmware would be long
>> enough that Linux would stall.  To avoid the
>> long running duration of various aspects of this
>> inherent long running operation we divide up
>> the tasks in smaller chunks to avoid cumulating
>> time spent in opal firmware.  The return codes
>> are used to distinguish differences between cases
>> where a re-queue of the transaction would be
>> needed versus a true failure return code.
>
>> PR_TRACE used since PR_DEBUG seems to always trigger,
>> unable to figure out why.
> https://lists.ozlabs.org/pipermail/skiboot/2019-October/015645.html
>
>> Due to the nature of redefining the sync versus
>> async capabilities, this patch is larger than
>> desired due to interrelationships of changes
>> to functionality.

During the evolution of designing, code and testing your

suggestions were purused and then iterations evolved.

Design points which took highest priority in factoring the

code was consolidating the runtime checks (like are we booting)

in a single location, moving the other checks to the various

other functions.  This design decision also made it

clear to the future maintainers that consideration needs

to be made for sync versus async.

One main design point which is not intuitively

obvious (I'll add some comments) is that its necessary

to know the ASYNC_REQUIRED across the layers and the only

way to communicate this is via the blocklevel_flags (unless we

wanted to change the interfaces which is not desired).

When the boot is in progress and the transition made where

the kernel immediately comes in with an async flash request,

we must differentiate the call so that the appropriate move

window symantics occur.  Various other alternative methods

were pursued and the chosen design is the cleanest.

When the functionality is implemented its necessary to have

the appropriate debug/trace instrumentation in place, otherwise

the design decision lacks some nuance if in the future someone

was to bisect this patch and find that its not functional, therefore

it was deemed best to leave as a whole.

Will update comments to better describe in appropriate places.

> Yeah, nah.
>
> The usual pattern for sync->async conversions is to re-implement the
> sync parts using the async implementation internally and by having the
> "sync" functions wait on a flag variable internally. Doing that then
> allows you to expand the async bits gradually into the callers. See
> i2c_request_sync() for an example.
>
> Even after this patch there's a lot of HIOMAP commands that are still
> required to be sync, so what I'd do is:
>
> 1. Keep hiomap_queue_msg_sync() around and continue using it for
> commands that need to be sync rather than faking it. That'll remove a
> pile of churn to begin with.
>
> 2. Split the transction lock() changes into a seperate patch,
> preferable with a commit mesage explaining why its needed.
>
> 3. Re-implement the window movement using the new async mathods. Keep
> the external interface the same.
>
> 4. Now convert flash.c to use the new async interface. This might
> require moving around where changes occur so that cyril's async-lite
> patch comes after the changes above.
>
> 5. Move some of the debug prints out of this patch and into a follow
> up. Pruning them while reviewing them took out ~200 lines so it'll
> help.
>
> Doing that should let you get this down to a few hundred lines at most.
> That's still too large, but whatever, at least it's manageable.
>
> I'm not asking you to do this for for the sake of being a pain. I spent
> all day trying to unpack what this patch actually does and that only
> took all day because it's over a thousand lines of random stuff
> happening for random reasons. We don't ask for large changes to be
> split into a smaller ones to because we demand the platonic ideal of a
> patch. We do it so that it's possible to review them without sending
> the reviewer insane.
>
>> Signed-off-by: Deb McLemore <debmc at linux.ibm.com>
>> ---
>>  core/flash.c           | 154 +++++++-
>>  libflash/blocklevel.h  |   5 +
>>  libflash/errors.h      |   1 +
>>  libflash/ipmi-hiomap.c | 955 ++++++++++++++++++++++++++++++++++++++-----------
>>  libflash/ipmi-hiomap.h |  41 ++-
>>  5 files changed, 936 insertions(+), 220 deletions(-)
> flash changes are in the other mail
>
>> diff --git a/libflash/ipmi-hiomap.c b/libflash/ipmi-hiomap.c
>> index 7327b83..08af005 100644
>> --- a/libflash/ipmi-hiomap.c
>> +++ b/libflash/ipmi-hiomap.c
>> @@ -11,6 +11,10 @@
>>  #include <stdbool.h>
>>  #include <stdint.h>
>>  #include <string.h>
>> +#include <lock.h>
>> +#include <debug_descriptor.h>
>> +#include <timebase.h>
>> +#include <cpu.h>
>>  
>>  #include <ccan/container_of/container_of.h>
>>  
>> @@ -24,7 +28,7 @@ struct ipmi_hiomap_result {
>>  	int16_t cc;
>>  };
>>  
>> -#define RESULT_INIT(_name, _ctx) struct ipmi_hiomap_result _name = { _ctx, -1 }
>> +static struct hiomap_v2_create_window *window_parms;
>>  
>>  static inline uint32_t blocks_to_bytes(struct ipmi_hiomap *ctx, uint16_t blocks)
>>  {
>> @@ -62,9 +66,20 @@ static int hiomap_protocol_ready(struct ipmi_hiomap *ctx)
>>  	return 0;
>>  }
>>  
>> -static int hiomap_queue_msg_sync(struct ipmi_hiomap *ctx, struct ipmi_msg *msg)
>> +static int hiomap_queue_msg(struct ipmi_hiomap *ctx, struct ipmi_msg *msg)
>>  {
>>  	int rc;
>> +	int bl_flags;
>> +
>> +	lock(&ctx->lock);
>> +	bl_flags = ctx->bl.flags;
>> +	unlock(&ctx->lock);
>> +
>> +	/*
>> +	 * during boot caution to stay duration within skiboot/
>> +	 * no exit re-entry due to poller conflicts with synchronous window moves
>> +	 * asynchronous usage intended for opal_flash_op and flash_poll paths
>> +	 */
> doesn't parse
>
>
>>  	/*
>>  	 * There's an unavoidable TOCTOU race here with the BMC sending an
>> @@ -73,17 +88,23 @@ static int hiomap_queue_msg_sync(struct ipmi_hiomap *ctx, struct ipmi_msg *msg)
>>  	 * hiomap_queue_msg_sync() exists to capture the race in a single
>>  	 * location.
>>  	 */
>> -	lock(&ctx->lock);
>> -	rc = hiomap_protocol_ready(ctx);
>> -	unlock(&ctx->lock);
>> -	if (rc) {
>> -		ipmi_free_msg(msg);
>> -		return rc;
>> -	}
>>  
>> -	ipmi_queue_msg_sync(msg);
>> +	if ((opal_booting()) || (!(bl_flags & ASYNC_REQUIRED))) {
>> +		lock(&ctx->lock);
>> +		rc = hiomap_protocol_ready(ctx);
>> +		unlock(&ctx->lock);
>> +		if (rc) {
>> +			ipmi_free_msg(msg);
>> +			return rc;
>> +		}
>> +		prlog(PR_TRACE, "%s SENDING SYNC\n", __func__);
>> +		ipmi_queue_msg_sync(msg);
>> +	} else {
>> +		prlog(PR_TRACE, "%s SENDING ASYNC\n", __func__);
>> +		rc = ipmi_queue_msg(msg);
Yes, during a protocol reset we have some interesting corner cases.
> Is the hiomap_protocol_ready() check required in the async branch too?
>
>> +	}
>>  
>> -	return 0;
>> +	return rc;
>>  }
>>  
>>  /* Call under ctx->lock */
>> @@ -100,12 +121,178 @@ static int hiomap_window_valid(struct ipmi_hiomap *ctx, uint64_t pos,
>>  		return FLASH_ERR_PARM_ERROR;
>>  	if (pos < ctx->current.cur_pos)
>>  		return FLASH_ERR_PARM_ERROR;
>> -	if ((pos + len) > (ctx->current.cur_pos + ctx->current.size))
>> -		return FLASH_ERR_PARM_ERROR;
>> +	if ((pos + len) > (ctx->current.cur_pos + ctx->current.size)) {
>> +		/* we will compensate the proper values in caller */

See other patch comments on when the async chunk straddles

the current window sizes, etc.

> compensate?
>
>> +		if ((pos + ctx->current.size) <= (ctx->current.cur_pos + ctx->current.size)) {
>> +			prlog(PR_TRACE, "%s OK pos=%llu "
>> +				"ctx->current.size=0x%x "
>> +				"ctx->current.cur_pos=0x%x\n",
>> +				__func__,
>> +				pos,
>> +				ctx->current.size,
>> +				ctx->current.cur_pos);
>> +		} else {
>> +			prlog(PR_TRACE, "%s CHECKING further pos=%llu "
>> +				"for len=%llu ctx->current.size=0x%x "
>> +				"ctx->current.cur_pos=0x%x\n",
>> +				__func__,
>> +				pos,
>> +				len,
>> +				ctx->current.size,
>> +				ctx->current.cur_pos);
>> +			if ((pos + ctx->current.adjusted_window_size) <= (ctx->current.cur_pos + ctx->current.size)) {
>> +				prlog(PR_TRACE, "%s OK use ADJUSTED pos=%llu "
>> +					"adjusted_len=%i for len=%llu "
>> +					"ctx->current.size=0x%x "
>> +					"ctx->current.cur_pos=0x%x\n",
>> +					__func__,
>> +					pos,
>> +					ctx->current.adjusted_window_size,
>> +					len,
>> +					ctx->current.size,
>> +					ctx->current.cur_pos);
>> +			} else {
>> +				prlog(PR_TRACE, "%s we need to MOVE the window\n", __func__);
>> +				return FLASH_ERR_PARM_ERROR;
>> +			}
>> +		}
>> +	}
>>  
>> +	prlog(PR_TRACE, "%s ALL GOOD, no move needed\n", __func__);
>>  	return 0;
>>  }
>>  
>> +static void move_cb(struct ipmi_msg *msg)
>> +{
>> +	/*
>> +	 * we leave the move_cb outside of the ipmi_hiomap_cmd_cb
>> +	 * based on the command we need to special close the window
>> +	 */

Closing the window allows the asynchronous call which may take

minutes to complete to force the next iteration to properly setup

the window.

> I don't see why this can't be handled in-line with the reset of the
> hiomap commands. Why is closing the window required? Does the existing
> sync path do that?
>
>> +	struct ipmi_hiomap_result *res = msg->user_data;
>> +	struct ipmi_hiomap *ctx = res->ctx;
>> +	/*
>> +	 * only a few iterations to try for lock/
>> +	 * contention is probably hiomap_window_move trying to setup again
>> +	 */
>> +	int lock_try_counter = 10;
>> +
>> +	if ((msg->resp_size != 8) || (msg->cc != IPMI_CC_NO_ERROR) || (msg->data[1] != ctx->inflight_seq)) {
>> +		lock(&ctx->lock);
>> +		ctx->cc = OPAL_HARDWARE;
>> +		ctx->window_state = closed_window;
>> +		goto out;
>> +	}
>> +
>> +	window_parms = (struct hiomap_v2_create_window *)&msg->data[2];
>> +	while (!try_lock(&ctx->lock)) {
>> +		--lock_try_counter;
>> +		if (lock_try_counter == 0) {
>> +			break;
>> +		}
>> +	}
>> +	if (lock_try_counter == 0) {
>> +		/*
>> +		 * we cannot get the lock, but update anyway
>> +		 * because we cannot communicate this completion
>> +		 * and someone will need to retry
>> +		 * contention usually with handle_events or window_move
>> +		 * this code path is the critical path that will open the window
>> +		 */
>> +		ctx->window_state = closed_window;
>> +		ctx->cc = OPAL_PARAMETER;
>> +		goto out2;
>> +	}
See other patch comments on logic being optimized away.
> This lock retry stuff seems fundementally sketchy and I don't see why
> it's needed.
>
>> +	/* If here, we got the lock, cc consumed higher up so need in ctx */
>> +
>> +	ctx->cc = IPMI_CC_NO_ERROR;
>> +	ctx->current.lpc_addr =
>> +		blocks_to_bytes(ctx, le16_to_cpu(window_parms->lpc_addr));
>> +	ctx->current.size =
>> +		blocks_to_bytes(ctx, le16_to_cpu(window_parms->size));
>> +	ctx->current.cur_pos =
>> +		blocks_to_bytes(ctx, le16_to_cpu(window_parms->offset));
>> +	/* refresh to current */
>> +	ctx->current.adjusted_window_size = ctx->current.size;
>> +
>> +	/* now that we have moved stuff the values */
>> +	*ctx->active_size = ctx->requested_len;
>> +	/*
>> +	 * Is length past the end of the window?
>> +	 * if this condition happens it can cause the async.retry_counter to fail
See other comments on async chunk and straddling current window.
> Again, explain why. The fact it can happen is interesting, but comments
> should be answering more questions then they raise.
>
>> +	if ((ctx->requested_pos + ctx->requested_len) > (ctx->current.cur_pos + ctx->current.size)) {
>> +		/*
>> +		 * Adjust size to meet current window
>> +		 * active_size goes back to caller,
>> +		 * but caller may expire and we need to store for future use
>> +		 */
>> +		*ctx->active_size = (ctx->current.cur_pos + ctx->current.size) - ctx->requested_pos;
>> +		ctx->current.adjusted_window_size = (ctx->current.cur_pos + ctx->current.size) - ctx->requested_pos;
>> +	}

See other comments on async chunk and straddling current window

and then subsequent schedule delays and retry.

Static variables are needed since scope of calls are completed