Re: [PATCH] Support for in-kernel mmio handlers

Chris Wright
2007-04-05T00:49:06Z

* Gregory Haskins (ghaskins-Et1tbQHTxzrQT0dZR+AlfA@public.gmane.org) wrote:
> LAPICs can be remapped on a per-cpu basis via an MSR, whereas something
> like an IOAPIC is a system-wide resource.

Yes, I see now, no vcpu in kvm_io_device callbacks' context (admittedly,
I'm used to the Xen implementation ;-)

> >> +struct kvm_io_device {
> >> + unsigned long (*read)(struct kvm_io_device *this,
> >> + gpa_t addr,
> >> + unsigned long length);
> >> + void (*write)(struct kvm_io_device *this,
> >> + gpa_t addr,
> >> + unsigned long length,
> >> + unsigned long val);
> >> + int (*in_range)(struct kvm_io_device *this, gpa_t addr);
> >> +
> >> + void *private;
> >
> > This looks unused, what is it meant for?
>
> Its unused in this patch, because the primary consumer is a follow on
> patch that is not yet released. The original patch had this logic + the
> logic that used it all together and it was requested to break them apart.

Makes sense, I'll wait to see a user to understand how it'w used.

> >> +++ b/drivers/kvm/kvm_main.c
> >> @@ - 294,6 +294,7 @@ static struct kvm *kvm_create_vm(void)
> >>
> >> spin_lock_init(&kvm- >lock);
> >> INIT_LIST_HEAD(&kvm- >active_mmu_pages);
> >> + kvm_io_bus_init(&kvm- >mmio_bus);
> >
> > I'd just do INIT_LIST_HEAD, unless you havd bigger plans for this wrapper?
>
> The motivation for wrapping the init is because I want to abstract
> the fact that its a list. This means I can update the mechanism to
> do something more intelligent with address lookup (e.g. b-tree, etc)
> without changing code all over the place. Right now there are only
> two consumers, put I envision there will be some more. For instance,
> I would like to get PIOs using this mechanism at some point (so I can
> snarf accesses to the 8259s at 0x20/0xa0)

Right, you even alluded to that in your comments. I didn't expect a list
to really become that long where it needed a more complex data structure.

thanks,
-chris