Re: [PATCH v5 10/10] test: tpm-tis: Add Sysbus TPM-TIS device test

[Peter Maydell]

On Thu, 12 May 2022 at 16:59, Eric Auger <eric.auger@redhat.com> wrote:
>
> Hi Peter,
>
> On 5/12/22 15:08, Peter Maydell wrote:
> > On Thu, 5 Mar 2020 at 16:52, Eric Auger <eric.auger@redhat.com> wrote:
> >> The tests themselves are the same as the ISA device ones.
> >> Only the main() changes as the "tpm-tis-device" device gets
> >> instantiated. Also the base address of the device is not
> >> 0xFED40000 anymore but matches the base address of the
> >> ARM virt platform bus.
> >>
> >> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> >> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> > Hi Eric; the commit adding this test is from back in 2020, but I've
> > just noticed something a bit odd about it:
> >
> >> +    args = g_strdup_printf(
> >> +        "-machine virt,gic-version=max -accel tcg "
> >> +        "-chardev socket,id=chr,path=%s "
> >> +        "-tpmdev emulator,id=dev,chardev=chr "
> >> +        "-device tpm-tis-device,tpmdev=dev",
> >> +        test.addr->u.q_unix.path);
> > This 'virt' command line doesn't specify a CPU type, so it
> > will end up running with a Cortex-A15 (32-bit). Was
> > that intended? Also, it will get a GICv3, which is a
> > definitely odd combination with an A15, which was a GICv2 CPU...
> no it is not intended. I guess it should include "-cpu max" too
> as arm-cpu-features.c does?

Seems like a reasonable thing to set, yes.

> > I noticed this because I have some recent GICv3 patches which
> > end up asserting if the GICv3 and a non-GICv3 CPU are used together,
> > and this test case triggers them. Since the user can also cause
> > an assert with that kind of command line I'm going to rework them
> > (either to make the virt board fail cleanly or else to make the
> > GICv3 code do something plausible even if the real hardware CPU
> > nominally didn't have a GICv3). But maybe we should make this
> > test case not use a non-standard combination anyway? (The meson
> > conversion seems to have resulted in this test being run under
> > qemu-system-arm as well, incidentally, so I guess we would want
> > it to specify either 'a 64 bit CPU and GICv3' or 'a 32 bit
> > CPU and GICv2' accordingly. Or limit the test to aarch64...)
> limiting the test to aarch64 may be enough?

Mmm, if running the test under 'qemu-system-arm' isn't giving
us interesting extra coverage we might as well save the CI cycles.

-- PMM