Re: [RFC v11 45/55] target/arm: cpu-sve: new module

[Claudio Fontana <cfontana@suse.de>]

On 3/25/21 7:40 PM, Richard Henderson wrote:
> On 3/23/21 9:46 AM, Claudio Fontana wrote:
>> extract the SVE-related cpu object properties and functions,
>> and move them to a separate module.
>>
>> Disentangle SVE from pauth that is a separate, TCG-only feature.
>>
>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>> ---
>>   target/arm/cpu-sve.h     |  40 +++++
>>   target/arm/cpu.h         |  22 +--
>>   target/arm/cpu-sve.c     | 360 +++++++++++++++++++++++++++++++++++++++
>>   target/arm/cpu.c         |   5 +-
>>   target/arm/cpu64.c       | 333 +-----------------------------------
>>   target/arm/kvm/kvm-cpu.c |   2 +-
>>   target/arm/meson.build   |   1 +
>>   7 files changed, 417 insertions(+), 346 deletions(-)
>>   create mode 100644 target/arm/cpu-sve.h
>>   create mode 100644 target/arm/cpu-sve.c
>>
>> diff --git a/target/arm/cpu-sve.h b/target/arm/cpu-sve.h
>> new file mode 100644
>> index 0000000000..b1be575265
>> --- /dev/null
>> +++ b/target/arm/cpu-sve.h
>> @@ -0,0 +1,40 @@
>> +/*
>> + * QEMU AArch64 CPU SVE Extensions for TARGET_AARCH64
>> + *
>> + * Copyright (c) 2013 Linaro Ltd
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * as published by the Free Software Foundation; either version 2
>> + * of the License, or (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, see
>> + * <http://www.gnu.org/licenses/gpl-2.0.html>
>> + */
>> +
>> +#ifndef CPU_SVE_H
>> +#define CPU_SVE_H
>> +
>> +/* note: SVE is an AARCH64-only option, only include this for TARGET_AARCH64 */
>> +
>> +/* called by arm_cpu_finalize_features in realizefn */
>> +void cpu_sve_finalize_features(ARMCPU *cpu, Error **errp);
>> +
>> +/* add the CPU SVE properties */
>> +void cpu_sve_add_props(Object *obj);
>> +
>> +/* add the CPU SVE properties specific to the "MAX" CPU */
>> +void cpu_sve_add_props_max(Object *obj);
>> +
>> +/* In AArch32 mode, predicate registers do not exist at all.  */
>> +typedef struct ARMPredicateReg {
>> +    uint64_t p[DIV_ROUND_UP(2 * ARM_MAX_VQ, 8)] QEMU_ALIGNED(16);
>> +} ARMPredicateReg;
> 
> I don't agree with moving this out of cpu.h, next to the rest of the vector 
> definitions.
> 
> I agree that we should be using more files, but if we're going to have an 
> cpu-sve.c, why did some of the sve functions go into cpu-common.c?

maybe cpu-sve-props.c would be a better name, it really contains only cpu sve properties code.

> 
> I don't agree with moving functions and renaming them simultaneously.  I'm not 
> even sure why you're renaming them, or why you've suddenly chosen "cpu_sve_*" 
> as the prefix to use.
> 
> 
> r~
> 

I think the idea was trying to create a cpu_sve module handling everything related to sve,
and consistently using the name of the module as the prefix.

It might be too early to attempt that anyway; as you noted, there is other SVE-related functionality all over the place,
so better to revisit this.

I'd suggest renaming this to cpu_sve_props, and moving everything not props related out of it.

Thanks,

Claudio