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

[Avi Kivity]

(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 linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/