[Qemu-devel] Re: [PATCH 1/1] KVM/userspace: Support for assigning PCI devices to guests

[Anthony Liguori <anthony@codemonkey.ws>]

Amit Shah wrote:
> * On Wednesday 27 Aug 2008 19:20:26 Anthony Liguori wrote:
>   
>
>>>  libkvm/libkvm-x86.c         |   14 +
>>>  libkvm/libkvm.h             |   27 ++
>>>  qemu/Makefile.target        |    1 +
>>>  qemu/hw/device-assignment.c |  600
>>> +++++++++++++++++++++++++++++++++++++++++++ qemu/hw/device-assignment.h |
>>>   94 +++++++
>>>  qemu/hw/isa.h               |    2 +
>>>  qemu/hw/pc.c                |    9 +
>>>  qemu/hw/pci.c               |   12 +
>>>  qemu/hw/pci.h               |    1 +
>>>  qemu/hw/piix_pci.c          |   19 ++
>>>  qemu/vl.c                   |   18 ++
>>>  11 files changed, 797 insertions(+), 0 deletions(-)
>>>  create mode 100644 qemu/hw/device-assignment.c
>>>  create mode 100644 qemu/hw/device-assignment.h
>>>       
>> This patch is too big on it's on.  It should be split into logical parts.
>>     
>
> However, it just adds device assignment support and does nothing else. I don't 
> see a way of splitting this any more.
>   

The libkvm changes can go in there own patch.  The changes necessary to 
piix_pci can get in their own patch.  If you get creative, it's probably 
possible to split up device-assignment.c too.

> As you noticed, it's quite different. They have a lot more code in there. (3k+ 
> lines vs 600 lines). I guess most of it went away when we realised we didn't 
> need all that. Also, I noticed most of the code there is not in the qemu 
> style.
>   

What are you looking at?  In my version of xen-unstable, I see 
pass-through.c at 694 lines.  It's all in the right style too.

>> We try to avoid open coding calls to libkvm functions directly within
>> QEMU.  At the least, it should be wrapped with an if (kvm_enabled()).
>>     
>
> With the direct-mmio patch, we just start depending on KVM. Ben-Ami, is there 
> a chance you can keep the MMIO-via-userspace method supported as well for 
> non-kvm hosts?
>   

Open-coding calls to libkvm functions breaks the build when not using 
KVM.  They all need qemu-kvm.c wrappers.

> From your other mail:
>
>   
>> Where did this come from originally?  It's completely different from
>> what is in xen-unstable.  What's in xen-unstable is actually a lot nicer
>> IMHO.
>>     
>
> Nicer in what way?
>
> I think the original code was picked up from before the time it was merged in 
> Xen.
>   

It's in the right style, it avoids layering violations (there are no 
direct calls to the piix device within the code).  At some point in 
time, both KVM and Xen are going to live in the upstream QEMU tree so 
this code will have to be shared.  Since we're basing our code off what 
appears to be older Xen code, I think rebasing it to the latest bits 
makes sense.

Regards,

Anthony Liguori

> Amit
>