Re: [Qemu-devel] [vfio-users] [PATCH v3 00/11] igd passthrough chipset tweaks

From: Alex Williamson <alex.williamson@redhat.com>
To: Gerd Hoffmann <kraxel@redhat.com>
Cc: igvt-g@ml01.01.org, xen-devel@lists.xensource.com,
	Eduardo Habkost <ehabkost@redhat.com>,
	Stefano Stabellini <stefano.stabellini@eu.citrix.com>,
	qemu-devel@nongnu.org, Cao jin <caoj.fnst@cn.fujitsu.com>,
	vfio-users@redhat.com
Subject: Re: [Qemu-devel] [vfio-users] [PATCH v3 00/11] igd passthrough chipset tweaks
Date: Mon, 01 Feb 2016 15:16:56 -0700	[thread overview]
Message-ID: <1454365016.10542.12.camel@redhat.com> (raw)
In-Reply-To: <1454330962.10168.34.camel@redhat.com>

On Mon, 2016-02-01 at 13:49 +0100, Gerd Hoffmann wrote:
> > > Maybe we should define the interface as "guest writes 0xfc to pick
> > > address, qemu takes care to place opregion there".  That gives us the
> > > freedom to change the qemu implementation (either copy host opregion or
> > > map the host opregion) without breaking things.
> > 
> > Ok, so seabios allocates two pages, writes the base address of those
> > pages to 0xfc and looks to see whether the signature appears at that
> > address due to qemu mapping.  It verifies the size and does a
> > free/realloc if not the right size.
> 
> I think seabios first needs to reserve something big enough for a
> temporary mapping, to check signature + size, otherwise the opregion
> might scratch data structures beyond opregion in case it happens to be
> larger than 8k.
> 
> How likely is it that the opregion size ever changes?  Should we better
> be prepared to handle it?  Or would it be ok to have a ...
> 
>    if (opregion_size > 8k)
>       panic();
> 
> ... style sanity check?
> 

The patch below is what I'm working with now, it assumes that the
opregion is 8K, maps, verifies, and re-allocs if it's a different size.
Maybe it is safer to abort if it is over 8K, but we're not actually
clobbering anything with the mapping, we're just temporarily mapping
over it.  So if there's not another thread of execution that could be
accessing something there and we're not stepping on our own stack or
data, it doesn't seem like there's a problem.

diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
index c31c2fa..4f3251e 100644
--- a/src/fw/pciinit.c
+++ b/src/fw/pciinit.c
@@ -257,6 +257,52 @@ static void ich9_smbus_setup(struct pci_device *dev, void *
     pci_config_writeb(bdf, ICH9_SMB_HOSTC, ICH9_SMB_HOSTC_HST_EN);
 }
 
+static void intel_igd_opregion_setup(struct pci_device *dev, void *arg)
+{
+    u16 bdf = dev->bdf;
+    u32 orig;
+    void *opregion;
+    int size = 8;
+
+    if (!CONFIG_QEMU)
+        return;
+
+    orig = pci_config_readl(bdf, 0xFC);
+
+realloc:
+    opregion = malloc_high(size * 1024);
+    if (!opregion) {
+        warn_noalloc();
+        return;
+    }
+
+    /*
+     * QEMU maps the OpRegion into system memory at the address written here,
+     * this overlaps our malloc, which marks the range e820 reserved.
+     */
+    pci_config_writel(bdf, 0xFC, cpu_to_le32((u32)opregion));
+
+    if (memcmp(opregion, "IntelGraphicsMem", 16)) {
+        pci_config_writel(bdf, 0xFC, orig);
+        free(opregion);
+        return; /* the opregion didn't magically appear, not supported */
+    }
+
+    if (size == le32_to_cpu(*(u32 *)(opregion + 16))) {
+        dprintf(1, "Intel IGD OpRegion enabled on %02x:%02x.%x\n",
+                pci_bdf_to_bus(bdf), pci_bdf_to_dev(bdf), pci_bdf_to_fn(bdf));
+        return; /* success! */
+    }
+
+    pci_config_writel(bdf, 0xFC, orig);
+    free(opregion);
+
+    if (size == 8) { /* try once more with a new size */
+        size = le32_to_cpu(*(u32 *)(opregion + 16));
+        goto realloc;
+    }
+}
+
 static const struct pci_device_id pci_device_tbl[] = {
     /* PIIX3/PIIX4 PCI to ISA bridge */
     PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82371SB_0,
@@ -290,6 +336,10 @@ static const struct pci_device_id pci_device_tbl[] = {
     PCI_DEVICE_CLASS(PCI_VENDOR_ID_APPLE, 0x0017, 0xff00, apple_macio_setup),
     PCI_DEVICE_CLASS(PCI_VENDOR_ID_APPLE, 0x0022, 0xff00, apple_macio_setup),
 
+    /* Intel IGD OpRegion setup */
+    PCI_DEVICE_CLASS(PCI_VENDOR_ID_INTEL, PCI_ANY_ID, PCI_CLASS_DISPLAY_VGA,
+                     intel_igd_opregion_setup),
+
     PCI_DEVICE_END,
 };
 

> > If the graphics signature does not
> > appear, free those pages and assume no opregion support.
> 
> Yes.
> 
> > If we later
> > decide to use a copy, we'd need to disable the 0xfc automagic mapping
> > and probably pass the data via fw_cfg.  Sound right?
> 
> I'd have qemu copy the data on 0xfc write then, so things continue to
> work without updating seabios.  So, the firmware has to allocate space,
> reserve it etc.,  and programming the 0xfc register.  Qemu has to make
> sure the opregion appears at the address written by the firmware, by
> whatever method it prefers.

Ah, so here is where we'd clobber data in firmware.  I currently do
this in vfio's pci config write in QEMU:

        orig = pci_get_long(pdev->config + IGD_OPREGION);
        pci_default_write_config(pdev, addr, val, len);
        cur = pci_get_long(pdev->config + IGD_OPREGION);

        if (cur != orig) {
            if (orig) {
                memory_region_del_subregion(get_system_memory(),
                                            vdev->igd_opregion->mem);
            }

            if (cur) {
                memory_region_add_subregion(get_system_memory(),
                                            cur, vdev->igd_opregion->mem);
            }
        }

This means that fw can write 0x0 back to the ASL storage register and
the mapping goes away, no firmware data is overwritten and the overlap
was temporary.  If we copy it into the firmware provided buffer with
firmware not knowing the actual size then yes, we've just clobbered
something and it can't be recovered.  I'll post my patches and we can
hash out whether there's a better approach over something a little more
concrete.  I can see the opregion gets exposed and the guest driver does
use it.  I'm not entirely sure what functionality it's adding though
since a cursory test of booting an FC23 live iso image seems to
initialize the display correctly with or without the opregion.

> > > lpc bridge is no problem, only pci id fields are copied over and
> > > unprivileged access is allowed for them.
> > > 
> > > Copying the gfx registers of the host bridge is a problem indeed.
> > 
> > I would argue that both are really a problem, libvirt wants to put QEMU
> > in a container that prevents access to any host system files other than
> > those explicitly allowed.  Therefore libvirt needs to grant the process
> > access to the lpc sysfs config file even though it only needs user
> > visible register values.
> 
> Yes, correct.  We want svirt be as strict as possible.

So it might be a good idea to expose these through vfio.  What about
stolen memory?  I noted the IOMMU faults that I get when assigning IGD,
the bulk of it seems to be to the memory reserved as stolen for the GPU.
I can avoid those by clearing the guest view of the BDSM register, but I
think then we're just leaving stolen memory unused, which seems rather
wasteful.  Trying to identity map that stolen memory into the VM so that
we don't need to reconfigure the GPU seems problematic, but if vfio
exposed it as another region, we could do the same trick of mapping into
the VM address space.  The size of stolen memory is quite variable, so
we couldn't just assume a size.  We'd also need to know how to
reconfigure (and restore) the GPU for a new location, the BDSM register
just reports it.  Thanks,

Alex
