Re: [Qemu-devel] [IGDVFIO] [PATCH 1/8] RFC and help completing: Intel IGD Direct Assignment with VFIO

[Alex Williamson]

On Wed, 2014-09-24 at 14:20 +0100, Andrew Barnes wrote:
> hw/isa/lpc_ich9.c
> 
> this patch adds:
> * debug output, if enabled
> * enforces correct intel config, if enabled. (unsure if this is needed)
> * redirects some PCI Config to host
> * uses hosts LPC device id
> 
> patch
> ---------------------
> 
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index b846d81..e6a7fbd 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -6,6 +6,7 @@
>   *               Isaku Yamahata <yamahata at valinux co jp>
>   *               VA Linux Systems Japan K.K.
>   * Copyright (C) 2012 Jason Baron <address@hidden>
> + * Copyright (C) 2014 Andrew Barnes <address@hidden> IGD Support
>   *
>   * This is based on piix_pci.c, but heavily modified.
>   *
> @@ -46,6 +47,16 @@
>  #include "exec/address-spaces.h"
>  #include "sysemu/sysemu.h"
> 
> +/* #define DEBUG_LPC */
> +#ifdef DEBUG_LPC
> +#  define LPC_DPRINTF(format, ...) print("LPC: " format, ## __VA_ARGS__)
> +#else
> +#  define LPC_DPRINTF(format, ...) do {} while (0)
> +#endif
> +
> +/* For intel-spec conforming config */
> +#define CORRECT_CONFIG
> +
>  static int ich9_lpc_sci_irq(ICH9LPCState *lpc);
> 
>  
> /*****************************************************************************/
> @@ -53,6 +64,10 @@ static int ich9_lpc_sci_irq(ICH9LPCState *lpc);
> 
>  static void ich9_lpc_reset(DeviceState *qdev);
> 
> +/* BEWARE: only set this if you are passing IGD through to guest */
> +/* TODO: detect IGD automatically */
> +static bool IGD_PASSTHROUGH = true;
> +
>  /* chipset configuration register
>   * to access chipset configuration registers, pci_[sg]et_{byte, word, long}
>   * are used.
> @@ -425,6 +440,9 @@ static void ich9_lpc_config_write(PCIDevice *d,
>      ICH9LPCState *lpc = ICH9_LPC_DEVICE(d);
>      uint32_t rbca_old = pci_get_long(d->config + ICH9_LPC_RCBA);
> 
> +    LPC_DPRINTF("%s(%04x:%02x:%02x.%x, @0x%x, 0x%x, len=0x%x)\n", __func__,
> +                0000, 00, PCI_SLOT(d->devfn),PCI_FUNC(d->devfn), addr,
> val, len);
> +
>      pci_default_write_config(d, addr, val, len);
>      if (ranges_overlap(addr, len, ICH9_LPC_PMBASE, 4)) {
>          ich9_lpc_pmbase_update(lpc);
> @@ -440,6 +458,34 @@ static void ich9_lpc_config_write(PCIDevice *d,
>      }
>  }
> 
> +static uint32_t ich9_lpc_config_read(PCIDevice *d,
> +                                     uint32_t addr, int len)
> +{
> +    uint32_t val;
> +    if (IGD_PASSTHROUGH)
> +    {
> + if (ranges_overlap(addr, len, 0x2c, 2) || /* SVID - Subsystem Vendor
> Identification */
> +    ranges_overlap(addr, len, 0x2e, 2))   /* SID - Subsystem Identificaion
> */
> + {
> + val =
> host_pci_read_config(0,PCI_SLOT(d->devfn),PCI_FUNC(d->devfn),addr,len);
> + }
> + else
> + {
> + goto defaultread;
> + }
> +    }
> +    else
> +    {
> +defaultread:
> + val = pci_default_read_config(d,addr,len);
> +    }
> +
> +    LPC_DPRINTF("%s(%04x:%02x:%02x.%x, @0x%x, len=0x%x) %x\n", __func__,
> +                0000, 00, PCI_SLOT(d->devfn),PCI_FUNC(d->devfn), addr,
> len, val);
> +
> +    return val;
> +}

Hard to see the through the poor formatting, but this doesn't seem like
something that needs to be intercepted on every config read.  Instead we
should set the device subsystem IDs when we're initializing IGD
passthrough, but even then, I think Intel might be working on not
needing this based on previous feedback.

> +
>  static void ich9_lpc_reset(DeviceState *qdev)
>  {
>      PCIDevice *d = PCI_DEVICE(qdev);
> @@ -577,6 +623,13 @@ static int ich9_lpc_init(PCIDevice *d)
> 
>      isa_bus = isa_bus_new(&d->qdev, get_system_io());
> 
> +    #ifdef CORRECT_CONFIG
> +        pci_set_word(d->wmask + PCI_COMMAND,
> +                    (PCI_COMMAND_SERR | PCI_COMMAND_PARITY));
> +        pci_set_word(d->config + PCI_COMMAND,
> +                    (PCI_COMMAND_IO | PCI_COMMAND_MEMORY |
> PCI_COMMAND_MASTER));
> +    #endif
> +

We need a better way to do this, obviously.  I believe this change will
break migration on q35 (w/o IGD assignment).  A new q35 machine type
perhaps or some sort of migration helpers.

>      pci_set_long(d->wmask + ICH9_LPC_PMBASE,
>                   ICH9_LPC_PMBASE_BASE_ADDRESS_MASK);
> 
> @@ -665,11 +718,20 @@ static void ich9_lpc_class_init(ObjectClass *klass,
> void *data)
>      k->init = ich9_lpc_init;
>      dc->vmsd = &vmstate_ich9_lpc;
>      k->config_write = ich9_lpc_config_write;
> +    k->config_read = ich9_lpc_config_read;
>      dc->desc = "ICH9 LPC bridge";
>      k->vendor_id = PCI_VENDOR_ID_INTEL;
>      k->device_id = PCI_DEVICE_ID_INTEL_ICH9_8;
>      k->revision = ICH9_A2_LPC_REVISION;
>      k->class_id = PCI_CLASS_BRIDGE_ISA;
> +
> +    /* For a UN-MODIFIED guest, the following 3 registers need to be read
> from the host.
> +     * alternatively, modify i915_drv.c, intel_detect_pch, add a check for
> +     * PCI_DEVICE_ID_INTEL_ICH9_8 and copy the settings from the PCH you
> desire */
> +    k->vendor_id = host_pci_read_config(0,0x1f,0,0x00,2);
> +    k->device_id = host_pci_read_config(0,0x1f,0,0x02,2);
> +//    k->revision = host_pci_read_config(0,0x1f,0,0x08,1);

One of us is missing a patch because I can't find a
host_pci_read_config() in QEMU.  This creates all sorts of issue for
libvirt though since it's likely that this requires /sys file access.
Is Intel going to be able to modify the driver to not need this?  Should
there instead by -device options for the lpc to specify IDs and
subsystem IDs from the commandline rather than requiring QEMU to read
from the host?  Also, this again breaks migration when not doing IGD
assignment since the IDs will vary between source and target.  Thanks,

Alex