[Skiboot] [PATCH 1/3] libflash/ipmi-hiomap: Fix blocks count issue

[Andrew Jeffery]

On Fri, 5 Apr 2019, at 00:03, Vasant Hegde wrote:
> We convert data size to block count and pass block count to BMC.
> If data size is not block aligned then we endup sending block count
> less than actual data. BMC will write partial data to flash memory.
> 
> Sample log :
> [  594.388458416,7] HIOMAP: Marked flash dirty at 0x42010 for 8
> [  594.398756487,7] HIOMAP: Flushed writes
> [  594.409596439,7] HIOMAP: Marked flash dirty at 0x42018 for 3970
> [  594.419897507,7] HIOMAP: Flushed writes
> 
> In this case HIOMAP sent data with block count=0 and hence BMC didn't
> flush data to flash.
> 
> Lets fix this issue by adjusting block count before sending it to BMC.
> 
> Cc: Andrew Jeffery <andrew at aj.id.au>
> Cc: skiboot-stable at lists.ozlabs.org
> Signed-off-by: Vasant Hegde <hegdevasant at linux.vnet.ibm.com>
> ---
>  libflash/ipmi-hiomap.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/libflash/ipmi-hiomap.c b/libflash/ipmi-hiomap.c
> index 56492fa87..da5fa29c9 100644
> --- a/libflash/ipmi-hiomap.c
> +++ b/libflash/ipmi-hiomap.c
> @@ -49,6 +49,16 @@ static inline uint16_t bytes_to_blocks(struct 
> ipmi_hiomap *ctx, uint32_t bytes)
>  	return bytes >> ctx->block_size_shift;
>  }
>  

I have a bunch of nitty renames below, but I think it improves readability:

> +static inline uint16_t bytes_to_blocks_align_up(struct ipmi_hiomap 
> *ctx,
> +						uint32_t pos, uint32_t bytes)

s/bytes/len

> +{
> +	uint32_t size_shift = 1 << ctx->block_size_shift;

s/size_shift/block_size/

> +	uint32_t pos_offset = pos & (size_shift - 1);

Similarly s/pos_offset/delta/

> +	uint32_t len = ALIGN_UP((bytes + pos_offset), size_shift);

s/len/aligned/

Then:

uint32_t blocks = len >> ctx->block_size_shift;

We know that we need to fit blocks into a uint16_t as well, but the only
constraint on ctx->block_size_shift is that it's at least 12. If it is less than
16 we probably want to validate that the aligned length we've generated
fits in a uint16_t, so:

uint32_t mask = ((1 << 16) - 1);

assert(!(blocks & ~mask));

Then:

 return blocks & mask;

> +
> +	return len >> ctx->block_size_shift;
> +}

Putting it all together:

static inline uint16_t bytes_to_blocks_align_up(struct ipmi_hiomap *ctx,
						uint32_t pos, uint32_t len)
{
    uint32_t block_size = 1 << ctx->block_size_shift;
    uint32_t delta = pos & (block_size - 1);
    uint32_t aligned = ALIGN_UP((len + delta), block_size);
    uint32_t blocks = aligned >> ctx->block_size_shift;
    uint32_t mask = ((1 << 16) - 1);

    assert(!(blocks & ~mask));

    return blocks & mask;
}

Thoughts? Note that I haven't tested this suggestion :)

> +
>  /* Call under ctx->lock */
>  static int hiomap_protocol_ready(struct ipmi_hiomap *ctx)
>  {
> @@ -321,7 +331,7 @@ static int hiomap_window_move(struct ipmi_hiomap 
> *ctx, uint8_t command,
>  
>  	range = (struct hiomap_v2_range *)&req[2];
>  	range->offset = cpu_to_le16(bytes_to_blocks(ctx, pos));
> -	range->size = cpu_to_le16(bytes_to_blocks(ctx, len));
> +	range->size = cpu_to_le16(bytes_to_blocks_align_up(ctx, pos, len));
>  
>  	msg = ipmi_mkmsg(IPMI_DEFAULT_INTERFACE,
>  		         bmc_platform->sw->ipmi_oem_hiomap_cmd,
> @@ -384,7 +394,7 @@ static int hiomap_mark_dirty(struct ipmi_hiomap 
> *ctx, uint64_t offset,
>  	pos = offset - ctx->current.cur_pos;
>  	range = (struct hiomap_v2_range *)&req[2];
>  	range->offset = cpu_to_le16(bytes_to_blocks(ctx, pos));
> -	range->size = cpu_to_le16(bytes_to_blocks(ctx, size));
> +	range->size = cpu_to_le16(bytes_to_blocks_align_up(ctx, pos, size));
>  
>  	msg = ipmi_mkmsg(IPMI_DEFAULT_INTERFACE,
>  		         bmc_platform->sw->ipmi_oem_hiomap_cmd,
> @@ -494,7 +504,7 @@ static int hiomap_erase(struct ipmi_hiomap *ctx, 
> uint64_t offset,
>  	pos = offset - ctx->current.cur_pos;
>  	range = (struct hiomap_v2_range *)&req[2];
>  	range->offset = cpu_to_le16(bytes_to_blocks(ctx, pos));
> -	range->size = cpu_to_le16(bytes_to_blocks(ctx, size));
> +	range->size = cpu_to_le16(bytes_to_blocks_align_up(ctx, pos, size));
>  
>  	msg = ipmi_mkmsg(IPMI_DEFAULT_INTERFACE,
>  		         bmc_platform->sw->ipmi_oem_hiomap_cmd,

I've gone through the protocol docs and this appears to be a complete fix.
Thanks.

Andrew

> -- 
> 2.14.3
> 
>