Patchwork [RFC,v4,04/12] isa: Allow to un-assign I/O ports

login
register
mail settings
Submitter Andreas Färber
Date June 8, 2011, 6:55 p.m.
Message ID <1307559319-16183-5-git-send-email-andreas.faerber@web.de>
Download mbox | patch
Permalink /patch/99557/
State New
Headers show

Comments

Andreas Färber - June 8, 2011, 6:55 p.m.
Signed-off-by: Andreas Färber <andreas.faerber@web.de>
---
 hw/isa-bus.c |   14 ++++++++++++++
 hw/isa.h     |    1 +
 2 files changed, 15 insertions(+), 0 deletions(-)
Markus Armbruster - June 9, 2011, 3:03 p.m.
Andreas Färber <andreas.faerber@web.de> writes:

> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
> ---
>  hw/isa-bus.c |   14 ++++++++++++++
>  hw/isa.h     |    1 +
>  2 files changed, 15 insertions(+), 0 deletions(-)
>
> diff --git a/hw/isa-bus.c b/hw/isa-bus.c
> index d258932..1f64673 100644
> --- a/hw/isa-bus.c
> +++ b/hw/isa-bus.c
> @@ -105,6 +105,20 @@ void isa_init_ioport(ISADevice *dev, uint16_t ioport)
>      isa_init_ioport_range(dev, ioport, 1);
>  }
>  
> +void isa_discard_ioport_range(ISADevice *dev, uint16_t start, uint16_t length)
> +{
> +    int i, j;
> +    for (i = 0; i < dev->nioports; i++) {
> +        if (dev->ioports[i] == start) {
> +            for (j = 0; j < dev->nioports - i; j++) {
> +                dev->ioports[i + j] = dev->ioports[i + length + j];
> +            }
> +            dev->nioports -= length;
> +            break;
> +        }
> +    }
> +}
> +

"discard"?  It's the opposite of isa_init_ioport_range(), name should
make that obvious.  "uninit"?

Note: this only affects the I/O-port bookkeeping within ISADevice, not
the actual I/O port handler registration.  Any use must be accompanied
by a matching handler de-registration.  Just like any use of
isa_init_ioport_range() must be accompanied by matching
register_ioport_FOO()s.  You can thank Gleb for this mess.

[...]
Andreas Färber - June 12, 2011, 11:59 a.m.
Am 09.06.2011 um 17:03 schrieb Markus Armbruster:

> Andreas Färber <andreas.faerber@web.de> writes:
>
>> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
>> ---
>> hw/isa-bus.c |   14 ++++++++++++++
>> hw/isa.h     |    1 +
>> 2 files changed, 15 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/isa-bus.c b/hw/isa-bus.c
>> index d258932..1f64673 100644
>> --- a/hw/isa-bus.c
>> +++ b/hw/isa-bus.c
>> @@ -105,6 +105,20 @@ void isa_init_ioport(ISADevice *dev, uint16_t  
>> ioport)
>>     isa_init_ioport_range(dev, ioport, 1);
>> }
>>
>> +void isa_discard_ioport_range(ISADevice *dev, uint16_t start,  
>> uint16_t length)
>> +{
>> +    int i, j;
>> +    for (i = 0; i < dev->nioports; i++) {
>> +        if (dev->ioports[i] == start) {
>> +            for (j = 0; j < dev->nioports - i; j++) {
>> +                dev->ioports[i + j] = dev->ioports[i + length + j];
>> +            }
>> +            dev->nioports -= length;
>> +            break;
>> +        }
>> +    }
>> +}
>> +
>
> "discard"?  It's the opposite of isa_init_ioport_range(), name should
> make that obvious.  "uninit"?

"uninit" felt wrong.

> Note: this only affects the I/O-port bookkeeping within ISADevice, not
> the actual I/O port handler registration.  Any use must be accompanied
> by a matching handler de-registration.  Just like any use of
> isa_init_ioport_range() must be accompanied by matching
> register_ioport_FOO()s.  You can thank Gleb for this mess.

Right, I didn't spot any wrong usage though.

So what about cleaning up the mess and doing  
isa_[un]assign_ioport_range(), wrapping the ioport.c functions?
The overhead of calling it up to six times for the different widths  
and read/write would seem negligible as a startup cost. And for  
pc87312 we don't seriously have to care about performance imo.

Andreas
Gleb Natapov - June 12, 2011, 1:48 p.m.
On Sun, Jun 12, 2011 at 01:59:51PM +0200, Andreas Färber wrote:
> Am 09.06.2011 um 17:03 schrieb Markus Armbruster:
> 
> >Andreas Färber <andreas.faerber@web.de> writes:
> >
> >>Signed-off-by: Andreas Färber <andreas.faerber@web.de>
> >>---
> >>hw/isa-bus.c |   14 ++++++++++++++
> >>hw/isa.h     |    1 +
> >>2 files changed, 15 insertions(+), 0 deletions(-)
> >>
> >>diff --git a/hw/isa-bus.c b/hw/isa-bus.c
> >>index d258932..1f64673 100644
> >>--- a/hw/isa-bus.c
> >>+++ b/hw/isa-bus.c
> >>@@ -105,6 +105,20 @@ void isa_init_ioport(ISADevice *dev,
> >>uint16_t ioport)
> >>    isa_init_ioport_range(dev, ioport, 1);
> >>}
> >>
> >>+void isa_discard_ioport_range(ISADevice *dev, uint16_t start,
> >>uint16_t length)
> >>+{
> >>+    int i, j;
> >>+    for (i = 0; i < dev->nioports; i++) {
> >>+        if (dev->ioports[i] == start) {
> >>+            for (j = 0; j < dev->nioports - i; j++) {
> >>+                dev->ioports[i + j] = dev->ioports[i + length + j];
> >>+            }
> >>+            dev->nioports -= length;
> >>+            break;
> >>+        }
> >>+    }
> >>+}
> >>+
> >
> >"discard"?  It's the opposite of isa_init_ioport_range(), name should
> >make that obvious.  "uninit"?
> 
> "uninit" felt wrong.
> 
> >Note: this only affects the I/O-port bookkeeping within ISADevice, not
> >the actual I/O port handler registration.  Any use must be accompanied
> >by a matching handler de-registration.  Just like any use of
> >isa_init_ioport_range() must be accompanied by matching
> >register_ioport_FOO()s.  You can thank Gleb for this mess.
> 
> Right, I didn't spot any wrong usage though.
> 
> So what about cleaning up the mess and doing
> isa_[un]assign_ioport_range(), wrapping the ioport.c functions?
> The overhead of calling it up to six times for the different widths
> and read/write would seem negligible as a startup cost. And for
> pc87312 we don't seriously have to care about performance imo.
> 
Ideally every ioport registration should be moved to use IORange. I
tried to move all isa devices to it, but it is complicated for two
reasons. First not every device is qdevified and second some devises 
can be instantiated as isa device and non-isa device and they do ioport
registration in a common code. With your approach you will have to
duplicate ioport registration code for isa and non-isa case for such
devices and code duplication is not good.

It is always easier to call something a mess instead of actually fixing
it.

--
			Gleb.
Andreas Färber - June 12, 2011, 3:32 p.m.
Am 12.06.2011 um 15:48 schrieb Gleb Natapov:

> On Sun, Jun 12, 2011 at 01:59:51PM +0200, Andreas Färber wrote:
>> Am 09.06.2011 um 17:03 schrieb Markus Armbruster:
>>
>>> Andreas Färber <andreas.faerber@web.de> writes:
>>>
>>>> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
>>>> ---
>>>> hw/isa-bus.c |   14 ++++++++++++++
>>>> hw/isa.h     |    1 +
>>>> 2 files changed, 15 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/hw/isa-bus.c b/hw/isa-bus.c
>>>> index d258932..1f64673 100644
>>>> --- a/hw/isa-bus.c
>>>> +++ b/hw/isa-bus.c
>>>> @@ -105,6 +105,20 @@ void isa_init_ioport(ISADevice *dev,
>>>> uint16_t ioport)
>>>>   isa_init_ioport_range(dev, ioport, 1);
>>>> }
>>>>
>>>> +void isa_discard_ioport_range(ISADevice *dev, uint16_t start,
>>>> uint16_t length)
>>>> +{
>>>> +    int i, j;
>>>> +    for (i = 0; i < dev->nioports; i++) {
>>>> +        if (dev->ioports[i] == start) {
>>>> +            for (j = 0; j < dev->nioports - i; j++) {
>>>> +                dev->ioports[i + j] = dev->ioports[i + length +  
>>>> j];
>>>> +            }
>>>> +            dev->nioports -= length;
>>>> +            break;
>>>> +        }
>>>> +    }
>>>> +}
>>>> +
>>>
>>> "discard"?  It's the opposite of isa_init_ioport_range(), name  
>>> should
>>> make that obvious.  "uninit"?
>>
>> "uninit" felt wrong.
>>
>>> Note: this only affects the I/O-port bookkeeping within ISADevice,  
>>> not
>>> the actual I/O port handler registration.  Any use must be  
>>> accompanied
>>> by a matching handler de-registration.  Just like any use of
>>> isa_init_ioport_range() must be accompanied by matching
>>> register_ioport_FOO()s.  You can thank Gleb for this mess.
>>
>> Right, I didn't spot any wrong usage though.
>>
>> So what about cleaning up the mess and doing
>> isa_[un]assign_ioport_range(), wrapping the ioport.c functions?
>> The overhead of calling it up to six times for the different widths
>> and read/write would seem negligible as a startup cost. And for
>> pc87312 we don't seriously have to care about performance imo.
>>
> Ideally every ioport registration should be moved to use IORange. I
> tried to move all isa devices to it, but it is complicated for two
> reasons. First not every device is qdevified and second some devises
> can be instantiated as isa device and non-isa device and they do  
> ioport
> registration in a common code. With your approach you will have to
> duplicate ioport registration code for isa and non-isa case for such
> devices and code duplication is not good.

I'm not trying to duplicate anything! What I've been seeing is that  
many of the ISA devices I've touched in this series first register the  
I/O ports and then associate them with the ISADevice, in ISA-only  
code. So the numbers *are* duplicated and I'm proposing to consolidate  
this into one call.

The existing "init" would stay, so that code with disseparate ISA and  
common parts (like IDE) remains unaffected.

Andreas
Gleb Natapov - June 12, 2011, 5:14 p.m.
On Sun, Jun 12, 2011 at 05:32:57PM +0200, Andreas Färber wrote:
> Am 12.06.2011 um 15:48 schrieb Gleb Natapov:
> 
> >On Sun, Jun 12, 2011 at 01:59:51PM +0200, Andreas Färber wrote:
> >>Am 09.06.2011 um 17:03 schrieb Markus Armbruster:
> >>
> >>>Andreas Färber <andreas.faerber@web.de> writes:
> >>>
> >>>>Signed-off-by: Andreas Färber <andreas.faerber@web.de>
> >>>>---
> >>>>hw/isa-bus.c |   14 ++++++++++++++
> >>>>hw/isa.h     |    1 +
> >>>>2 files changed, 15 insertions(+), 0 deletions(-)
> >>>>
> >>>>diff --git a/hw/isa-bus.c b/hw/isa-bus.c
> >>>>index d258932..1f64673 100644
> >>>>--- a/hw/isa-bus.c
> >>>>+++ b/hw/isa-bus.c
> >>>>@@ -105,6 +105,20 @@ void isa_init_ioport(ISADevice *dev,
> >>>>uint16_t ioport)
> >>>>  isa_init_ioport_range(dev, ioport, 1);
> >>>>}
> >>>>
> >>>>+void isa_discard_ioport_range(ISADevice *dev, uint16_t start,
> >>>>uint16_t length)
> >>>>+{
> >>>>+    int i, j;
> >>>>+    for (i = 0; i < dev->nioports; i++) {
> >>>>+        if (dev->ioports[i] == start) {
> >>>>+            for (j = 0; j < dev->nioports - i; j++) {
> >>>>+                dev->ioports[i + j] = dev->ioports[i +
> >>>>length + j];
> >>>>+            }
> >>>>+            dev->nioports -= length;
> >>>>+            break;
> >>>>+        }
> >>>>+    }
> >>>>+}
> >>>>+
> >>>
> >>>"discard"?  It's the opposite of isa_init_ioport_range(), name
> >>>should
> >>>make that obvious.  "uninit"?
> >>
> >>"uninit" felt wrong.
> >>
> >>>Note: this only affects the I/O-port bookkeeping within
> >>>ISADevice, not
> >>>the actual I/O port handler registration.  Any use must be
> >>>accompanied
> >>>by a matching handler de-registration.  Just like any use of
> >>>isa_init_ioport_range() must be accompanied by matching
> >>>register_ioport_FOO()s.  You can thank Gleb for this mess.
> >>
> >>Right, I didn't spot any wrong usage though.
> >>
> >>So what about cleaning up the mess and doing
> >>isa_[un]assign_ioport_range(), wrapping the ioport.c functions?
> >>The overhead of calling it up to six times for the different widths
> >>and read/write would seem negligible as a startup cost. And for
> >>pc87312 we don't seriously have to care about performance imo.
> >>
> >Ideally every ioport registration should be moved to use IORange. I
> >tried to move all isa devices to it, but it is complicated for two
> >reasons. First not every device is qdevified and second some devises
> >can be instantiated as isa device and non-isa device and they do
> >ioport
> >registration in a common code. With your approach you will have to
> >duplicate ioport registration code for isa and non-isa case for such
> >devices and code duplication is not good.
> 
> I'm not trying to duplicate anything! What I've been seeing is that
> many of the ISA devices I've touched in this series first register
> the I/O ports and then associate them with the ISADevice, in
> ISA-only code. So the numbers *are* duplicated and I'm proposing to
> consolidate this into one call.
> 
> The existing "init" would stay, so that code with disseparate ISA
> and common parts (like IDE) remains unaffected.
> 
OK, if you think that it is less of a "mess" this way. In a perfect
world you would have something like:

void init_ioport(DeviceState *dev, IORange *r, const IORangeOps *ops,
                     uint64_t base, uint64_t len)

And it will do correct thing for each device type.

--
			Gleb.

Patch

diff --git a/hw/isa-bus.c b/hw/isa-bus.c
index d258932..1f64673 100644
--- a/hw/isa-bus.c
+++ b/hw/isa-bus.c
@@ -105,6 +105,20 @@  void isa_init_ioport(ISADevice *dev, uint16_t ioport)
     isa_init_ioport_range(dev, ioport, 1);
 }
 
+void isa_discard_ioport_range(ISADevice *dev, uint16_t start, uint16_t length)
+{
+    int i, j;
+    for (i = 0; i < dev->nioports; i++) {
+        if (dev->ioports[i] == start) {
+            for (j = 0; j < dev->nioports - i; j++) {
+                dev->ioports[i + j] = dev->ioports[i + length + j];
+            }
+            dev->nioports -= length;
+            break;
+        }
+    }
+}
+
 static int isa_qdev_init(DeviceState *qdev, DeviceInfo *base)
 {
     ISADevice *dev = DO_UPCAST(ISADevice, qdev, qdev);
diff --git a/hw/isa.h b/hw/isa.h
index 5d460ab..ba7a696 100644
--- a/hw/isa.h
+++ b/hw/isa.h
@@ -33,6 +33,7 @@  qemu_irq isa_get_irq(int isairq);
 void isa_init_irq(ISADevice *dev, qemu_irq *p, int isairq);
 void isa_init_ioport(ISADevice *dev, uint16_t ioport);
 void isa_init_ioport_range(ISADevice *dev, uint16_t start, uint16_t length);
+void isa_discard_ioport_range(ISADevice *dev, uint16_t start, uint16_t length);
 void isa_qdev_register(ISADeviceInfo *info);
 ISADevice *isa_create(const char *name);
 ISADevice *isa_try_create(const char *name);