Re: [PATCH v4 10/21] perf arm_spe: Fixup top byte for data virtual address

[Leo Yan]

Hi Andre,

On Tue, Oct 27, 2020 at 03:01:26PM +0000, André Przywara wrote:
> On 27/10/2020 03:09, Leo Yan wrote:
> > To establish a valid address from the address packet payload and finally
> > the address value can be used for parsing data symbol in DSO, current
> > code uses 0xff to replace the tag in the top byte of data virtual
> > address.
> > 
> > So far the code only fixups top byte for the memory layouts with 4KB
> > pages, it misses to support memory layouts with 64KB pages.
> > 
> > This patch adds the conditions for checking bits [55:48] are 0xf0 or
> > 0xfd, if detects the patterns it will fill 0xff into the top byte of the
> > address, also adds comment to explain the fixing up.
> > 
> > Signed-off-by: Leo Yan <leo.yan@xxxxxxxxxx>
> > ---
> >  .../util/arm-spe-decoder/arm-spe-decoder.c    | 24 ++++++++++++++++---
> >  1 file changed, 21 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
> > index 776b3e6628bb..e135ac01d94a 100644
> > --- a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
> > +++ b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
> > @@ -24,7 +24,7 @@
> >  
> >  static u64 arm_spe_calc_ip(int index, u64 payload)
> >  {
> > -	u64 ns, el;
> > +	u64 ns, el, val;
> >  
> >  	/* Instruction virtual address or Branch target address */
> >  	if (index == SPE_ADDR_PKT_HDR_INDEX_INS ||
> > @@ -45,8 +45,26 @@ static u64 arm_spe_calc_ip(int index, u64 payload)
> >  		/* Clean tags */
> >  		payload = SPE_ADDR_PKT_ADDR_GET_BYTES_0_6(payload);
> >  
> > -		/* Fill highest byte if bits [48..55] is 0xff */
> > -		if (SPE_ADDR_PKT_ADDR_GET_BYTE_6(payload) == 0xffULL)
> > +		/*
> > +		 * Armv8 ARM (ARM DDI 0487F.c), chapter "D10.2.1 Address packet"
> > +		 * defines the data virtual address payload format, the top byte
> > +		 * (bits [63:56]) is assigned as top-byte tag; so we only can
> > +		 * retrieve address value from bits [55:0].
> > +		 *
> > +		 * According to Documentation/arm64/memory.rst, if detects the
> > +		 * specific pattern in bits [55:48] of payload which falls in
> > +		 * the kernel space, should fixup the top byte and this allows
> > +		 * perf tool to parse DSO symbol for data address correctly.
> > +		 *
> > +		 * For this reason, if detects the bits [55:48] is one of
> > +		 * following values, will fill 0xff into the top byte:
> > +		 *
> > +		 *   - 0xff (for most kernel memory regions);
> > +		 *   - 0xf0 (for kernel logical memory map with 64KB pages);
> > +		 *   - 0xfd (for kasan shadow region with 64KB pages).
> > +		 */
> > +		val = SPE_ADDR_PKT_ADDR_GET_BYTE_6(payload);
> > +		if (val == 0xffULL || val == 0xf0ULL || val == 0xfdULL)
> 
> But those values are just the beginning of the region used by the
> kernel, aren't they? So the kernel logical map goes from 0xfff000.. to
> 0xfff7fff..., for instance.

Ouch ...  You are right :)

> But actually I wonder why were are so selective here? Wouldn't it just
> suffice to look at bits [55:52] to be either 0 or F?

I considered to compare bits [55:52] but finally I wanted to use more
accurate comparison.  As you pointed out this is incorrect, I will
change to compare bits [55:52].

Thanks,
Leo