[Qemu-devel] [PATCH] Fix legacy device aliases for s390

[Qemu-devel] [PATCH] Fix legacy device aliases for s390

[Christian Borntraeger]

When qemu is called with -device virtio-serial/blk/net on s390, this alias
is translated to virtio-serial/blk/net-pci instead of s390, since these
drivers are first in the alias table.
Let the core code check if the driver exist, if not lets search further.
This fixes errors like:

qemu-kvm: -device virtio-serial,id=virtio-serial0: Parameter 'driver'
expects device type

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 hw/qdev-monitor.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
index dc4e4e1..8d1b5ba 100644
--- a/hw/qdev-monitor.c
+++ b/hw/qdev-monitor.c
@@ -110,7 +110,8 @@ static const char *find_typename_by_alias(const char *alias)
     int i;
 
     for (i = 0; qdev_alias_table[i].alias; i++) {
-        if (strcmp(qdev_alias_table[i].alias, alias) == 0) {
+        if (strcmp(qdev_alias_table[i].alias, alias) == 0 &&
+            object_class_by_name(qdev_alias_table[i].typename)) {
             return qdev_alias_table[i].typename;
         }
     }
-- 
1.7.9.6

Re: [Qemu-devel] [PATCH] Fix legacy device aliases for s390

[Alexander Graf]

On 03.05.2012, at 10:47, Christian Borntraeger wrote:

> When qemu is called with -device virtio-serial/blk/net on s390, this alias
> is translated to virtio-serial/blk/net-pci instead of s390, since these
> drivers are first in the alias table.
> Let the core code check if the driver exist, if not lets search further.
> This fixes errors like:
> 
> qemu-kvm: -device virtio-serial,id=virtio-serial0: Parameter 'driver'
> expects device type
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>

The usual old fix was to not even compile them in. Why are they in the alias list in the s390 build now?


Alex

Re: [Qemu-devel] [PATCH] Fix legacy device aliases for s390

[Christian Borntraeger]

On 03/05/12 10:49, Alexander Graf wrote:
> 
> On 03.05.2012, at 10:47, Christian Borntraeger wrote:
> 
>> When qemu is called with -device virtio-serial/blk/net on s390, this alias
>> is translated to virtio-serial/blk/net-pci instead of s390, since these
>> drivers are first in the alias table.
>> Let the core code check if the driver exist, if not lets search further.
>> This fixes errors like:
>>
>> qemu-kvm: -device virtio-serial,id=virtio-serial0: Parameter 'driver'
>> expects device type
>>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> The usual old fix was to not even compile them in. Why are they in the alias list in the s390 build now?

Huh? The aliases always worked for me when I compiled qemu myself for s390. This changed with

commit 6acbe4c6f18e7de00481ff30574262b58526de45
Author: Anthony Liguori <aliguori@us.ibm.com>
Date:   Thu Dec 22 11:05:00 2011 -0600

    qdev: remove baked in notion of aliases (v2)

In other word v1.0 is fine, master is not. This looks like a regression, no?

Christian

Re: [Qemu-devel] [PATCH] Fix legacy device aliases for s390

[Alexander Graf]

On 03.05.2012, at 10:53, Christian Borntraeger wrote:

> On 03/05/12 10:49, Alexander Graf wrote:
>> 
>> On 03.05.2012, at 10:47, Christian Borntraeger wrote:
>> 
>>> When qemu is called with -device virtio-serial/blk/net on s390, this alias
>>> is translated to virtio-serial/blk/net-pci instead of s390, since these
>>> drivers are first in the alias table.
>>> Let the core code check if the driver exist, if not lets search further.
>>> This fixes errors like:
>>> 
>>> qemu-kvm: -device virtio-serial,id=virtio-serial0: Parameter 'driver'
>>> expects device type
>>> 
>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> 
>> The usual old fix was to not even compile them in. Why are they in the alias list in the s390 build now?
> 
> Huh? The aliases always worked for me when I compiled qemu myself for s390. This changed with
> 
> commit 6acbe4c6f18e7de00481ff30574262b58526de45
> Author: Anthony Liguori <aliguori@us.ibm.com>
> Date:   Thu Dec 22 11:05:00 2011 -0600
> 
>    qdev: remove baked in notion of aliases (v2)
> 
> In other word v1.0 is fine, master is not. This looks like a regression, no?

Yeah. According to your code however, we have the "virtio-blk-pci" and "virtio-blk-s390" modules both in the alias list, regardless of the architecture. The reason it worked before was that the alias list was specifically built based on qdev devices for each target. Apparently that's changed now.

Anthony, was this change on purpose? If so, Christian's patch is correct and should be applied.


Alex

Re: [Qemu-devel] [PATCH] Fix legacy device aliases for s390

[Paolo Bonzini]

> The usual old fix was to not even compile them in. Why are they in
> the alias list in the s390 build now?

Because the alias list is in target-independent code.

The old fix was brittle anyway, it dependent on the fact that
virtio-blk-pci was not part of libhw.  A similar trick broke for
cirrus-vga when it became part of libhw.

Christian fix is correct.

Paolo

Re: [Qemu-devel] [PATCH] Fix legacy device aliases for s390

[Paolo Bonzini]

> > The usual old fix was to not even compile them in. Why are they in
> > the alias list in the s390 build now?
> 
> Because the alias list is in target-independent code.
> 
> The old fix was brittle anyway, it dependent on the fact that
> virtio-blk-pci was not part of libhw.  A similar trick broke for
> cirrus-vga when it became part of libhw.
> 
> Christian fix is correct.

Uhm, Christian fix would have the same problem actually if
virtio-*-pci were to be moved in libhw.  IIRC I proposed the
same change on review and Anthony nacked it on these grounds.
You could move the alias list to target-dependent code, though.

Paolo

Re: [Qemu-devel] [PATCH] Fix legacy device aliases for s390

[Alexander Graf]

On 03.05.2012, at 11:05, Paolo Bonzini wrote:

> 
>>> The usual old fix was to not even compile them in. Why are they in
>>> the alias list in the s390 build now?
>> 
>> Because the alias list is in target-independent code.
>> 
>> The old fix was brittle anyway, it dependent on the fact that
>> virtio-blk-pci was not part of libhw.  A similar trick broke for
>> cirrus-vga when it became part of libhw.
>> 
>> Christian fix is correct.
> 
> Uhm, Christian fix would have the same problem actually if
> virtio-*-pci were to be moved in libhw.  IIRC I proposed the
> same change on review and Anthony nacked it on these grounds.
> You could move the alias list to target-dependent code, though.

Can't we just make the virtio-*-pci variants fail instantiation and based on that search the list on?


Alex

Re: [Qemu-devel] [PATCH] Fix legacy device aliases for s390

[Paolo Bonzini]

> > Uhm, Christian fix would have the same problem actually if
> > virtio-*-pci were to be moved in libhw.  IIRC I proposed the
> > same change on review and Anthony nacked it on these grounds.
> > You could move the alias list to target-dependent code, though.
> 
> Can't we just make the virtio-*-pci variants fail instantiation and
> based on that search the list on?

The alias list is used in other places than just instantiation
(e.g. -device ?).  If it were me I'd just go with Christian's patch;
it is conceptually broken, but in the same way as QEMU 1.0 (i.e.
relies on virtio-*-pci not being compiled into s390 binaries).  A
comment will do.

Paolo

Re: [Qemu-devel] [PATCH] Fix legacy device aliases for s390

[Anthony Liguori]

On 05/03/2012 04:07 AM, Alexander Graf wrote:
>
> On 03.05.2012, at 11:05, Paolo Bonzini wrote:
>
>>
>>>> The usual old fix was to not even compile them in. Why are they in
>>>> the alias list in the s390 build now?
>>>
>>> Because the alias list is in target-independent code.
>>>
>>> The old fix was brittle anyway, it dependent on the fact that
>>> virtio-blk-pci was not part of libhw.  A similar trick broke for
>>> cirrus-vga when it became part of libhw.
>>>
>>> Christian fix is correct.
>>
>> Uhm, Christian fix would have the same problem actually if
>> virtio-*-pci were to be moved in libhw.  IIRC I proposed the
>> same change on review and Anthony nacked it on these grounds.
>> You could move the alias list to target-dependent code, though.
>
> Can't we just make the virtio-*-pci variants fail instantiation and based on that search the list on?

No, but you could do:

diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
index dc4e4e1..f90966f 100644
--- a/hw/qdev-monitor.c
+++ b/hw/qdev-monitor.c
@@ -29,16 +29,17 @@ typedef struct QDevAlias
  {
      const char *typename;
      const char *alias;
+    uint32_t arch_mask;
  } QDevAlias;

  static const QDevAlias qdev_alias_table[] = {
-    { "virtio-blk-pci", "virtio-blk" },
-    { "virtio-net-pci", "virtio-net" },
-    { "virtio-serial-pci", "virtio-serial" },
-    { "virtio-balloon-pci", "virtio-balloon" },
-    { "virtio-blk-s390", "virtio-blk" },
-    { "virtio-net-s390", "virtio-net" },
-    { "virtio-serial-s390", "virtio-serial" },
+    { "virtio-blk-pci", "virtio-blk", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X },
+    { "virtio-net-pci", "virtio-net", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X },
+    { "virtio-serial-pci", "virtio-serial", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X },
+    { "virtio-balloon-pci", "virtio-balloon", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X },
+    { "virtio-blk-s390", "virtio-blk", QEMU_ARCH_S390X },
+    { "virtio-net-s390", "virtio-net", QEMU_ARCH_S390X },
+    { "virtio-serial-s390", "virtio-serial", QEMU_ARCH_S390X },
      { "lsi53c895a", "lsi" },
      { "ich9-ahci", "ahci" },
      { }
@@ -110,6 +111,11 @@ static const char *find_typename_by_alias(const char *alias)
      int i;

      for (i = 0; qdev_alias_table[i].alias; i++) {
+        if (qdev_alias_table[i].mask &&
+            !(qdev_alias_table[i].mask & arch_type)) {
+            continue;
+        }
+
          if (strcmp(qdev_alias_table[i].alias, alias) == 0) {
              return qdev_alias_table[i].typename;
          }


Not tested at all as I'm on holiday but if you wanted to take this patch and run 
with it, I think that would be a reasonable approach.

Regards,

Anthony Liguori

>
>
> Alex
>

Re: [Qemu-devel] [PATCH] Fix legacy device aliases for s390

[Alexander Graf]

On 03.05.2012, at 14:32, Anthony Liguori wrote:

> On 05/03/2012 04:07 AM, Alexander Graf wrote:
>> 
>> On 03.05.2012, at 11:05, Paolo Bonzini wrote:
>> 
>>> 
>>>>> The usual old fix was to not even compile them in. Why are they in
>>>>> the alias list in the s390 build now?
>>>> 
>>>> Because the alias list is in target-independent code.
>>>> 
>>>> The old fix was brittle anyway, it dependent on the fact that
>>>> virtio-blk-pci was not part of libhw.  A similar trick broke for
>>>> cirrus-vga when it became part of libhw.
>>>> 
>>>> Christian fix is correct.
>>> 
>>> Uhm, Christian fix would have the same problem actually if
>>> virtio-*-pci were to be moved in libhw.  IIRC I proposed the
>>> same change on review and Anthony nacked it on these grounds.
>>> You could move the alias list to target-dependent code, though.
>> 
>> Can't we just make the virtio-*-pci variants fail instantiation and based on that search the list on?
> 
> No, but you could do:

Ah, nice. Here is a fixed (and tested) version:


diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
index dc4e4e1..f7a03fe 100644
--- a/hw/qdev-monitor.c
+++ b/hw/qdev-monitor.c
@@ -20,6 +20,7 @@
 #include "qdev.h"
 #include "monitor.h"
 #include "qmp-commands.h"
+#include "arch_init.h"
 
 /*
  * Aliases were a bad idea from the start.  Let's keep them
@@ -29,16 +30,18 @@ typedef struct QDevAlias
 {
     const char *typename;
     const char *alias;
+    uint32_t arch_mask;
 } QDevAlias;
 
 static const QDevAlias qdev_alias_table[] = {
-    { "virtio-blk-pci", "virtio-blk" },
-    { "virtio-net-pci", "virtio-net" },
-    { "virtio-serial-pci", "virtio-serial" },
-    { "virtio-balloon-pci", "virtio-balloon" },
-    { "virtio-blk-s390", "virtio-blk" },
-    { "virtio-net-s390", "virtio-net" },
-    { "virtio-serial-s390", "virtio-serial" },
+    { "virtio-blk-pci", "virtio-blk", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X },
+    { "virtio-net-pci", "virtio-net", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X },
+    { "virtio-serial-pci", "virtio-serial", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X },
+    { "virtio-balloon-pci", "virtio-balloon",
+            QEMU_ARCH_ALL & ~QEMU_ARCH_S390X },
+    { "virtio-blk-s390", "virtio-blk", QEMU_ARCH_S390X },
+    { "virtio-net-s390", "virtio-net", QEMU_ARCH_S390X },
+    { "virtio-serial-s390", "virtio-serial", QEMU_ARCH_S390X },
     { "lsi53c895a", "lsi" },
     { "ich9-ahci", "ahci" },
     { }
@@ -50,6 +53,11 @@ static const char *qdev_class_get_alias(DeviceClass *dc)
     int i;
 
     for (i = 0; qdev_alias_table[i].typename; i++) {
+        if (qdev_alias_table[i].arch_mask &&
+            !(qdev_alias_table[i].arch_mask & arch_type)) {
+            continue;
+        }
+
         if (strcmp(qdev_alias_table[i].typename, typename) == 0) {
             return qdev_alias_table[i].alias;
         }
@@ -110,6 +118,11 @@ static const char *find_typename_by_alias(const char *alias)
     int i;
 
     for (i = 0; qdev_alias_table[i].alias; i++) {
+        if (qdev_alias_table[i].arch_mask &&
+            !(qdev_alias_table[i].arch_mask & arch_type)) {
+            continue;
+        }
+
         if (strcmp(qdev_alias_table[i].alias, alias) == 0) {
             return qdev_alias_table[i].typename;
         }

Re: [Qemu-devel] [PATCH] Fix legacy device aliases for s390

[Alexander Graf]

On 03.05.2012, at 15:34, Alexander Graf wrote:

> 
> On 03.05.2012, at 14:32, Anthony Liguori wrote:
> 
>> On 05/03/2012 04:07 AM, Alexander Graf wrote:
>>> 
>>> On 03.05.2012, at 11:05, Paolo Bonzini wrote:
>>> 
>>>> 
>>>>>> The usual old fix was to not even compile them in. Why are they in
>>>>>> the alias list in the s390 build now?
>>>>> 
>>>>> Because the alias list is in target-independent code.
>>>>> 
>>>>> The old fix was brittle anyway, it dependent on the fact that
>>>>> virtio-blk-pci was not part of libhw.  A similar trick broke for
>>>>> cirrus-vga when it became part of libhw.
>>>>> 
>>>>> Christian fix is correct.
>>>> 
>>>> Uhm, Christian fix would have the same problem actually if
>>>> virtio-*-pci were to be moved in libhw.  IIRC I proposed the
>>>> same change on review and Anthony nacked it on these grounds.
>>>> You could move the alias list to target-dependent code, though.
>>> 
>>> Can't we just make the virtio-*-pci variants fail instantiation and based on that search the list on?
>> 
>> No, but you could do:
> 
> Ah, nice. Here is a fixed (and tested) version:

Ping? What do we do about this one?


Alex

Re: [Qemu-devel] [PATCH] Fix legacy device aliases for s390

[Anthony Liguori]

On 05/17/2012 05:57 PM, Alexander Graf wrote:
>
> On 03.05.2012, at 15:34, Alexander Graf wrote:
>
>>
>> On 03.05.2012, at 14:32, Anthony Liguori wrote:
>>
>>> On 05/03/2012 04:07 AM, Alexander Graf wrote:
>>>>
>>>> On 03.05.2012, at 11:05, Paolo Bonzini wrote:
>>>>
>>>>>
>>>>>>> The usual old fix was to not even compile them in. Why are they in
>>>>>>> the alias list in the s390 build now?
>>>>>>
>>>>>> Because the alias list is in target-independent code.
>>>>>>
>>>>>> The old fix was brittle anyway, it dependent on the fact that
>>>>>> virtio-blk-pci was not part of libhw.  A similar trick broke for
>>>>>> cirrus-vga when it became part of libhw.
>>>>>>
>>>>>> Christian fix is correct.
>>>>>
>>>>> Uhm, Christian fix would have the same problem actually if
>>>>> virtio-*-pci were to be moved in libhw.  IIRC I proposed the
>>>>> same change on review and Anthony nacked it on these grounds.
>>>>> You could move the alias list to target-dependent code, though.
>>>>
>>>> Can't we just make the virtio-*-pci variants fail instantiation and based on that search the list on?
>>>
>>> No, but you could do:
>>
>> Ah, nice. Here is a fixed (and tested) version:
>
> Ping? What do we do about this one?

Are you going to submit your fixed and tested version as a proper patch?

You can add my SoB if you need to.

Regards,

Anthony Liguori

>
>
> Alex
>