[Skiboot] [PATCH] external/gard: Clear entire guard partition instead of entry by entry

[Cyril Bur]

On Thu, 2017-09-21 at 14:29 +1000, Suraj Jitindar Singh wrote:
> When using the current implementation of the gard tool to ecc clear the
> entire GUARD partition it is done one gard record at a time. While this
> may be ok when accessing the actual flash this is very slow when done
> from the host over the mbox protocol (on the order of 4 minutes) because
> the bmc side is required to do many read, erase, writes under the hood.
> 
> Fix this by rewriting the gard tool reset_partition() function. Now we
> allocate all the erased guard entries and (if required) apply ecc to the
> entire buffer. Then we can do one big erase and write of the entire
> partition. This reduces the time to clear the guard partition to on the
> order of 4 seconds.
> 

Looks good,

As discussed with Suraj - we should think about having this be a
feature in the blocklevel() code since pflash does a similar thing.

It should also be noted that I don't have access to or the cycles at
the moment to test this and see how hostboot reacts. Inspection seems
to indicate that the reset GARD partitions with or without this patch
will be the same. We've had bugs in the past with hostboot being
sensitive to seemingly unimportant differences - this is the kind of
thing that stops machines from booting.

For now though,

Reviewed-by: Cyril Bur <cyril.bur at au1.ibm.com

> Reported-by: Pridhiviraj Paidipeddi <ppaidipe at linux.vnet.ibm.com>
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh at gmail.com>
> ---
>  external/gard/gard.c | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/external/gard/gard.c b/external/gard/gard.c
> index 5dc14c5..88916c2 100644
> --- a/external/gard/gard.c
> +++ b/external/gard/gard.c
> @@ -454,24 +454,31 @@ static int do_clear_i(struct gard_ctx *ctx, int pos, struct gard_record *gard, v
>  
>  static int reset_partition(struct gard_ctx *ctx)
>  {
> -	int i, rc;
> -	struct gard_record gard;
> -	memset(&gard, 0xFF, sizeof(gard));
> +	int len, num_entries, rc = 0;
> +	struct gard_record *gard;
> +
> +	num_entries = ctx->gard_data_len / sizeof_gard(ctx);
> +	len = num_entries * sizeof(*gard);
> +	gard = malloc(len);
> +	if (!gard) {
> +		return FLASH_ERR_MALLOC_FAILED;
> +	}
> +	memset(gard, 0xFF, len);
>  
>  	rc = blocklevel_smart_erase(ctx->bl, ctx->gard_data_pos, ctx->gard_data_len);
>  	if (rc) {
>  		fprintf(stderr, "Couldn't erase the gard partition. Bailing out\n");
> -		return rc;
> +		goto out;
>  	}
> -	for (i = 0; i + sizeof_gard(ctx) < ctx->gard_data_len; i += sizeof_gard(ctx)) {
> -		rc = blocklevel_write(ctx->bl, ctx->gard_data_pos + i, &gard, sizeof(gard));
> -		if (rc) {
> -			fprintf(stderr, "Couldn't reset the entire gard partition. Bailing out\n");
> -			return rc;
> -		}
> +	rc = blocklevel_write(ctx->bl, ctx->gard_data_pos, gard, len);
> +	if (rc) {
> +		fprintf(stderr, "Couldn't reset the entire gard partition. Bailing out\n");
> +		goto out;
>  	}
>  
> -	return 0;
> +out:
> +	free(gard);
> +	return rc;
>  }
>  
>  static int do_clear(struct gard_ctx *ctx, int argc, char **argv)