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

[Andrew Jones]

On Fri, Jun 28, 2019 at 09:27:39AM +0200, Andrew Jones wrote:
> On Thu, Jun 27, 2019 at 06:49:02PM +0200, Richard Henderson wrote:
> > 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);
> 
> I guess I don't have a strong opinion on one or two bitmaps. I'm not a big
> fan of adding temporary variables to data structures (even if the same
> amount of storage is being allocated a different way), but I can change
> this for v3.

On second thought, since in this case there is storage savings (one less
long), then I'd rather we keep it a single bitmap. Maybe I can just add
some comments to these bitmap operations?

Thanks,
drew