Re: [Qemu-devel] [PATCH v2 07/14] target/arm/cpu64: max cpu: Introduce sve<vl-bits> properties

[Richard Henderson <richard.henderson@linaro.org>]

On 6/21/19 6:34 PM, Andrew Jones wrote:
> +    /*
> +     * In sve_vq_map each set bit is a supported vector length of
> +     * (bit-number + 1) * 16 bytes, i.e. each bit number + 1 is the vector
> +     * length in quadwords. We need a map size twice the maximum
> +     * quadword length though because we use two bits for each vector
> +     * length in order to track three states: uninitialized, off, and on.
> +     */
> +    DECLARE_BITMAP(sve_vq_map, ARM_MAX_VQ * 2);

I don't see that having one continuous bitmap is more convenient than two.
Indeed, there appear to be several places that would be clearer with two.

> +static arm_vq_state arm_cpu_vq_map_get(ARMCPU *cpu, int vq)
> +{
> +    assert(vq <= ARM_MAX_VQ);
> +
> +    return test_bit(vq - 1, cpu->sve_vq_map) |
> +           test_bit(vq - 1 + ARM_MAX_VQ, cpu->sve_vq_map) << 1;
> +}

Neither easier nor more complex w/ one or two bitmaps.

> +static void arm_cpu_vq_map_init(ARMCPU *cpu)
> +{
> +    bitmap_zero(cpu->sve_vq_map, ARM_MAX_VQ * 2);
> +    bitmap_set(cpu->sve_vq_map, ARM_MAX_VQ, ARM_MAX_VQ);
> +}

Clearer with two bitmaps.

	bitmap_zero(cpu->sve_vq_map, ARM_MAX_VQ);
	bitmap_set(cpu->sve_vq_uninit_map, 0, ARM_MAX_VQ);

> +static bool arm_cpu_vq_map_is_finalized(ARMCPU *cpu)
> +{
> +    DECLARE_BITMAP(map, ARM_MAX_VQ * 2);
> +
> +    bitmap_zero(map, ARM_MAX_VQ * 2);
> +    bitmap_set(map, ARM_MAX_VQ, ARM_MAX_VQ);
> +    bitmap_and(map, map, cpu->sve_vq_map, ARM_MAX_VQ * 2);
> +
> +    return bitmap_empty(map, ARM_MAX_VQ * 2);
> +}

Definitely clearer w/ 2 bitmaps,

	return bitmap_empty(cpu->sve_vq_uninit_map);


As for how sve-max-vq=Y and sveX={on,off} interoperate...  I wonder if we can
just remove cpu->sve_max_vq.  That might simplify your code a bit.

What if sve-max-vq=Y "expands" to

	for (X = 1; X <= Y; X++) { sve(X*128)=on }

Then you've got a reasonable in-order definition of how those two sets of
switches interoperate.

The uses of cpu->sve_max_vq within cpu.c and cpu64.c are all initialization
stuff that you're replacing.

The use within sve_zcr_len_for_el can be replaced by AVR_MAX_VQ.  Your "select
supported vector size not larger than" code goes at the end of that function,
such that we select a supported maximum no larger than the raw .LEN values.

The use within aarch64_sve_narrow_vq should in fact assert that the given vq is
set within cpu->sve_vq_map.


r~