Re: [Qemu-devel] [SeaBIOS] [RFC PATCH] fw/pci: Add support for mapping Intel IGD OpRegion via QEMU

[Alex Williamson <alex.williamson@redhat.com>]

On Tue, 2016-02-02 at 12:43 +0100, Laszlo Ersek wrote:
> On 02/02/16 05:15, Alex Williamson wrote:
> > The proposed IGD OpRegion support in QEMU via vfio maps the host
> > OpRegion into VM system memory at the address written to the ASL
> > Storage register (0xFC).  The OpRegion contains a 16-byte signature
> > followed by a 4-byte size field.  Therefore SeaBIOS can allocate a
> > buffer of the typical size (8KB), this results in a matching e820
> > reserved entry for the range, then write the buffer address to the ASL
> > Storage register, blinking the OpRegion into the VM address space.  At
> > that point SeaBIOS can validate the signature and size and remap if
> > necessary.  If the OpRegion is larger than 8KB, this would result in
> > any conflicting ranges being temporarily mapped with the OpRegion,
> > which probably needs further discussion on what that could break.
> > Other options might be to use the same algorithm with an obscenely
> > sized initial buffer to make sure we do not overlap, always
> > re-allocating the proper sized buffer, or perhaps we could pass the
> > OpRegion itself or information about the OpRegion through fw_cfg.
> > 
> > With the posted kernel series[1] and QEMU series[2] (on top of Gerd's
> > IGD patches[3]), this makes the OpRegion available to the VM and
> > tracing shows that the guest driver does access it.
> > 
> > [1] https://lkml.org/lkml/2016/2/1/884
> > [2] https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg00202.html
> > [3] https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg00244.html
> > 
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> >  src/fw/pciinit.c |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 50 insertions(+)
> > 
> > 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 *arg)
> >      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;
> 
> Is this idiomatic SeaBIOS coding style?
> 
> How about "for (;;)" -- the code has many instances -- and reworking
> this last branch accordingly?
> 
> (Apologies, not a very important comment.)

I did check that I wasn't the only one using a goto in SeaBIOS and I
don't see any sort of CodingStyle doc precluding it, but I apologize if
there have been previous discussions that I've missed.  I don't have the
same aversion to gotos as many people do, but of course the loop can be
reworked in any number of ways if there's a preference to avoid them.
Thanks,

Alex


> > +    }
> > +}
> > +
> >  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,
> >  };
> >  
> > 
> > 
> > _______________________________________________
> > SeaBIOS mailing list
> > SeaBIOS@seabios.org
> > http://www.seabios.org/mailman/listinfo/seabios
> > 
>