Re: [Qemu-devel] [RFC PATCH v3 3/3] fw/pci: Allocate IGD stolen memory

[Kevin O'Connor <kevin@koconnor.net>]

On Fri, Feb 12, 2016 at 05:23:18PM -0700, Alex Williamson wrote:
> Intel IGD makes use of memory allocated and marked reserved by the
> BIOS as a stolen memory range.  For the most part, guest drivers don't
> make use of this, but our achilles heel is the vBIOS.  The vBIOS
> programs the device to use the host stolen memory range and it's used
> in the pre-boot environment.  Generally the guest won't have access to
> the host stolen memory area, so these accesses either land in VM
> memory or unassigned space and generate IOMMU faults.  By allocating
> this range in SeaBIOS and programming it into the device, QEMU (via
> vfio) can make sure this guest allocated stolen memory range is used
> instead.

What does "vBIOS" mean in this context?  Is it the video device option
rom or something else?

> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>  src/fw/pciinit.c |   13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
> index 92170d5..c1ad5d4 100644
> --- a/src/fw/pciinit.c
> +++ b/src/fw/pciinit.c
> @@ -260,7 +260,7 @@ static void ich9_smbus_setup(struct pci_device *dev, void *arg)
>  static void intel_igd_opregion_setup(struct pci_device *dev, void *arg)
>  {
>      struct romfile_s *file = romfile_find("etc/igd-opregion");
> -    void *opregion;
> +    void *opregion, *bdsm;
>      u16 bdf = dev->bdf;
>  
>      if (!file || !file->size)
> @@ -281,6 +281,17 @@ static void intel_igd_opregion_setup(struct pci_device *dev, void *arg)
>  
>      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));
> +
> +    bdsm = memalign_high(1024 * 1024, 1024 * 1024);
> +    if (!bdsm) {
> +        warn_noalloc();
> +        return;
> +    }

The "high" memory pool is not a good fit for such a large allocation.
For so much space, I'd use memalign_tmphigh() followed by
e820_add(addr, size, E820_RESERVED).

> +    pci_config_writel(bdf, 0x5C, cpu_to_le32((u32)bdsm));
> +
> +    dprintf(1, "Allocated 1MB reserved memory for Intel IGD stolen memory at "
> +            "0x%08x\n", (u32)bdsm);
>  }

Does this make sense to do unconditionally in the firmware whenever
the device is present?  The SeaBIOS release schedule is not in sync
with QEMU, so changes in the firmware tend to take longer to deploy if
something needs to be done differently in the future.

What happens if this register is not set?  Does anything go wrong if
register 0xFC is set, but 0x5C is not (eg, due to allocation failure).

Thanks.
-Kevin