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

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

On Wed, 2014-09-24 at 14:20 +0100, Andrew Barnes wrote:
> hw/pci/pci.c
> 
> this patch adds:
> * read / write host PCI config space
> 
> patch
> ---------------------
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 42995e6..041f6f1 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2,6 +2,8 @@
>   * QEMU PCI bus manager
>   *
>   * Copyright (c) 2004 Fabrice Bellard
> + *    2014 Andrew Barnes <andy@outsideglobe.com> IGD Support
> + *        Temporarily extended to provide host config read/write.
>   *
>   * Permission is hereby granted, free of charge, to any person obtaining a
> copy
>   * of this software and associated documentation files (the "Software"),
> to deal
> @@ -37,9 +39,9 @@
>  #include "exec/address-spaces.h"
>  #include "hw/hotplug.h"
> 
> -//#define DEBUG_PCI
> +/* #define DEBUG_PCI */
>  #ifdef DEBUG_PCI
> -# define PCI_DPRINTF(format, ...)       printf(format, ## __VA_ARGS__)
> +# define PCI_DPRINTF(format, ...)       printf("pci:" format, ##
> __VA_ARGS__)
>  #else
>  # define PCI_DPRINTF(format, ...)       do { } while (0)
>  #endif
> @@ -60,6 +62,81 @@ static Property pci_props[] = {
>      DEFINE_PROP_END_OF_LIST()
>  };
> 
> +/* Provides config reads from the host */
> +uint32_t host_pci_read_config(int bus, int slot, int fn, uint32_t address,
> int len)
> +{
> +    uint32_t val = 0;
> +    int fd;
> +    int domain = 0;
> +    ssize_t ret;
> +    char dir[128], name[128];
> +
> +    snprintf(dir, sizeof(dir), "/sys/bus/pci/devices/%04x:%02x:%02x.%x/",
> +             domain, bus, slot, fn);
> +    snprintf(name, sizeof(name), "%sconfig", dir);
> +
> +    fd = open(name, O_RDONLY);
> +
> +    if (fd >= 0)
> +    {
> +        do
> +        {
> +            ret = pread(fd,&val,len,address);
> +        } while ((ret < 0) && (errno == EINTR || errno == EAGAIN));
> +
> +        if (ret != len)
> +        {
> +            PCI_DPRINTF("%s(%04x:%02x:%02x.%x, @0x%x, len=0x%x) %s ret=%zd
> error=%d\n",
> +                 __func__, 0000, bus, slot, fn, address, len, "pread
> Failed",ret,errno);
> +            val = (1UL << (len * 8)) - 1;
> +        }
> +    }
> +    else
> +    {
> +        PCI_DPRINTF("%s(%04x:%02x:%02x.%x, @0x%x, len=0x%x) %s\n",
> +                 __func__, 0000, bus, slot, fn, address, len, "Open
> Failed");
> +    }
> +
> +    PCI_DPRINTF("%s(%04x:%02x:%02x.%x, @0x%x, len=0x%x) %x\n",
> +                 __func__, 0000, bus, slot, fn, address, len, val);
> +    return val;
> +}
> +
> +/* Provides config writes to the host*/
> +void host_pci_write_config(int bus, int slot, int fn, uint32_t address,
> int len, uint32_t val)
> +{
> +    int fd;
> +    int domain = 0;
> +    ssize_t ret;
> +    char dir[128], name[128];
> +
> +    snprintf(dir, sizeof(dir), "/sys/bus/pci/devices/%04x:%02x:%02x.%x/",
> +             domain, bus, slot, fn);
> +    snprintf(name, sizeof(name), "%sconfig", dir);
> +
> +    fd = open(name, O_RDWR);
> +
> +    if (fd >= 0)
> +    {
> +        do
> +        {
> +            ret = pwrite(fd,&val,len,address);
> +        } while ((ret < 0) && (errno == EINTR || errno == EAGAIN));
> +
> +
> +        if (ret != len)
> +        {
> +            PCI_DPRINTF("%s(%04x:%02x:%02x.%x, @0x%x, len=0x%x) %x %s
> ret=%zd error=%d\n",
> +                 __func__, 0000, bus, slot, fn, address, len, val, "pread
> Failed",ret,errno);
> +        }
> +    }
> +    else
> +    {
> +        PCI_DPRINTF("%s(%04x:%02x:%02x.%x, @0x%x, len=0x%x) %s\n",
> +                 __func__, 0000, bus, slot, fn, address, len, "Open
> Failed");
> +    }
> +}
> +
>  static const VMStateDescription vmstate_pcibus = {
>      .name = "PCIBUS",
>      .version_id = 1,

I expect a huge NAK on this one.  When run from libvirt and with any
sort of process containment, QEMU won't have access to these.  Not to
mention that many of the uses of these break migration.

In the case of things like stolen memory, GTT stolen memory, and tseg,
we shouldn't need to use these accessors because we shouldn't really
care what the host value is.  We only care currently because we need the
offset into /dev/mem.  We should consider that in designing the vfio
interface.