Re: [KVM PATCH v2 2/3] kvm: cleanup io_device code

[Gregory Haskins]

Chris Wright wrote:
> * Gregory Haskins (ghaskins@xxxxxxxxxx) wrote:
>   
>> We modernize the io_device code so that we use container_of() instead of
>> dev->private, and move the vtable to a separate ops structure
>> (theoretically allows better caching for multiple instances of the same
>> ops structure)
>>     
>
> Looks like a nice cleanup.  Couple minor nits.
>
>   
>> +static struct kvm_io_device_ops pit_dev_ops = {
>> +	.read     = pit_ioport_read,
>> +	.write    = pit_ioport_write,
>> +	.in_range = pit_in_range,
>> +};
>> +
>> +static struct kvm_io_device_ops speaker_dev_ops = {
>> +	.read     = speaker_ioport_read,
>> +	.write    = speaker_ioport_write,
>> +	.in_range = speaker_in_range,
>> +};
>>     
>
> kvm_io_device_ops instances could be made const.
>   

Ack

>   
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -2227,7 +2227,7 @@ static struct kvm_io_device *vcpu_find_pervcpu_dev(struct kvm_vcpu *vcpu,
>>  
>>  	if (vcpu->arch.apic) {
>>  		dev = &vcpu->arch.apic->dev;
>> -		if (dev->in_range(dev, addr, len, is_write))
>> +		if (dev->ops->in_range(dev, addr, len, is_write))
>>  			return dev;
>>     
>
>   
>> --- a/virt/kvm/iodev.h
>> +++ b/virt/kvm/iodev.h
>> @@ -18,7 +18,9 @@
>>  
>>  #include <linux/kvm_types.h>
>>  
>> -struct kvm_io_device {
>> +struct kvm_io_device;
>> +
>> +struct kvm_io_device_ops {
>>  	void (*read)(struct kvm_io_device *this,
>>  		     gpa_t addr,
>>  		     int len,
>> @@ -30,16 +32,25 @@ struct kvm_io_device {
>>  	int (*in_range)(struct kvm_io_device *this, gpa_t addr, int len,
>>  			int is_write);
>>  	void (*destructor)(struct kvm_io_device *this);
>> +};
>> +
>>  
>> -	void             *private;
>> +struct kvm_io_device {
>> +	struct kvm_io_device_ops *ops;
>>  };
>>     
>
> Did you plan to extend kvm_io_device struct?
>
>   
>> +static inline void kvm_iodevice_init(struct kvm_io_device *dev,
>> +				     struct kvm_io_device_ops *ops)
>> +{
>> +	dev->ops = ops;
>> +}
>>     
>
> And similarly, did you have a plan to do more with kvm_iodevice_init()?
> Otherwise looking a bit like overkill to me.
>   

Yeah.  As of right now my plan is to wait for Marcelo's lock cleanup to
go in and integrate with that, and then convert the MMIO/PIO code to use
RCU to acquire a reference to the io_device (so we run as fine-graned
and lockless as possible).  When that happens, you will see an atomic_t
in the struct/init as well.

Even if that doesn't make the cut after review, I am thinking that we
may be making the structure more complex in the future (for instance, to
use a rbtree/hlist instead of the array, or to do tricks with caching
the MRU device, etc.) and this will simplify that effort by already
having all users call the abstracted init. 

That said, we could just defer these hunks until needed.  I just figured
"while Im in here" but its nbd either way.

>   
>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -2456,7 +2456,7 @@ struct kvm_io_device *kvm_io_bus_find_dev(struct kvm_io_bus *bus,
>>  	for (i = 0; i < bus->dev_count; i++) {
>>  		struct kvm_io_device *pos = bus->devs[i];
>>  
>> -		if (pos->in_range(pos, addr, len, is_write))
>> +		if (kvm_iodevice_inrange(pos, addr, len, is_write))
>>  			return pos;
>>  	}
>>     
>
> You converted this to the helper but not vcpu_find_pervcpu_dev() (not
> convinced it actually helps readability, but consistency is good).

Oops..oversight.  Will fix.

>   BTW,
> while there, s/kvm_iodevice_inrange/kvm_iodevice_in_range/ would be nice.
>   

Yeah, good idea.  Will fix.

Thanks Chris,
-Greg

Attachment: signature.asc
Description: OpenPGP digital signature