On Thu, May 08, 2025 at 11:05:32AM +0100, Alexandru Elisei wrote:
> Hi Drew,
>
> On Thu, May 08, 2025 at 11:39:54AM +0200, Andrew Jones wrote:
> > On Thu, May 08, 2025 at 09:52:38AM +0100, Alexandru Elisei wrote:
> > > Hi Drew,
> > >
> > > On Wed, May 07, 2025 at 06:02:31PM +0200, Andrew Jones wrote:
> > > > On Wed, May 07, 2025 at 04:12:43PM +0100, Alexandru Elisei wrote:
> > > > > Only arm and arm64 are allowed to set --target to kvmtool; the rest
> > > > > of the
> > > > > architectures can only set --target to 'qemu', which is also the
> > > > > default.
> > > > >
> > > > > Needed to make the changes necessary to add support for kvmtool to
> > > > > the test
> > > > > runner.
> > > > >
> > > > > kvmtool also supports running the riscv tests, so it's not outside of
> > > > > the
> > > > > realm of the possibily for the riscv tests to get support for kvmtool.
> > > > >
> > > > > Signed-off-by: Alexandru Elisei <alexandru.eli...@arm.com>
> > > > > ---
> > > > > configure | 36 ++++++++++++++++++++++++------------
> > > > > 1 file changed, 24 insertions(+), 12 deletions(-)
> > > > >
> > > >
> > > > Reviewed-by: Andrew Jones <andrew.jo...@linux.dev>
> > >
> > > Thank you for the review!
> > >
> > > Just to be clear, you are ok with this happening because of the patch:
> > >
> > > $ git pull
> > > $ make clean && make
> > > $ ./run_tests.sh
> > > scripts/runtime.bash: line 24: scripts/arch-run.bash: line 444: [: =:
> > > unary operator expected
> > > timeout -k 1s --foreground 90s /usr/bin/qemu-system-x86_64 --no-reboot
> > > -nodefaults -device pc-testdev -device
> > > isa-debug-exit,iobase=0xf4,iosize=0x4 -vnc none -serial stdio -device
> > > pci-testdev -machine accel=kvm -kernel _NO_FILE_4Uhere_ 2 #
> > > /tmp/tmp.bME9I2BZRG
> > > qemu-system-x86_64: 2: Could not open '2': No such file or directory
> > > scripts/arch-run.bash: line 19: 1: command not found: No such file or
> > > directory
> > > FAIL apic-split
> > > scripts/runtime.bash: line 24: scripts/arch-run.bash: line 444: [: =:
> > > unary operator expected
> > > timeout -k 1s --foreground 90s /usr/bin/qemu-system-x86_64 --no-reboot
> > > -nodefaults -device pc-testdev -device
> > > isa-debug-exit,iobase=0xf4,iosize=0x4 -vnc none -serial stdio -device
> > > pci-testdev -machine accel=kvm -kernel _NO_FILE_4Uhere_ 1 #
> > > /tmp/tmp.11und6qZbL
> > > qemu-system-x86_64: 1: Could not open '1': No such file or directory
> > > scripts/arch-run.bash: line 19: 1: command not found: No such file or
> > > directory
> > > FAIL ioapic-split
> > > [..]
> > >
> > > That's because TARGET is missing from config.mak. If you're ok with the
> > > error, I'll make it clear in the commit message why this is happening.
> > >
> >
> > It's not ideal, but I think it's pretty common to run configure before
> > make after an update to the git repo, so it's not horrible. However,
> > as you pointed out in your cover letter, this can be mitigated if we
> > use function wrappers for the associative array accesses, allowing
> > $TARGET to be checked before it's used. I'd prefer the function wrappers
> > anyway for readability reasons, so let's do that.
>
> I'm all for the function wrappers, I was planning to reply to that comment
> later.
>
> As to this patch, is this what you're thinking:
>
> function vmm_optname_nr_cpus()
> {
> if [ -z $TARGET ]; then
> echo vmm_opts[qemu:nr_cpus]
> else
> echo vmm_opts[$TARGET:nr_cpus]
> fi
> }
Yup
>
> But checking if $TARGET is defined makes this patch useless, and I would
> rather drop it if that's the case.
Sounds good.
Thanks,
drew
