[PATCH-RFC,02/13] kvm: add API to set ioeventfd

Submitted by Michael S. Tsirkin on Jan. 11, 2010, 5:17 p.m.

Details

Message ID 20100111171703.GC11936@redhat.com
State New
Headers show

Commit Message

Michael S. Tsirkin Jan. 11, 2010, 5:17 p.m.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 kvm-all.c |   24 ++++++++++++++++++++++++
 kvm.h     |    3 +++
 2 files changed, 27 insertions(+), 0 deletions(-)

Comments

Anthony Liguori Jan. 12, 2010, 10:35 p.m.
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
Michael S. Tsirkin Jan. 25, 2010, 7:20 p.m.
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?
Michael S. Tsirkin Jan. 25, 2010, 7:28 p.m.
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?
>>
>>
Anthony Liguori Jan. 25, 2010, 7:28 p.m.
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?
>
>
Anthony Liguori Jan. 25, 2010, 7:44 p.m.
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

Patch hide | download patch | download mbox

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