| Message ID | 20100111171703.GC11936@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 01/11/2010 11:17 AM, Michael S. Tsirkin wrote: > Signed-off-by: Michael S. Tsirkin<mst@redhat.com> > --- > kvm-all.c | 24 ++++++++++++++++++++++++ > kvm.h | 3 +++ > 2 files changed, 27 insertions(+), 0 deletions(-) > > diff --git a/kvm-all.c b/kvm-all.c > index a312654..aa00119 100644 > --- a/kvm-all.c > +++ b/kvm-all.c > @@ -1113,3 +1113,27 @@ void kvm_remove_all_breakpoints(CPUState *current_env) > { > } > #endif /* !KVM_CAP_SET_GUEST_DEBUG */ > + > +#ifdef KVM_IOEVENTFD > +int kvm_set_ioeventfd(uint16_t addr, uint16_t data, int fd, bool assigned) > +{ > + struct kvm_ioeventfd kick = { > + .datamatch = data, > + .addr = addr, > + .len = 2, > + .flags = KVM_IOEVENTFD_FLAG_DATAMATCH | KVM_IOEVENTFD_FLAG_PIO, > + .fd = fd, > + }; > + if (!assigned) > + kick.flags |= KVM_IOEVENTFD_FLAG_DEASSIGN; > + int r = kvm_vm_ioctl(kvm_state, KVM_IOEVENTFD,&kick); > + if (r< 0) > + return r; > + return 0; > +} > I think we really ought to try to avoid having global state used here. That means either we need to pass a CPUState to this function or we need to have a ioeventfd be allocated as a structure that is then passed around that can store a copy of the kvm_state fetched through CPUState. I think the later is best. We really don't want ioeventfd scattered throughout the code. Regards, Anthony Liguori
On Tue, Jan 12, 2010 at 04:35:17PM -0600, Anthony Liguori wrote: > On 01/11/2010 11:17 AM, Michael S. Tsirkin wrote: >> Signed-off-by: Michael S. Tsirkin<mst@redhat.com> >> --- >> kvm-all.c | 24 ++++++++++++++++++++++++ >> kvm.h | 3 +++ >> 2 files changed, 27 insertions(+), 0 deletions(-) >> >> diff --git a/kvm-all.c b/kvm-all.c >> index a312654..aa00119 100644 >> --- a/kvm-all.c >> +++ b/kvm-all.c >> @@ -1113,3 +1113,27 @@ void kvm_remove_all_breakpoints(CPUState *current_env) >> { >> } >> #endif /* !KVM_CAP_SET_GUEST_DEBUG */ >> + >> +#ifdef KVM_IOEVENTFD >> +int kvm_set_ioeventfd(uint16_t addr, uint16_t data, int fd, bool assigned) >> +{ >> + struct kvm_ioeventfd kick = { >> + .datamatch = data, >> + .addr = addr, >> + .len = 2, >> + .flags = KVM_IOEVENTFD_FLAG_DATAMATCH | KVM_IOEVENTFD_FLAG_PIO, >> + .fd = fd, >> + }; >> + if (!assigned) >> + kick.flags |= KVM_IOEVENTFD_FLAG_DEASSIGN; >> + int r = kvm_vm_ioctl(kvm_state, KVM_IOEVENTFD,&kick); >> + if (r< 0) >> + return r; >> + return 0; >> +} >> > > I think we really ought to try to avoid having global state used here. > That means either we need to pass a CPUState to this function or we need > to have a ioeventfd be allocated as a structure that is then passed > around that can store a copy of the kvm_state fetched through CPUState. > > I think the later is best. We really don't want ioeventfd scattered > throughout the code. > > Regards, > > Anthony Liguori I don't really understand this comment. I have no idea where to get CPUState in a device and how to get kvm state from there. And all other kvm-all functions use kvm_state already, let's fix them first if we want to get rid of global state? Avi, what's your take on this patch?
On Mon, Jan 25, 2010 at 01:28:49PM -0600, Anthony Liguori wrote: > On 01/25/2010 01:20 PM, Michael S. Tsirkin wrote: >> On Tue, Jan 12, 2010 at 04:35:17PM -0600, Anthony Liguori wrote: >> >>> On 01/11/2010 11:17 AM, Michael S. Tsirkin wrote: >>> >>>> Signed-off-by: Michael S. Tsirkin<mst@redhat.com> >>>> --- >>>> kvm-all.c | 24 ++++++++++++++++++++++++ >>>> kvm.h | 3 +++ >>>> 2 files changed, 27 insertions(+), 0 deletions(-) >>>> >>>> diff --git a/kvm-all.c b/kvm-all.c >>>> index a312654..aa00119 100644 >>>> --- a/kvm-all.c >>>> +++ b/kvm-all.c >>>> @@ -1113,3 +1113,27 @@ void kvm_remove_all_breakpoints(CPUState *current_env) >>>> { >>>> } >>>> #endif /* !KVM_CAP_SET_GUEST_DEBUG */ >>>> + >>>> +#ifdef KVM_IOEVENTFD >>>> +int kvm_set_ioeventfd(uint16_t addr, uint16_t data, int fd, bool assigned) >>>> +{ >>>> + struct kvm_ioeventfd kick = { >>>> + .datamatch = data, >>>> + .addr = addr, >>>> + .len = 2, >>>> + .flags = KVM_IOEVENTFD_FLAG_DATAMATCH | KVM_IOEVENTFD_FLAG_PIO, >>>> + .fd = fd, >>>> + }; >>>> + if (!assigned) >>>> + kick.flags |= KVM_IOEVENTFD_FLAG_DEASSIGN; >>>> + int r = kvm_vm_ioctl(kvm_state, KVM_IOEVENTFD,&kick); >>>> + if (r< 0) >>>> + return r; >>>> + return 0; >>>> +} >>>> >>>> >>> I think we really ought to try to avoid having global state used here. >>> That means either we need to pass a CPUState to this function or we need >>> to have a ioeventfd be allocated as a structure that is then passed >>> around that can store a copy of the kvm_state fetched through CPUState. >>> >>> I think the later is best. We really don't want ioeventfd scattered >>> throughout the code. >>> >>> Regards, >>> >>> Anthony Liguori >>> >> >> I don't really understand this comment. >> I have no idea where to get CPUState in >> a device and how to get kvm state from there. >> > > This function doesn't seem to get called in the current patches so it's > hard to evaluate what the right thing to do is. >> And all other kvm-all functions use kvm_state >> already, let's fix them first if we want to >> get rid of global state? >> > > Any time an operation is tied to a CPU in some way, we should not rely > on global state. Based on the name and the fact that it references PIO, > it strongly suggests to me that it's somehow related to a CPU but since > it's not used in the current code it's hard to validate that. > > Regards, > > Anthony Liguori It is called in the binding. Pls take a look. The function does not reference a specific CPU: it binds an eventfd descriptor to a specific address/data pair for all CPUs. > >> Avi, what's your take on this patch? >> >>
On 01/25/2010 01:20 PM, Michael S. Tsirkin wrote: > On Tue, Jan 12, 2010 at 04:35:17PM -0600, Anthony Liguori wrote: > >> On 01/11/2010 11:17 AM, Michael S. Tsirkin wrote: >> >>> Signed-off-by: Michael S. Tsirkin<mst@redhat.com> >>> --- >>> kvm-all.c | 24 ++++++++++++++++++++++++ >>> kvm.h | 3 +++ >>> 2 files changed, 27 insertions(+), 0 deletions(-) >>> >>> diff --git a/kvm-all.c b/kvm-all.c >>> index a312654..aa00119 100644 >>> --- a/kvm-all.c >>> +++ b/kvm-all.c >>> @@ -1113,3 +1113,27 @@ void kvm_remove_all_breakpoints(CPUState *current_env) >>> { >>> } >>> #endif /* !KVM_CAP_SET_GUEST_DEBUG */ >>> + >>> +#ifdef KVM_IOEVENTFD >>> +int kvm_set_ioeventfd(uint16_t addr, uint16_t data, int fd, bool assigned) >>> +{ >>> + struct kvm_ioeventfd kick = { >>> + .datamatch = data, >>> + .addr = addr, >>> + .len = 2, >>> + .flags = KVM_IOEVENTFD_FLAG_DATAMATCH | KVM_IOEVENTFD_FLAG_PIO, >>> + .fd = fd, >>> + }; >>> + if (!assigned) >>> + kick.flags |= KVM_IOEVENTFD_FLAG_DEASSIGN; >>> + int r = kvm_vm_ioctl(kvm_state, KVM_IOEVENTFD,&kick); >>> + if (r< 0) >>> + return r; >>> + return 0; >>> +} >>> >>> >> I think we really ought to try to avoid having global state used here. >> That means either we need to pass a CPUState to this function or we need >> to have a ioeventfd be allocated as a structure that is then passed >> around that can store a copy of the kvm_state fetched through CPUState. >> >> I think the later is best. We really don't want ioeventfd scattered >> throughout the code. >> >> Regards, >> >> Anthony Liguori >> > > I don't really understand this comment. > I have no idea where to get CPUState in > a device and how to get kvm state from there. > This function doesn't seem to get called in the current patches so it's hard to evaluate what the right thing to do is. > And all other kvm-all functions use kvm_state > already, let's fix them first if we want to > get rid of global state? > Any time an operation is tied to a CPU in some way, we should not rely on global state. Based on the name and the fact that it references PIO, it strongly suggests to me that it's somehow related to a CPU but since it's not used in the current code it's hard to validate that. Regards, Anthony Liguori > Avi, what's your take on this patch? > >
On 01/25/2010 01:28 PM, Michael S. Tsirkin wrote: > It is called in the binding. Pls take a look. > The function does not reference a specific CPU: it binds > an eventfd descriptor to a specific address/data pair > for all CPUs. > This problem is really a symptom of a bigger problem. Let me respond to another patch to provide better context. Regards, Anthony Liguori
diff --git a/kvm-all.c b/kvm-all.c index a312654..aa00119 100644 --- a/kvm-all.c +++ b/kvm-all.c @@ -1113,3 +1113,27 @@ void kvm_remove_all_breakpoints(CPUState *current_env) { } #endif /* !KVM_CAP_SET_GUEST_DEBUG */ + +#ifdef KVM_IOEVENTFD +int kvm_set_ioeventfd(uint16_t addr, uint16_t data, int fd, bool assigned) +{ + struct kvm_ioeventfd kick = { + .datamatch = data, + .addr = addr, + .len = 2, + .flags = KVM_IOEVENTFD_FLAG_DATAMATCH | KVM_IOEVENTFD_FLAG_PIO, + .fd = fd, + }; + if (!assigned) + kick.flags |= KVM_IOEVENTFD_FLAG_DEASSIGN; + int r = kvm_vm_ioctl(kvm_state, KVM_IOEVENTFD, &kick); + if (r < 0) + return r; + return 0; +} +#else +int kvm_set_ioeventfd(uint16_t data, uint16_t addr, int fd, bool assigned) +{ + return -ENOSYS; +} +#endif diff --git a/kvm.h b/kvm.h index 672d511..2d723b8 100644 --- a/kvm.h +++ b/kvm.h @@ -14,6 +14,7 @@ #ifndef QEMU_KVM_H #define QEMU_KVM_H +#include <stdbool.h> #include "config.h" #include "qemu-queue.h" @@ -131,4 +132,6 @@ static inline void cpu_synchronize_state(CPUState *env) } } +int kvm_set_ioeventfd(uint16_t addr, uint16_t data, int fd, bool assigned); + #endif
Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- kvm-all.c | 24 ++++++++++++++++++++++++ kvm.h | 3 +++ 2 files changed, 27 insertions(+), 0 deletions(-)