| Message ID | 1287934469-16624-2-git-send-email-avi@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 10/24/2010 05:34 PM, Avi Kivity wrote: > Currently the old and new methods exist side by side; once the old way is gone, > we can also save a bunch of memory since the new method requires one pointer > per ioport instead of 6. Actually, 1:7, we replace 3 read callbacks, 3 write callbacks, and 1 opaque pointer with a single IOPort pointer.
On Sun, Oct 24, 2010 at 3:34 PM, Avi Kivity <avi@redhat.com> wrote: > The current ioport callbacks are not type-safe, in that they accept an "opaque" > pointer as an argument whose type must match the argument to the registration > function; this is not checked by the compiler. > > This patch adds an alternative that is type-safe. Instead of an opaque > argument, both registation and the callback use a new IOPort type. The > callback then uses container_of() to access its main structures. > > Currently the old and new methods exist side by side; once the old way is gone, > we can also save a bunch of memory since the new method requires one pointer > per ioport instead of 6. > > Signed-off-by: Avi Kivity <avi@redhat.com> If we are going to change the interface, let's do it so that it's useful for other uses too: http://article.gmane.org/gmane.comp.emulators.qemu/76984 > --- > ioport.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > ioport.h | 16 +++++++++++++++ > 2 files changed, 80 insertions(+), 0 deletions(-) > > diff --git a/ioport.c b/ioport.c > index ec3dc65..47eafc3 100644 > --- a/ioport.c > +++ b/ioport.c > @@ -174,6 +174,70 @@ int register_ioport_write(pio_addr_t start, int length, int size, > return 0; > } > > +static uint32_t ioport_readb_thunk(void *opaque, uint32_t addr) > +{ > + IOPort *ioport = opaque; > + > + return ioport->ops->readb(ioport, addr); > +} > + > +static uint32_t ioport_readw_thunk(void *opaque, uint32_t addr) > +{ > + IOPort *ioport = opaque; > + > + return ioport->ops->readw(ioport, addr); > +} > + > +static uint32_t ioport_readl_thunk(void *opaque, uint32_t addr) > +{ > + IOPort *ioport = opaque; > + > + return ioport->ops->readl(ioport, addr); > +} > + > +static void ioport_writeb_thunk(void *opaque, uint32_t addr, uint32_t data) > +{ > + IOPort *ioport = opaque; > + > + return ioport->ops->writeb(ioport, addr, data); > +} > + > +static void ioport_writew_thunk(void *opaque, uint32_t addr, uint32_t data) > +{ > + IOPort *ioport = opaque; > + > + return ioport->ops->writew(ioport, addr, data); > +} > + > +static void ioport_writel_thunk(void *opaque, uint32_t addr, uint32_t data) > +{ > + IOPort *ioport = opaque; > + > + return ioport->ops->writel(ioport, addr, data); > +} > + > +void ioport_register(IOPort *ioport, pio_addr_t start, pio_addr_t length) > +{ > + if (ioport->ops->readb) { > + register_ioport_read(start, length, 1, ioport_readb_thunk, ioport); > + } > + if (ioport->ops->readw) { > + register_ioport_read(start, length, 2, ioport_readw_thunk, ioport); > + } > + if (ioport->ops->readl) { > + register_ioport_read(start, length, 4, ioport_readl_thunk, ioport); > + } > + if (ioport->ops->writeb) { > + register_ioport_write(start, length, 1, ioport_writeb_thunk, ioport); > + } > + if (ioport->ops->writew) { > + register_ioport_write(start, length, 2, ioport_writew_thunk, ioport); > + } > + if (ioport->ops->writel) { > + register_ioport_write(start, length, 4, ioport_writel_thunk, ioport); > + } > +} > + > void isa_unassign_ioport(pio_addr_t start, int length) > { > int i; > diff --git a/ioport.h b/ioport.h > index 3d3c8a3..8036e59 100644 > --- a/ioport.h > +++ b/ioport.h > @@ -36,6 +36,22 @@ typedef uint32_t pio_addr_t; > typedef void (IOPortWriteFunc)(void *opaque, uint32_t address, uint32_t data); > typedef uint32_t (IOPortReadFunc)(void *opaque, uint32_t address); > > +struct IOPort; > + > +typedef struct IOPortOps { > + uint32_t (*readb)(struct IOPort *ioport, pio_addr_t addr); > + uint32_t (*readw)(struct IOPort *ioport, pio_addr_t addr); > + uint32_t (*readl)(struct IOPort *ioport, pio_addr_t addr); > + void (*writeb)(struct IOPort *ioport, pio_addr_t addr, uint32_t data); > + void (*writew)(struct IOPort *ioport, pio_addr_t addr, uint32_t data); > + void (*writel)(struct IOPort *ioport, pio_addr_t addr, uint32_t data); > +} IOPortOps; > + > +typedef struct IOPort { > + IOPortOps *ops; const
On 10/24/2010 08:14 PM, Blue Swirl wrote: > On Sun, Oct 24, 2010 at 3:34 PM, Avi Kivity<avi@redhat.com> wrote: > > The current ioport callbacks are not type-safe, in that they accept an "opaque" > > pointer as an argument whose type must match the argument to the registration > > function; this is not checked by the compiler. > > > > This patch adds an alternative that is type-safe. Instead of an opaque > > argument, both registation and the callback use a new IOPort type. The > > callback then uses container_of() to access its main structures. > > > > Currently the old and new methods exist side by side; once the old way is gone, > > we can also save a bunch of memory since the new method requires one pointer > > per ioport instead of 6. > > > > Signed-off-by: Avi Kivity<avi@redhat.com> > > If we are going to change the interface, let's do it so that it's > useful for other uses too: > http://article.gmane.org/gmane.comp.emulators.qemu/76984 I don't really see why we need registration; cpu_register_io() takes function pointers, a size, and an opaque, and gives an integer handle in return. With the IOPort object approach, you set up the IOPort with function pointers, size is implied, and the opaque is derived using container_of(); the handle is simply the address of the object. > > +typedef struct IOPort { > > + IOPortOps *ops; > > const Yup, will fix. Will repost once we agree on the approach.
Avi Kivity <avi@redhat.com> wrote: > +static void ioport_writeb_thunk(void *opaque, uint32_t addr, uint32_t data) > +{ > + IOPort *ioport = opaque; > + > + return ioport->ops->writeb(ioport, addr, data); > +} > + > +static void ioport_writew_thunk(void *opaque, uint32_t addr, uint32_t data) > +{ > + IOPort *ioport = opaque; > + > + return ioport->ops->writew(ioport, addr, data); > +} > + > +static void ioport_writel_thunk(void *opaque, uint32_t addr, uint32_t data) > +{ > + IOPort *ioport = opaque; > + > + return ioport->ops->writel(ioport, addr, data); > +} "return" is not needed on this three functions. Rest of aproach looks good. Later, Juan.
On 10/25/2010 02:54 PM, Juan Quintela wrote: > Avi Kivity<avi@redhat.com> wrote: > > > +static void ioport_writeb_thunk(void *opaque, uint32_t addr, uint32_t data) > > +{ > > + IOPort *ioport = opaque; > > + > > + return ioport->ops->writeb(ioport, addr, data); > > +} > > + > > +static void ioport_writew_thunk(void *opaque, uint32_t addr, uint32_t data) > > +{ > > + IOPort *ioport = opaque; > > + > > + return ioport->ops->writew(ioport, addr, data); > > +} > > + > > +static void ioport_writel_thunk(void *opaque, uint32_t addr, uint32_t data) > > +{ > > + IOPort *ioport = opaque; > > + > > + return ioport->ops->writel(ioport, addr, data); > > +} > > "return" is not needed on this three functions. > Fixed.
On Mon, Oct 25, 2010 at 10:00 AM, Avi Kivity <avi@redhat.com> wrote: > On 10/24/2010 08:14 PM, Blue Swirl wrote: >> >> On Sun, Oct 24, 2010 at 3:34 PM, Avi Kivity<avi@redhat.com> wrote: >> > The current ioport callbacks are not type-safe, in that they accept an >> > "opaque" >> > pointer as an argument whose type must match the argument to the >> > registration >> > function; this is not checked by the compiler. >> > >> > This patch adds an alternative that is type-safe. Instead of an opaque >> > argument, both registation and the callback use a new IOPort type. The >> > callback then uses container_of() to access its main structures. >> > >> > Currently the old and new methods exist side by side; once the old way >> > is gone, >> > we can also save a bunch of memory since the new method requires one >> > pointer >> > per ioport instead of 6. >> > >> > Signed-off-by: Avi Kivity<avi@redhat.com> >> >> If we are going to change the interface, let's do it so that it's >> useful for other uses too: >> http://article.gmane.org/gmane.comp.emulators.qemu/76984 > > I don't really see why we need registration; cpu_register_io() takes > function pointers, a size, and an opaque, and gives an integer handle in > return. With the IOPort object approach, you set up the IOPort with > function pointers, size is implied, and the opaque is derived using > container_of(); the handle is simply the address of the object. With the handle, we can separate setting up the structures at device level, and mapping the object using only the handle at bus or other higher level. Can this be done with the object approach? The purpose of that patch series was to perform the separation for PCI BARs. I wasn't so happy with the series, so I never pushed.
On 10/25/2010 08:38 PM, Blue Swirl wrote: > > > > I don't really see why we need registration; cpu_register_io() takes > > function pointers, a size, and an opaque, and gives an integer handle in > > return. With the IOPort object approach, you set up the IOPort with > > function pointers, size is implied, and the opaque is derived using > > container_of(); the handle is simply the address of the object. > > With the handle, we can separate setting up the structures at device > level, and mapping the object using only the handle at bus or other > higher level. Can this be done with the object approach? I believe so. The handle is simply an indirect pointer, no? > The purpose of that patch series was to perform the separation for PCI > BARs. I wasn't so happy with the series, so I never pushed. In fact I think an IOPort is even more suitable; if we need additional attributes we can use a derived object: struct PCIIOPort { IOPort ioport; /* additional fields */ };
On Tue, Oct 26, 2010 at 8:05 AM, Avi Kivity <avi@redhat.com> wrote: > On 10/25/2010 08:38 PM, Blue Swirl wrote: >> >> > >> > I don't really see why we need registration; cpu_register_io() takes >> > function pointers, a size, and an opaque, and gives an integer handle >> > in >> > return. With the IOPort object approach, you set up the IOPort with >> > function pointers, size is implied, and the opaque is derived using >> > container_of(); the handle is simply the address of the object. >> >> With the handle, we can separate setting up the structures at device >> level, and mapping the object using only the handle at bus or other >> higher level. Can this be done with the object approach? > > I believe so. The handle is simply an indirect pointer, no? Yes, but then the object should also contain size information. That should not be needed for mapping at higher level. >> The purpose of that patch series was to perform the separation for PCI >> BARs. I wasn't so happy with the series, so I never pushed. > > In fact I think an IOPort is even more suitable; if we need additional > attributes we can use a derived object: > > struct PCIIOPort { > IOPort ioport; > /* additional fields */ > }; One issue with my series was that it would be great if the devices just had some BAR structures (used by PCI layer to map the devices) inside PCI/qdev structures, but I invented that too late. Maybe this can be addressed in your design?
On 10/26/2010 05:09 PM, Blue Swirl wrote: > On Tue, Oct 26, 2010 at 8:05 AM, Avi Kivity<avi@redhat.com> wrote: > > On 10/25/2010 08:38 PM, Blue Swirl wrote: > >> > >> > > >> > I don't really see why we need registration; cpu_register_io() takes > >> > function pointers, a size, and an opaque, and gives an integer handle > >> > in > >> > return. With the IOPort object approach, you set up the IOPort with > >> > function pointers, size is implied, and the opaque is derived using > >> > container_of(); the handle is simply the address of the object. > >> > >> With the handle, we can separate setting up the structures at device > >> level, and mapping the object using only the handle at bus or other > >> higher level. Can this be done with the object approach? > > > > I believe so. The handle is simply an indirect pointer, no? > > Yes, but then the object should also contain size information. That > should not be needed for mapping at higher level. Sorry, I don't follow your meaning. When I said "size is implied" I meant that the IOPort object has a separate function pointer for sizes 1, 2, and 4, so it ioport_register() doesn't need a size parameter. But I don't see how that relates to your comment. > >> The purpose of that patch series was to perform the separation for PCI > >> BARs. I wasn't so happy with the series, so I never pushed. > > > > In fact I think an IOPort is even more suitable; if we need additional > > attributes we can use a derived object: > > > > struct PCIIOPort { > > IOPort ioport; > > /* additional fields */ > > }; > > One issue with my series was that it would be great if the devices > just had some BAR structures (used by PCI layer to map the devices) > inside PCI/qdev structures, but I invented that too late. Maybe this > can be addressed in your design? It looks to be orthogonal. It would be great to have a BAR object; that object can then use your API, my API, or the existing API.
On 10/26/2010 12:18 PM, Avi Kivity wrote: > On 10/26/2010 05:09 PM, Blue Swirl wrote: >> On Tue, Oct 26, 2010 at 8:05 AM, Avi Kivity<avi@redhat.com> wrote: >> > On 10/25/2010 08:38 PM, Blue Swirl wrote: >> >> >> >> > >> >> > I don't really see why we need registration; >> cpu_register_io() takes >> >> > function pointers, a size, and an opaque, and gives an >> integer handle >> >> > in >> >> > return. With the IOPort object approach, you set up the >> IOPort with >> >> > function pointers, size is implied, and the opaque is derived >> using >> >> > container_of(); the handle is simply the address of the object. >> >> >> >> With the handle, we can separate setting up the structures at device >> >> level, and mapping the object using only the handle at bus or other >> >> higher level. Can this be done with the object approach? >> > >> > I believe so. The handle is simply an indirect pointer, no? >> >> Yes, but then the object should also contain size information. That >> should not be needed for mapping at higher level. > > Sorry, I don't follow your meaning. > > When I said "size is implied" I meant that the IOPort object has a > separate function pointer for sizes 1, 2, and 4, so it > ioport_register() doesn't need a size parameter. But I don't see how > that relates to your comment. Yeah, I don't think it makes sense to combine "this is how to dispatch I/O" with "this is a region of I/O address space". I think an IORegion should contain an IOPort structure though. I think the name needs rethinking. Maybe: struct PortIOHandler; struct MemoryIOHandler; And it would be good to add a memory callback to this series too. Regards, Anthony Liguori >> >> The purpose of that patch series was to perform the separation >> for PCI >> >> BARs. I wasn't so happy with the series, so I never pushed. >> > >> > In fact I think an IOPort is even more suitable; if we need >> additional >> > attributes we can use a derived object: >> > >> > struct PCIIOPort { >> > IOPort ioport; >> > /* additional fields */ >> > }; >> >> One issue with my series was that it would be great if the devices >> just had some BAR structures (used by PCI layer to map the devices) >> inside PCI/qdev structures, but I invented that too late. Maybe this >> can be addressed in your design? > > It looks to be orthogonal. It would be great to have a BAR object; > that object can then use your API, my API, or the existing API. >
On 10/26/2010 07:27 PM, Anthony Liguori wrote: >> Sorry, I don't follow your meaning. >> >> When I said "size is implied" I meant that the IOPort object has a >> separate function pointer for sizes 1, 2, and 4, so it >> ioport_register() doesn't need a size parameter. But I don't see how >> that relates to your comment. > > > Yeah, I don't think it makes sense to combine "this is how to dispatch > I/O" with "this is a region of I/O address space". Oh, so Blue meant the size of the region in ports, not the size of the individual ports. I think that putting the range length (but not base address) in the IOPort structure may make sense. > > I think an IORegion should contain an IOPort structure though. I > think the name needs rethinking. > > Maybe: > > struct PortIOHandler; > struct MemoryIOHandler; Why two types? I think some devices use PIO on a PC and MMIO on other architectures. Sharing the type would allow sharing code.
On Tue, Oct 26, 2010 at 5:35 PM, Avi Kivity <avi@redhat.com> wrote: > On 10/26/2010 07:27 PM, Anthony Liguori wrote: >>> >>> Sorry, I don't follow your meaning. >>> >>> When I said "size is implied" I meant that the IOPort object has a >>> separate function pointer for sizes 1, 2, and 4, so it ioport_register() >>> doesn't need a size parameter. But I don't see how that relates to your >>> comment. >> >> >> Yeah, I don't think it makes sense to combine "this is how to dispatch >> I/O" with "this is a region of I/O address space". > > Oh, so Blue meant the size of the region in ports, not the size of the > individual ports. I think that putting the range length (but not base > address) in the IOPort structure may make sense. Yes, that's what I meant. Consider for example the handlers: they expect that the port is within some range. >> >> I think an IORegion should contain an IOPort structure though. I think >> the name needs rethinking. >> >> Maybe: >> >> struct PortIOHandler; >> struct MemoryIOHandler; > > Why two types? I think some devices use PIO on a PC and MMIO on other > architectures. Sharing the type would allow sharing code. Then there are the functions provided by rwhandler.c. I think that interface makes even more sense compared to 8/16/32 (and 64?) bit handlers in many cases.
On 10/26/2010 08:33 PM, Blue Swirl wrote: > > > > Why two types? I think some devices use PIO on a PC and MMIO on other > > architectures. Sharing the type would allow sharing code. > > Then there are the functions provided by rwhandler.c. I think that > interface makes even more sense compared to 8/16/32 (and 64?) bit > handlers in many cases. On the other hand, that makes the transition harder. Perhaps we can have a type with {read,write}(addr, width), and another built on top that provides the traditional {read,write}[bwl](addr) to ease the transition.
On Wed, Oct 27, 2010 at 9:26 AM, Avi Kivity <avi@redhat.com> wrote: > On 10/26/2010 08:33 PM, Blue Swirl wrote: >> >> > >> > Why two types? I think some devices use PIO on a PC and MMIO on other >> > architectures. Sharing the type would allow sharing code. >> >> Then there are the functions provided by rwhandler.c. I think that >> interface makes even more sense compared to 8/16/32 (and 64?) bit >> handlers in many cases. > > On the other hand, that makes the transition harder. > > Perhaps we can have a type with {read,write}(addr, width), and another built > on top that provides the traditional {read,write}[bwl](addr) to ease the > transition. That should work.
diff --git a/ioport.c b/ioport.c index ec3dc65..47eafc3 100644 --- a/ioport.c +++ b/ioport.c @@ -174,6 +174,70 @@ int register_ioport_write(pio_addr_t start, int length, int size, return 0; } +static uint32_t ioport_readb_thunk(void *opaque, uint32_t addr) +{ + IOPort *ioport = opaque; + + return ioport->ops->readb(ioport, addr); +} + +static uint32_t ioport_readw_thunk(void *opaque, uint32_t addr) +{ + IOPort *ioport = opaque; + + return ioport->ops->readw(ioport, addr); +} + +static uint32_t ioport_readl_thunk(void *opaque, uint32_t addr) +{ + IOPort *ioport = opaque; + + return ioport->ops->readl(ioport, addr); +} + +static void ioport_writeb_thunk(void *opaque, uint32_t addr, uint32_t data) +{ + IOPort *ioport = opaque; + + return ioport->ops->writeb(ioport, addr, data); +} + +static void ioport_writew_thunk(void *opaque, uint32_t addr, uint32_t data) +{ + IOPort *ioport = opaque; + + return ioport->ops->writew(ioport, addr, data); +} + +static void ioport_writel_thunk(void *opaque, uint32_t addr, uint32_t data) +{ + IOPort *ioport = opaque; + + return ioport->ops->writel(ioport, addr, data); +} + +void ioport_register(IOPort *ioport, pio_addr_t start, pio_addr_t length) +{ + if (ioport->ops->readb) { + register_ioport_read(start, length, 1, ioport_readb_thunk, ioport); + } + if (ioport->ops->readw) { + register_ioport_read(start, length, 2, ioport_readw_thunk, ioport); + } + if (ioport->ops->readl) { + register_ioport_read(start, length, 4, ioport_readl_thunk, ioport); + } + if (ioport->ops->writeb) { + register_ioport_write(start, length, 1, ioport_writeb_thunk, ioport); + } + if (ioport->ops->writew) { + register_ioport_write(start, length, 2, ioport_writew_thunk, ioport); + } + if (ioport->ops->writel) { + register_ioport_write(start, length, 4, ioport_writel_thunk, ioport); + } +} + void isa_unassign_ioport(pio_addr_t start, int length) { int i; diff --git a/ioport.h b/ioport.h index 3d3c8a3..8036e59 100644 --- a/ioport.h +++ b/ioport.h @@ -36,6 +36,22 @@ typedef uint32_t pio_addr_t; typedef void (IOPortWriteFunc)(void *opaque, uint32_t address, uint32_t data); typedef uint32_t (IOPortReadFunc)(void *opaque, uint32_t address); +struct IOPort; + +typedef struct IOPortOps { + uint32_t (*readb)(struct IOPort *ioport, pio_addr_t addr); + uint32_t (*readw)(struct IOPort *ioport, pio_addr_t addr); + uint32_t (*readl)(struct IOPort *ioport, pio_addr_t addr); + void (*writeb)(struct IOPort *ioport, pio_addr_t addr, uint32_t data); + void (*writew)(struct IOPort *ioport, pio_addr_t addr, uint32_t data); + void (*writel)(struct IOPort *ioport, pio_addr_t addr, uint32_t data); +} IOPortOps; + +typedef struct IOPort { + IOPortOps *ops; +} IOPort; + +void ioport_register(IOPort *ioport, pio_addr_t start, pio_addr_t length); int register_ioport_read(pio_addr_t start, int length, int size, IOPortReadFunc *func, void *opaque); int register_ioport_write(pio_addr_t start, int length, int size,
The current ioport callbacks are not type-safe, in that they accept an "opaque" pointer as an argument whose type must match the argument to the registration function; this is not checked by the compiler. This patch adds an alternative that is type-safe. Instead of an opaque argument, both registation and the callback use a new IOPort type. The callback then uses container_of() to access its main structures. Currently the old and new methods exist side by side; once the old way is gone, we can also save a bunch of memory since the new method requires one pointer per ioport instead of 6. Signed-off-by: Avi Kivity <avi@redhat.com> --- ioport.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ ioport.h | 16 +++++++++++++++ 2 files changed, 80 insertions(+), 0 deletions(-)