Re: [POC 3/5] sframe: add new SFrame library

[Peter Zijlstra <peterz@infradead.org>]

On Mon, May 01, 2023 at 01:04:08PM -0700, Indu Bhagat wrote:

> +static int sframe_fre_copy(struct sframe_fre *dst,
> +			   struct sframe_fre *src)
> +{
> +	if (dst == NULL || src == NULL)
> +		return SFRAME_ERR;
> +
> +	memcpy(dst, src, sizeof(struct sframe_fre));
> +	return 0;
> +}

> +int sframe_sec_find_fre(struct sframe_sec *sfsec, int32_t pc,
> +			struct sframe_fre *frep)
> +{
> +	struct sframe_func_desc_entry *fdep;
> +	uint32_t start_address, i;
> +	struct sframe_fre cur_fre, next_fre;
> +	unsigned char *fres;
> +	unsigned int fre_type, fde_type;
> +	size_t esz;
> +	int err = 0;
> +	size_t size = 0;
> +	/*
> +	 * For regular FDEs(i.e. fde_type SFRAME_FDE_TYPE_PCINC),
> +	 * where the start address in the FRE is an offset from start pc,
> +	 * use a bitmask with all bits set so that none of the address bits are
> +	 * ignored.  In this case, we need to return the FRE where
> +	 * (PC >= FRE_START_ADDR).
> +	 */
> +	uint64_t bitmask = 0xffffffff;
> +
> +	if ((sfsec == NULL) || (frep == NULL))
> +		return SFRAME_ERR_INVAL;
> +
> +	/* Find the FDE which contains the PC, then scan its FRE entries. */
> +	fdep = sframe_sec_find_fde(sfsec, pc, &err);
> +	if (fdep == NULL || sfsec->fres == NULL)
> +		return SFRAME_ERR_INIT_INVAL;
> +
> +	fre_type = sframe_get_fre_type(fdep);
> +	fde_type = sframe_get_fde_type(fdep);
> +
> +	/*
> +	 * For FDEs for repetitive pattern of insns, we need to return the FRE
> +	 * such that(PC & FRE_START_ADDR_AS_MASK >= FRE_START_ADDR_AS_MASK).
> +	 * so, update the bitmask to the start address.
> +	 */
> +	/* FIXME - the bitmask. */
> +	if (fde_type == SFRAME_FDE_TYPE_PCMASK)
> +		bitmask = 0xff;
> +
> +	fres = (unsigned char *)sfsec->fres + fdep->func_start_fre_off;
> +	for (i = 0; i < fdep->func_num_fres; i++) {
> +		err = sframe_sec_read_fre((const char *)fres, &next_fre,
> +					  fre_type, &esz);
> +		start_address = next_fre.start_ip_offset;
> +
> +		if (((fdep->func_start_address
> +		      + (int32_t)start_address) & bitmask) <= (pc & bitmask)) {
> +			sframe_fre_copy(&cur_fre, &next_fre);
> +
> +			/* Get the next FRE in sequence. */
> +			if (i < fdep->func_num_fres - 1) {
> +				fres += esz;
> +				err = sframe_sec_read_fre((const char *)fres,
> +							  &next_fre,
> +							  fre_type, &esz);
> +
> +				/* Sanity check the next FRE. */
> +				if (!sframe_fre_sanity_check_p(&next_fre))
> +					return SFRAME_ERR_FRE_INVAL;
> +
> +				size = next_fre.start_ip_offset;
> +			} else {
> +				size = fdep->func_size;
> +			}
> +
> +			if (((fdep->func_start_address
> +			      + (int32_t)size) & bitmask) > (pc & bitmask)) {
> +				/* Cur FRE contains the PC, return it. */
> +				sframe_fre_copy(frep, &cur_fre);
> +				return 0;
> +			}
> +		} else {
> +			return SFRAME_ERR_FRE_INVAL;
> +		}
> +	}
> +	return SFRAME_ERR_FDE_INVAL;
> +}

What's the purpose of that sframe_fre_copy() thing? Why can't you do:

	cur_fre = next_fre;

	...

	if (frep)
		*frep = cur_fre;


and call it a day? C is quite capable of writing that memcpy for you.