[Qemu-devel] [4261] Errors while registering ioports are not fatal (Glauber Costa).

[Qemu-devel] [4261] Errors while registering ioports are not fatal (Glauber Costa).

[Andrzej Zaborowski]

Revision: 4261
          http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=4261
Author:   balrog
Date:     2008-04-26 16:04:29 +0000 (Sat, 26 Apr 2008)

Log Message:
-----------
Errors while registering ioports are not fatal (Glauber Costa).

Modified Paths:
--------------
    trunk/vl.c

Modified: trunk/vl.c
===================================================================
--- trunk/vl.c	2008-04-26 15:56:05 UTC (rev 4260)
+++ trunk/vl.c	2008-04-26 16:04:29 UTC (rev 4261)
@@ -324,13 +324,13 @@
     } else if (size == 4) {
         bsize = 2;
     } else {
-        hw_error("register_ioport_read: invalid size");
+        fprintf(stderr, "register_ioport_read: invalid size\n");
         return -1;
     }
     for(i = start; i < start + length; i += size) {
         ioport_read_table[bsize][i] = func;
         if (ioport_opaque[i] != NULL && ioport_opaque[i] != opaque)
-            hw_error("register_ioport_read: invalid opaque");
+            fprintf(stderr, "register_ioport_read: invalid opaque\n");
         ioport_opaque[i] = opaque;
     }
     return 0;
@@ -349,13 +349,15 @@
     } else if (size == 4) {
         bsize = 2;
     } else {
-        hw_error("register_ioport_write: invalid size");
+        fprintf(stderr, "register_ioport_write: invalid size\n");
         return -1;
     }
     for(i = start; i < start + length; i += size) {
         ioport_write_table[bsize][i] = func;
-        if (ioport_opaque[i] != NULL && ioport_opaque[i] != opaque)
-            hw_error("register_ioport_write: invalid opaque");
+        if (ioport_opaque[i] != NULL && ioport_opaque[i] != opaque) {
+            fprintf(stderr, "register_ioport_write: invalid opaque\n");
+            return -1;
+        }
         ioport_opaque[i] = opaque;
     }
     return 0;

Re: [Qemu-devel] [4261] Errors while registering ioports are not fatal (Glauber Costa).

[Paul Brook]

On Saturday 26 April 2008, Andrzej Zaborowski wrote:
> Revision: 4261
>           http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=4261
> Author:   balrog
> Date:     2008-04-26 16:04:29 +0000 (Sat, 26 Apr 2008)
>
> Log Message:
> -----------
> Errors while registering ioports are not fatal (Glauber Costa).

Why shouldn't they be fatal? How can this be anything other than a serious bug 
in the device emulation?

Paul

Re: [Qemu-devel] [4261] Errors while registering ioports are not fatal (Glauber Costa).

[Anthony Liguori]

Paul Brook wrote:
> On Saturday 26 April 2008, Andrzej Zaborowski wrote:
>   
>> Revision: 4261
>>           http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=4261
>> Author:   balrog
>> Date:     2008-04-26 16:04:29 +0000 (Sat, 26 Apr 2008)
>>
>> Log Message:
>> -----------
>> Errors while registering ioports are not fatal (Glauber Costa).
>>     
>
> Why shouldn't they be fatal? How can this be anything other than a serious bug 
> in the device emulation?
>   

I think the idea is that the device should fail to initialize rather the 
VM being destroyed.  Consider the case of PCI hotplug.  It's a 
recoverable error if register ioport fails during hot add.

In general, it's better to avoid exit()'ing deep in the code and instead 
propagate errors.  With that said, I don't think we should just 
eliminate the hw_error() call without going through and updating all the 
callers of register_ioport to handle that function failing.

Regards,

Anthony Liguori

> Paul
>
>
>   

Re: [Qemu-devel] [4261] Errors while registering ioports are not fatal (Glauber Costa).

[andrzej zaborowski]

On 26/04/2008, Paul Brook <paul@codesourcery.com> wrote:
> On Saturday 26 April 2008, Andrzej Zaborowski wrote:
>  > Revision: 4261
>  >           http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=4261
>  > Author:   balrog
>  > Date:     2008-04-26 16:04:29 +0000 (Sat, 26 Apr 2008)
>  >
>  > Log Message:
>  > -----------
>  > Errors while registering ioports are not fatal (Glauber Costa).
>
>  Why shouldn't they be fatal? How can this be anything other than a serious bug
>  in the device emulation?

This change is perhaps not useful, it would be useful with hot-plugged
/ proxied pci devices.  I think they are desirable features. But the
patchsets submitted turn out to depend on too much kvm code to ever
work alone so I might just as well revert :(

You might not want qemu to quit a running session if it's possible to
continue running, even if there turns out to be a serious bug.

Regards

Re: [Qemu-devel] [4261] Errors while registering ioports are not fatal (Glauber Costa).

[Paul Brook]

On Saturday 26 April 2008, Anthony Liguori wrote:
> Paul Brook wrote:
> > On Saturday 26 April 2008, Andrzej Zaborowski wrote:
> >> Revision: 4261
> >>           http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=4261
> >> Author:   balrog
> >> Date:     2008-04-26 16:04:29 +0000 (Sat, 26 Apr 2008)
> >>
> >> Log Message:
> >> -----------
> >> Errors while registering ioports are not fatal (Glauber Costa).
> >
> > Why shouldn't they be fatal? How can this be anything other than a
> > serious bug in the device emulation?
>
> I think the idea is that the device should fail to initialize rather the
> VM being destroyed.  Consider the case of PCI hotplug.  It's a
> recoverable error if register ioport fails during hot add.

The errors that get suppressed aren't the sort of thing that should ever 
happen. How exactly do you end up with an IO port that is not 1, 2 or 4 bytes 
in size? If this ever happens I want qemu do die right there and then. This 
isn't just a failure, it is an indication that something is broken beyond 
hope.

Paul

Re: [Qemu-devel] [4261] Errors while registering ioports are not fatal (Glauber Costa).

[Paul Brook]

On Saturday 26 April 2008, andrzej zaborowski wrote:
> On 26/04/2008, Paul Brook <paul@codesourcery.com> wrote:
> > On Saturday 26 April 2008, Andrzej Zaborowski wrote:
> >  > Revision: 4261
> >  >          
> >  > http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=4261 Author:
> >  >   balrog
> >  > Date:     2008-04-26 16:04:29 +0000 (Sat, 26 Apr 2008)
> >  >
> >  > Log Message:
> >  > -----------
> >  > Errors while registering ioports are not fatal (Glauber Costa).
> >
> >  Why shouldn't they be fatal? How can this be anything other than a
> > serious bug in the device emulation?
>
> This change is perhaps not useful, it would be useful with hot-plugged
> / proxied pci devices.  

I fail to see how hotplugging or proxing has anything to do with it. IO port 
registration is not something that can reasonably fail.

If the real problem is that we can't cope with multiple devices registering 
the same IO port than you need to fix that. Blindly punting to the caller to 
cope is IMHO not an acceptable solution, especially when none of the callers 
check the return value.

Paul

Re: [Qemu-devel] [4261] Errors while registering ioports are not fatal (Glauber Costa).

[Anthony Liguori]

Paul Brook wrote:
> On Saturday 26 April 2008, Anthony Liguori wrote:
>   
>> Paul Brook wrote:
>>     
>>> On Saturday 26 April 2008, Andrzej Zaborowski wrote:
>>>       
>>>> Revision: 4261
>>>>           http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=4261
>>>> Author:   balrog
>>>> Date:     2008-04-26 16:04:29 +0000 (Sat, 26 Apr 2008)
>>>>
>>>> Log Message:
>>>> -----------
>>>> Errors while registering ioports are not fatal (Glauber Costa).
>>>>         
>>> Why shouldn't they be fatal? How can this be anything other than a
>>> serious bug in the device emulation?
>>>       
>> I think the idea is that the device should fail to initialize rather the
>> VM being destroyed.  Consider the case of PCI hotplug.  It's a
>> recoverable error if register ioport fails during hot add.
>>     
>
> The errors that get suppressed aren't the sort of thing that should ever 
> happen. How exactly do you end up with an IO port that is not 1, 2 or 4 bytes 
> in size? If this ever happens I want qemu do die right there and then. This 
> isn't just a failure, it is an indication that something is broken beyond 
> hope.
>   

The intended error to be suppressed was registering an ioport range that 
is already registered by something else.

Regards,

Anthony Liguori

> Paul
>   

Re: [Qemu-devel] [4261] Errors while registering ioports are not fatal (Glauber Costa).

[Anthony Liguori]

Paul Brook wrote:
> On Saturday 26 April 2008, andrzej zaborowski wrote:
>   
>> On 26/04/2008, Paul Brook <paul@codesourcery.com> wrote:
>>     
>>> On Saturday 26 April 2008, Andrzej Zaborowski wrote:
>>>  > Revision: 4261
>>>  >          
>>>  > http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=4261 Author:
>>>  >   balrog
>>>  > Date:     2008-04-26 16:04:29 +0000 (Sat, 26 Apr 2008)
>>>  >
>>>  > Log Message:
>>>  > -----------
>>>  > Errors while registering ioports are not fatal (Glauber Costa).
>>>
>>>  Why shouldn't they be fatal? How can this be anything other than a
>>> serious bug in the device emulation?
>>>       
>> This change is perhaps not useful, it would be useful with hot-plugged
>> / proxied pci devices.  
>>     
>
> I fail to see how hotplugging or proxing has anything to do with it. IO port 
> registration is not something that can reasonably fail.
>
> If the real problem is that we can't cope with multiple devices registering 
> the same IO port than you need to fix that. Blindly punting to the caller to 
>   

There is no fix for that.  You can't have two devices that use the same 
IO port.

> cope is IMHO not an acceptable solution, especially when none of the callers 
> check the return value.
>   

IO port range conflicts can still happen even with PCI devices.  Two PCI 
IDE controllers would conflict with each other for instance.  It's much 
more likely with ISA of course but it's still possible.  register_ioport 
really should have a return code and callers should actively be checking it.

Regards,

Anthony Liguori

> Paul
>
>
>   

Re: [Qemu-devel] [4261] Errors while registering ioports are not fatal (Glauber Costa).

[Glauber Costa]

On Sat, Apr 26, 2008 at 4:57 PM, andrzej zaborowski <balrogg@gmail.com> wrote:
>
> On 26/04/2008, Paul Brook <paul@codesourcery.com> wrote:
>  > On Saturday 26 April 2008, Andrzej Zaborowski wrote:
>  >  > Revision: 4261
>  >  >           http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=4261
>  >  > Author:   balrog
>  >  > Date:     2008-04-26 16:04:29 +0000 (Sat, 26 Apr 2008)
>  >  >
>  >  > Log Message:
>  >  > -----------
>  >  > Errors while registering ioports are not fatal (Glauber Costa).
>  >
>  >  Why shouldn't they be fatal? How can this be anything other than a serious bug
>  >  in the device emulation?
>
>  This change is perhaps not useful, it would be useful with hot-plugged
>  / proxied pci devices.  I think they are desirable features. But the
>  patchsets submitted turn out to depend on too much kvm code to ever
>  work alone so I might just as well revert :(

Yes, reverting is probably better. I must confess I was surprised to
see this going in alone.
It is highly kvm dependant, because it was written in the context of
pci passtrough. In the mails I sent,
qemu-devel copied, I explicitly said it was an RFC. Reason?
As aliguori said, the callers must all be fixed, and it was not
something I was willing to do if the patches were not to be accepted.
Unfortunately the patches got no attention at the time, but I'm happy
to see the it happening now.
As for being dependant of kvm code, the patches were written in the
context of kvm pci-passthrough. And that´s why.

>  You might not want qemu to quit a running session if it's possible to
>  continue running, even if there turns out to be a serious bug.
>
>  Regards
>
>
>



-- 
Glauber Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."

Re: [Qemu-devel] [4261] Errors while registering ioports are not fatal (Glauber Costa).

[Anthony Liguori]

andrzej zaborowski wrote:
> On 26/04/2008, Paul Brook <paul@codesourcery.com> wrote:
>   
>> On Saturday 26 April 2008, Andrzej Zaborowski wrote:
>>  > Revision: 4261
>>  >           http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=4261
>>  > Author:   balrog
>>  > Date:     2008-04-26 16:04:29 +0000 (Sat, 26 Apr 2008)
>>  >
>>  > Log Message:
>>  > -----------
>>  > Errors while registering ioports are not fatal (Glauber Costa).
>>
>>  Why shouldn't they be fatal? How can this be anything other than a serious bug
>>  in the device emulation?
>>     
>
> This change is perhaps not useful, it would be useful with hot-plugged
> / proxied pci devices.  I think they are desirable features. But the
> patchsets submitted turn out to depend on too much kvm code to ever
> work alone so I might just as well revert :(
>   

It's not at all kvm specific.  Even if QEMU never merged PCI hotplug 
(although I see no reason why not to), it's the right direction to move 
toward.

In the future, if we add configuration files to specific the hardware 
associated with a machine, you want to be able to gracefully detect when 
a configuration file results in IO port conflicts.  Just exiting deep 
within register_ioport() is the wrong approach as a user will never know 
what the problem was.

Being able to propagate the error gives the configuration parsing 
routines an opportunity to present a more human readable error like 
"cannot add device 'blah' because of conflict IO port range with device 
'foo'".

Regards,

Anthony Liguori

> You might not want qemu to quit a running session if it's possible to
> continue running, even if there turns out to be a serious bug.
>
> Regards
>
>
>   

Re: [Qemu-devel] [4261] Errors while registering ioports are not fatal (Glauber Costa).

[Glauber Costa]

On Sat, Apr 26, 2008 at 4:57 PM, Paul Brook <paul@codesourcery.com> wrote:
> On Saturday 26 April 2008, Anthony Liguori wrote:
>  > Paul Brook wrote:
>  > > On Saturday 26 April 2008, Andrzej Zaborowski wrote:
>  > >> Revision: 4261
>  > >>           http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=4261
>  > >> Author:   balrog
>  > >> Date:     2008-04-26 16:04:29 +0000 (Sat, 26 Apr 2008)
>  > >>
>  > >> Log Message:
>  > >> -----------
>  > >> Errors while registering ioports are not fatal (Glauber Costa).
>  > >
>  > > Why shouldn't they be fatal? How can this be anything other than a
>  > > serious bug in the device emulation?
>  >
>  > I think the idea is that the device should fail to initialize rather the
>  > VM being destroyed.  Consider the case of PCI hotplug.  It's a
>  > recoverable error if register ioport fails during hot add.
>
>  The errors that get suppressed aren't the sort of thing that should ever
>  happen. How exactly do you end up with an IO port that is not 1, 2 or 4 bytes
>  in size? If this ever happens I want qemu do die right there and then. This
>  isn't just a failure, it is an indication that something is broken beyond
>  hope.
>
>  Paul

Errors in size are not the thing we´re trying to catch here. If
preferred, they can still be fatal, which makes some sense.
Problem is that some devices might use an ioport that is already
registered. In the specific problem I had:

* qemu statically register an ide controller, (can´t recall specific
port numbers now)
* kvm wants to passthrough a device, choosen by the user. It might be
the case that this device is an IDE controller, which will use the
same ports.

Would it be better to test it, and not even register the device in the
first place. Sure. I agree 120 % with this. But how?

You won´t have the conflicting ioport registered until it is too late
in the process, because all ports only get registered in
update_mappings(). But now that I got your attention, if there is a
better proposed solution for this case, I'll be happy to hear and
implement it.

>
>



-- 
Glauber Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."

Re: [Qemu-devel] [4261] Errors while registering ioports are not fatal (Glauber Costa).

[Paul Brook]

> > I fail to see how hotplugging or proxing has anything to do with it. IO
> > port registration is not something that can reasonably fail.
> >
> > If the real problem is that we can't cope with multiple devices
> > registering the same IO port than you need to fix that. Blindly punting
> > to the caller to
>
> There is no fix for that.  You can't have two devices that use the same
> IO port.

You probably can't meaningfully access the IO port. However I think the 
registration itself should be allowed.

The device shouldn't have to handle this. Just like real hardware doesn't. 
If you configure two devices with the same IO port I'd expect writes to go to 
one or both of them, or cause a CPU fault. Just like when you have 
overlapping memory ranges. The device doesn't suddenly disappear in a puff of 
smoke because the OS assigned overlapping IO ranges.

> > cope is IMHO not an acceptable solution, especially when none of the
> > callers check the return value.
>
> IO port range conflicts can still happen even with PCI devices.  Two PCI
> IDE controllers would conflict with each other for instance.  It's much
> more likely with ISA of course but it's still possible.  register_ioport
> really should have a return code and callers should actively be checking
> it.

This is why bouncing the error to the device is the wrong thing to do. 
Once the OS resolves the conflict I'd expect the remaining device to just 
work.

Paul

Re: [Qemu-devel] [4261] Errors while registering ioports are not fatal (Glauber Costa).

[Anthony Liguori]

Paul Brook wrote:
>>> I fail to see how hotplugging or proxing has anything to do with it. IO
>>> port registration is not something that can reasonably fail.
>>>
>>> If the real problem is that we can't cope with multiple devices
>>> registering the same IO port than you need to fix that. Blindly punting
>>> to the caller to
>>>       
>> There is no fix for that.  You can't have two devices that use the same
>> IO port.
>>     
>
> You probably can't meaningfully access the IO port. However I think the 
> registration itself should be allowed.
>
> The device shouldn't have to handle this. Just like real hardware doesn't. 
>   

We can be better than real hardware here.  In the ISA days, having to 
manually configure a cards IRQ lines to avoid conflicts was a huge pain 
for users.  This is not behavior we should strive to emulate :-)

> If you configure two devices with the same IO port I'd expect writes to go to 
> one or both of them, or cause a CPU fault. Just like when you have 
> overlapping memory ranges. The device doesn't suddenly disappear in a puff of 
> smoke because the OS assigned overlapping IO ranges.
>   

The issue isn't the guest OS assigning overlapping IO regions.  The 
issues is that some devices (like IDE), register ioports directly 
independently of PCI regions.

>>> cope is IMHO not an acceptable solution, especially when none of the
>>> callers check the return value.
>>>       
>> IO port range conflicts can still happen even with PCI devices.  Two PCI
>> IDE controllers would conflict with each other for instance.  It's much
>> more likely with ISA of course but it's still possible.  register_ioport
>> really should have a return code and callers should actively be checking
>> it.
>>     
>
> This is why bouncing the error to the device is the wrong thing to do. 
> Once the OS resolves the conflict I'd expect the remaining device to just 
> work.
>   

I don't know how hardware normally handles this.  If this is how normal 
hardware works, then we should emulate that.  It's also true though that 
exit()'ing in register_ioport is the wrong thing to do though.

Regards,

Anthony Liguori

> Paul
>   

Re: [Qemu-devel] [4261] Errors while registering ioports are not fatal (Glauber Costa).

[andrzej zaborowski]

On 26/04/2008, Anthony Liguori <anthony@codemonkey.ws> wrote:
> andrzej zaborowski wrote:
>
> > On 26/04/2008, Paul Brook <paul@codesourcery.com> wrote:
> >
> >
> > > On Saturday 26 April 2008, Andrzej Zaborowski wrote:
> > >  > Revision: 4261
> > >  >
> http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=4261
> > >  > Author:   balrog
> > >  > Date:     2008-04-26 16:04:29 +0000 (Sat, 26 Apr 2008)
> > >  >
> > >  > Log Message:
> > >  > -----------
> > >  > Errors while registering ioports are not fatal (Glauber Costa).
> > >
> > >  Why shouldn't they be fatal? How can this be anything other than a
> serious bug
> > >  in the device emulation?
> > >
> > >
> >
> > This change is perhaps not useful, it would be useful with hot-plugged
> > / proxied pci devices.  I think they are desirable features. But the
> > patchsets submitted turn out to depend on too much kvm code to ever
> > work alone so I might just as well revert :(
> >
> >
>
>  It's not at all kvm specific.  Even if QEMU never merged PCI hotplug
> (although I see no reason why not to), it's the right direction to move
> toward.

Right, what I mean is I missed the fact that there had been more
changes which didn't go into qemu on which Marcelo Tosatti's patches
depended and it seems like this included rather significant changes.

Regards

Re: [Qemu-devel] [4261] Errors while registering ioports are not fatal (Glauber Costa).

[Paul Brook]

> > If you configure two devices with the same IO port I'd expect writes to
> > go to one or both of them, or cause a CPU fault. Just like when you have
> > overlapping memory ranges. The device doesn't suddenly disappear in a
> > puff of smoke because the OS assigned overlapping IO ranges.
>
> The issue isn't the guest OS assigning overlapping IO regions.  The
> issues is that some devices (like IDE), register ioports directly
> independently of PCI regions.

This is a device bug. They shouldn't do that. This is exactly the "manual 
configuring of IO regions" you mentioned.

Paul