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

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Jul 01, 2014 at 05:50:27PM +0800, Chen, Tiejun wrote:
> On 2014/7/1 13:47, 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!
> >
> >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.
> >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.
> 
> Its not simple to share something between AssignedDevice and the similar
> structure, XenHostPCIDevice.
> 
> As I said previously AssignedDevice is just activated in kvm_enabled(), then
> set all properties to this structure.
> 
> But kvm_enabled() is zero since we always disable this in xen scenario. So
> this means we have to invoke pci-assign framework completely if we try to
> force to use this. So actually this would be equal to merge kvm and xen.
> 
> Or if you already figure out to avoid this happened let me know.
> 
> Thanks
> Tiejun

For now fill in the structure separately in xen.

-- 
MST