Re: [PATCH v3] IO: Intelligent device lookup on bus

[Avi Kivity <avi@redhat.com>]

On 07/27/2011 03:01 PM, Sasha Levin wrote:
> >  >
> >  >  -static inline struct kvm_pic *to_pic(struct kvm_io_device *dev)
> >  >  +static inline struct kvm_pic *to_pic(struct kvm_io_device *dev, gpa_t addr)
> >  >    {
> >  >  -	return container_of(dev, struct kvm_pic, dev);
> >  >  +	switch (addr) {
> >  >  +	case 0x20:
> >  >  +	case 0x21:
> >  >  +		return container_of(dev, struct kvm_pic, dev_master);
> >  >  +	case 0xa0:
> >  >  +	case 0xa1:
> >  >  +		return container_of(dev, struct kvm_pic, dev_slave);
> >  >  +	case 0x4d0:
> >  >  +	case 0x4d1:
> >  >  +		return container_of(dev, struct kvm_pic, dev_eclr);
> >  >  +	}
> >  >  +
> >  >  +	return NULL;
> >  >    }
> >
> >  Somewhat ugly.  I think
> >
> >       int picdev_write_master(...)
> >      {
> >           return pcidev_write(container_of(...), ...);
> >       }
> >
> >  is nicer, no?
>
> It would mean we need a total of 6 wrappers for master, slave and eclr
> instead of this switch, if that sounds ok I'll change it.

IMO, they're better than the switch.

> >
> >
> >  >    	if (ret<   0)
> >  >    		goto out_free_dev;
> >  >    	list_add_tail(&dev->list,&kvm->coalesced_zones);
> >  >  diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
> >  >  index 73358d2..f59c1e8 100644
> >  >  --- a/virt/kvm/eventfd.c
> >  >  +++ b/virt/kvm/eventfd.c
> >  >  @@ -586,7 +586,8 @@ kvm_assign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
> >  >
> >  >    	kvm_iodevice_init(&p->dev,&ioeventfd_ops);
> >  >
> >  >  -	ret = kvm_io_bus_register_dev(kvm, bus_idx,&p->dev);
> >  >  +	ret = kvm_io_bus_register_dev(kvm, bus_idx, p->addr, p->length,
> >  >  +				&p->dev);
> >
> >  Should this be p->length or 1?
>
> We register p->length since when we process a write, the operation
> should be fully contained within the IO space of the device.
>
> We verify that the write happens on the first byte within ioeventfd
> write handler.

Ok.

> >
> >  >    #include<asm/processor.h>
> >  >    #include<asm/io.h>
> >  >  @@ -2391,24 +2393,94 @@ static void kvm_io_bus_destroy(struct kvm_io_bus *bus)
> >  >    	int i;
> >  >
> >  >    	for (i = 0; i<   bus->dev_count; i++) {
> >  >  -		struct kvm_io_device *pos = bus->devs[i];
> >  >  +		struct kvm_io_device *pos = bus->range[i].dev;
> >  >
> >
> >  This will call the destructor three times for the PIC.  Is this safe?
>
> PIC doesn't have a destructor for devices, the code above just does
> nothing for PIC devices.

Ok.

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