| Message ID | 1403662641-28526-3-git-send-email-tiejun.chen@intel.com |
|---|---|
| State | New |
| Headers | show |
Il 25/06/2014 04:17, Tiejun Chen ha scritto: > +static int create_pseudo_pch_isa_bridge(PCIBus *bus, XenHostPCIDevice *hdev) > +{ > + struct PCIDevice *dev; > + > + char rid; > + > + /* We havt to use a simple PCI device to fake this ISA bridge > + * to avoid making some confusion to BIOS and ACPI. > + */ > + dev = pci_create(bus, PCI_DEVFN(0x1f, 0), "pseudo-intel-pch-isa-bridge"); > + > + qdev_init_nofail(&dev->qdev); > + > + pci_config_set_vendor_id(dev->config, hdev->vendor_id); > + pci_config_set_device_id(dev->config, hdev->device_id); > + > + xen_host_pci_get_block(hdev, PCI_REVISION_ID, (uint8_t *)&rid, 1); > + > + pci_config_set_revision(dev->config, rid); > + > + XEN_PT_LOG(dev, "The pseudo Intel PCH ISA bridge created.\n"); > + return 0; > +} This patch doesn't compile on its own (this static function is unused). pci_create_pch should be moved in this patch. Paolo
On Wed, Jun 25, 2014 at 10:17:18AM +0800, Tiejun Chen wrote: > ISA bridge is needed since Intel gfx drive will probe with Dev31:Fun0 > to make graphics device passthrough work well for VMM, that only need > to expose this pseudo ISA bridge to let driver know the real hardware > underneath. > > The original patch is from Allen Kay <allen.m.kay@intel.com> > > Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com> > Signed-off-by: Tiejun Chen <tiejun.chen@intel.com> > Cc: Allen Kay <allen.m.kay@intel.com> > --- > v5: > > * Don't set this ISA class property, instead, just fake this ISA bridge > with 00:1f.0. > > v4: > > * Remove some unnecessary "return" in void foo(). > > v3: > > * Fix some typos. > * Improve some return paths. > > v2: > > * Nothing is changed. > > hw/xen/xen_pt_graphics.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 61 insertions(+) > > diff --git a/hw/xen/xen_pt_graphics.c b/hw/xen/xen_pt_graphics.c > index 461e526..974b7e9 100644 > --- a/hw/xen/xen_pt_graphics.c > +++ b/hw/xen/xen_pt_graphics.c > @@ -230,3 +230,64 @@ out: > g_free(bios); > return rc; > } > + > +static uint32_t isa_bridge_read_config(PCIDevice *d, uint32_t addr, int len) > +{ > + return pci_default_read_config(d, addr, len); > +} > + > +static void isa_bridge_write_config(PCIDevice *d, uint32_t addr, uint32_t v, > + int len) > +{ > + pci_default_write_config(d, addr, v, len); > +} > + > +static void isa_bridge_class_init(ObjectClass *klass, void *data) > +{ > + PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); > + > + k->config_read = isa_bridge_read_config; > + k->config_write = isa_bridge_write_config; > +}; You don't need these stubs, just don't fill anything, pci core will use defaults then. > + > +typedef struct { > + PCIDevice dev; > +} ISABridgeState; > + Nor do you need an empty structure if it has no state. > +static TypeInfo isa_bridge_info = { > + .name = "pseudo-intel-pch-isa-bridge", > + .parent = TYPE_PCI_DEVICE, > + .instance_size = sizeof(ISABridgeState), > + .class_init = isa_bridge_class_init, > +}; > + > +static void xen_pt_graphics_register_types(void) > +{ > + type_register_static(&isa_bridge_info); > +} > + > +type_init(xen_pt_graphics_register_types) > + > +static int create_pseudo_pch_isa_bridge(PCIBus *bus, XenHostPCIDevice *hdev) > +{ > + struct PCIDevice *dev; > + > + char rid; > + > + /* We havt to use a simple PCI device to fake this ISA bridge > + * to avoid making some confusion to BIOS and ACPI. > + */ A typo and confusing wording above, I'm not really sure what this comment means. Maybe: /* Create a fake ISA bridge device at the location expected by guests. */ > + dev = pci_create(bus, PCI_DEVFN(0x1f, 0), "pseudo-intel-pch-isa-bridge"); > + > + qdev_init_nofail(&dev->qdev); > + > + pci_config_set_vendor_id(dev->config, hdev->vendor_id); > + pci_config_set_device_id(dev->config, hdev->device_id); > + > + xen_host_pci_get_block(hdev, PCI_REVISION_ID, (uint8_t *)&rid, 1); Host PCI device is the VGA card? So why does it make sense to get it's vendor/device/revision and stick in the ISA bridge? Also change rid to uint8_t, you won't need to cast then. > + > + pci_config_set_revision(dev->config, rid); > + > + XEN_PT_LOG(dev, "The pseudo Intel PCH ISA bridge created.\n"); > + return 0; > +} > -- > 1.9.1
On 2014/6/25 14:22, Paolo Bonzini wrote: > Il 25/06/2014 04:17, Tiejun Chen ha scritto: >> +static int create_pseudo_pch_isa_bridge(PCIBus *bus, XenHostPCIDevice >> *hdev) >> +{ >> + struct PCIDevice *dev; >> + >> + char rid; >> + >> + /* We havt to use a simple PCI device to fake this ISA bridge >> + * to avoid making some confusion to BIOS and ACPI. >> + */ >> + dev = pci_create(bus, PCI_DEVFN(0x1f, 0), >> "pseudo-intel-pch-isa-bridge"); >> + >> + qdev_init_nofail(&dev->qdev); >> + >> + pci_config_set_vendor_id(dev->config, hdev->vendor_id); >> + pci_config_set_device_id(dev->config, hdev->device_id); >> + >> + xen_host_pci_get_block(hdev, PCI_REVISION_ID, (uint8_t *)&rid, 1); >> + >> + pci_config_set_revision(dev->config, rid); >> + >> + XEN_PT_LOG(dev, "The pseudo Intel PCH ISA bridge created.\n"); >> + return 0; >> +} > > This patch doesn't compile on its own (this static function is unused). > > pci_create_pch should be moved in this patch. > Okay I will try this. Thanks Tiejun
On 2014/6/25 14:45, Michael S. Tsirkin wrote: > On Wed, Jun 25, 2014 at 10:17:18AM +0800, Tiejun Chen wrote: >> ISA bridge is needed since Intel gfx drive will probe with Dev31:Fun0 >> to make graphics device passthrough work well for VMM, that only need >> to expose this pseudo ISA bridge to let driver know the real hardware >> underneath. >> >> The original patch is from Allen Kay <allen.m.kay@intel.com> >> >> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com> >> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com> >> Cc: Allen Kay <allen.m.kay@intel.com> >> --- >> v5: >> >> * Don't set this ISA class property, instead, just fake this ISA bridge >> with 00:1f.0. >> >> v4: >> >> * Remove some unnecessary "return" in void foo(). >> >> v3: >> >> * Fix some typos. >> * Improve some return paths. >> >> v2: >> >> * Nothing is changed. >> >> hw/xen/xen_pt_graphics.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 61 insertions(+) >> >> diff --git a/hw/xen/xen_pt_graphics.c b/hw/xen/xen_pt_graphics.c >> index 461e526..974b7e9 100644 >> --- a/hw/xen/xen_pt_graphics.c >> +++ b/hw/xen/xen_pt_graphics.c >> @@ -230,3 +230,64 @@ out: >> g_free(bios); >> return rc; >> } >> + >> +static uint32_t isa_bridge_read_config(PCIDevice *d, uint32_t addr, int len) >> +{ >> + return pci_default_read_config(d, addr, len); >> +} >> + >> +static void isa_bridge_write_config(PCIDevice *d, uint32_t addr, uint32_t v, >> + int len) >> +{ >> + pci_default_write_config(d, addr, v, len); >> +} >> + >> +static void isa_bridge_class_init(ObjectClass *klass, void *data) >> +{ >> + PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); >> + >> + k->config_read = isa_bridge_read_config; >> + k->config_write = isa_bridge_write_config; >> +}; > > You don't need these stubs, just don't fill anything, > pci core will use defaults then. I guess these stubs are left to extend something in the future. But maybe we can remove them now. > >> + >> +typedef struct { >> + PCIDevice dev; >> +} ISABridgeState; >> + > > Nor do you need an empty structure if it has no state. > >> +static TypeInfo isa_bridge_info = { >> + .name = "pseudo-intel-pch-isa-bridge", >> + .parent = TYPE_PCI_DEVICE, >> + .instance_size = sizeof(ISABridgeState), >> + .class_init = isa_bridge_class_init, >> +}; >> + >> +static void xen_pt_graphics_register_types(void) >> +{ >> + type_register_static(&isa_bridge_info); >> +} >> + >> +type_init(xen_pt_graphics_register_types) >> + >> +static int create_pseudo_pch_isa_bridge(PCIBus *bus, XenHostPCIDevice *hdev) >> +{ >> + struct PCIDevice *dev; >> + >> + char rid; >> + >> + /* We havt to use a simple PCI device to fake this ISA bridge >> + * to avoid making some confusion to BIOS and ACPI. >> + */ > > A typo and confusing wording above, I'm not really > sure what this comment means. > Maybe: > > /* Create a fake ISA bridge device at the location expected by guests. */ > Good comments so thanks so much. > >> + dev = pci_create(bus, PCI_DEVFN(0x1f, 0), "pseudo-intel-pch-isa-bridge"); >> + >> + qdev_init_nofail(&dev->qdev); >> + >> + pci_config_set_vendor_id(dev->config, hdev->vendor_id); >> + pci_config_set_device_id(dev->config, hdev->device_id); >> + >> + xen_host_pci_get_block(hdev, PCI_REVISION_ID, (uint8_t *)&rid, 1); > > Host PCI device is the VGA card? This is a real ISA bridge. > So why does it make sense to get it's vendor/device/revision and > stick in the ISA bridge? The Intel generation of integrated graphics needs to probe this ISA bridge to initialize the i915 driver properly. Thanks Tiejun > > Also change rid to uint8_t, you won't need to cast then. > >> + >> + pci_config_set_revision(dev->config, rid); >> + >> + XEN_PT_LOG(dev, "The pseudo Intel PCH ISA bridge created.\n"); >> + return 0; >> +} >> -- >> 1.9.1 > >
On Wed, Jun 25, 2014 at 04:10:44PM +0800, Chen, Tiejun wrote: > On 2014/6/25 14:45, Michael S. Tsirkin wrote: > >On Wed, Jun 25, 2014 at 10:17:18AM +0800, Tiejun Chen wrote: > >>ISA bridge is needed since Intel gfx drive will probe with Dev31:Fun0 > >>to make graphics device passthrough work well for VMM, that only need > >>to expose this pseudo ISA bridge to let driver know the real hardware > >>underneath. > >> > >>The original patch is from Allen Kay <allen.m.kay@intel.com> > >> > >>Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com> > >>Signed-off-by: Tiejun Chen <tiejun.chen@intel.com> > >>Cc: Allen Kay <allen.m.kay@intel.com> > >>--- > >>v5: > >> > >>* Don't set this ISA class property, instead, just fake this ISA bridge > >> with 00:1f.0. > >> > >>v4: > >> > >>* Remove some unnecessary "return" in void foo(). > >> > >>v3: > >> > >>* Fix some typos. > >>* Improve some return paths. > >> > >>v2: > >> > >>* Nothing is changed. > >> > >> hw/xen/xen_pt_graphics.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 61 insertions(+) > >> > >>diff --git a/hw/xen/xen_pt_graphics.c b/hw/xen/xen_pt_graphics.c > >>index 461e526..974b7e9 100644 > >>--- a/hw/xen/xen_pt_graphics.c > >>+++ b/hw/xen/xen_pt_graphics.c > >>@@ -230,3 +230,64 @@ out: > >> g_free(bios); > >> return rc; > >> } > >>+ > >>+static uint32_t isa_bridge_read_config(PCIDevice *d, uint32_t addr, int len) > >>+{ > >>+ return pci_default_read_config(d, addr, len); > >>+} > >>+ > >>+static void isa_bridge_write_config(PCIDevice *d, uint32_t addr, uint32_t v, > >>+ int len) > >>+{ > >>+ pci_default_write_config(d, addr, v, len); > >>+} > >>+ > >>+static void isa_bridge_class_init(ObjectClass *klass, void *data) > >>+{ > >>+ PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); > >>+ > >>+ k->config_read = isa_bridge_read_config; > >>+ k->config_write = isa_bridge_write_config; > >>+}; > > > >You don't need these stubs, just don't fill anything, > >pci core will use defaults then. > > I guess these stubs are left to extend something in the future. But maybe we > can remove them now. > > > > >>+ > >>+typedef struct { > >>+ PCIDevice dev; > >>+} ISABridgeState; > >>+ > > > >Nor do you need an empty structure if it has no state. > > > >>+static TypeInfo isa_bridge_info = { > >>+ .name = "pseudo-intel-pch-isa-bridge", > >>+ .parent = TYPE_PCI_DEVICE, > >>+ .instance_size = sizeof(ISABridgeState), > >>+ .class_init = isa_bridge_class_init, > >>+}; > >>+ > >>+static void xen_pt_graphics_register_types(void) > >>+{ > >>+ type_register_static(&isa_bridge_info); > >>+} > >>+ > >>+type_init(xen_pt_graphics_register_types) > >>+ > >>+static int create_pseudo_pch_isa_bridge(PCIBus *bus, XenHostPCIDevice *hdev) > >>+{ > >>+ struct PCIDevice *dev; > >>+ > >>+ char rid; > >>+ > >>+ /* We havt to use a simple PCI device to fake this ISA bridge > >>+ * to avoid making some confusion to BIOS and ACPI. > >>+ */ > > > >A typo and confusing wording above, I'm not really > >sure what this comment means. > >Maybe: > > > >/* Create a fake ISA bridge device at the location expected by guests. */ > > > > Good comments so thanks so much. > > > > >>+ dev = pci_create(bus, PCI_DEVFN(0x1f, 0), "pseudo-intel-pch-isa-bridge"); > >>+ > >>+ qdev_init_nofail(&dev->qdev); > >>+ > >>+ pci_config_set_vendor_id(dev->config, hdev->vendor_id); > >>+ pci_config_set_device_id(dev->config, hdev->device_id); > >>+ > >>+ xen_host_pci_get_block(hdev, PCI_REVISION_ID, (uint8_t *)&rid, 1); > > > >Host PCI device is the VGA card? > > This is a real ISA bridge. > > >So why does it make sense to get it's vendor/device/revision and > >stick in the ISA bridge? > > The Intel generation of integrated graphics needs to probe this ISA bridge > to initialize the i915 driver properly. > > Thanks > Tiejun So vendor/device/revision IDs must match real device then? Stick this in the comment then. 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? > > > >Also change rid to uint8_t, you won't need to cast then. > > > >>+ > >>+ pci_config_set_revision(dev->config, rid); > >>+ > >>+ XEN_PT_LOG(dev, "The pseudo Intel PCH ISA bridge created.\n"); > >>+ return 0; > >>+} > >>-- > >>1.9.1 > > > >
On 2014/6/25 16:28, Michael S. Tsirkin wrote: > On Wed, Jun 25, 2014 at 04:10:44PM +0800, Chen, Tiejun wrote: >> On 2014/6/25 14:45, Michael S. Tsirkin wrote: >>> On Wed, Jun 25, 2014 at 10:17:18AM +0800, Tiejun Chen wrote: >>>> ISA bridge is needed since Intel gfx drive will probe with Dev31:Fun0 >>>> to make graphics device passthrough work well for VMM, that only need >>>> to expose this pseudo ISA bridge to let driver know the real hardware >>>> underneath. >>>> >>>> The original patch is from Allen Kay <allen.m.kay@intel.com> >>>> >>>> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com> >>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com> >>>> Cc: Allen Kay <allen.m.kay@intel.com> >>>> --- >>>> v5: >>>> >>>> * Don't set this ISA class property, instead, just fake this ISA bridge >>>> with 00:1f.0. >>>> >>>> v4: >>>> >>>> * Remove some unnecessary "return" in void foo(). >>>> >>>> v3: >>>> >>>> * Fix some typos. >>>> * Improve some return paths. >>>> >>>> v2: >>>> >>>> * Nothing is changed. >>>> >>>> hw/xen/xen_pt_graphics.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 61 insertions(+) >>>> >>>> diff --git a/hw/xen/xen_pt_graphics.c b/hw/xen/xen_pt_graphics.c >>>> index 461e526..974b7e9 100644 >>>> --- a/hw/xen/xen_pt_graphics.c >>>> +++ b/hw/xen/xen_pt_graphics.c >>>> @@ -230,3 +230,64 @@ out: >>>> g_free(bios); >>>> return rc; >>>> } >>>> + >>>> +static uint32_t isa_bridge_read_config(PCIDevice *d, uint32_t addr, int len) >>>> +{ >>>> + return pci_default_read_config(d, addr, len); >>>> +} >>>> + >>>> +static void isa_bridge_write_config(PCIDevice *d, uint32_t addr, uint32_t v, >>>> + int len) >>>> +{ >>>> + pci_default_write_config(d, addr, v, len); >>>> +} >>>> + >>>> +static void isa_bridge_class_init(ObjectClass *klass, void *data) >>>> +{ >>>> + PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); >>>> + >>>> + k->config_read = isa_bridge_read_config; >>>> + k->config_write = isa_bridge_write_config; >>>> +}; >>> >>> You don't need these stubs, just don't fill anything, >>> pci core will use defaults then. >> >> I guess these stubs are left to extend something in the future. But maybe we >> can remove them now. >> >>> >>>> + >>>> +typedef struct { >>>> + PCIDevice dev; >>>> +} ISABridgeState; >>>> + >>> >>> Nor do you need an empty structure if it has no state. >>> >>>> +static TypeInfo isa_bridge_info = { >>>> + .name = "pseudo-intel-pch-isa-bridge", >>>> + .parent = TYPE_PCI_DEVICE, >>>> + .instance_size = sizeof(ISABridgeState), >>>> + .class_init = isa_bridge_class_init, >>>> +}; >>>> + >>>> +static void xen_pt_graphics_register_types(void) >>>> +{ >>>> + type_register_static(&isa_bridge_info); >>>> +} >>>> + >>>> +type_init(xen_pt_graphics_register_types) >>>> + >>>> +static int create_pseudo_pch_isa_bridge(PCIBus *bus, XenHostPCIDevice *hdev) >>>> +{ >>>> + struct PCIDevice *dev; >>>> + >>>> + char rid; >>>> + >>>> + /* We havt to use a simple PCI device to fake this ISA bridge >>>> + * to avoid making some confusion to BIOS and ACPI. >>>> + */ >>> >>> A typo and confusing wording above, I'm not really >>> sure what this comment means. >>> Maybe: >>> >>> /* Create a fake ISA bridge device at the location expected by guests. */ >>> >> >> Good comments so thanks so much. >> >>> >>>> + dev = pci_create(bus, PCI_DEVFN(0x1f, 0), "pseudo-intel-pch-isa-bridge"); >>>> + >>>> + qdev_init_nofail(&dev->qdev); >>>> + >>>> + pci_config_set_vendor_id(dev->config, hdev->vendor_id); >>>> + pci_config_set_device_id(dev->config, hdev->device_id); >>>> + >>>> + xen_host_pci_get_block(hdev, PCI_REVISION_ID, (uint8_t *)&rid, 1); >>> >>> Host PCI device is the VGA card? >> >> This is a real ISA bridge. >> >>> So why does it make sense to get it's vendor/device/revision and >>> stick in the ISA bridge? >> >> The Intel generation of integrated graphics needs to probe this ISA bridge >> to initialize the i915 driver properly. >> >> Thanks >> Tiejun > > So vendor/device/revision IDs must match real device then? Yes, the i915 driver needs these information of PCH to determine how to work. And this is just why we expose this ISA bridge to guest ugly :( > Stick this in the comment then. > Sure. > 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 > > > >>> >>> Also change rid to uint8_t, you won't need to cast then. >>> >>>> + >>>> + pci_config_set_revision(dev->config, rid); >>>> + >>>> + XEN_PT_LOG(dev, "The pseudo Intel PCH ISA bridge created.\n"); >>>> + return 0; >>>> +} >>>> -- >>>> 1.9.1 >>> >>> > > >
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.
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 > > > >
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. > > > > > > > >
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
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.
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
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 :) At least, don't add duplicate code for ROM.
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. Thanks Tiejun
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. Thanks Tiejun
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
On 2014/7/1 3:34, 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 :-) Thanks for your information. So now what's next I should do now? Thanks Tiejun > > http://lists.xen.org/archives/html/xen-devel/2011-10/msg00195.html > >
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.
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
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.
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 >
diff --git a/hw/xen/xen_pt_graphics.c b/hw/xen/xen_pt_graphics.c index 461e526..974b7e9 100644 --- a/hw/xen/xen_pt_graphics.c +++ b/hw/xen/xen_pt_graphics.c @@ -230,3 +230,64 @@ out: g_free(bios); return rc; } + +static uint32_t isa_bridge_read_config(PCIDevice *d, uint32_t addr, int len) +{ + return pci_default_read_config(d, addr, len); +} + +static void isa_bridge_write_config(PCIDevice *d, uint32_t addr, uint32_t v, + int len) +{ + pci_default_write_config(d, addr, v, len); +} + +static void isa_bridge_class_init(ObjectClass *klass, void *data) +{ + PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); + + k->config_read = isa_bridge_read_config; + k->config_write = isa_bridge_write_config; +}; + +typedef struct { + PCIDevice dev; +} ISABridgeState; + +static TypeInfo isa_bridge_info = { + .name = "pseudo-intel-pch-isa-bridge", + .parent = TYPE_PCI_DEVICE, + .instance_size = sizeof(ISABridgeState), + .class_init = isa_bridge_class_init, +}; + +static void xen_pt_graphics_register_types(void) +{ + type_register_static(&isa_bridge_info); +} + +type_init(xen_pt_graphics_register_types) + +static int create_pseudo_pch_isa_bridge(PCIBus *bus, XenHostPCIDevice *hdev) +{ + struct PCIDevice *dev; + + char rid; + + /* We havt to use a simple PCI device to fake this ISA bridge + * to avoid making some confusion to BIOS and ACPI. + */ + dev = pci_create(bus, PCI_DEVFN(0x1f, 0), "pseudo-intel-pch-isa-bridge"); + + qdev_init_nofail(&dev->qdev); + + pci_config_set_vendor_id(dev->config, hdev->vendor_id); + pci_config_set_device_id(dev->config, hdev->device_id); + + xen_host_pci_get_block(hdev, PCI_REVISION_ID, (uint8_t *)&rid, 1); + + pci_config_set_revision(dev->config, rid); + + XEN_PT_LOG(dev, "The pseudo Intel PCH ISA bridge created.\n"); + return 0; +}