Re: [kvm-unit-tests PATCH v2 4/5] configure: Add --qemu-cpu option

[Jean-Philippe Brucker <jean-philippe@linaro.org>]

On Mon, Mar 24, 2025 at 02:13:13PM +0100, Andrew Jones wrote:
> On Mon, Mar 24, 2025 at 10:41:03AM +0000, Alexandru Elisei wrote:
> > Hi Drew,
> > 
> > On Mon, Mar 24, 2025 at 09:19:27AM +0100, Andrew Jones wrote:
> > > On Sun, Mar 23, 2025 at 11:16:19AM +0000, Alexandru Elisei wrote:
> > > ...
> > > > > > +if [ -z "$qemu_cpu" ]; then
> > > > > > +	if ( [ "$ACCEL" = "kvm" ] || [ "$ACCEL" = "hvf" ] ) &&
> > > > > > +	   ( [ "$HOST" = "aarch64" ] || [ "$HOST" = "arm" ] ); then
> > > > > > +		qemu_cpu="host"
> > > > > >  		if [ "$ARCH" = "arm" ] && [ "$HOST" = "aarch64" ]; then
> > > > > > -			processor+=",aarch64=off"
> > > > > > +			qemu_cpu+=",aarch64=off"
> > > > > >  		fi
> > > > > > +	elif [ "$ARCH" = "arm64" ]; then
> > > > > > +		qemu_cpu="cortex-a57"
> > > > > > +	else
> > > > > > +		qemu_cpu="cortex-a15"
> > > > > 
> > > > > configure could set this in config.mak as DEFAULT_PROCESSOR, avoiding the
> > > > > need to duplicate it here.
> > > > 
> > > > That was my first instinct too, having the default value in config.mak seemed
> > > > like the correct solution.
> > > > 
> > > > But the problem with this is that the default -cpu type depends on -accel (set
> > > > via unittests.cfg or as an environment variable), host and test architecture
> > > > combination. All of these variables are known only at runtime.
> > > > 
> > > > Let's say we have DEFAULT_QEMU_CPU=cortex-a57 in config.mak. If we keep the
> > > > above heuristic, arm/run will override it with host,aarch64=off. IMO, having it
> > > > in config.mak, but arm/run using it only under certain conditions is worse than
> > > > not having it at all. arm/run choosing the default value **all the time** is at
> > > > least consistent.
> > > 
> > > I think having 'DEFAULT' in the name implies that it will only be used if
> > > there's nothing better, and we don't require everything in config.mak to
> > > be used (there's even some s390x-specific stuff in there for all
> > > architectures...)
> > 
> > I'm still leaning towards having the default value and the heuristics for
> > when to pick it in one place ($ARCH/run) as being more convenient, but I
> > can certainly see your point of view.
> 
> I wouldn't mind it only being in $ARCH/run, but this series adds the same
> logic to $ARCH/run and to ./configure. I think an additional, potentially
> unused, variable in config.mak is better than code duplication.

I agree with this. However the next version ends up replacing both
cortex-* types here with "max", so we won't need to pass these values in
the end. The "processor" selection in configure will only be used by the
Makefile for the compiler flag.

Thanks,
Jean