[Pdbg] [PATCH v2 3/3] Add fircheck support

Alistair Popple alistair at popple.id.au
Fri May 18 11:52:21 AEST 2018


Thanks for looking into this Balbir, I have a couple of comments below.

> +static uint64_t global_mc_fir_addr;
> +
> +extern int for_each_target(char *class, int (*cb)(struct pdbg_target *, uint32_t, uint64_t *, uint64_t *),
> +				uint64_t *arg1, uint64_t *arg2);

I'm trying to make this function go away as the library now has some iterators
which are nice to use than callbacks with random arguments and should cover most
cases. Eg. pdbg_for_each_target(...).

> +static int getfir(struct pdbg_target *target, uint32_t index, uint64_t *addr, uint64_t *ignored)
> +{
> +	int rc;
> +	uint64_t value;
> +	struct pdbg_target *dn;
> +	uint32_t i;
> +
> +	rc = pib_read(target, *addr, &value);
> +	if (value == 0)
> +		printf("%s%d: No checkstop detected\n", "p", index);
> +
> +	dt_for_each_compatible(dt_root, dn, "ibm,power9-chiplet") {
> +		i = dt_prop_get_u32(dn, "index");
> +		if (!i)
> +			continue; /* we should abort really */
> +		if (value & PPC_BIT(i)) {
> +			printf("%s%d: FIR bit set for chiplet %s\n", "p", index, dn->dn_name);

Ideally I'd rather see any user output in the application (src/*) rather than
the library. Which I guess means this function should just return the actual FIR
bit mask and the leave the analysis/printing to the application.

> +			value &= (value - 1);
> +		}
> +	}
> +
> +	return rc;
> +}
> +
> +/*
> + * Print a summary of the FIR check errors.
> + *
> + * NOTE: The current implementation looks at the top most bits
> + * there is much more to do. We need to dig deeper and look at
> + * recoverable and xstop bits and in the future FFDC bits as
> + * well
> + */
> +int handle_fircheck(int optind, int argc, char *argv[])
> +{
> +	struct pdbg_target *global_fir_dn;
> +	int rc;
> +
> +	global_fir_dn = dt_find_by_name(dt_root, "global_fir");
> +	if (!global_fir_dn) {
> +		rc = -1;
> +		PR_ERROR("Failed to find global multicast scom fir address"
> +			 "Please check the device tree bindings\n");
> +		goto out;
> +	}
> +
> +	if (!global_mc_fir_addr)
> +		global_mc_fir_addr = dt_get_address(global_fir_dn, 0, NULL);
> +	if (!global_mc_fir_addr) {
> +		rc = -1;
> +		PR_ERROR("Failed to find global multicast scom fir address"
> +			 "Please check the device tree bindings\n");
> +		goto out;
> +	}

It would be nice if we could move the calculation of FIR address into the
library though. So maybe add this to getfir() above and have getfir() return the
result which can loop over chiplet numbers here? I guess we could also return a
list of struct pdbg_target pointers which have a xstop rather than the bitmask,
but that's probably overkill for now at least.

> +	rc = for_each_target("pib", getfir, &global_mc_fir_addr, NULL);

I'd rather we use the iterators here as they're safer and more readable IMHO.
Eg:

pdbg_for_each_class_target("pib", target) {
	/* Find chiplet target by index and print out name if xstopped */
}

> +	if (rc) {
> +		PR_ERROR("Failed to read value from multicast scom"
> +			 "address %llx\n", global_mc_fir_addr);
> +		goto out;
> +	}
> +
> +out:
> +	return !rc;
> +}
> +
> diff --git a/src/main.c b/src/main.c
> index a121972..6d38c93 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -64,6 +64,7 @@ static int *chipsel[MAX_PROCESSORS][MAX_CHIPS];
>  static int threadsel[MAX_PROCESSORS][MAX_CHIPS][MAX_THREADS];
>  
>  static int handle_probe(int optind, int argc, char *argv[]);
> +extern int handle_fircheck(int optind, int argc, char *argv[]);
>  
>  static struct {
>  	const char *name;
> @@ -100,6 +101,7 @@ static struct {
>  	{ "htm_analyse", "", "[derepcated use 'htm nest analyse'] Stop and dump buffer to file", &run_htm_analyse },
>  	{ "htm", "(core | nest) (start | stop | status | reset | dump | trace | analyse", "Hardware Trace Macro", &run_htm },
>  	{ "probe", "", "", &handle_probe },
> +	{ "fircheck", "", "Get information about triggered FIR bits", &handle_fircheck },
>  };
>  
>  static void print_usage(char *pname)
> @@ -481,10 +483,19 @@ void print_target(struct pdbg_target *target, int level)
>  		else if (!strcmp(pdbg_target_class_name(target), "thread"))
>  			c = 't';
>  
> -		if (c)
> -			printf("%c%d: %s\n", c, pdbg_target_index(target), pdbg_target_name(target));
> -		else
> +		if (c) {
> +			/*
> +			 * For power9 the core chiplets are indexed from 32. Find a better way
> +			 * to do this
> +			 */
> +			if (!strncmp(target->compatible, "ibm,power9-core", strlen(target->compatible))) {
> +				printf("%c%d: %s\n", c, pdbg_target_index(target)-32, pdbg_target_name(target));
> +			} else {
> +				printf("%c%d: %s\n", c, pdbg_target_index(target), pdbg_target_name(target));
> +			}
> +		} else {
>  			printf("%s\n", pdbg_target_name(target));
> +		}
>  	}
>  
>  	pdbg_for_each_child_target(target, next) {
> 




More information about the Pdbg mailing list