[Qemu-devel] Re: [PATCHv2 02/12] kvm: add API to set ioeventfd

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Feb 25, 2010 at 01:19:30PM -0600, Anthony Liguori wrote:
> On 02/25/2010 12:28 PM, Michael S. Tsirkin wrote:
>> Comment on kvm usage: rather than require users to do if (kvm_enabled())
>> and/or ifdefs, this patch adds an API that, internally, is defined to
>> stub function on non-kvm build, and checks kvm_enabled for non-kvm
>> run.
>>
>> While rest of qemu code still uses if (kvm_enabled()), I think this
>> approach is cleaner, and we should convert rest of code to it
>> long term.
>>    
>
> I'm not opposed to that.
>
>> Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
>> ---
>>
>> Avi, Marcelo, pls review/ack.
>>
>>   kvm-all.c |   22 ++++++++++++++++++++++
>>   kvm.h     |   16 ++++++++++++++++
>>   2 files changed, 38 insertions(+), 0 deletions(-)
>>
>> diff --git a/kvm-all.c b/kvm-all.c
>> index 1a02076..9742791 100644
>> --- a/kvm-all.c
>> +++ b/kvm-all.c
>> @@ -1138,3 +1138,25 @@ int kvm_set_signal_mask(CPUState *env, const sigset_t *sigset)
>>
>>       return r;
>>   }
>> +
>> +#ifdef KVM_IOEVENTFD
>> +int kvm_set_ioeventfd(uint16_t addr, uint16_t data, int fd, bool assigned)
>>    
>
> I think this API could use some love.  You're using a very limited set  
> of things that ioeventfd can do and you're multiplexing creation and  
> destruction in a single call.
>
> I think:
>
> kvm_set_ioeventfd_pio_word(int fd, uint16_t addr, uint16_t data);
> kvm_unset_ioeventfd_pio_word(int fd, uint16_t addr, uint16_t data);
>
> Would be better.  Alternatively, an API that matched ioeventfd exactly:
>
> kvm_set_ioeventfd(int fd, uint64_t addr, int size, uint64_t data, int  
> flags);
> kvm_unset_ioeventfd(...);
>
> Could work too.
>
> Regards,
>
> Anthony Liguori
>

So I renamed to kvm_set_ioeventfd_pio_word, but I still left assign
boolean in place because both implementation and usage take up
less code this way.

It's just an internal function, so no biggie to change it later ...

-- 
MST