(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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
