Re: [Qemu-devel] [PATCH 1/2] Type-safe ioport callbacks
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.
--
error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [PATCH 1/2] Type-safe ioport callbacks
On Wed, Oct 27, 2010 at 9:26 AM, Avi Kivity a...@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.
Re: [Qemu-devel] [PATCH 1/2] Type-safe ioport callbacks
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 */
};
--
error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [PATCH 1/2] Type-safe ioport callbacks
On Tue, Oct 26, 2010 at 8:05 AM, Avi Kivity a...@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?
Re: [Qemu-devel] [PATCH 1/2] Type-safe ioport callbacks
On 10/26/2010 05:09 PM, Blue Swirl wrote:
On Tue, Oct 26, 2010 at 8:05 AM, Avi Kivitya...@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.
--
error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [PATCH 1/2] Type-safe ioport callbacks
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 Kivitya...@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.
Re: [Qemu-devel] [PATCH 1/2] Type-safe ioport callbacks
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. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [PATCH 1/2] Type-safe ioport callbacks
On Tue, Oct 26, 2010 at 5:35 PM, Avi Kivity a...@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.
Re: [Qemu-devel] [PATCH 1/2] Type-safe ioport callbacks
On 10/24/2010 08:14 PM, Blue Swirl wrote:
On Sun, Oct 24, 2010 at 3:34 PM, Avi Kivitya...@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 Kivitya...@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.
--
error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [PATCH 1/2] Type-safe ioport callbacks
On Mon, Oct 25, 2010 at 10:00 AM, Avi Kivity a...@redhat.com wrote: On 10/24/2010 08:14 PM, Blue Swirl wrote: On Sun, Oct 24, 2010 at 3:34 PM, Avi Kivitya...@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 Kivitya...@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.
[Qemu-devel] [PATCH 1/2] Type-safe ioport callbacks
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 a...@redhat.com
---
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;
+} 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,
--
1.7.3.1
Re: [Qemu-devel] [PATCH 1/2] Type-safe ioport callbacks
On Sun, Oct 24, 2010 at 3:34 PM, Avi Kivity a...@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 a...@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
