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

[Anthony Liguori <anthony@codemonkey.ws>]

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?
>
>