Re: [Xen-devel] [Qemu-devel] [v5][PATCH 2/5] xen, gfx passthrough: create pseudo intel isa bridge

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Tue, 1 Jul 2014, Michael S. Tsirkin wrote:
> On Mon, Jun 30, 2014 at 08:34:46PM +0100, Stefano Stabellini wrote:
> > On Fri, 27 Jun 2014, Chen, Tiejun wrote:
> > > On 2014/6/25 17:58, Chen, Tiejun wrote:
> > > > On 2014/6/25 17:44, Michael S. Tsirkin wrote:
> > > > > On Wed, Jun 25, 2014 at 05:28:48PM +0800, Chen, Tiejun wrote:
> > > > > > On 2014/6/25 17:21, Michael S. Tsirkin wrote:
> > > > > > > On Wed, Jun 25, 2014 at 05:14:30PM +0800, Chen, Tiejun wrote:
> > > > > > > > On 2014/6/25 17:04, Michael S. Tsirkin wrote:
> > > > > > > > > On Wed, Jun 25, 2014 at 04:48:02PM +0800, Chen, Tiejun wrote:
> > > > > > > > > > On 2014/6/25 16:43, Michael S. Tsirkin wrote:
> > > > > > > > > > > On Wed, Jun 25, 2014 at 04:39:07PM +0800, Chen, Tiejun 
> > > > > > > > > > > wrote:
> > > > > > > > > > > > > In fact it's exactly what passthrough does.
> > > > > > > > > > > > > I wonder if more bits from ./hw/i386/kvm/pci-assign.c
> > > > > > > > > > > > > can be reused. How do you poke at the host device? 
> > > > > > > > > > > > > sysfs?
> > > > > > > > > > > > 
> > > > > > > > > > > > Yes, sysfs.
> > > > > > > > > > > > 
> > > > > > > > > > > > Thanks
> > > > > > > > > > > > Tiejun
> > > > > > > > > > > 
> > > > > > > > > > > Then you should be able to re-use large chunks of
> > > > > > > > > > > ./hw/i386/kvm/pci-assign.c: basically everything
> > > > > > > > > > > that deals with emulation.
> > > > > > > > > > 
> > > > > > > > > > Do you mean those hooks to get info from the real device? 
> > > > > > > > > > Xen
> > > > > > > > > > have its own
> > > > > > > > > > wrapper, xen_host_pci_get_block(), so we always go there in 
> > > > > > > > > > xen
> > > > > > > > > > scenario.
> > > > > > > > > > 
> > > > > > > > > > Thanks
> > > > > > > > > > Tiejun
> > > > > > > > > 
> > > > > > > > > Yes and that's not good.  We have two pieces of code doing 
> > > > > > > > > mostly
> > > > > > > > > identical things slightly differently.
> > > > > > > > > hw/i386/kvm/pci-assign.c is a bit younger so it's cleaner,
> > > > > > > > > but these really need to be unified.
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > Sorry, take a look at this again,
> > > > > > > > 
> > > > > > > > xen_host_pci_get_block(XenHostPCIDevice *d, int pos, uint8_t 
> > > > > > > > *buf,
> > > > > > > > int len)
> > > > > > > >     |
> > > > > > > >     + xen_host_pci_config_read(d, pos, buf, len)
> > > > > > > >         |
> > > > > > > >         + pread(d->config_fd, buf, len, pos)
> > > > > > > > 
> > > > > > > > I thinks this should be same as kvm.
> > > > > > > > 
> > > > > > > > Thanks
> > > > > > > > Tiejun
> > > > > > > 
> > > > > > > get_block is trivial.
> > > > > > > 
> > > > > > > I really mean the whole PT infrastructure for
> > > > > > > - discovering host devices through sysfs
> > > > > > > - virtualizing devices
> > > > > > > 
> > > > > > > rom, bars, msi ...
> > > > > > > the list goes on.
> > > > > > > 
> > > > > > > logic is mostly the same.
> > > > > > > 
> > > > > > 
> > > > > > Looks you mean we can unify the entire PT infrastructure between kvm
> > > > > > and xen
> > > > > > inside qemu. But I'm afraid its not easy to do in a short time, so
> > > > > > maybe we
> > > > > > can queue this as next phase.
> > > > > > 
> > > > > > Thanks
> > > > > > Tiejun
> > > > > 
> > > > > I'm afraid once we merge your code, you'll lose interest :)
> > > > > 
> > > > 
> > > > Currently we have to push this feature into upstream as our first
> > > > priority, so unless something is really needed to address. Of course I
> > > > hope this point what we're talking is not such a thing :)
> > > > 
> > > > But I can promise here I'd like to do this optimization with your guide
> > > > next :)
> > > > 
> > > > > At least, don't add duplicate code for ROM.
> > > > > 
> > > > 
> > > > Let me try this.
> > > > 
> > > 
> > > Its not easy as expected.
> > > 
> > > kvm always work with this structure, AssignedDevice, and especially this 
> > > is
> > > just activated in kvm_enabled(). And then set all properties to this
> > > structure.
> > > 
> > > In xen case, the similar structure, XenHostPCIDevice, is not easy 
> > > transferred
> > > into the structure, AssignedDevice. So this mean we have to split
> > > assigned_dev_load_option_rom() as line by line for xen and kvm, 
> > > respectively.
> > > 
> > > I really agree we definitely need to unify PT infrastructure between kvm 
> > > and
> > > xen after this try since I can't understand why we originally introduce 
> > > same
> > > way to do same thing :(
> > > 
> > > Do you have better idea? If not, I prefer we open this completely as next
> > > action to follow-up. But this time I'm afraid I can't get in this.
> > 
> > The Xen PT code came first. At the time Anthony Liguori and I argued for
> > sharing the PT code with KVM going forward but Avi wanted to retain the
> > KVM PT code as is in order not to introduce any regressions on KVM.
> > 
> > Ah.. the good old times :-)
> > 
> > http://lists.xen.org/archives/html/xen-devel/2011-10/msg00195.html
> 
> Right, I remember that there was an idea to first make the code
> reusable before merging either Xen or KVM PT, but in the end 
> the plan to merge both and then abstract it out, won.
> We never got to abstracting it out but we should!

I agree


> Yet, I do not think we should require abstracting all of Xen PT as
> a pre-requisite for this work, but I think we can
> avoid adding to the duplication without a lot of effort.

It seems like a good idea


> To start sharing with ROM code, I think we should rename
> AssignedDevice->KvmAssignedDevice, and move fields that can be shared
> with Xen from there to AssignedDevice.
>
> -- 
> MST
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel