[Qemu-devel] isa-applesmc doesn't handle second instantiation gracefully

[Qemu-devel] isa-applesmc doesn't handle second instantiation gracefully

[Markus Armbruster]

When I try -device isa-applesmc -device isa-applesmc, I get

    WARNING: Using AppleSMC with invalid key
    qemu: hardware error: register_ioport_read: invalid opaque
    [...]

and a core dump.

I know nothing about this device.  Instantiating twice may well make no
sense.  But hw_error() is not a nice way to reject a command line
option.

[Qemu-devel] Re: isa-applesmc doesn't handle second instantiation gracefully

[Alexander Graf]

On 12.10.2010, at 14:50, Markus Armbruster wrote:

> When I try -device isa-applesmc -device isa-applesmc, I get
> 
>    WARNING: Using AppleSMC with invalid key
>    qemu: hardware error: register_ioport_read: invalid opaque
>    [...]
> 
> and a core dump.
> 
> I know nothing about this device.  Instantiating twice may well make no
> sense.  But hw_error() is not a nice way to reject a command line
> option.

I'd guess it fails because it defaults to the same port twice, so allocating it twice would put two ISA devices on the same PIO port which doesn't work.

I don't think there's any way around this, or is there?


Alex

[Qemu-devel] qdev: Some ISA devices don't handle second instantiation gracefully (was: isa-applesmc doesn't handle second instantiation gracefully)

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> When I try -device isa-applesmc -device isa-applesmc, I get
>
>     WARNING: Using AppleSMC with invalid key
>     qemu: hardware error: register_ioport_read: invalid opaque
>     [...]
>
> and a core dump.
>
> I know nothing about this device.  Instantiating twice may well make no
> sense.  But hw_error() is not a nice way to reject a command line
> option.

Actually, ib700 and isa-debugcon fail the same way.

They call register_ioport_write(), which aborts via hw_error() when the
port is already in use.  This is okay for non-configurable parts of a
board emulation, but not okay for a qdev device, unless it has no_user
set.

Related: when isa_init_irq() finds the requested IRQ already in use, it
fails with exit(1).  Maybe register_ioport_write() & friends should do
that as well.

Or maybe qdev device models should have an "at most one" flag.

[Qemu-devel] Re: qdev: Some ISA devices don't handle second instantiation gracefully (was: isa-applesmc doesn't handle second instantiation gracefully)

[Alexander Graf]

On 12.10.2010, at 15:00, Markus Armbruster wrote:

> Markus Armbruster <armbru@redhat.com> writes:
> 
>> When I try -device isa-applesmc -device isa-applesmc, I get
>> 
>>    WARNING: Using AppleSMC with invalid key
>>    qemu: hardware error: register_ioport_read: invalid opaque
>>    [...]
>> 
>> and a core dump.
>> 
>> I know nothing about this device.  Instantiating twice may well make no
>> sense.  But hw_error() is not a nice way to reject a command line
>> option.
> 
> Actually, ib700 and isa-debugcon fail the same way.
> 
> They call register_ioport_write(), which aborts via hw_error() when the
> port is already in use.  This is okay for non-configurable parts of a
> board emulation, but not okay for a qdev device, unless it has no_user
> set.
> 
> Related: when isa_init_irq() finds the requested IRQ already in use, it
> fails with exit(1).  Maybe register_ioport_write() & friends should do
> that as well.
> 
> Or maybe qdev device models should have an "at most one" flag.

There can be multiple of these devices on different ports, but a single PIO port can still only be managed by a single device. By creating the same device twice without giving it a PIO option, you put two devices on the same PIO port which can't work.

As this is a configuration error, I'd guess the best way to deal with it is to return an error for register_ioport which makes the qdev init fail. Then the respective instantiator can do what is fit. If the device is passed in on the cmdline, it'd refuse to create the machine and exit. If it's a hotplug event, it would fail to hotplug.


Alex

[Qemu-devel] Re: qdev: Some ISA devices don't handle second instantiation gracefully

[Gerd Hoffmann]

   Hi,

>> They call register_ioport_write(), which aborts via hw_error() when
>> the port is already in use.  This is okay for non-configurable
>> parts of a board emulation, but not okay for a qdev device, unless
>> it has no_user set.
>>
>> Related: when isa_init_irq() finds the requested IRQ already in
>> use, it fails with exit(1).  Maybe register_ioport_write()&
>> friends should do that as well.
>>
>> Or maybe qdev device models should have an "at most one" flag.
>
> There can be multiple of these devices on different ports, but a
> single PIO port can still only be managed by a single device. By
> creating the same device twice without giving it a PIO option, you
> put two devices on the same PIO port which can't work.
>
> As this is a configuration error, I'd guess the best way to deal with
> it is to return an error for register_ioport which makes the qdev
> init fail. Then the respective instantiator can do what is fit. If
> the device is passed in on the cmdline, it'd refuse to create the
> machine and exit. If it's a hotplug event, it would fail to hotplug.

ISA isn't hotpluggable anyway, so just exiting unconditionally here 
isn't a big issue IMHO as effectively only ISA devices are affected by 
this.

register_ioport_write() should exit with a more useful error message 
here instead of calling hw_error() though.

We could also make register_ioport_write() pass up an error and have all 
callers check that.  Not sure this is worth the trouble though.

An "at most one" flag might make sense.  But on a PC all ISA devices 
which exist only once at a fixed address (keyboard, floppy, ...) are 
automagically created so no_user does the trick here.  The other ones 
(serial, ne2k_isa, ...) actually can be created multiple times if you 
configure different iobases using properties.

Dunno what kind of device apple-smc is ...

cheers,
   Gerd

Re: [Qemu-devel] qdev: Some ISA devices don't handle second instantiation gracefully

[Anthony Liguori]

On 10/12/2010 08:00 AM, Markus Armbruster wrote:
> Markus Armbruster<armbru@redhat.com>  writes:
>
>    
>> When I try -device isa-applesmc -device isa-applesmc, I get
>>
>>      WARNING: Using AppleSMC with invalid key
>>      qemu: hardware error: register_ioport_read: invalid opaque
>>      [...]
>>
>> and a core dump.
>>
>> I know nothing about this device.  Instantiating twice may well make no
>> sense.  But hw_error() is not a nice way to reject a command line
>> option.
>>      
> Actually, ib700 and isa-debugcon fail the same way.
>
> They call register_ioport_write(), which aborts via hw_error() when the
> port is already in use.  This is okay for non-configurable parts of a
> board emulation, but not okay for a qdev device, unless it has no_user
> set.
>    

It's definitely right to fail but I agree, it's better to propagate the 
error.

> Related: when isa_init_irq() finds the requested IRQ already in use, it
> fails with exit(1).  Maybe register_ioport_write()&  friends should do
> that as well.
>
> Or maybe qdev device models should have an "at most one" flag.
>    

I think the proper thing to do is remove all exit(1)s and propagate 
errors instead.

A simple approach would be to make register_ioport_{read,write}() return 
an int, then do a query-replace on the source tree to make all 
invocations of it simply check the return value and exit if it's non-zero.

Regards,

Anthony Liguori

Re: [Qemu-devel] qdev: Some ISA devices don't handle second instantiation gracefully

[Markus Armbruster]

Anthony Liguori <anthony@codemonkey.ws> writes:

> On 10/12/2010 08:00 AM, Markus Armbruster wrote:
>> Markus Armbruster<armbru@redhat.com>  writes:
>>
>>    
>>> When I try -device isa-applesmc -device isa-applesmc, I get
>>>
>>>      WARNING: Using AppleSMC with invalid key
>>>      qemu: hardware error: register_ioport_read: invalid opaque
>>>      [...]
>>>
>>> and a core dump.
>>>
>>> I know nothing about this device.  Instantiating twice may well make no
>>> sense.  But hw_error() is not a nice way to reject a command line
>>> option.
>>>      
>> Actually, ib700 and isa-debugcon fail the same way.
>>
>> They call register_ioport_write(), which aborts via hw_error() when the
>> port is already in use.  This is okay for non-configurable parts of a
>> board emulation, but not okay for a qdev device, unless it has no_user
>> set.
>>    
>
> It's definitely right to fail but I agree, it's better to propagate
> the error.
>
>> Related: when isa_init_irq() finds the requested IRQ already in use, it
>> fails with exit(1).  Maybe register_ioport_write()&  friends should do
>> that as well.
>>
>> Or maybe qdev device models should have an "at most one" flag.
>>    
>
> I think the proper thing to do is remove all exit(1)s and propagate
> errors instead.

exit() is good enough during startup, i.e. -device.  It's wrong for hot
plug; anything to be used in a hot plug path must propagate errors.  We
could keep exiting in code that's only used by non-hotpluggable devices.

> A simple approach would be to make register_ioport_{read,write}()
> return an int, then do a query-replace on the source tree to make all
> invocations of it simply check the return value and exit if it's
> non-zero.

In similar cases, we've used a simple FOO_nofail() wrapper in places
that want to exit.

I'll see what I can do.

Re: [Qemu-devel] qdev: Some ISA devices don't handle second instantiation gracefully

[Anthony Liguori]

On 10/12/2010 08:54 AM, Markus Armbruster wrote:
>
>> I think the proper thing to do is remove all exit(1)s and propagate
>> errors instead.
>>      
> exit() is good enough during startup, i.e. -device.  It's wrong for hot
> plug; anything to be used in a hot plug path must propagate errors.  We
> could keep exiting in code that's only used by non-hotpluggable devices.
>    

I'm not really suggesting that we move exit()s out of all devices right 
now.  Am just suggesting that we bump them up one level.

>> A simple approach would be to make register_ioport_{read,write}()
>> return an int, then do a query-replace on the source tree to make all
>> invocations of it simply check the return value and exit if it's
>> non-zero.
>>      
> In similar cases, we've used a simple FOO_nofail() wrapper in places
> that want to exit.
>
> I'll see what I can do.
>    

I prefer propagating the exit but am not deeply opposed to nofail().

Regards,

Anthony Liguori

[Qemu-devel] Re: qdev: Some ISA devices don't handle second instantiation gracefully (was: isa-applesmc doesn't handle second instantiation gracefully)

[Richard W.M. Jones]

On Tue, Oct 12, 2010 at 03:00:13PM +0200, Markus Armbruster wrote:
> Markus Armbruster <armbru@redhat.com> writes:
> 
> > When I try -device isa-applesmc -device isa-applesmc, I get
> >
> >     WARNING: Using AppleSMC with invalid key
> >     qemu: hardware error: register_ioport_read: invalid opaque
> >     [...]
> >
> > and a core dump.
> >
> > I know nothing about this device.  Instantiating twice may well make no
> > sense.  But hw_error() is not a nice way to reject a command line
> > option.
> 
> Actually, ib700 and isa-debugcon fail the same way.

I will just comment that ib700 should not be instantiated twice.  It
lives at a fixed IO address, so it wouldn't make sense even for a real
machine (nevermind that having two watchdogs would be useless).

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://et.redhat.com/~rjones/libguestfs/
See what it can do: http://et.redhat.com/~rjones/libguestfs/recipes.html