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

[ppaidipe]

On 2017-09-21 11:10, Cyril Bur wrote:
> 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,

Tested on a p9dsu platform with the below testcase, which works fine
and clear/read/write gard records are faster.
https://github.com/open-power/op-test-framework/pull/171

> 
> 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)