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

login
register
mail settings
Submitter Michael S. Tsirkin
Date Jan. 11, 2010, 5:17 p.m.
Message ID <20100111171703.GC11936@redhat.com>
Download mbox | patch
Permalink /patch/42634/
State New
Headers show

Comments

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(-)
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

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