Messages in this thread | | | Date | Tue, 07 Jul 2009 14:53:18 +0300 | From | Avi Kivity <> | Subject | Re: [KVM PATCH v9 2/2] KVM: add iosignalfd support |
| |
(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
| |