| Message ID | 1306392135-16993-1-git-send-email-levinsasha928@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2011-05-26 at 09:42 +0300, Sasha Levin wrote: > Allow specifying an optional parameter when registering an > ioport range. The callback functions provided by the registering > module will be called with the same parameter. > > This may be used to keep context during callbacks on IO operations. > > Signed-off-by: Sasha Levin <levinsasha928@gmail.com> > --- > tools/kvm/include/kvm/ioport.h | 3 ++ > tools/kvm/ioport.c | 54 +++++++++++++++++++++++++++++---------- > 2 files changed, 43 insertions(+), 14 deletions(-) > > diff --git a/tools/kvm/include/kvm/ioport.h b/tools/kvm/include/kvm/ioport.h > index 8253938..2a8d74d 100644 > --- a/tools/kvm/include/kvm/ioport.h > +++ b/tools/kvm/include/kvm/ioport.h > @@ -25,11 +25,14 @@ struct kvm; > struct ioport_operations { > bool (*io_in)(struct kvm *kvm, u16 port, void *data, int size, u32 count); > bool (*io_out)(struct kvm *kvm, u16 port, void *data, int size, u32 count); > + bool (*io_in_param)(struct kvm *kvm, u16 port, void *data, int size, u32 count, void *param); > + bool (*io_out_param)(struct kvm *kvm, u16 port, void *data, int size, u32 count, void *param); So why not make that 'param' unconditional for io_in and io_out and just pass NULL if it's not needed? -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2011-05-26 at 11:53 +0300, Pekka Enberg wrote: > On Thu, 2011-05-26 at 09:42 +0300, Sasha Levin wrote: > > Allow specifying an optional parameter when registering an > > ioport range. The callback functions provided by the registering > > module will be called with the same parameter. > > > > This may be used to keep context during callbacks on IO operations. > > > > Signed-off-by: Sasha Levin <levinsasha928@gmail.com> > > --- > > tools/kvm/include/kvm/ioport.h | 3 ++ > > tools/kvm/ioport.c | 54 +++++++++++++++++++++++++++++---------- > > 2 files changed, 43 insertions(+), 14 deletions(-) > > > > diff --git a/tools/kvm/include/kvm/ioport.h b/tools/kvm/include/kvm/ioport.h > > index 8253938..2a8d74d 100644 > > --- a/tools/kvm/include/kvm/ioport.h > > +++ b/tools/kvm/include/kvm/ioport.h > > @@ -25,11 +25,14 @@ struct kvm; > > struct ioport_operations { > > bool (*io_in)(struct kvm *kvm, u16 port, void *data, int size, u32 count); > > bool (*io_out)(struct kvm *kvm, u16 port, void *data, int size, u32 count); > > + bool (*io_in_param)(struct kvm *kvm, u16 port, void *data, int size, u32 count, void *param); > > + bool (*io_out_param)(struct kvm *kvm, u16 port, void *data, int size, u32 count, void *param); > > So why not make that 'param' unconditional for io_in and io_out and just > pass NULL if it's not needed? > I've wanted to keep the original interface clean, Most of the IO port users don't (and probably won't) require a parameter.
On Thu, May 26, 2011 at 12:02 PM, Sasha Levin <levinsasha928@gmail.com> wrote: > On Thu, 2011-05-26 at 11:53 +0300, Pekka Enberg wrote: >> On Thu, 2011-05-26 at 09:42 +0300, Sasha Levin wrote: >> > Allow specifying an optional parameter when registering an >> > ioport range. The callback functions provided by the registering >> > module will be called with the same parameter. >> > >> > This may be used to keep context during callbacks on IO operations. >> > >> > Signed-off-by: Sasha Levin <levinsasha928@gmail.com> >> > --- >> > tools/kvm/include/kvm/ioport.h | 3 ++ >> > tools/kvm/ioport.c | 54 +++++++++++++++++++++++++++++---------- >> > 2 files changed, 43 insertions(+), 14 deletions(-) >> > >> > diff --git a/tools/kvm/include/kvm/ioport.h b/tools/kvm/include/kvm/ioport.h >> > index 8253938..2a8d74d 100644 >> > --- a/tools/kvm/include/kvm/ioport.h >> > +++ b/tools/kvm/include/kvm/ioport.h >> > @@ -25,11 +25,14 @@ struct kvm; >> > struct ioport_operations { >> > bool (*io_in)(struct kvm *kvm, u16 port, void *data, int size, u32 count); >> > bool (*io_out)(struct kvm *kvm, u16 port, void *data, int size, u32 count); >> > + bool (*io_in_param)(struct kvm *kvm, u16 port, void *data, int size, u32 count, void *param); >> > + bool (*io_out_param)(struct kvm *kvm, u16 port, void *data, int size, u32 count, void *param); >> >> So why not make that 'param' unconditional for io_in and io_out and just >> pass NULL if it's not needed? >> > > I've wanted to keep the original interface clean, Most of the IO port > users don't (and probably won't) require a parameter. Well now struct ioport_operations isn't very clean is it - or the code that needs to determine which function pointer to call?-) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2011-05-26 at 12:04 +0300, Pekka Enberg wrote: > On Thu, May 26, 2011 at 12:02 PM, Sasha Levin <levinsasha928@gmail.com> wrote: > > On Thu, 2011-05-26 at 11:53 +0300, Pekka Enberg wrote: > >> On Thu, 2011-05-26 at 09:42 +0300, Sasha Levin wrote: > >> > Allow specifying an optional parameter when registering an > >> > ioport range. The callback functions provided by the registering > >> > module will be called with the same parameter. > >> > > >> > This may be used to keep context during callbacks on IO operations. > >> > > >> > Signed-off-by: Sasha Levin <levinsasha928@gmail.com> > >> > --- > >> > tools/kvm/include/kvm/ioport.h | 3 ++ > >> > tools/kvm/ioport.c | 54 +++++++++++++++++++++++++++++---------- > >> > 2 files changed, 43 insertions(+), 14 deletions(-) > >> > > >> > diff --git a/tools/kvm/include/kvm/ioport.h b/tools/kvm/include/kvm/ioport.h > >> > index 8253938..2a8d74d 100644 > >> > --- a/tools/kvm/include/kvm/ioport.h > >> > +++ b/tools/kvm/include/kvm/ioport.h > >> > @@ -25,11 +25,14 @@ struct kvm; > >> > struct ioport_operations { > >> > bool (*io_in)(struct kvm *kvm, u16 port, void *data, int size, u32 count); > >> > bool (*io_out)(struct kvm *kvm, u16 port, void *data, int size, u32 count); > >> > + bool (*io_in_param)(struct kvm *kvm, u16 port, void *data, int size, u32 count, void *param); > >> > + bool (*io_out_param)(struct kvm *kvm, u16 port, void *data, int size, u32 count, void *param); > >> > >> So why not make that 'param' unconditional for io_in and io_out and just > >> pass NULL if it's not needed? > >> > > > > I've wanted to keep the original interface clean, Most of the IO port > > users don't (and probably won't) require a parameter. > > Well now struct ioport_operations isn't very clean is it - or the code > that needs to determine which function pointer to call?-) struct ioport_operations is a bit more messy, but it's one spot instead of adding a 'parameter' to each module that doesn't really need it. My assumption is that most ioport users now and in the future won't need it, it just solves several special cases more easily (multiple devices which share same handling functions).
On Thu, May 26, 2011 at 12:14 PM, Sasha Levin <levinsasha928@gmail.com> wrote: >> > I've wanted to keep the original interface clean, Most of the IO port >> > users don't (and probably won't) require a parameter. >> >> Well now struct ioport_operations isn't very clean is it - or the code >> that needs to determine which function pointer to call?-) > > struct ioport_operations is a bit more messy, but it's one spot instead > of adding a 'parameter' to each module that doesn't really need it. > > My assumption is that most ioport users now and in the future won't need > it, it just solves several special cases more easily (multiple devices > which share same handling functions). Hey, that's not an excuse to make struct ioport_operations 'bit messy'! Look at any kernel code that uses ops like we do here and you will see we don't do APIs like this. One option here is to rename 'struct ioport_entry' to 'struct ioport' and pass a pointer to that as the first argument to all of the ops. That's what most APIs in the kernel do anyway. Pekka -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2011-05-26 at 12:20 +0300, Pekka Enberg wrote: > On Thu, May 26, 2011 at 12:14 PM, Sasha Levin <levinsasha928@gmail.com> wrote: > >> > I've wanted to keep the original interface clean, Most of the IO port > >> > users don't (and probably won't) require a parameter. > >> > >> Well now struct ioport_operations isn't very clean is it - or the code > >> that needs to determine which function pointer to call?-) > > > > struct ioport_operations is a bit more messy, but it's one spot instead > > of adding a 'parameter' to each module that doesn't really need it. > > > > My assumption is that most ioport users now and in the future won't need > > it, it just solves several special cases more easily (multiple devices > > which share same handling functions). > > Hey, that's not an excuse to make struct ioport_operations 'bit > messy'! Look at any kernel code that uses ops like we do here and you > will see we don't do APIs like this. > > One option here is to rename 'struct ioport_entry' to 'struct ioport' > and pass a pointer to that as the first argument to all of the ops. > That's what most APIs in the kernel do anyway. Why do it like that? this way users of the callback functions will need to know the internal structure of struct ioport_entry.
On Thu, May 26, 2011 at 12:38 PM, Sasha Levin <levinsasha928@gmail.com> wrote: >> One option here is to rename 'struct ioport_entry' to 'struct ioport' >> and pass a pointer to that as the first argument to all of the ops. >> That's what most APIs in the kernel do anyway. > > Why do it like that? this way users of the callback functions will need > to know the internal structure of struct ioport_entry. Look at 'struct inode' or similar data structure in the kernel. That's how we do it. You can then also do s/params/priv/. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sasha, On Thu, May 26, 2011 at 12:38 PM, Sasha Levin <levinsasha928@gmail.com> wrote: >>> One option here is to rename 'struct ioport_entry' to 'struct ioport' >>> and pass a pointer to that as the first argument to all of the ops. >>> That's what most APIs in the kernel do anyway. >> >> Why do it like that? this way users of the callback functions will need >> to know the internal structure of struct ioport_entry. On Thu, May 26, 2011 at 12:43 PM, Pekka Enberg <penberg@kernel.org> wrote: > Look at 'struct inode' or similar data structure in the kernel. That's > how we do it. You can then also do s/params/priv/. Btw, the whole notion of 'internal structure' for structs in C code is a pretty broken concept. In most cases, you just end up passing untyped fragments of the data to callers which makes following the data flow in code difficult. Passing 'struct ioport' down to the code makes the code more obvious and readable. Encapsulation is important but emulating that with hiding structs in .c files isn't helpful at all. Face it, there's no proper support for that in C so you just need to rely on conventions to do it. Pekka -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/tools/kvm/include/kvm/ioport.h b/tools/kvm/include/kvm/ioport.h index 8253938..2a8d74d 100644 --- a/tools/kvm/include/kvm/ioport.h +++ b/tools/kvm/include/kvm/ioport.h @@ -25,11 +25,14 @@ struct kvm; struct ioport_operations { bool (*io_in)(struct kvm *kvm, u16 port, void *data, int size, u32 count); bool (*io_out)(struct kvm *kvm, u16 port, void *data, int size, u32 count); + bool (*io_in_param)(struct kvm *kvm, u16 port, void *data, int size, u32 count, void *param); + bool (*io_out_param)(struct kvm *kvm, u16 port, void *data, int size, u32 count, void *param); }; void ioport__setup_legacy(void); void ioport__register(u16 port, struct ioport_operations *ops, int count); +void ioport__register_param(u16 port, struct ioport_operations *ops, int count, void *param); static inline u8 ioport__read8(u8 *data) { diff --git a/tools/kvm/ioport.c b/tools/kvm/ioport.c index 1f13960..159d089 100644 --- a/tools/kvm/ioport.c +++ b/tools/kvm/ioport.c @@ -18,6 +18,7 @@ struct ioport_entry { struct rb_int_node node; struct ioport_operations *ops; + void *param; }; static struct rb_root ioport_tree = RB_ROOT; @@ -89,6 +90,29 @@ void ioport__register(u16 port, struct ioport_operations *ops, int count) ioport_insert(&ioport_tree, entry); } +void ioport__register_param(u16 port, struct ioport_operations *ops, int count, void *param) +{ + struct ioport_entry *entry; + + entry = ioport_search(&ioport_tree, port); + if (entry) { + pr_warning("ioport re-registered: %x", port); + rb_int_erase(&ioport_tree, &entry->node); + } + + entry = malloc(sizeof(*entry)); + if (entry == NULL) + die("Failed allocating new ioport entry"); + + *entry = (struct ioport_entry) { + .node = RB_INT_INIT(port, port + count), + .ops = ops, + .param = param, + }; + + ioport_insert(&ioport_tree, entry); +} + static const char *to_direction(int direction) { if (direction == KVM_EXIT_IO_IN) @@ -105,30 +129,32 @@ static void ioport_error(u16 port, void *data, int direction, int size, u32 coun bool kvm__emulate_io(struct kvm *kvm, u16 port, void *data, int direction, int size, u32 count) { struct ioport_operations *ops; - bool ret; + bool ret = false; struct ioport_entry *entry; + void *param; entry = ioport_search(&ioport_tree, port); if (!entry) goto error; - ops = entry->ops; + ops = entry->ops; + param = entry->param; if (direction == KVM_EXIT_IO_IN) { - if (!ops->io_in) - goto error; - - ret = ops->io_in(kvm, port, data, size, count); - if (!ret) - goto error; + if (!param && ops->io_in) + ret = ops->io_in(kvm, port, data, size, count); + if (param && ops->io_in_param) + ret = ops->io_in_param(kvm, port, data, size, count, param); } else { - if (!ops->io_out) - goto error; - - ret = ops->io_out(kvm, port, data, size, count); - if (!ret) - goto error; + if (!param && ops->io_out) + ret = ops->io_out(kvm, port, data, size, count); + if (param && ops->io_out_param) + ret = ops->io_out_param(kvm, port, data, size, count, param); } + + if (!ret) + goto error; + return true; error: if (ioport_debug)
Allow specifying an optional parameter when registering an ioport range. The callback functions provided by the registering module will be called with the same parameter. This may be used to keep context during callbacks on IO operations. Signed-off-by: Sasha Levin <levinsasha928@gmail.com> --- tools/kvm/include/kvm/ioport.h | 3 ++ tools/kvm/ioport.c | 54 +++++++++++++++++++++++++++++---------- 2 files changed, 43 insertions(+), 14 deletions(-)