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

[Igor Mammedov <imammedo@redhat.com>]

On Thu, 12 May 2022 17:05:41 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> 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.

agreed,
we probably should limit all ARM tests in bios-tables-test to aarch64
'qemu-system-arm' doesn't give us anything extra on top of what aarch64
already does.

> 
> -- PMM
>