Patchwork [V10,3/8] Introduce XenHostPCIDevice to access a pci device on the host.

login
register
mail settings
Submitter Anthony PERARD
Date March 28, 2012, 11:41 a.m.
Message ID <1332934907-24080-4-git-send-email-anthony.perard@citrix.com>
Download mbox | patch
Permalink /patch/149190/
State New
Headers show

Comments

Anthony PERARD - March 28, 2012, 11:41 a.m.
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 Makefile.target          |    3 +
 hw/xen-host-pci-device.c |  354 ++++++++++++++++++++++++++++++++++++++++++++++
 hw/xen-host-pci-device.h |   78 ++++++++++
 3 files changed, 435 insertions(+), 0 deletions(-)
 create mode 100644 hw/xen-host-pci-device.c
 create mode 100644 hw/xen-host-pci-device.h
Konrad Rzeszutek Wilk - March 28, 2012, 6:52 p.m.
> +static int xen_host_pci_sysfs_path(const XenHostPCIDevice *d,
> +                                   const char *name, char *buf, ssize_t size)
> +{
> +    int rc;
> +
> +    rc = snprintf(buf, size, "/sys/bus/pci/devices/%04x:%02x:%02x.%x/%s",

The format is actually " %04x:%02x:%02x.%d"

> +                  d->domain, d->bus, d->dev, d->func, name);
> +
> +    if (rc >= size || rc < 0) {
> +        /* The ouput is truncated or an other error is encountered */
> +        return -1;

-ENODEV

> +    }
> +    return 0;
> +}
> +
> +static int xen_host_pci_get_resource(XenHostPCIDevice *d)
> +{
> +    int i, rc, fd;
> +    char path[PATH_MAX];
> +    char buf[512];

You should have a #define for the 512.

> +    unsigned long long start, end, flags, size;
> +    char *endptr, *s;
> +
> +    if (xen_host_pci_sysfs_path(d, "resource", path, sizeof (path))) {
> +        return -1;

-ENODEV

> +    }
> +    fd = open(path, O_RDONLY);
> +    if (fd == -1) {
> +        XEN_HOST_PCI_LOG("Error: Can't open %s: %s\n", path, strerror(errno));
> +        return -errno;
> +    }
> +
> +    do {
> +        rc = read(fd, &buf, sizeof (buf));
> +        if (rc < 0 && errno != EINTR) {
> +            rc = -errno;

So you are using the errnor types, so you should use them throughout
this function.

> +            goto out;
> +        }
> +    } while (rc < 0);
> +    buf[rc] = 0;
> +    rc = 0;
> +
> +    s = buf;
> +    for (i = 0; i < PCI_NUM_REGIONS; i++) {
> +        start = strtoll(s, &endptr, 16);
> +        if (*endptr != ' ' || s == endptr) {
> +            break;
> +        }
> +        s = endptr + 1;
> +        end = strtoll(s, &endptr, 16);
> +        if (*endptr != ' ' || s == endptr) {
> +            break;
> +        }
> +        s = endptr + 1;
> +        flags = strtoll(s, &endptr, 16);
> +        if (*endptr != '\n' || s == endptr) {
> +            break;
> +        }
> +        s = endptr + 1;
> +
> +        if (start) {
> +            size = end - start + 1;
> +        } else {
> +            size = 0;
> +        }
> +
> +        if (i < PCI_ROM_SLOT) {
> +            d->io_regions[i].base_addr = start;
> +            d->io_regions[i].size = size;
> +            d->io_regions[i].flags = flags;
> +        } else {
> +            d->rom.base_addr = start;
> +            d->rom.size = size;
> +            d->rom.flags = flags;
> +        }
> +    }
> +    if (i != PCI_NUM_REGIONS) {
> +        rc = -1;

-ENODEV


> +    }
> +
> +out:
> +    close(fd);
> +    return rc;
> +}
> +
> +static int xen_host_pci_get_value(XenHostPCIDevice *d, const char *name,
> +                                  unsigned int *pvalue, int base)
> +{
> +    char path[PATH_MAX];
> +    char buf[42];

You should use a #define

> +    int fd, rc;
> +    unsigned long value;
> +    char *endptr;
> +
> +    if (xen_host_pci_sysfs_path(d, name, path, sizeof (path))) {
> +        return -1;

-ENODEV

> +    }
> +    fd = open(path, O_RDONLY);
> +    if (fd == -1) {
> +        XEN_HOST_PCI_LOG("Error: Can't open %s: %s\n", path, strerror(errno));
> +        return -errno;
> +    }
> +    do {
> +        rc = read(fd, &buf, sizeof (buf) - 1);

Here you have sizeof(buf) - 1, but in the function xen_host_pci_get_resource(..)
you didn't do that. Any particular reason?


> +        if (rc < 0 && errno != EINTR) {
> +            rc = -errno;
> +            goto out;
> +        }
> +    } while (rc < 0);
> +    buf[rc] = 0;
> +    value = strtol(buf, &endptr, base);
> +    if (endptr == buf || *endptr != '\n') {
> +        rc = -1;

??? -Exx something I think

> +    } else if ((value == LONG_MIN || value == LONG_MAX) && errno == ERANGE) {
> +        rc = -errno;
> +    } else {
> +        rc = 0;
> +        *pvalue = value;
> +    }
> +out:
> +    close(fd);
> +    return rc;
> +}
> +
> +static inline int xen_host_pci_get_hex_value(XenHostPCIDevice *d,
> +                                             const char *name,
> +                                             unsigned int *pvalue)
> +{
> +    return xen_host_pci_get_value(d, name, pvalue, 16);
> +}
> +
> +static inline int xen_host_pci_get_dec_value(XenHostPCIDevice *d,
> +                                             const char *name,
> +                                             unsigned int *pvalue)
> +{
> +    return xen_host_pci_get_value(d, name, pvalue, 10);
> +}
> +
> +static bool xen_host_pci_dev_is_virtfn(XenHostPCIDevice *d)
> +{
> +    char path[PATH_MAX];
> +    struct stat buf;
> +
> +    if (xen_host_pci_sysfs_path(d, "physfn", path, sizeof (path))) {
> +        return false;
> +    }
> +    return !stat(path, &buf);
> +}
> +
> +static int xen_host_pci_config_open(XenHostPCIDevice *d)
> +{
> +    char path[PATH_MAX];
> +
> +    if (xen_host_pci_sysfs_path(d, "config", path, sizeof (path))) {
> +        return -1;

-ENODEV

> +    }
> +    d->config_fd = open(path, O_RDWR);
> +    if (d->config_fd < 0) {
> +        return -errno;
> +    }
> +    return 0;
> +}
> +
> +static int xen_host_pci_config_read(XenHostPCIDevice *d,
> +                                    int pos, void *buf, int len)
> +{
> +    int rc;
> +
> +    do {
> +        rc = pread(d->config_fd, buf, len, pos);
> +    } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
> +    if (rc != len) {
> +        return -errno;
> +    }
> +    return 0;
> +}
> +
> +static int xen_host_pci_config_write(XenHostPCIDevice *d,
> +                                     int pos, const void *buf, int len)
> +{
> +    int rc;
> +
> +    do {
> +        rc = pwrite(d->config_fd, buf, len, pos);
> +    } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
> +    if (rc != len) {
> +        return -errno;
> +    }
> +    return 0;
> +}
> +
> +int xen_host_pci_get_byte(XenHostPCIDevice *d, int pos, uint8_t *p)
> +{
> +    uint8_t buf;
> +    int rc = xen_host_pci_config_read(d, pos, &buf, 1);
> +    if (!rc) {
> +        *p = buf;
> +    }
> +    return rc;
> +}
> +
> +int xen_host_pci_get_word(XenHostPCIDevice *d, int pos, uint16_t *p)
> +{
> +    uint16_t buf;
> +    int rc = xen_host_pci_config_read(d, pos, &buf, 2);
> +    if (!rc) {
> +        *p = le16_to_cpu(buf);
> +    }
> +    return rc;
> +}
> +
> +int xen_host_pci_get_long(XenHostPCIDevice *d, int pos, uint32_t *p)
> +{
> +    uint32_t buf;
> +    int rc = xen_host_pci_config_read(d, pos, &buf, 4);
> +    if (!rc) {
> +        *p = le32_to_cpu(buf);
> +    }
> +    return rc;
> +}
> +
> +int xen_host_pci_get_block(XenHostPCIDevice *d, int pos, uint8_t *buf, int len)
> +{
> +    return xen_host_pci_config_read(d, pos, buf, len);
> +}
> +
> +int xen_host_pci_set_byte(XenHostPCIDevice *d, int pos, uint8_t data)
> +{
> +    return xen_host_pci_config_write(d, pos, &data, 1);
> +}
> +
> +int xen_host_pci_set_word(XenHostPCIDevice *d, int pos, uint16_t data)
> +{
> +    data = cpu_to_le16(data);
> +    return xen_host_pci_config_write(d, pos, &data, 2);
> +}
> +
> +int xen_host_pci_set_long(XenHostPCIDevice *d, int pos, uint32_t data)
> +{
> +    data = cpu_to_le32(data);
> +    return xen_host_pci_config_write(d, pos, &data, 4);
> +}
> +
> +int xen_host_pci_set_block(XenHostPCIDevice *d, int pos, uint8_t *buf, int len)
> +{
> +    return xen_host_pci_config_write(d, pos, buf, len);
> +}
> +
> +int xen_host_pci_find_ext_cap_offset(XenHostPCIDevice *d, uint32_t cap)
> +{
> +    uint32_t header = 0;
> +    int max_cap = XEN_HOST_PCI_MAX_EXT_CAP;
> +    int pos = PCI_CONFIG_SPACE_SIZE;
> +
> +    do {
> +        if (xen_host_pci_get_long(d, pos, &header)) {
> +            break;
> +        }
> +        /*
> +         * If we have no capabilities, this is indicated by cap ID,
> +         * cap version and next pointer all being 0.
> +         */
> +        if (header == 0) {
> +            break;
> +        }
> +
> +        if (PCI_EXT_CAP_ID(header) == cap) {
> +            return pos;
> +        }
> +
> +        pos = PCI_EXT_CAP_NEXT(header);
> +        if (pos < PCI_CONFIG_SPACE_SIZE) {
> +            break;
> +        }
> +
> +        max_cap--;
> +    } while (max_cap > 0);
> +
> +    return 0;

If we fail in this function (which can happen [https://bugzilla.redhat.com/show_bug.cgi?id=767742])
shouldn't this function return something else besides 0?


Besides those comments, the patch looks good to me.
Anthony PERARD - March 30, 2012, 2:21 p.m.
On Wed, Mar 28, 2012 at 19:52, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
>> +static int xen_host_pci_sysfs_path(const XenHostPCIDevice *d,
>> +                                   const char *name, char *buf, ssize_t size)
>> +{
>> +    int rc;
>> +
>> +    rc = snprintf(buf, size, "/sys/bus/pci/devices/%04x:%02x:%02x.%x/%s",
>
> The format is actually " %04x:%02x:%02x.%d"
>
>> +                  d->domain, d->bus, d->dev, d->func, name);
>> +
>> +    if (rc >= size || rc < 0) {
>> +        /* The ouput is truncated or an other error is encountered */
>> +        return -1;
>
> -ENODEV
>
>> +    }
>> +    return 0;
>> +}
>> +
>> +static int xen_host_pci_get_resource(XenHostPCIDevice *d)
>> +{
>> +    int i, rc, fd;
>> +    char path[PATH_MAX];
>> +    char buf[512];
>
> You should have a #define for the 512.
>
>> +    unsigned long long start, end, flags, size;
>> +    char *endptr, *s;
>> +
>> +    if (xen_host_pci_sysfs_path(d, "resource", path, sizeof (path))) {
>> +        return -1;
>
> -ENODEV

I still does not understand why return ENODEV (No such device) when an
error occure. Is it the better way to say that a device does not
behave like it should, or that a "module" using a device encounter any
kind of error that prevent it to use the device ?


>> +    }
>> +    fd = open(path, O_RDONLY);
>> +    if (fd == -1) {
>> +        XEN_HOST_PCI_LOG("Error: Can't open %s: %s\n", path, strerror(errno));
>> +        return -errno;
>> +    }
>> +
>> +    do {
>> +        rc = read(fd, &buf, sizeof (buf));
>> +        if (rc < 0 && errno != EINTR) {
>> +            rc = -errno;
>
> So you are using the errnor types, so you should use them throughout
> this function.
>
>> +            goto out;
>> +        }
>> +    } while (rc < 0);
>> +    buf[rc] = 0;
>> +    rc = 0;
>> +
>> +    s = buf;
>> +    for (i = 0; i < PCI_NUM_REGIONS; i++) {
>> +        start = strtoll(s, &endptr, 16);
>> +        if (*endptr != ' ' || s == endptr) {
>> +            break;
>> +        }
>> +        s = endptr + 1;
>> +        end = strtoll(s, &endptr, 16);
>> +        if (*endptr != ' ' || s == endptr) {
>> +            break;
>> +        }
>> +        s = endptr + 1;
>> +        flags = strtoll(s, &endptr, 16);
>> +        if (*endptr != '\n' || s == endptr) {
>> +            break;
>> +        }
>> +        s = endptr + 1;
>> +
>> +        if (start) {
>> +            size = end - start + 1;
>> +        } else {
>> +            size = 0;
>> +        }
>> +
>> +        if (i < PCI_ROM_SLOT) {
>> +            d->io_regions[i].base_addr = start;
>> +            d->io_regions[i].size = size;
>> +            d->io_regions[i].flags = flags;
>> +        } else {
>> +            d->rom.base_addr = start;
>> +            d->rom.size = size;
>> +            d->rom.flags = flags;
>> +        }
>> +    }
>> +    if (i != PCI_NUM_REGIONS) {
>> +        rc = -1;
>
> -ENODEV
>
>
>> +    }
>> +
>> +out:
>> +    close(fd);
>> +    return rc;
>> +}
>> +
>> +static int xen_host_pci_get_value(XenHostPCIDevice *d, const char *name,
>> +                                  unsigned int *pvalue, int base)
>> +{
>> +    char path[PATH_MAX];
>> +    char buf[42];
>
> You should use a #define
>
>> +    int fd, rc;
>> +    unsigned long value;
>> +    char *endptr;
>> +
>> +    if (xen_host_pci_sysfs_path(d, name, path, sizeof (path))) {
>> +        return -1;
>
> -ENODEV
>
>> +    }
>> +    fd = open(path, O_RDONLY);
>> +    if (fd == -1) {
>> +        XEN_HOST_PCI_LOG("Error: Can't open %s: %s\n", path, strerror(errno));
>> +        return -errno;
>> +    }
>> +    do {
>> +        rc = read(fd, &buf, sizeof (buf) - 1);
>
> Here you have sizeof(buf) - 1, but in the function xen_host_pci_get_resource(..)
> you didn't do that. Any particular reason?

Nop, it's just a mistake.

>> +        if (rc < 0 && errno != EINTR) {
>> +            rc = -errno;
>> +            goto out;
>> +        }
>> +    } while (rc < 0);
>> +    buf[rc] = 0;
>> +    value = strtol(buf, &endptr, base);
>> +    if (endptr == buf || *endptr != '\n') {
>> +        rc = -1;
>
> ??? -Exx something I think
>
>> +    } else if ((value == LONG_MIN || value == LONG_MAX) && errno == ERANGE) {
>> +        rc = -errno;
>> +    } else {
>> +        rc = 0;
>> +        *pvalue = value;
>> +    }
>> +out:
>> +    close(fd);
>> +    return rc;
>> +}
>> +
>> +static inline int xen_host_pci_get_hex_value(XenHostPCIDevice *d,
>> +                                             const char *name,
>> +                                             unsigned int *pvalue)
>> +{
>> +    return xen_host_pci_get_value(d, name, pvalue, 16);
>> +}
>> +
>> +static inline int xen_host_pci_get_dec_value(XenHostPCIDevice *d,
>> +                                             const char *name,
>> +                                             unsigned int *pvalue)
>> +{
>> +    return xen_host_pci_get_value(d, name, pvalue, 10);
>> +}
>> +
>> +static bool xen_host_pci_dev_is_virtfn(XenHostPCIDevice *d)
>> +{
>> +    char path[PATH_MAX];
>> +    struct stat buf;
>> +
>> +    if (xen_host_pci_sysfs_path(d, "physfn", path, sizeof (path))) {
>> +        return false;
>> +    }
>> +    return !stat(path, &buf);
>> +}
>> +
>> +static int xen_host_pci_config_open(XenHostPCIDevice *d)
>> +{
>> +    char path[PATH_MAX];
>> +
>> +    if (xen_host_pci_sysfs_path(d, "config", path, sizeof (path))) {
>> +        return -1;
>
> -ENODEV
>
>> +    }
>> +    d->config_fd = open(path, O_RDWR);
>> +    if (d->config_fd < 0) {
>> +        return -errno;
>> +    }
>> +    return 0;
>> +}

[...]

>> +int xen_host_pci_find_ext_cap_offset(XenHostPCIDevice *d, uint32_t cap)
>> +{
>> +    uint32_t header = 0;
>> +    int max_cap = XEN_HOST_PCI_MAX_EXT_CAP;
>> +    int pos = PCI_CONFIG_SPACE_SIZE;
>> +
>> +    do {
>> +        if (xen_host_pci_get_long(d, pos, &header)) {
>> +            break;
>> +        }
>> +        /*
>> +         * If we have no capabilities, this is indicated by cap ID,
>> +         * cap version and next pointer all being 0.
>> +         */
>> +        if (header == 0) {
>> +            break;
>> +        }
>> +
>> +        if (PCI_EXT_CAP_ID(header) == cap) {
>> +            return pos;
>> +        }
>> +
>> +        pos = PCI_EXT_CAP_NEXT(header);
>> +        if (pos < PCI_CONFIG_SPACE_SIZE) {
>> +            break;
>> +        }
>> +
>> +        max_cap--;
>> +    } while (max_cap > 0);
>> +
>> +    return 0;
>
> If we fail in this function (which can happen [https://bugzilla.redhat.com/show_bug.cgi?id=767742])
> shouldn't this function return something else besides 0?

-1 should be good enough, as it's not an offset.

> Besides those comments, the patch looks good to me.

Thanks,
Konrad Rzeszutek Wilk - March 30, 2012, 2:39 p.m.
On Fri, Mar 30, 2012 at 03:21:07PM +0100, Anthony PERARD wrote:
> On Wed, Mar 28, 2012 at 19:52, Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com> wrote:
> >> +static int xen_host_pci_sysfs_path(const XenHostPCIDevice *d,
> >> +                                   const char *name, char *buf, ssize_t size)
> >> +{
> >> +    int rc;
> >> +
> >> +    rc = snprintf(buf, size, "/sys/bus/pci/devices/%04x:%02x:%02x.%x/%s",
> >
> > The format is actually " %04x:%02x:%02x.%d"
> >
> >> +                  d->domain, d->bus, d->dev, d->func, name);
> >> +
> >> +    if (rc >= size || rc < 0) {
> >> +        /* The ouput is truncated or an other error is encountered */
> >> +        return -1;
> >
> > -ENODEV
> >
> >> +    }
> >> +    return 0;
> >> +}
> >> +
> >> +static int xen_host_pci_get_resource(XenHostPCIDevice *d)
> >> +{
> >> +    int i, rc, fd;
> >> +    char path[PATH_MAX];
> >> +    char buf[512];
> >
> > You should have a #define for the 512.
> >
> >> +    unsigned long long start, end, flags, size;
> >> +    char *endptr, *s;
> >> +
> >> +    if (xen_host_pci_sysfs_path(d, "resource", path, sizeof (path))) {
> >> +        return -1;
> >
> > -ENODEV
> 
> I still does not understand why return ENODEV (No such device) when an
> error occure. Is it the better way to say that a device does not
> behave like it should, or that a "module" using a device encounter any
> kind of error that prevent it to use the device ?

That is fine too - there is probably a -Exxx that is exactly for that.
Perhaps ENOSPC?
> 
> 
> >> +    }
> >> +    fd = open(path, O_RDONLY);
> >> +    if (fd == -1) {
> >> +        XEN_HOST_PCI_LOG("Error: Can't open %s: %s\n", path, strerror(errno));
> >> +        return -errno;
> >> +    }
> >> +
> >> +    do {
> >> +        rc = read(fd, &buf, sizeof (buf));
> >> +        if (rc < 0 && errno != EINTR) {
> >> +            rc = -errno;
> >
> > So you are using the errnor types, so you should use them throughout
> > this function.
> >
> >> +            goto out;
> >> +        }
> >> +    } while (rc < 0);
> >> +    buf[rc] = 0;
> >> +    rc = 0;
> >> +
> >> +    s = buf;
> >> +    for (i = 0; i < PCI_NUM_REGIONS; i++) {
> >> +        start = strtoll(s, &endptr, 16);
> >> +        if (*endptr != ' ' || s == endptr) {
> >> +            break;
> >> +        }
> >> +        s = endptr + 1;
> >> +        end = strtoll(s, &endptr, 16);
> >> +        if (*endptr != ' ' || s == endptr) {
> >> +            break;
> >> +        }
> >> +        s = endptr + 1;
> >> +        flags = strtoll(s, &endptr, 16);
> >> +        if (*endptr != '\n' || s == endptr) {
> >> +            break;
> >> +        }
> >> +        s = endptr + 1;
> >> +
> >> +        if (start) {
> >> +            size = end - start + 1;
> >> +        } else {
> >> +            size = 0;
> >> +        }
> >> +
> >> +        if (i < PCI_ROM_SLOT) {
> >> +            d->io_regions[i].base_addr = start;
> >> +            d->io_regions[i].size = size;
> >> +            d->io_regions[i].flags = flags;
> >> +        } else {
> >> +            d->rom.base_addr = start;
> >> +            d->rom.size = size;
> >> +            d->rom.flags = flags;
> >> +        }
> >> +    }
> >> +    if (i != PCI_NUM_REGIONS) {
> >> +        rc = -1;
> >
> > -ENODEV
> >
> >
> >> +    }
> >> +
> >> +out:
> >> +    close(fd);
> >> +    return rc;
> >> +}
> >> +
> >> +static int xen_host_pci_get_value(XenHostPCIDevice *d, const char *name,
> >> +                                  unsigned int *pvalue, int base)
> >> +{
> >> +    char path[PATH_MAX];
> >> +    char buf[42];
> >
> > You should use a #define
> >
> >> +    int fd, rc;
> >> +    unsigned long value;
> >> +    char *endptr;
> >> +
> >> +    if (xen_host_pci_sysfs_path(d, name, path, sizeof (path))) {
> >> +        return -1;
> >
> > -ENODEV
> >
> >> +    }
> >> +    fd = open(path, O_RDONLY);
> >> +    if (fd == -1) {
> >> +        XEN_HOST_PCI_LOG("Error: Can't open %s: %s\n", path, strerror(errno));
> >> +        return -errno;
> >> +    }
> >> +    do {
> >> +        rc = read(fd, &buf, sizeof (buf) - 1);
> >
> > Here you have sizeof(buf) - 1, but in the function xen_host_pci_get_resource(..)
> > you didn't do that. Any particular reason?
> 
> Nop, it's just a mistake.
> 
> >> +        if (rc < 0 && errno != EINTR) {
> >> +            rc = -errno;
> >> +            goto out;
> >> +        }
> >> +    } while (rc < 0);
> >> +    buf[rc] = 0;
> >> +    value = strtol(buf, &endptr, base);
> >> +    if (endptr == buf || *endptr != '\n') {
> >> +        rc = -1;
> >
> > ??? -Exx something I think
> >
> >> +    } else if ((value == LONG_MIN || value == LONG_MAX) && errno == ERANGE) {
> >> +        rc = -errno;
> >> +    } else {
> >> +        rc = 0;
> >> +        *pvalue = value;
> >> +    }
> >> +out:
> >> +    close(fd);
> >> +    return rc;
> >> +}
> >> +
> >> +static inline int xen_host_pci_get_hex_value(XenHostPCIDevice *d,
> >> +                                             const char *name,
> >> +                                             unsigned int *pvalue)
> >> +{
> >> +    return xen_host_pci_get_value(d, name, pvalue, 16);
> >> +}
> >> +
> >> +static inline int xen_host_pci_get_dec_value(XenHostPCIDevice *d,
> >> +                                             const char *name,
> >> +                                             unsigned int *pvalue)
> >> +{
> >> +    return xen_host_pci_get_value(d, name, pvalue, 10);
> >> +}
> >> +
> >> +static bool xen_host_pci_dev_is_virtfn(XenHostPCIDevice *d)
> >> +{
> >> +    char path[PATH_MAX];
> >> +    struct stat buf;
> >> +
> >> +    if (xen_host_pci_sysfs_path(d, "physfn", path, sizeof (path))) {
> >> +        return false;
> >> +    }
> >> +    return !stat(path, &buf);
> >> +}
> >> +
> >> +static int xen_host_pci_config_open(XenHostPCIDevice *d)
> >> +{
> >> +    char path[PATH_MAX];
> >> +
> >> +    if (xen_host_pci_sysfs_path(d, "config", path, sizeof (path))) {
> >> +        return -1;
> >
> > -ENODEV
> >
> >> +    }
> >> +    d->config_fd = open(path, O_RDWR);
> >> +    if (d->config_fd < 0) {
> >> +        return -errno;
> >> +    }
> >> +    return 0;
> >> +}
> 
> [...]
> 
> >> +int xen_host_pci_find_ext_cap_offset(XenHostPCIDevice *d, uint32_t cap)
> >> +{
> >> +    uint32_t header = 0;
> >> +    int max_cap = XEN_HOST_PCI_MAX_EXT_CAP;
> >> +    int pos = PCI_CONFIG_SPACE_SIZE;
> >> +
> >> +    do {
> >> +        if (xen_host_pci_get_long(d, pos, &header)) {
> >> +            break;
> >> +        }
> >> +        /*
> >> +         * If we have no capabilities, this is indicated by cap ID,
> >> +         * cap version and next pointer all being 0.
> >> +         */
> >> +        if (header == 0) {
> >> +            break;
> >> +        }
> >> +
> >> +        if (PCI_EXT_CAP_ID(header) == cap) {
> >> +            return pos;
> >> +        }
> >> +
> >> +        pos = PCI_EXT_CAP_NEXT(header);
> >> +        if (pos < PCI_CONFIG_SPACE_SIZE) {
> >> +            break;
> >> +        }
> >> +
> >> +        max_cap--;
> >> +    } while (max_cap > 0);
> >> +
> >> +    return 0;
> >
> > If we fail in this function (which can happen [https://bugzilla.redhat.com/show_bug.cgi?id=767742])
> > shouldn't this function return something else besides 0?
> 
> -1 should be good enough, as it's not an offset.
> 
> > Besides those comments, the patch looks good to me.
> 
> Thanks,
> 
> -- 
> Anthony PERARD

Patch

diff --git a/Makefile.target b/Makefile.target
index 44b2e83..70386a7 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -235,6 +235,9 @@  obj-$(CONFIG_NO_XEN) += xen-stub.o
 
 obj-i386-$(CONFIG_XEN) += xen_platform.o
 
+# Xen PCI Passthrough
+obj-i386-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen-host-pci-device.o
+
 # Inter-VM PCI shared memory
 CONFIG_IVSHMEM =
 ifeq ($(CONFIG_KVM), y)
diff --git a/hw/xen-host-pci-device.c b/hw/xen-host-pci-device.c
new file mode 100644
index 0000000..a1932d9
--- /dev/null
+++ b/hw/xen-host-pci-device.c
@@ -0,0 +1,354 @@ 
+/*
+ * Copyright (C) 2011       Citrix Ltd.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu-common.h"
+#include "xen-host-pci-device.h"
+
+#define XEN_HOST_PCI_MAX_EXT_CAP \
+    ((PCIE_CONFIG_SPACE_SIZE - PCI_CONFIG_SPACE_SIZE) / (PCI_CAP_SIZEOF + 4))
+
+#ifdef XEN_HOST_PCI_DEVICE_DEBUG
+#  define XEN_HOST_PCI_LOG(f, a...) fprintf(stderr, "%s: " f, __func__, ##a)
+#else
+#  define XEN_HOST_PCI_LOG(f, a...) (void)0
+#endif
+
+static int xen_host_pci_sysfs_path(const XenHostPCIDevice *d,
+                                   const char *name, char *buf, ssize_t size)
+{
+    int rc;
+
+    rc = snprintf(buf, size, "/sys/bus/pci/devices/%04x:%02x:%02x.%x/%s",
+                  d->domain, d->bus, d->dev, d->func, name);
+
+    if (rc >= size || rc < 0) {
+        /* The ouput is truncated or an other error is encountered */
+        return -1;
+    }
+    return 0;
+}
+
+static int xen_host_pci_get_resource(XenHostPCIDevice *d)
+{
+    int i, rc, fd;
+    char path[PATH_MAX];
+    char buf[512];
+    unsigned long long start, end, flags, size;
+    char *endptr, *s;
+
+    if (xen_host_pci_sysfs_path(d, "resource", path, sizeof (path))) {
+        return -1;
+    }
+    fd = open(path, O_RDONLY);
+    if (fd == -1) {
+        XEN_HOST_PCI_LOG("Error: Can't open %s: %s\n", path, strerror(errno));
+        return -errno;
+    }
+
+    do {
+        rc = read(fd, &buf, sizeof (buf));
+        if (rc < 0 && errno != EINTR) {
+            rc = -errno;
+            goto out;
+        }
+    } while (rc < 0);
+    buf[rc] = 0;
+    rc = 0;
+
+    s = buf;
+    for (i = 0; i < PCI_NUM_REGIONS; i++) {
+        start = strtoll(s, &endptr, 16);
+        if (*endptr != ' ' || s == endptr) {
+            break;
+        }
+        s = endptr + 1;
+        end = strtoll(s, &endptr, 16);
+        if (*endptr != ' ' || s == endptr) {
+            break;
+        }
+        s = endptr + 1;
+        flags = strtoll(s, &endptr, 16);
+        if (*endptr != '\n' || s == endptr) {
+            break;
+        }
+        s = endptr + 1;
+
+        if (start) {
+            size = end - start + 1;
+        } else {
+            size = 0;
+        }
+
+        if (i < PCI_ROM_SLOT) {
+            d->io_regions[i].base_addr = start;
+            d->io_regions[i].size = size;
+            d->io_regions[i].flags = flags;
+        } else {
+            d->rom.base_addr = start;
+            d->rom.size = size;
+            d->rom.flags = flags;
+        }
+    }
+    if (i != PCI_NUM_REGIONS) {
+        rc = -1;
+    }
+
+out:
+    close(fd);
+    return rc;
+}
+
+static int xen_host_pci_get_value(XenHostPCIDevice *d, const char *name,
+                                  unsigned int *pvalue, int base)
+{
+    char path[PATH_MAX];
+    char buf[42];
+    int fd, rc;
+    unsigned long value;
+    char *endptr;
+
+    if (xen_host_pci_sysfs_path(d, name, path, sizeof (path))) {
+        return -1;
+    }
+    fd = open(path, O_RDONLY);
+    if (fd == -1) {
+        XEN_HOST_PCI_LOG("Error: Can't open %s: %s\n", path, strerror(errno));
+        return -errno;
+    }
+    do {
+        rc = read(fd, &buf, sizeof (buf) - 1);
+        if (rc < 0 && errno != EINTR) {
+            rc = -errno;
+            goto out;
+        }
+    } while (rc < 0);
+    buf[rc] = 0;
+    value = strtol(buf, &endptr, base);
+    if (endptr == buf || *endptr != '\n') {
+        rc = -1;
+    } else if ((value == LONG_MIN || value == LONG_MAX) && errno == ERANGE) {
+        rc = -errno;
+    } else {
+        rc = 0;
+        *pvalue = value;
+    }
+out:
+    close(fd);
+    return rc;
+}
+
+static inline int xen_host_pci_get_hex_value(XenHostPCIDevice *d,
+                                             const char *name,
+                                             unsigned int *pvalue)
+{
+    return xen_host_pci_get_value(d, name, pvalue, 16);
+}
+
+static inline int xen_host_pci_get_dec_value(XenHostPCIDevice *d,
+                                             const char *name,
+                                             unsigned int *pvalue)
+{
+    return xen_host_pci_get_value(d, name, pvalue, 10);
+}
+
+static bool xen_host_pci_dev_is_virtfn(XenHostPCIDevice *d)
+{
+    char path[PATH_MAX];
+    struct stat buf;
+
+    if (xen_host_pci_sysfs_path(d, "physfn", path, sizeof (path))) {
+        return false;
+    }
+    return !stat(path, &buf);
+}
+
+static int xen_host_pci_config_open(XenHostPCIDevice *d)
+{
+    char path[PATH_MAX];
+
+    if (xen_host_pci_sysfs_path(d, "config", path, sizeof (path))) {
+        return -1;
+    }
+    d->config_fd = open(path, O_RDWR);
+    if (d->config_fd < 0) {
+        return -errno;
+    }
+    return 0;
+}
+
+static int xen_host_pci_config_read(XenHostPCIDevice *d,
+                                    int pos, void *buf, int len)
+{
+    int rc;
+
+    do {
+        rc = pread(d->config_fd, buf, len, pos);
+    } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
+    if (rc != len) {
+        return -errno;
+    }
+    return 0;
+}
+
+static int xen_host_pci_config_write(XenHostPCIDevice *d,
+                                     int pos, const void *buf, int len)
+{
+    int rc;
+
+    do {
+        rc = pwrite(d->config_fd, buf, len, pos);
+    } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
+    if (rc != len) {
+        return -errno;
+    }
+    return 0;
+}
+
+int xen_host_pci_get_byte(XenHostPCIDevice *d, int pos, uint8_t *p)
+{
+    uint8_t buf;
+    int rc = xen_host_pci_config_read(d, pos, &buf, 1);
+    if (!rc) {
+        *p = buf;
+    }
+    return rc;
+}
+
+int xen_host_pci_get_word(XenHostPCIDevice *d, int pos, uint16_t *p)
+{
+    uint16_t buf;
+    int rc = xen_host_pci_config_read(d, pos, &buf, 2);
+    if (!rc) {
+        *p = le16_to_cpu(buf);
+    }
+    return rc;
+}
+
+int xen_host_pci_get_long(XenHostPCIDevice *d, int pos, uint32_t *p)
+{
+    uint32_t buf;
+    int rc = xen_host_pci_config_read(d, pos, &buf, 4);
+    if (!rc) {
+        *p = le32_to_cpu(buf);
+    }
+    return rc;
+}
+
+int xen_host_pci_get_block(XenHostPCIDevice *d, int pos, uint8_t *buf, int len)
+{
+    return xen_host_pci_config_read(d, pos, buf, len);
+}
+
+int xen_host_pci_set_byte(XenHostPCIDevice *d, int pos, uint8_t data)
+{
+    return xen_host_pci_config_write(d, pos, &data, 1);
+}
+
+int xen_host_pci_set_word(XenHostPCIDevice *d, int pos, uint16_t data)
+{
+    data = cpu_to_le16(data);
+    return xen_host_pci_config_write(d, pos, &data, 2);
+}
+
+int xen_host_pci_set_long(XenHostPCIDevice *d, int pos, uint32_t data)
+{
+    data = cpu_to_le32(data);
+    return xen_host_pci_config_write(d, pos, &data, 4);
+}
+
+int xen_host_pci_set_block(XenHostPCIDevice *d, int pos, uint8_t *buf, int len)
+{
+    return xen_host_pci_config_write(d, pos, buf, len);
+}
+
+int xen_host_pci_find_ext_cap_offset(XenHostPCIDevice *d, uint32_t cap)
+{
+    uint32_t header = 0;
+    int max_cap = XEN_HOST_PCI_MAX_EXT_CAP;
+    int pos = PCI_CONFIG_SPACE_SIZE;
+
+    do {
+        if (xen_host_pci_get_long(d, pos, &header)) {
+            break;
+        }
+        /*
+         * If we have no capabilities, this is indicated by cap ID,
+         * cap version and next pointer all being 0.
+         */
+        if (header == 0) {
+            break;
+        }
+
+        if (PCI_EXT_CAP_ID(header) == cap) {
+            return pos;
+        }
+
+        pos = PCI_EXT_CAP_NEXT(header);
+        if (pos < PCI_CONFIG_SPACE_SIZE) {
+            break;
+        }
+
+        max_cap--;
+    } while (max_cap > 0);
+
+    return 0;
+}
+
+int xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
+                            uint8_t bus, uint8_t dev, uint8_t func)
+{
+    unsigned int v;
+    int rc = 0;
+
+    d->config_fd = -1;
+    d->domain = domain;
+    d->bus = bus;
+    d->dev = dev;
+    d->func = func;
+
+    rc = xen_host_pci_config_open(d);
+    if (rc) {
+        goto error;
+    }
+    rc = xen_host_pci_get_resource(d);
+    if (rc) {
+        goto error;
+    }
+    rc = xen_host_pci_get_hex_value(d, "vendor", &v);
+    if (rc) {
+        goto error;
+    }
+    d->vendor_id = v;
+    rc = xen_host_pci_get_hex_value(d, "device", &v);
+    if (rc) {
+        goto error;
+    }
+    d->device_id = v;
+    rc = xen_host_pci_get_dec_value(d, "irq", &v);
+    if (rc) {
+        goto error;
+    }
+    d->irq = v;
+    d->is_virtfn = xen_host_pci_dev_is_virtfn(d);
+
+    return 0;
+error:
+    if (d->config_fd >= 0) {
+        close(d->config_fd);
+        d->config_fd = -1;
+    }
+    return rc;
+}
+
+void xen_host_pci_device_put(XenHostPCIDevice *d)
+{
+    if (d->config_fd >= 0) {
+        close(d->config_fd);
+        d->config_fd = -1;
+    }
+}
diff --git a/hw/xen-host-pci-device.h b/hw/xen-host-pci-device.h
new file mode 100644
index 0000000..c5b183b
--- /dev/null
+++ b/hw/xen-host-pci-device.h
@@ -0,0 +1,78 @@ 
+#ifndef XEN_HOST_PCI_DEVICE_H
+#define XEN_HOST_PCI_DEVICE_H
+
+#include "pci.h"
+
+/*
+ * from linux/ioport.h
+ * IO resources have these defined flags.
+ */
+#define IORESOURCE_BITS         0x000000ff      /* Bus-specific bits */
+
+#define IORESOURCE_TYPE_BITS    0x00000f00      /* Resource type */
+#define IORESOURCE_IO           0x00000100
+#define IORESOURCE_MEM          0x00000200
+#define IORESOURCE_IRQ          0x00000400
+#define IORESOURCE_DMA          0x00000800
+
+#define IORESOURCE_PREFETCH     0x00001000      /* No side effects */
+#define IORESOURCE_READONLY     0x00002000
+#define IORESOURCE_CACHEABLE    0x00004000
+#define IORESOURCE_RANGELENGTH  0x00008000
+#define IORESOURCE_SHADOWABLE   0x00010000
+
+#define IORESOURCE_SIZEALIGN    0x00020000      /* size indicates alignment */
+#define IORESOURCE_STARTALIGN   0x00040000      /* start field is alignment */
+
+#define IORESOURCE_MEM_64       0x00100000
+
+/* Userland may not map this resource */
+#define IORESOURCE_EXCLUSIVE    0x08000000
+#define IORESOURCE_DISABLED     0x10000000
+#define IORESOURCE_UNSET        0x20000000
+#define IORESOURCE_AUTO         0x40000000
+/* Driver has marked this resource busy */
+#define IORESOURCE_BUSY         0x80000000
+
+typedef struct XenHostPCIIORegion {
+    unsigned long flags;
+    pcibus_t base_addr;
+    pcibus_t size;
+} XenHostPCIIORegion;
+
+typedef struct XenHostPCIDevice {
+    uint16_t domain;
+    uint8_t bus;
+    uint8_t dev;
+    uint8_t func;
+
+    uint16_t vendor_id;
+    uint16_t device_id;
+    int irq;
+
+    XenHostPCIIORegion io_regions[PCI_NUM_REGIONS - 1];
+    XenHostPCIIORegion rom;
+
+    bool is_virtfn;
+
+    int config_fd;
+} XenHostPCIDevice;
+
+int xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
+                            uint8_t bus, uint8_t dev, uint8_t func);
+void xen_host_pci_device_put(XenHostPCIDevice *pci_dev);
+
+int xen_host_pci_get_byte(XenHostPCIDevice *d, int pos, uint8_t *p);
+int xen_host_pci_get_word(XenHostPCIDevice *d, int pos, uint16_t *p);
+int xen_host_pci_get_long(XenHostPCIDevice *d, int pos, uint32_t *p);
+int xen_host_pci_get_block(XenHostPCIDevice *d, int pos, uint8_t *buf,
+                           int len);
+int xen_host_pci_set_byte(XenHostPCIDevice *d, int pos, uint8_t data);
+int xen_host_pci_set_word(XenHostPCIDevice *d, int pos, uint16_t data);
+int xen_host_pci_set_long(XenHostPCIDevice *d, int pos, uint32_t data);
+int xen_host_pci_set_block(XenHostPCIDevice *d, int pos, uint8_t *buf,
+                           int len);
+
+int xen_host_pci_find_ext_cap_offset(XenHostPCIDevice *s, uint32_t cap);
+
+#endif /* !XEN_HOST_PCI_DEVICE_H_ */