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

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

[Andrew Barnes]

[-- Attachment #1: Type: text/plain, Size: 4386 bytes --]

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 <jbaron@redhat.com>
+ * Copyright (C) 2014 Andrew Barnes <andy@outsideglobe.com> 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;
+}
+
 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
+
     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);
+
     /*
      * Reason: part of ICH9 southbridge, needs to be wired up by
      * pc_q35_init()

[-- Attachment #2: Type: text/html, Size: 6660 bytes --]

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 544 bytes --]

[meta-comment]

On 09/24/2014 07:20 AM, Andrew Barnes wrote:
> hw/isa/lpc_ich9.c
> 

Your patches came through unthreaded (mail 1 through 8 are all lacking
In-Reply-To the message id of 0/8,
<CAKJ_wKTVEzT3HeFHJg=pD6qBnRNrqUWbEiRvmyKqSqnRPZRnow@mail.gmail.com>).
This makes it harder to review.  You may want to double-check your git
send-email settings, or look for advice at
http://wiki.qemu.org/Contribute/SubmitAPatch

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3309 bytes --]

On 09/24/2014 07:20 AM, 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
> ---------------------
> 

Stylistic review (I'll leave the content review to someone more
knowledgeable)

> @@ -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__)

Most debugging prints to stderr, not stdout.  Even better is using a
tracepoint instead of a macro.

> +#else
> +#  define LPC_DPRINTF(format, ...) do {} while (0)

There has also been work to make sure that debugging printfs are still
compiled for the sake of format checking, although optimized out by if
(0), in the normal case, so as to avoid bit-rot in the debug format
strings when not enabled.

> 
> +/* BEWARE: only set this if you are passing IGD through to guest */
> +/* TODO: detect IGD automatically */
> +static bool IGD_PASSTHROUGH = true;

all-caps names are usually reserved for macros and constants, not variables.

> +
>  /* 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,

Style: space after comma.


> +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 */

Indentation is off.  May be related to how you pasted the patch in your
mailer, rather than a flaw in your actual patch.

> +    ranges_overlap(addr, len, 0x2e, 2))   /* SID - Subsystem Identificaion
> */
> + {

Style - we prefer the { on the same line as the if.  Running your
patches through scripts/checkpatch.pl will catch many of the style issues.

> + val =
> host_pci_read_config(0,PCI_SLOT(d->devfn),PCI_FUNC(d->devfn),addr,len);

space after comma

> + }
> + else
> + {
> + goto defaultread;
> + }

More indentation damage.


 +
> +    /* 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);

/**/ comments are preferred over //; also, it's good to explain with a
comment why you are adding dead code.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

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 <jbaron@redhat.com>
> + * Copyright (C) 2014 Andrew Barnes <andy@outsideglobe.com> 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