[Qemu-devel] Re: [PATCH-RFC 02/13] kvm: add API to set ioeventfd

[Michael S. Tsirkin]

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<address@hidden>
>>>> ---
>>>>    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?
>>
>>