|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [Qemu-devel] [RFC][PATCH 2/2] xen:i386:pc_piix: create isa bridge specific to IGD passthrough
On Mon, Nov 17, 2014 at 04:48:32PM +0800, Chen, Tiejun wrote:
> On 2014/11/17 14:10, Michael S. Tsirkin wrote:
> >On Mon, Nov 17, 2014 at 10:47:56AM +0800, Chen, Tiejun wrote:
> >>On 2014/11/5 22:09, Michael S. Tsirkin wrote:
> >>>On Wed, Nov 05, 2014 at 03:22:59PM +0800, Tiejun Chen wrote:
> >>>>Currently IGD drivers always need to access PCH by 1f.0, and
> >>>>PCH vendor/device id is used to identify the card.
> >>>>
> >>>>Signed-off-by: Tiejun Chen <tiejun.chen@xxxxxxxxx>
> >>>>---
> >>>> hw/i386/pc_piix.c | 28 +++++++++++++++++++++++++++-
> >>>> 1 file changed, 27 insertions(+), 1 deletion(-)
> >>>>
> >>>>diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> >>>>index b559181..b19c7a9 100644
> >>>>--- a/hw/i386/pc_piix.c
> >>>>+++ b/hw/i386/pc_piix.c
> >>>>@@ -50,7 +50,7 @@
> >>>> #include "cpu.h"
> >>>> #include "qemu/error-report.h"
> >>>> #ifdef CONFIG_XEN
> >>>>-# include <xen/hvm/hvm_info_table.h>
> >>>>+#include <xen/hvm/hvm_info_table.h>
> >>>> #endif
> >>>>
> >>>> #define MAX_IDE_BUS 2
> >>>>@@ -452,6 +452,31 @@ static void pc_init_isa(MachineState *machine)
> >>>> }
> >>>>
> >>>> #ifdef CONFIG_XEN
> >>>>+static void xen_igd_passthrough_isa_bridge_create(PCIBus *bus)
> >>>>+{
> >>>>+ struct PCIDevice *dev;
> >>>>+ Error *local_err = NULL;
> >>>>+ uint16_t device_id = 0xffff;
> >>>>+
> >>>>+ /* Currently IGD drivers always need to access PCH by 1f.0. */
> >>>>+ dev = pci_create_simple(bus, PCI_DEVFN(0x1f, 0),
> >>>>+ "xen-igd-passthrough-isa-bridge");
> >>>>+
> >>>>+ /* Identify PCH card with its own real vendor/device ids.
> >>>>+ * Here that vendor id is always PCI_VENDOR_ID_INTEL.
> >>>>+ */
> >>>>+ if (dev) {
> >>>>+ device_id = object_property_get_int(OBJECT(dev), "device-id",
> >>>>+ &local_err);
> >>>>+ if (!local_err && device_id != 0xffff) {
> >>>>+ pci_config_set_device_id(dev->config, device_id);
> >>>>+ return;
> >>>>+ }
> >>>>+ }
> >>>>+
> >>>>+ fprintf(stderr, "xen set xen-igd-passthrough-isa-bridge failed\n");
> >>>>+}
> >>>>+
> >>>> static void pc_xen_hvm_init(MachineState *machine)
> >>>> {
> >>>> PCIBus *bus;
> >>>>@@ -461,6 +486,7 @@ static void pc_xen_hvm_init(MachineState *machine)
> >>>> bus = pci_find_primary_bus();
> >>>> if (bus != NULL) {
> >>>> pci_create_simple(bus, -1, "xen-platform");
> >>>>+ xen_igd_passthrough_isa_bridge_create(bus);
> >>>> }
> >>>> }
> >>>> #endif
> >>>
> >>>Can't we defer this step until the GPU is added?
> >>
> >>Sounds great but I can't figure out where we can to do this exactly.
> >>
> >>>This way there won't be need to poke at host device
> >>>directly, you could get all info from dev->config
> >>>of the host device.
> >>
> >>As I understand We have two steps here:
> >>
> >>#1 At first I have to write something to check if we're registering 00:02.0
> >>& IGD, right? But where? While registering each pci device?
> >
> >In xen_pt_initfn.
> >Just check the device and vendor ID against the table you have.
> >
>
> Okay. Please see the follows which is just compiled:
>
> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
> index c6466dc..f3ea313 100644
> --- a/hw/xen/xen_pt.c
> +++ b/hw/xen/xen_pt.c
> @@ -632,6 +632,94 @@ static const MemoryListener xen_pt_io_listener = {
> .priority = 10,
> };
>
> +typedef struct {
> + uint16_t gpu_device_id;
> + uint16_t pch_device_id;
> +} XenIGDDeviceIDInfo;
> +
> +/* In real world different GPU should have different PCH. But actually
> + * the different PCH DIDs likely map to different PCH SKUs. We do the
> + * same thing for the GPU. For PCH, the different SKUs are going to be
> + * all the same silicon design and implementation, just different
> + * features turn on and off with fuses. The SW interfaces should be
> + * consistent across all SKUs in a given family (eg LPT). But just same
> + * features may not be supported.
> + *
> + * Most of these different PCH features probably don't matter to the
> + * Gfx driver, but obviously any difference in display port connections
> + * will so it should be fine with any PCH in case of passthrough.
> + *
> + * So currently use one PCH version, 0x8c4e, to cover all HSW(Haswell)
> + * scenarios, 0x9cc3 for BDW(Broadwell).
> + */
> +static const XenIGDDeviceIDInfo xen_igd_combo_id_infos[] = {
> + /* HSW Classic */
> + {0x0402, 0x8c4e}, /* HSWGT1D, HSWD_w7 */
> + {0x0406, 0x8c4e}, /* HSWGT1M, HSWM_w7 */
> + {0x0412, 0x8c4e}, /* HSWGT2D, HSWD_w7 */
> + {0x0416, 0x8c4e}, /* HSWGT2M, HSWM_w7 */
> + {0x041E, 0x8c4e}, /* HSWGT15D, HSWD_w7 */
> + /* HSW ULT */
> + {0x0A06, 0x8c4e}, /* HSWGT1UT, HSWM_w7 */
> + {0x0A16, 0x8c4e}, /* HSWGT2UT, HSWM_w7 */
> + {0x0A26, 0x8c4e}, /* HSWGT3UT, HSWM_w7 */
> + {0x0A2E, 0x8c4e}, /* HSWGT3UT28W, HSWM_w7 */
> + {0x0A1E, 0x8c4e}, /* HSWGT2UX, HSWM_w7 */
> + {0x0A0E, 0x8c4e}, /* HSWGT1ULX, HSWM_w7 */
> + /* HSW CRW */
> + {0x0D26, 0x8c4e}, /* HSWGT3CW, HSWM_w7 */
> + {0x0D22, 0x8c4e}, /* HSWGT3CWDT, HSWD_w7 */
> + /* HSW Server */
> + {0x041A, 0x8c4e}, /* HSWSVGT2, HSWD_w7 */
> + /* HSW SRVR */
> + {0x040A, 0x8c4e}, /* HSWSVGT1, HSWD_w7 */
> + /* BSW */
> + {0x1606, 0x9cc3}, /* BDWULTGT1, BDWM_w7 */
> + {0x1616, 0x9cc3}, /* BDWULTGT2, BDWM_w7 */
> + {0x1626, 0x9cc3}, /* BDWULTGT3, BDWM_w7 */
> + {0x160E, 0x9cc3}, /* BDWULXGT1, BDWM_w7 */
> + {0x161E, 0x9cc3}, /* BDWULXGT2, BDWM_w7 */
> + {0x1602, 0x9cc3}, /* BDWHALOGT1, BDWM_w7 */
> + {0x1612, 0x9cc3}, /* BDWHALOGT2, BDWM_w7 */
> + {0x1622, 0x9cc3}, /* BDWHALOGT3, BDWM_w7 */
> + {0x162B, 0x9cc3}, /* BDWHALO28W, BDWM_w7 */
> + {0x162A, 0x9cc3}, /* BDWGT3WRKS, BDWM_w7 */
> + {0x162D, 0x9cc3}, /* BDWGT3SRVR, BDWM_w7 */
> +};
> +
> +static void
> +xen_igd_passthrough_isa_bridge_create(XenPCIPassthroughState *s,
> + XenHostPCIDevice *dev)
> +{
> + struct PCIDevice *pci_dev;
> + int i, num;
> + uint16_t gpu_id = 0xffff,
no need to init gpu_id.
> pch_id = 0xffff;
> + PCIDevice *d = &s->dev;
> +
> + if (is_vga_passthrough(dev)) {
I would do if (!is_vga_passthrough) { return; } here.
Makes following code shorter.
> + gpu_id = dev->device_id;
> + num = ARRAY_SIZE(xen_igd_combo_id_infos);
> + for (i = 0; i < num; i++)
> + if (gpu_id == xen_igd_combo_id_infos[i].gpu_device_id)
> + pch_id = xen_igd_combo_id_infos[i].pch_device_id;
Add {} to match our coding style please.
At this point I would do
if (pch_id == 0xffff) {
return;
}
maybe replace 0xffff with 0x0, then you can do if (!pch_id) { return; }.
> +
> + /* Currently IGD drivers always need to access PCH by 1f.0. */
> + pci_dev = pci_create_simple(d->bus, PCI_DEVFN(0x1f, 0),
> + "xen-igd-passthrough-isa-bridge");
> +
> + /*
> + * Identify PCH card with its own real vendor/device ids.
> + * Here that vendor id is always PCI_VENDOR_ID_INTEL.
s/Here/Note/
> + */
> + if (pci_dev) {
> + pci_config_set_device_id(pci_dev->config, pch_id);
> + return;
> + }
> +
> + fprintf(stderr, "xen set xen-igd-passthrough-isa-bridge
> failed!\n");
Cleaner:
if (!pci_dev) {
fprintf
return;
}
pci_config_set_device_id(pci_dev->config, pch_id);
> + }
> +}
> +
> /* init */
>
> static int xen_pt_initfn(PCIDevice *d)
> @@ -682,6 +770,9 @@ static int xen_pt_initfn(PCIDevice *d)
> return -1;
> }
>
> + /* Register ISA bridge for passthrough GFX. */
> + xen_igd_passthrough_isa_bridge_create(s, &s->real_device);
> +
> /* reinitialize each config register to be emulated */
> if (xen_pt_config_init(s)) {
> XEN_PT_ERR(d, "PCI Config space initialisation failed.\n");
>
> Note I will introduce a inline function in another patch,
>
> +static inline int is_vga_passthrough(XenHostPCIDevice *dev)
> +{
> + return (xen_has_gfx_passthru && (dev->vendor_id == PCI_VENDOR_ID_INTEL)
> + && ((dev->class_code >> 0x8) == PCI_CLASS_DISPLAY_VGA));
> +}
>
> Thanks
> Tiejun
Why bother with all these conditions?
Won't it be enough to check dev->vendor_id == PCI_VENDOR_ID_INTEL?
--
MST
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |