Re: [Qemu-devel] [PATCH 1/2] Keep track of ISA ports ISA device is using in qdev.

[Markus Armbruster]

Gleb Natapov <address@hidden> writes:

> On Mon, Oct 25, 2010 at 08:01:13PM +0200, Markus Armbruster wrote:
>> Gleb Natapov <address@hidden> writes:
>> 
>> > On Mon, Oct 25, 2010 at 05:06:51PM +0200, Markus Armbruster wrote:
>> >> Gleb Natapov <address@hidden> writes:
>> >> 
>> >> > Prevent two devices from claiming the same io port.
>> >> 
>> >> Really?
>> >> 
>> >> Your new check for double-claim is in the new isa_init_ioport(), which
>> >> is for ISADevice only.  Thus, only qdevified ISA devices can opt for
>> >> this protection, by calling isa_init_ioport().  It doesn't protect from
>> >> devices who don't or can't opt in, such as PCI devices.
>> >> 
>> > I didn't claim different. This obviously works only for ISA qdev
>> > devices.
>> 
>> I read "Prevent two devices from claiming the same io port" as such.
>> 
> And have you read subject with that?

Yes.  Nevertheless, I found it a bit misleading, so I commented.

>> >> Anyway, we already check for double-claim in
>> >> register_ioport_{read,write}().  The check has issues --- hw_error() is
>> >> wrong there for hot plug.  But it's where the check should be, isn't it?
>> > You don't like double-claim checking? Forget about it (all 3 lines
>> > of code). The real point of the patch is to have ISA resources used
>> > by devices to be stored in common place (ISADevice) which allows
>> > get_dev_path() to be implemented.
>> 
>> We already track I/O port use, in ioport.c.  I don't like that
>> duplicated.  Even less so if the dupe catches fewer double-claims than
>> the original.
> Consider it removed although we do keep track of irqs there and this
> tracking is also incomplete. Why is it there?

Where's "there"?

Let's not add to the code duplication if we can help it.  It's bad
enough as it is.

>> Perhaps your new function should wrap around register_ioport_*() instead
>> of supplementing it.
> register_ioport_*() is disconnected from qdev in general and from ISADEvice
> in particular, so how "wrap around register_ioport_*()" will help me to
> have get_dev_path() for ISABus is beyond my understanding.

I was too terse, sorry.

Maybe we should have functions for ISADevices to register ISA resources.
Functions that are *not* disconnected from qdev.  Instead of code like

        register_ioport_write(pm_io_base, 64, 2, pm_ioport_writew, s);
        register_ioport_read(pm_io_base, 64, 2, pm_ioport_readw, s);
        register_ioport_write(pm_io_base, 64, 4, pm_ioport_writel, s);
        register_ioport_read(pm_io_base, 64, 4, pm_ioport_readl, s);
        isa_init_ioport_range(dev, pm_ioport_base, 64);

or, with Avi's proposed interface

        ioport_register(&s->ioport, pm_io_base, 64);
        isa_init_ioport_range(dev, pm_ioport_base, 64);

we'd get something like

        isa_ioport_register(dev, &s->ioport, pm_io_base, 64);

Isn't that neater?

Since some PCI devices also register ISA resources, we'd have to 
offer functions for them as well, properly integrated.

Now, I didn't mean to ask you to do all that as a precondition for
getting your patch in.  I was just observing.