Patchwork Fix legacy device aliases for s390

login
register
mail settings
Submitter Christian Borntraeger
Date May 3, 2012, 8:47 a.m.
Message ID <1336034836-17892-1-git-send-email-borntraeger@de.ibm.com>
Download mbox | patch
Permalink /patch/156628/
State New
Headers show

Comments

Christian Borntraeger - May 3, 2012, 8:47 a.m.
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(-)
Alexander Graf - May 3, 2012, 8:49 a.m.
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
Christian Borntraeger - May 3, 2012, 8:53 a.m.
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
Alexander Graf - May 3, 2012, 8:55 a.m.
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
Paolo Bonzini - May 3, 2012, 9:03 a.m.
> 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
Paolo Bonzini - May 3, 2012, 9:05 a.m.
> > 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
Alexander Graf - May 3, 2012, 9:07 a.m.
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
Paolo Bonzini - May 3, 2012, 9:16 a.m.
> > 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

Patch

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;
         }
     }