[Qemu-devel] [PATCH v2 for-3.1] hw/arm/sysbus-fdt: Only call match_fn callback if the type matches

[Qemu-devel] [PATCH v2 for-3.1] hw/arm/sysbus-fdt: Only call match_fn callback if the type matches

[Eric Auger]

Commit af7d64ede0b9 (hw/arm/sysbus-fdt: Allow device matching with DT
compatible value) introduced a match_fn callback which gets called
for each registered combo to check whether a sysbus device can be
dynamically instantiated. However the callback gets called even if
the device type does not match the binding combo typename field.
This causes an assert when passing "-device ramfb" to the qemu
command line as vfio_platform_match() gets called on a non
vfio-platform device.

To fix this regression, let's change the add_fdt_node() logic so
that we first check the type and if the match_fn callback is defined,
then we also call it.

Binding combos only requesting a type check do not define the
match_fn callback.

Fixes: af7d64ede0b9 (hw/arm/sysbus-fdt: Allow device matching with
DT compatible value)

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Reported-by: Thomas Huth <thuth@redhat.com>

---

v1 -> v2:
- use "if (!iter->match_fn || iter->match_fn(sbdev, iter)) {"
  as suggested by Peter to avoid code duplication
- mention the ramfb regression fixed by this patch in the
  commit message.
---
 hw/arm/sysbus-fdt.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
index 0e24c803a1..ad698d4832 100644
--- a/hw/arm/sysbus-fdt.c
+++ b/hw/arm/sysbus-fdt.c
@@ -449,7 +449,7 @@ static bool type_match(SysBusDevice *sbdev, const BindingEntry *entry)
     return !strcmp(object_get_typename(OBJECT(sbdev)), entry->typename);
 }
 
-#define TYPE_BINDING(type, add_fn) {(type), NULL, (add_fn), type_match}
+#define TYPE_BINDING(type, add_fn) {(type), NULL, (add_fn), NULL}
 
 /* list of supported dynamic sysbus bindings */
 static const BindingEntry bindings[] = {
@@ -481,10 +481,12 @@ static void add_fdt_node(SysBusDevice *sbdev, void *opaque)
     for (i = 0; i < ARRAY_SIZE(bindings); i++) {
         const BindingEntry *iter = &bindings[i];
 
-        if (iter->match_fn(sbdev, iter)) {
-            ret = iter->add_fn(sbdev, opaque);
-            assert(!ret);
-            return;
+        if (type_match(sbdev, iter)) {
+            if (!iter->match_fn || iter->match_fn(sbdev, iter)) {
+                ret = iter->add_fn(sbdev, opaque);
+                assert(!ret);
+                return;
+            }
         }
     }
     error_report("Device %s can not be dynamically instantiated",
-- 
2.17.2

Re: [Qemu-devel] [PATCH v2 for-3.1] hw/arm/sysbus-fdt: Only call match_fn callback if the type matches

[Geert Uytterhoeven]

Hi Eric,

On Tue, Nov 6, 2018 at 7:42 PM Eric Auger <eric.auger@redhat.com> wrote:
> Commit af7d64ede0b9 (hw/arm/sysbus-fdt: Allow device matching with DT
> compatible value) introduced a match_fn callback which gets called
> for each registered combo to check whether a sysbus device can be
> dynamically instantiated. However the callback gets called even if
> the device type does not match the binding combo typename field.
> This causes an assert when passing "-device ramfb" to the qemu
> command line as vfio_platform_match() gets called on a non
> vfio-platform device.
>
> To fix this regression, let's change the add_fdt_node() logic so
> that we first check the type and if the match_fn callback is defined,
> then we also call it.
>
> Binding combos only requesting a type check do not define the
> match_fn callback.
>
> Fixes: af7d64ede0b9 (hw/arm/sysbus-fdt: Allow device matching with
> DT compatible value)
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Reported-by: Thomas Huth <thuth@redhat.com>
>
> ---
>
> v1 -> v2:
> - use "if (!iter->match_fn || iter->match_fn(sbdev, iter)) {"
>   as suggested by Peter to avoid code duplication
> - mention the ramfb regression fixed by this patch in the
>   commit message.

Thanks for looking into this!

After applying "hw/arm/sysbus-fdt: Add support for instantiating generic
devices", "-device vfio-platform,host=ee300000.sata" still works fine.

Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [Qemu-devel] [PATCH v2 for-3.1] hw/arm/sysbus-fdt: Only call match_fn callback if the type matches

[Alex Williamson]

On Tue,  6 Nov 2018 19:42:12 +0100
Eric Auger <eric.auger@redhat.com> wrote:

> Commit af7d64ede0b9 (hw/arm/sysbus-fdt: Allow device matching with DT
> compatible value) introduced a match_fn callback which gets called
> for each registered combo to check whether a sysbus device can be
> dynamically instantiated. However the callback gets called even if
> the device type does not match the binding combo typename field.
> This causes an assert when passing "-device ramfb" to the qemu
> command line as vfio_platform_match() gets called on a non
> vfio-platform device.
> 
> To fix this regression, let's change the add_fdt_node() logic so
> that we first check the type and if the match_fn callback is defined,
> then we also call it.
> 
> Binding combos only requesting a type check do not define the
> match_fn callback.
> 
> Fixes: af7d64ede0b9 (hw/arm/sysbus-fdt: Allow device matching with
> DT compatible value)
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Reported-by: Thomas Huth <thuth@redhat.com>

The commit this fixes went through the vfio tree, but since the fix
itself only lives in arm code, I'll leave it to arm maintainers to
shepherd this fix and offer:

Reviewed-by: Alex Williamson <alex.williamson@redhat.com>

> ---
> 
> v1 -> v2:
> - use "if (!iter->match_fn || iter->match_fn(sbdev, iter)) {"
>   as suggested by Peter to avoid code duplication
> - mention the ramfb regression fixed by this patch in the
>   commit message.
> ---
>  hw/arm/sysbus-fdt.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
> index 0e24c803a1..ad698d4832 100644
> --- a/hw/arm/sysbus-fdt.c
> +++ b/hw/arm/sysbus-fdt.c
> @@ -449,7 +449,7 @@ static bool type_match(SysBusDevice *sbdev, const BindingEntry *entry)
>      return !strcmp(object_get_typename(OBJECT(sbdev)), entry->typename);
>  }
>  
> -#define TYPE_BINDING(type, add_fn) {(type), NULL, (add_fn), type_match}
> +#define TYPE_BINDING(type, add_fn) {(type), NULL, (add_fn), NULL}
>  
>  /* list of supported dynamic sysbus bindings */
>  static const BindingEntry bindings[] = {
> @@ -481,10 +481,12 @@ static void add_fdt_node(SysBusDevice *sbdev, void *opaque)
>      for (i = 0; i < ARRAY_SIZE(bindings); i++) {
>          const BindingEntry *iter = &bindings[i];
>  
> -        if (iter->match_fn(sbdev, iter)) {
> -            ret = iter->add_fn(sbdev, opaque);
> -            assert(!ret);
> -            return;
> +        if (type_match(sbdev, iter)) {
> +            if (!iter->match_fn || iter->match_fn(sbdev, iter)) {
> +                ret = iter->add_fn(sbdev, opaque);
> +                assert(!ret);
> +                return;
> +            }
>          }
>      }
>      error_report("Device %s can not be dynamically instantiated",

Re: [Qemu-devel] [PATCH v2 for-3.1] hw/arm/sysbus-fdt: Only call match_fn callback if the type matches

[Peter Maydell]

On 7 November 2018 at 16:27, Alex Williamson <alex.williamson@redhat.com> wrote:
> On Tue,  6 Nov 2018 19:42:12 +0100
> Eric Auger <eric.auger@redhat.com> wrote:
>
>> Commit af7d64ede0b9 (hw/arm/sysbus-fdt: Allow device matching with DT
>> compatible value) introduced a match_fn callback which gets called
>> for each registered combo to check whether a sysbus device can be
>> dynamically instantiated. However the callback gets called even if
>> the device type does not match the binding combo typename field.
>> This causes an assert when passing "-device ramfb" to the qemu
>> command line as vfio_platform_match() gets called on a non
>> vfio-platform device.
>>
>> To fix this regression, let's change the add_fdt_node() logic so
>> that we first check the type and if the match_fn callback is defined,
>> then we also call it.
>>
>> Binding combos only requesting a type check do not define the
>> match_fn callback.
>>
>> Fixes: af7d64ede0b9 (hw/arm/sysbus-fdt: Allow device matching with
>> DT compatible value)
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Reported-by: Thomas Huth <thuth@redhat.com>
>
> The commit this fixes went through the vfio tree, but since the fix
> itself only lives in arm code, I'll leave it to arm maintainers to
> shepherd this fix and offer:
>
> Reviewed-by: Alex Williamson <alex.williamson@redhat.com>

Applied to target-arm.next, thanks.

-- PMM