diff mbox series

[2/2] tests/qtest: Special case sbsa-ref and xlnx-versal-virt if !CONFIG_ARM_GIC_TCG

Message ID 20220131154531.429533-3-eric.auger@redhat.com
State New
Headers show
Series hw/arm/virt, qtests: Fix make check-qtest-aarch64 when CONFIG_ARM_GIC_TCG is unset | expand

Commit Message

Eric Auger Jan. 31, 2022, 3:45 p.m. UTC 
qom-test and test-hmp shall not run tests on sbsa-ref and
xlnx-versal-virt if CONFIG_ARM_GIC_TCG is unset as those machines
always instantiate GICv3.

Otherwise the tests fail with
ERROR:../qom/object.c:715:object_new_with_type: assertion failed: (type != NULL)

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Fixes: a8a5546798c3 ("hw/intc/arm_gicv3: Introduce CONFIG_ARM_GIC_TCG Kconfig selector")
---
 tests/qtest/libqtest.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Andrew Jones Jan. 31, 2022, 3:59 p.m. UTC | #1 
On Mon, Jan 31, 2022 at 04:45:31PM +0100, Eric Auger wrote:
> qom-test and test-hmp shall not run tests on sbsa-ref and
> xlnx-versal-virt if CONFIG_ARM_GIC_TCG is unset as those machines
> always instantiate GICv3.
> 
> Otherwise the tests fail with
> ERROR:../qom/object.c:715:object_new_with_type: assertion failed: (type != NULL)
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Fixes: a8a5546798c3 ("hw/intc/arm_gicv3: Introduce CONFIG_ARM_GIC_TCG Kconfig selector")
> ---
>  tests/qtest/libqtest.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
> index 41f4da4e54..f53983a28e 100644
> --- a/tests/qtest/libqtest.c
> +++ b/tests/qtest/libqtest.c
> @@ -1394,6 +1394,12 @@ void qtest_cb_for_every_machine(void (*cb)(const char *machine),
>              g_str_equal("xenpv", machines[i].name)) {
>              continue;
>          }
> +#ifndef CONFIG_ARM_GIC_TCG
> +        if (!strncmp("sbsa-ref", machines[i].name, 8) ||
> +            !strncmp("xlnx-versal-virt", machines[i].name, 16)) {
> +            continue;
> +        }
> +#endif

Hmm, if these machine types completely depend on userspace gicv3
emulation, i.e. no way to use in-kernel gic or another tcg gic
model, then I guess they shouldn't be built at all when ARM_GIC_TCG
isn't configured. I.e.

diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 2e0049196d6c..d7cc028b049d 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -209,6 +209,7 @@ config REALVIEW
 
 config SBSA_REF
     bool
+    depends on ARM_GIC_TCG
     imply PCI_DEVICES
     select AHCI
     select ARM_SMMUV3
@@ -378,6 +379,7 @@ config XLNX_ZYNQMP_ARM
 
 config XLNX_VERSAL
     bool
+    depends on ARM_GIC_TCG
     select ARM_GIC
     select PL011
     select CADENCE


Thanks,
drew

>          if (!skip_old_versioned ||
>              !qtest_is_old_versioned_machine(machines[i].name)) {
>              cb(machines[i].name);
> -- 
> 2.26.3
>
Peter Maydell Jan. 31, 2022, 4:05 p.m. UTC | #2 
On Mon, 31 Jan 2022 at 15:59, Andrew Jones <drjones@redhat.com> wrote:
> Hmm, if these machine types completely depend on userspace gicv3
> emulation, i.e. no way to use in-kernel gic or another tcg gic
> model, then I guess they shouldn't be built at all when ARM_GIC_TCG
> isn't configured. I.e.
>
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index 2e0049196d6c..d7cc028b049d 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -209,6 +209,7 @@ config REALVIEW
>
>  config SBSA_REF
>      bool
> +    depends on ARM_GIC_TCG
>      imply PCI_DEVICES
>      select AHCI
>      select ARM_SMMUV3
> @@ -378,6 +379,7 @@ config XLNX_ZYNQMP_ARM
>
>  config XLNX_VERSAL
>      bool
> +    depends on ARM_GIC_TCG
>      select ARM_GIC
>      select PL011
>      select CADENCE

I kind of agree, but isn't this kind of mixing two things?

(1) Both these machines require a GICv3 and a GICv2 won't do,
so they should do something that says "if you want this
machine type, you need a GICv3 device"

(2) Both these machines don't work with KVM or hvf, so if we're
not building TCG then there's no point configuring in these
machine models (a property they share with every other arm
machine type except "virt", currently)

thanks
-- PMM
Andrew Jones Jan. 31, 2022, 4:14 p.m. UTC | #3 
On Mon, Jan 31, 2022 at 04:05:06PM +0000, Peter Maydell wrote:
> On Mon, 31 Jan 2022 at 15:59, Andrew Jones <drjones@redhat.com> wrote:
> > Hmm, if these machine types completely depend on userspace gicv3
> > emulation, i.e. no way to use in-kernel gic or another tcg gic
> > model, then I guess they shouldn't be built at all when ARM_GIC_TCG
> > isn't configured. I.e.
> >
> > diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> > index 2e0049196d6c..d7cc028b049d 100644
> > --- a/hw/arm/Kconfig
> > +++ b/hw/arm/Kconfig
> > @@ -209,6 +209,7 @@ config REALVIEW
> >
> >  config SBSA_REF
> >      bool
> > +    depends on ARM_GIC_TCG
> >      imply PCI_DEVICES
> >      select AHCI
> >      select ARM_SMMUV3
> > @@ -378,6 +379,7 @@ config XLNX_ZYNQMP_ARM
> >
> >  config XLNX_VERSAL
> >      bool
> > +    depends on ARM_GIC_TCG
> >      select ARM_GIC
> >      select PL011
> >      select CADENCE
> 
> I kind of agree, but isn't this kind of mixing two things?

How about two dependencies?

> 
> (1) Both these machines require a GICv3 and a GICv2 won't do,
> so they should do something that says "if you want this
> machine type, you need a GICv3 device"

depends on ARM_GIC_TCG   (IMO, could use a rename to be gicv3 specific)

> 
> (2) Both these machines don't work with KVM or hvf, so if we're
> not building TCG then there's no point configuring in these
> machine models (a property they share with every other arm
> machine type except "virt", currently)

depends on TCG

Thanks,
drew
Eric Auger Jan. 31, 2022, 4:15 p.m. UTC | #4 
Hi Drew,

On 1/31/22 4:59 PM, Andrew Jones wrote:
> On Mon, Jan 31, 2022 at 04:45:31PM +0100, Eric Auger wrote:
>> qom-test and test-hmp shall not run tests on sbsa-ref and
>> xlnx-versal-virt if CONFIG_ARM_GIC_TCG is unset as those machines
>> always instantiate GICv3.
>>
>> Otherwise the tests fail with
>> ERROR:../qom/object.c:715:object_new_with_type: assertion failed: (type != NULL)
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Fixes: a8a5546798c3 ("hw/intc/arm_gicv3: Introduce CONFIG_ARM_GIC_TCG Kconfig selector")
>> ---
>>  tests/qtest/libqtest.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
>> index 41f4da4e54..f53983a28e 100644
>> --- a/tests/qtest/libqtest.c
>> +++ b/tests/qtest/libqtest.c
>> @@ -1394,6 +1394,12 @@ void qtest_cb_for_every_machine(void (*cb)(const char *machine),
>>              g_str_equal("xenpv", machines[i].name)) {
>>              continue;
>>          }
>> +#ifndef CONFIG_ARM_GIC_TCG
>> +        if (!strncmp("sbsa-ref", machines[i].name, 8) ||
>> +            !strncmp("xlnx-versal-virt", machines[i].name, 16)) {
>> +            continue;
>> +        }
>> +#endif
> Hmm, if these machine types completely depend on userspace gicv3
> emulation, i.e. no way to use in-kernel gic or another tcg gic
> model, then I guess they shouldn't be built at all when ARM_GIC_TCG
> isn't configured. I.e.

Yes your suggestion make sense to me, thatw would be a better fix indeed.

Eric
>
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index 2e0049196d6c..d7cc028b049d 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -209,6 +209,7 @@ config REALVIEW
>  
>  config SBSA_REF
>      bool
> +    depends on ARM_GIC_TCG
>      imply PCI_DEVICES
>      select AHCI
>      select ARM_SMMUV3
> @@ -378,6 +379,7 @@ config XLNX_ZYNQMP_ARM
>  
>  config XLNX_VERSAL
>      bool
> +    depends on ARM_GIC_TCG
>      select ARM_GIC
>      select PL011
>      select CADENCE
>
>
> Thanks,
> drew
>
>>          if (!skip_old_versioned ||
>>              !qtest_is_old_versioned_machine(machines[i].name)) {
>>              cb(machines[i].name);
>> -- 
>> 2.26.3
>>
Peter Maydell Jan. 31, 2022, 4:18 p.m. UTC | #5 
On Mon, 31 Jan 2022 at 16:14, Andrew Jones <drjones@redhat.com> wrote:
>
> On Mon, Jan 31, 2022 at 04:05:06PM +0000, Peter Maydell wrote:
> > On Mon, 31 Jan 2022 at 15:59, Andrew Jones <drjones@redhat.com> wrote:
> > > Hmm, if these machine types completely depend on userspace gicv3
> > > emulation, i.e. no way to use in-kernel gic or another tcg gic
> > > model, then I guess they shouldn't be built at all when ARM_GIC_TCG
> > > isn't configured. I.e.
> > >
> > > diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> > > index 2e0049196d6c..d7cc028b049d 100644
> > > --- a/hw/arm/Kconfig
> > > +++ b/hw/arm/Kconfig
> > > @@ -209,6 +209,7 @@ config REALVIEW
> > >
> > >  config SBSA_REF
> > >      bool
> > > +    depends on ARM_GIC_TCG
> > >      imply PCI_DEVICES
> > >      select AHCI
> > >      select ARM_SMMUV3
> > > @@ -378,6 +379,7 @@ config XLNX_ZYNQMP_ARM
> > >
> > >  config XLNX_VERSAL
> > >      bool
> > > +    depends on ARM_GIC_TCG
> > >      select ARM_GIC
> > >      select PL011
> > >      select CADENCE
> >
> > I kind of agree, but isn't this kind of mixing two things?
>
> How about two dependencies?
>
> >
> > (1) Both these machines require a GICv3 and a GICv2 won't do,
> > so they should do something that says "if you want this
> > machine type, you need a GICv3 device"
>
> depends on ARM_GIC_TCG   (IMO, could use a rename to be gicv3 specific)

ARM_GIC_TCG has a "depends on ARM_GIC && TCG", though.
"I need a GICv3" ought in principle to be satisfiable by
the KVM GICv3.

Part of the problem here is that we've let ARM_GIC mean both
GICv2 and GICv3.

> > (2) Both these machines don't work with KVM or hvf, so if we're
> > not building TCG then there's no point configuring in these
> > machine models (a property they share with every other arm
> > machine type except "virt", currently)
>
> depends on TCG

I would prefer a way of phrasing this that only required
us to say it in the one machine that can handle not-TCG
(virt) rather than in the large number of machines that cannot...

-- PMM
Eric Auger Jan. 31, 2022, 4:18 p.m. UTC | #6 
Hi Drew,

On 1/31/22 5:14 PM, Andrew Jones wrote:
> On Mon, Jan 31, 2022 at 04:05:06PM +0000, Peter Maydell wrote:
>> On Mon, 31 Jan 2022 at 15:59, Andrew Jones <drjones@redhat.com> wrote:
>>> Hmm, if these machine types completely depend on userspace gicv3
>>> emulation, i.e. no way to use in-kernel gic or another tcg gic
>>> model, then I guess they shouldn't be built at all when ARM_GIC_TCG
>>> isn't configured. I.e.
>>>
>>> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
>>> index 2e0049196d6c..d7cc028b049d 100644
>>> --- a/hw/arm/Kconfig
>>> +++ b/hw/arm/Kconfig
>>> @@ -209,6 +209,7 @@ config REALVIEW
>>>
>>>  config SBSA_REF
>>>      bool
>>> +    depends on ARM_GIC_TCG
>>>      imply PCI_DEVICES
>>>      select AHCI
>>>      select ARM_SMMUV3
>>> @@ -378,6 +379,7 @@ config XLNX_ZYNQMP_ARM
>>>
>>>  config XLNX_VERSAL
>>>      bool
>>> +    depends on ARM_GIC_TCG
>>>      select ARM_GIC
>>>      select PL011
>>>      select CADENCE
>> I kind of agree, but isn't this kind of mixing two things?
> How about two dependencies?
>
>> (1) Both these machines require a GICv3 and a GICv2 won't do,
>> so they should do something that says "if you want this
>> machine type, you need a GICv3 device"
> depends on ARM_GIC_TCG   (IMO, could use a rename to be gicv3 specific)
Yep I think it would be clearer to rename the CONFIG.
>
>> (2) Both these machines don't work with KVM or hvf, so if we're
>> not building TCG then there's no point configuring in these
>> machine models (a property they share with every other arm
>> machine type except "virt", currently)
> depends on TCG

That's what I would be inclined to do as well

Thanks

Eric
>
> Thanks,
> drew
>
diff mbox series

Patch

diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index 41f4da4e54..f53983a28e 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -1394,6 +1394,12 @@  void qtest_cb_for_every_machine(void (*cb)(const char *machine),
             g_str_equal("xenpv", machines[i].name)) {
             continue;
         }
+#ifndef CONFIG_ARM_GIC_TCG
+        if (!strncmp("sbsa-ref", machines[i].name, 8) ||
+            !strncmp("xlnx-versal-virt", machines[i].name, 16)) {
+            continue;
+        }
+#endif
         if (!skip_old_versioned ||
             !qtest_is_old_versioned_machine(machines[i].name)) {
             cb(machines[i].name);