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

[Eric Blake <eblake@redhat.com>]

[-- 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 --]