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

Gregory Haskins
2007-04-04T23:04:12Z

Hi Chris,
Thanks for the feedback. Ive answered inline below.

>>> On Wed, Apr 4, 2007 at 6:48 PM, in message
<20070404224806.GA15078-JyIX8gxvWYPr2PDY2+4mTGD2FQJk+8+b@public.gmane.org>, Chris Wright
<chrisw-69jw2NvuJkxg9hUCZPvPmw@public.gmane.org> wrote:
> * Gregory Haskins (ghaskins-Et1tbQHTxzrQT0dZR+AlfA@public.gmane.org) wrote:
>> The MMIO registration code has been broken out as a new patch from the
> in- kernel APIC work with the following changes per Avi's request:
>>
>> 1) Supports dynamic registration
>> 2) Uses gpa_t addresses
>> 3) Explicit per- cpu mappings
>>
>> In addition, I have added the concept of distinct VCPU and VM level
> registrations (where VCPU devices will eclipse competing VM registrations (if
> any). This will be key down the road where LAPICs should use VCPU
> registration, but IOAPICs should use VM level.
>
> hmm, i'm surprised it makes a difference.

LAPICs can be remapped on a per-cpu basis via an MSR, whereas something like an IOAPIC is a system-wide resource.

>
>> Signed- off- by: Gregory Haskins <ghaskins-Et1tbQHTxzrQT0dZR+AlfA@public.gmane.org>
>>
>> ---
>> drivers/kvm/kvm.h | 50 +++++++++++++++++++++++++++++++++++++++++++++
>> drivers/kvm/kvm_main.c | 53 +++++++++++++++++++++++++++++++++++++++--------
>> 2 files changed, 94 insertions(+), 9 deletions(- )
>>
>> diff -- git a/drivers/kvm/kvm.h b/drivers/kvm/kvm.h
>> index fceeb84..3334730 100644
>> --- a/drivers/kvm/kvm.h
>> +++ b/drivers/kvm/kvm.h
>> @@ - 236,6 +236,54 @@ struct kvm_pio_request {
>> int rep;
>> };
>>
>> +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.

>
>> + struct list_head link;
>> +};
>> +
>> +/* It would be nice to use something smarter than a linear search, TBD...
>> + Thankfully we dont expect many devices to register (famous last words
> :),
>> + so until then it will suffice. At least its abstracted so we can change
>> + in one place.
>> + */
>> +struct kvm_io_bus {
>> + struct list_head list;
>> +};
>> +
>> +static inline void
>> +kvm_io_bus_init(struct kvm_io_bus *bus)
>> +{
>> + INIT_LIST_HEAD(&bus- >list);
>> +}
>> +
>> +static inline struct kvm_io_device*
>> +kvm_io_bus_find_dev(struct kvm_io_bus *bus, gpa_t addr)
>> +{
>> + struct kvm_io_device *pos = NULL;
>> +
>> + list_for_each_entry(pos, &bus- >list, link) {
>> + if(pos- >in_range(pos, addr))
>
> linux style nit, missing space after if -- > if (pos- >in_range(pos, addr))

Yeah, old habits die hard ;) I will fix all of these.

>
>> + return pos;
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> +static inline void
>> +kvm_io_bus_register_dev(struct kvm_io_bus *bus, struct kvm_io_device *dev)
>> +{
>> + list_add_tail(&dev- >link, &bus- >list);
>> +}
>> +
>> struct kvm_vcpu {
>> struct kvm *kvm;
>> union {
>> @@ - 294,6 +342,7 @@ struct kvm_vcpu {
>> gpa_t mmio_phys_addr;
>> struct kvm_pio_request pio;
>> void *pio_data;
>> + struct kvm_io_bus mmio_bus;
>>
>> int sigset_active;
>> sigset_t sigset;
>> @@ - 345,6 +394,7 @@ struct kvm {
>> unsigned long rmap_overflow;
>> struct list_head vm_list;
>> struct file *filp;
>> + struct kvm_io_bus mmio_bus;
>> };
>>
>> struct kvm_stat {
>> diff -- git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
>> index 4473174..da119c0 100644
>> --- a/drivers/kvm/kvm_main.c
>> +++ 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)

>
>> for (i = 0; i < KVM_MAX_VCPUS; ++i) {
>> struct kvm_vcpu *vcpu = &kvm- >vcpus[i];
>>
>> @@ - 302,6 +303,7 @@ static struct kvm *kvm_create_vm(void)
>> vcpu- >kvm = kvm;
>> vcpu- >mmu.root_hpa = INVALID_PAGE;
>> INIT_LIST_HEAD(&vcpu- >free_pages);
>> + kvm_io_bus_init(&vcpu- >mmio_bus);
>
> ditto
>
>> spin_lock(&kvm_lock);
>> list_add(&kvm- >vm_list, &vm_list);
>> spin_unlock(&kvm_lock);
>> @@ - 1015,12 +1017,30 @@ static int emulator_write_std(unsigned long addr,
>> return X86EMUL_UNHANDLEABLE;
>> }
>>
>> +static struct kvm_io_device* vcpu_find_mmio_dev(struct kvm_vcpu *vcpu,
>> + gpa_t addr)
>> +{
>> + struct kvm_io_device *mmio_dev;
>> +
>> + /* First check the local CPU addresses */
>> + mmio_dev = kvm_io_bus_find_dev(&vcpu- >mmio_bus, addr);
>> + if(!mmio_dev) {
>
> same style nit.

ack

> and why do you have local vs global check

see my first comment: re LAPIC is a relocateble per-cpu resource, IOAPIC is not

>(or dynamic registration for that matter)?

the xAPIC support will be optional, so the ability to register for MMIO handing has to be dynamic.

>
>> + /* Then check the entire VM */
>> + mmio_dev = kvm_io_bus_find_dev(&vcpu- >kvm- >mmio_bus, addr);
>> + }
>> +
>> + return mmio_dev;
>> +}
>> +
>> static int emulator_read_emulated(unsigned long addr,
>> unsigned long *val,
>> unsigned int bytes,
>> struct x86_emulate_ctxt *ctxt)
>> {
>> struct kvm_vcpu *vcpu = ctxt- >vcpu;
>> + gpa_t gpa;
>> + int i;
>> + struct kvm_io_device *mmio_dev;
>>
>> if (vcpu- >mmio_read_completed) {
>> memcpy(val, vcpu- >mmio_data, bytes);
>> @@ - 1029,18 +1049,24 @@ static int emulator_read_emulated(unsigned long addr,
>> } else if (emulator_read_std(addr, val, bytes, ctxt)
>> == X86EMUL_CONTINUE)
>> return X86EMUL_CONTINUE;
>> - else {
>> - gpa_t gpa = vcpu- >mmu.gva_to_gpa(vcpu, addr);
>>
>> - if (gpa == UNMAPPED_GVA)
>> - return X86EMUL_PROPAGATE_FAULT;
>> - vcpu- >mmio_needed = 1;
>> - vcpu- >mmio_phys_addr = gpa;
>> - vcpu- >mmio_size = bytes;
>> - vcpu- >mmio_is_write = 0;
>> + gpa = vcpu- >mmu.gva_to_gpa(vcpu, addr);
>> + if (gpa == UNMAPPED_GVA)
>> + return vcpu_printf(vcpu, "not present\n"), X86EMUL_PROPAGATE_FAULT;
>>
>> - return X86EMUL_UNHANDLEABLE;
>> + /* Is thie MMIO handled locally? */
>
> s/thie/this/

ack

>
>> + mmio_dev = vcpu_find_mmio_dev(vcpu, gpa);
>> + if(mmio_dev) {
>
> style

ack

>
>> + *val = mmio_dev- >read(mmio_dev, gpa, bytes);
>> + return X86EMUL_CONTINUE;
>> }
>> +
>> + vcpu- >mmio_needed = 1;
>> + vcpu- >mmio_phys_addr = gpa;
>> + vcpu- >mmio_size = bytes;
>> + vcpu- >mmio_is_write = 0;
>> +
>> + return X86EMUL_UNHANDLEABLE;
>> }
>>
>> static int emulator_write_phys(struct kvm_vcpu *vcpu, gpa_t gpa,
>> @@ - 1070,6 +1096,8 @@ static int emulator_write_emulated(unsigned long addr,
>> {
>> struct kvm_vcpu *vcpu = ctxt- >vcpu;
>> gpa_t gpa = vcpu- >mmu.gva_to_gpa(vcpu, addr);
>> + int i;
>> + struct kvm_io_device *mmio_dev;
>>
>> if (gpa == UNMAPPED_GVA)
>> return X86EMUL_PROPAGATE_FAULT;
>> @@ - 1077,6 +1105,13 @@ static int emulator_write_emulated(unsigned long addr,
>> if (emulator_write_phys(vcpu, gpa, val, bytes))
>> return X86EMUL_CONTINUE;
>>
>> + /* Is thie MMIO handled locally? */
>
> s/thie/this/

ack

>
>> + mmio_dev = vcpu_find_mmio_dev(vcpu, gpa);
>> + if(mmio_dev) {
>
> style

ack

>
>> + mmio_dev- >write(mmio_dev, gpa, bytes, val);
>> + return X86EMUL_CONTINUE;
>> + }
>> +
>> vcpu- >mmio_needed = 1;
>> vcpu- >mmio_phys_addr = gpa;
>> vcpu- >mmio_size = bytes;

Regards,
-Greg