[Skiboot] [RFC PATCH 6/6] libflash: add blocklevel_raw_prepare()

[Stewart Smith]

Vasant Hegde <hegdevasant at linux.vnet.ibm.com> writes:

> On 02/28/2019 11:48 AM, Stewart Smith wrote:
>> The logic of a prepare call is to move the window (or at least start the
>> process of moving the window) so that when a read/write/erase call is
>> made, we don't have to wait for a IPMI round trip. >
>> For non-hiomap backends, one could use similar logic.
>> 
>> ---
>> Not Signed-off-by yet as I'm totally not convinced
>
> Even I'm not convinced about this approach. Its still error prone.
> May be best is to split read/write/erase call to two parts (one initiates 
> request to BMC
> and second one does post processing after getting data from BMC).
> That way we can remove synchronous message completely from these
> paths.

Yeah, that was my thought as well... It involves more surgery to
libflash though, which is annoying.

> Few comments based on current code.
>
>
>> ---
>>   core/flash.c           |  9 +++++++--
>>   libflash/blocklevel.c  | 22 +++++++++++++++++++++
>>   libflash/blocklevel.h  |  2 ++
>>   libflash/ipmi-hiomap.c | 44 ++++++++++++++++++++++++++++++++++++++++--
>>   4 files changed, 73 insertions(+), 4 deletions(-)
>> 
>> diff --git a/core/flash.c b/core/flash.c
>> index 04d99348515e..e267aaa0373d 100644
>> --- a/core/flash.c
>> +++ b/core/flash.c
>> @@ -1,4 +1,4 @@
>> -/* Copyright 2013-2018 IBM Corp.
>> +/* Copyright 2013-2019 IBM Corp.
>>    *
>>    * Licensed under the Apache License, Version 2.0 (the "License");
>>    * you may not use this file except in compliance with the License.
>> @@ -239,7 +239,7 @@ static void flash_poll(struct timer *t __unused, void *data, uint64_t now __unus
>>   {
>>   	struct flash *flash = data;
>>   	uint64_t offset, buf, len;
>> -	int rc;
>> +	int rc = 0;
>> 
>>   	offset = flash->async.pos;
>>   	buf = flash->async.buf;
>> @@ -254,6 +254,10 @@ static void flash_poll(struct timer *t __unused, void *data, uint64_t now __unus
>>   	len = MIN(flash->async.len, flash->block_size);
>>   	prlog(PR_TRACE, "Flash poll op %d len %llu\n", flash->async.op, len);
>> 
>> +	/* If we've done work to prepare, come back later */
>> +	if (blocklevel_raw_prepare(flash->bl, (flash->async.op!=FLASH_OP_READ), offset, len))
>> +		goto schedule;
>
> Need a way to differentiate errors vs waiting to get response from
> BMC.

yep.

>> +
>>   	switch (flash->async.op) {
>>   	case FLASH_OP_READ:
>>   		rc = blocklevel_raw_read(flash->bl, offset, (void *)buf, len);
>> @@ -274,6 +278,7 @@ static void flash_poll(struct timer *t __unused, void *data, uint64_t now __unus
>>   	flash->async.pos += len;
>>   	flash->async.buf += len;
>>   	flash->async.len -= len;
>> +schedule:
>>   	if (!rc && flash->async.len) {
>>   		/*
>>   		 * We want to get called pretty much straight away, just have
>> diff --git a/libflash/blocklevel.c b/libflash/blocklevel.c
>> index 5b57d3c6194f..9659c7fd68af 100644
>
> .../...
>
>
>> 
>> +static int ipmi_hiomap_prepare(struct blocklevel_device *bl, bool rw,
>> +			       uint64_t pos, uint64_t len)
>> +{
>> +	struct ipmi_hiomap *ctx;
>> +	enum lpc_window_state want_state;
>> +	uint64_t size;
>> +	uint8_t command = HIOMAP_C_CREATE_READ_WINDOW;
>> +	bool valid_state;
>> +	bool is_read;
>> +	int rc = 0;
>> +
>> +	/* LPC is only 32bit */
>> +	if (pos > UINT_MAX || len > UINT_MAX)
>> +		return FLASH_ERR_PARM_ERROR;
>> +
>> +	if (rw)
>> +		command = HIOMAP_C_CREATE_WRITE_WINDOW;
>> +
>> +	ctx = container_of(bl, struct ipmi_hiomap, bl);
>> +
>> +	is_read = (command == HIOMAP_C_CREATE_READ_WINDOW);
>> +	want_state = is_read ? read_window : write_window;
>> +
>> +	lock(&ctx->lock);
>> +	valid_state = want_state == ctx->window_state;
>> +	rc = hiomap_window_valid(ctx, pos, len);
>> +	if (valid_state && !rc) {
>> +		unlock(&ctx->lock);
>> +		return 0;
>> +	}
>> +	unlock(&ctx->lock);
>> +
>> +	rc = ipmi_hiomap_handle_events(ctx);
>> +	if (rc)
>> +		return rc;
>> +
>> +	rc = hiomap_window_move(ctx, command, pos, len, &size);
>
> This still uses synchronous path. So not going to help much.

Yeah, it doesn't make a huge impact, but amazingly enough, there is
*some* improvement (tens of ms)... but it needs work.


-- 
Stewart Smith
OPAL Architect, IBM.