Re: [KVM PATCH v9 2/2] KVM: add iosignalfd support

Avi Kivity
2009-07-07T11:53:18Z

(adding Davide, there's a small comment for you in the middle, search
for eventfd)

On 07/07/2009 02:20 PM, Michael S. Tsirkin wrote:
>
>> @@ -307,6 +307,19 @@ struct kvm_guest_debug {
>> struct kvm_guest_debug_arch arch;
>> };
>>
>> +#define KVM_IOSIGNALFD_FLAG_TRIGGER (1<< 0) /* trigger is valid */
>>
>
> can we rename trigger -> value?
>

Or maybe data_match?

Speaking of renames, how about IOSIGNALFD -> IOEVENTFD? I have some
vague uneasiness seeing signals all the time.

>> +#define KVM_IOSIGNALFD_FLAG_PIO (1<< 1) /* is a pio (otherwise mmio) */
>> +#define KVM_IOSIGNALFD_FLAG_DEASSIGN (1<< 2)
>> +
>> +struct kvm_iosignalfd {
>> + __u64 trigger;
>>
>
> for length<8, it's the 8*len least significant bits that are used, right?
> That's a bit ugly ... Maybe just put an 8 byte array here instead, then
> the first len bytes are valid.
>
>

We're matching the value as the guest wrote it. I think this is fine.

>> + struct kvm_io_device dev;
>> + int wildcard:1;
>>
>
> don't use bitfields
>

Yeah, bool is better.

>> + /* address-range must be precise for a hit */
>>
>
> So there's apparently no way to specify that
> you want 1,2, or 4 byte writes at address X?
>

Why would you want that?

>
>> + return false;
>> +
>> + if (p->wildcard)
>> + /* all else equal, wildcard is always a hit */
>> + return true;
>> +
>> + /* otherwise, we have to actually compare the data */
>> +
>> + BUG_ON(!IS_ALIGNED((unsigned long)val, len));
>> +
>> + switch (len) {
>> + case 1:
>> + _val = *(u8 *)val;
>> + break;
>> + case 2:
>> + _val = *(u16 *)val;
>> + break;
>> + case 4:
>> + _val = *(u32 *)val;
>> + break;
>> + case 8:
>> + _val = *(u64 *)val;
>> + break;
>> + default:
>> + return false;
>> + }
>> +
>> + return _val == p->match ? true : false;
>>
>
> Life be simpler if we use an 8 byte array for match
> and just do memcmp here.
>

My plan is to change the io_dev interface to pass a u64.

>> +
>> + eventfd = eventfd_ctx_fdget(args->fd);
>> + if (IS_ERR(eventfd))
>> + return PTR_ERR(eventfd);
>>
>
> since this eventfd is kept around indefinitely, we should keep the
> file * around as well, so that this eventfd is accounted for
> properly with # of open files limit set by the admin.
>

Won't all eventfd_ctx_get() uses suffer from that?

Davide, I think this is better handled in eventfd. Or else we can
ignore it and trust whoever holds the eventfd_ctx to limit the mount of
objects.

>> +
>> +int
>> +kvm_iosignalfd(struct kvm *kvm, struct kvm_iosignalfd *args)
>> +{
>> + if (args->flags& KVM_IOSIGNALFD_FLAG_DEASSIGN)
>> + return kvm_deassign_iosignalfd(kvm, args);
>>
>
> Better check that only known flag values are present.
> Otherwise when you add more flags things just break
> silently.
>

Good comment and something that we miss a lot.

>> + case KVM_IOSIGNALFD: {
>> + struct kvm_iosignalfd data;
>> +
>> + r = -EFAULT;
>>
>
> this trick is nice, it saves a line of code for the closing brace
> but why waste it on an empty line above then?
>

Traditionally C code separates declarations from code.

--
error compiling committee.c: too many arguments to function