Re: [PATCH v2 07/19] arm64: insn: Add encoder for bitwise operations using litterals

[James Morse <james.morse@arm.com>]

Hi Marc,

On 11/12/17 14:49, Marc Zyngier wrote:
> We lack a way to encode operations such as AND, ORR, EOR that take
> an immediate value. Doing so is quite involved, and is all about
> reverse engineering the decoding algorithm described in the
> pseudocode function DecodeBitMasks().


As this is over my head, I've been pushing random encodings through gas/objdump
and then tracing them through here.... can this encode 0xf80000000fffffff?

gas thinks this is legal:
|   0:   92458000        and     x0, x0, #0xf80000000fffffff

I make that N=1, S=0x20, R=0x05.
(I'm still working out what 'S' means)


> diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
> index 7e432662d454..326b17016485 100644
> --- a/arch/arm64/kernel/insn.c
> +++ b/arch/arm64/kernel/insn.c

> +static u32 aarch64_encode_immediate(u64 imm,
> +				    enum aarch64_insn_variant variant,
> +				    u32 insn)
> +{
> +	unsigned int immr, imms, n, ones, ror, esz, tmp;
> +	u64 mask;

[...]

> +	/* N is only set if we're encoding a 64bit value */
> +	n = esz == 64;
> +
> +	/* Trim imm to the element size */
> +	mask = BIT(esz - 1) - 1;
> +	imm &= mask;

Won't this lose the top bit of a 64bit immediate?

(but then you put it back later, so something funny is going on)

This becomes 0x780000000fffffff,


> +
> +	/* That's how many ones we need to encode */
> +	ones = hweight64(imm);

meaning we're short a one here,


> +
> +	/*
> +	 * imms is set to (ones - 1), prefixed with a string of ones
> +	 * and a zero if they fit. Cap it to 6 bits.
> +	 */
> +	imms  = ones - 1;
> +	imms |= 0xf << ffs(esz);
> +	imms &= BIT(6) - 1;

so imms is 0x1f, not 0x20.


> +	/* Compute the rotation */
> +	if (range_of_ones(imm)) {
> +		/*
> +		 * Pattern: 0..01..10..0
> +		 *
> +		 * Compute how many rotate we need to align it right
> +		 */
> +		ror = ffs(imm) - 1;

(how come range_of_ones() uses __ffs64() on the same value?)


> +	} else {
> +		/*
> +		 * Pattern: 0..01..10..01..1
> +		 *
> +		 * Fill the unused top bits with ones, and check if
> +		 * the result is a valid immediate (all ones with a
> +		 * contiguous ranges of zeroes).
> +		 */

> +		imm |= ~mask;

but here we put the missing one back,


> +		if (!range_of_ones(~imm))
> +			return AARCH64_BREAK_FAULT;

meaning we pass this check and carry on, (even though 0x780000000fffffff isn't a
legal value)


(this next bit I haven't worked out yet)
> +		/*
> +		 * Compute the rotation to get a continuous set of
> +		 * ones, with the first bit set at position 0
> +		 */
> +		ror = fls(~imm);
> +	}
> +
> +	/*
> +	 * immr is the number of bits we need to rotate back to the
> +	 * original set of ones. Note that this is relative to the
> +	 * element size...
> +	 */
> +	immr = (esz - ror) & (esz - 1);


If I've followed this through correctly, this results in:
|   0:   92457c00        and     x0, x0, #0xf800000007ffffff

... which wasn't the immediate I started with.


Unless I've gone wrong, I think the 'Trim imm to the element size' code needs to
move up into the esz-reducing loop so it doesn't happen for a 64bit immediate.



Thanks,

James