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

["Chen, Tiejun" <tiejun.chen@xxxxxxxxx>]

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@xxxxxxxxx>
> > > >
> > > > Signed-off-by: Yang Zhang <yang.z.zhang@xxxxxxxxx>
> > > > Signed-off-by: Tiejun Chen <tiejun.chen@xxxxxxxxx>
> > > > Cc: Allen Kay <allen.m.kay@xxxxxxxxx>
> > > > ---
> > > > 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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel