Re: [Qemu-devel] [PATCH RFC V1 01/11] Introduce HostPCIDevice to access a pci device on the host.

[Jan Kiszka <jan.kiszka@web.de>]

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

This wasn't run through checkpatch.pl, I bet.

On 2011-10-04 16:51, Anthony PERARD wrote:
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  hw/host-pci-device.c |  192 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/host-pci-device.h |   36 +++++++++
>  2 files changed, 228 insertions(+), 0 deletions(-)
>  create mode 100644 hw/host-pci-device.c
>  create mode 100644 hw/host-pci-device.h
> 
> diff --git a/hw/host-pci-device.c b/hw/host-pci-device.c
> new file mode 100644
> index 0000000..b3f2899
> --- /dev/null
> +++ b/hw/host-pci-device.c
> @@ -0,0 +1,192 @@
> +#include "qemu-common.h"
> +#include "host-pci-device.h"
> +
> +static int path_to(const HostPCIDevice *d,
> +                   const char *name, char *buf, ssize_t size)
> +{
> +    return snprintf(buf, size, "/sys/bus/pci/devices/%04x:%02x:%02x.%x/%s",
> +                    d->domain, d->bus, d->dev, d->func, name);
> +}
> +
> +static int get_resource(HostPCIDevice *d)
> +{
> +    int i;
> +    FILE *f;
> +    char path[PATH_MAX];
> +    unsigned long long start, end, flags, size;
> +
> +    path_to(d, "resource", path, sizeof (path));
> +    f = fopen(path, "r");
> +    if (!f) {
> +        fprintf(stderr, "Error: Can't open %s: %s\n", path, strerror(errno));
> +        return -1;
> +    }
> +
> +    for (i = 0; i < PCI_NUM_REGIONS; i++) {
> +        if (fscanf(f, "%llx %llx %llx", &start, &end, &flags) != 3) {
> +            fprintf(stderr, "Error: Syntax error in %s\n", path);
> +            break;
> +        }
> +        if (start) {
> +            size = end - start + 1;
> +        } else {
> +            size = 0;
> +        }
> +
> +        flags &= 0xf;

No magic numbers please.

It also looks a bit strange to me: It's the resource type encoded in the
second byte? Aren't you interested in it?

> +
> +        if (i < PCI_ROM_SLOT) {
> +            d->base_addr[i] = start | flags;
> +            d->size[i] = size;
> +        } else {
> +            d->rom_base_addr = start | flags;
> +            d->rom_size = size;
> +        }
> +    }
> +
> +    fclose(f);
> +    return 0;
> +}
> +
> +static unsigned long get_value(HostPCIDevice *d, const char *name)
> +{
> +    char path[PATH_MAX];
> +    FILE *f;
> +    unsigned long value;
> +
> +    path_to(d, name, path, sizeof (path));
> +    f = fopen(path, "r");
> +    if (!f) {
> +        fprintf(stderr, "Error: Can't open %s: %s\n", path, strerror(errno));
> +        return -1;
> +    }
> +    if (fscanf(f, "%lx\n", &value) != 1) {
> +        fprintf(stderr, "Error: Syntax error in %s\n", path);
> +        value = -1;
> +    }
> +    fclose(f);
> +    return value;
> +}
> +
> +static int pci_dev_is_virtfn(HostPCIDevice *d)
> +{
> +    int rc;
> +    char path[PATH_MAX];
> +    struct stat buf;
> +
> +    path_to(d, "physfn", path, sizeof (path));
> +    rc = !stat(path, &buf);
> +
> +    return rc;
> +}
> +
> +static int host_pci_config_fd(HostPCIDevice *d)

[ We will also need the reverse: pass in open file descriptors that
HostPCIDevice should use. Can be added later. ]

> +{
> +    char path[PATH_MAX];
> +
> +    if (d->config_fd < 0) {
> +        path_to(d, "config", path, sizeof (path));
> +        d->config_fd = open(path, O_RDWR);
> +        if (d->config_fd < 0) {
> +            fprintf(stderr, "HostPCIDevice: Can not open '%s': %s\n",
> +                    path, strerror(errno));
> +        }
> +    }
> +    return d->config_fd;
> +}
> +static int host_pci_config_read(HostPCIDevice *d, int pos, void *buf, int len)
> +{
> +    int fd = host_pci_config_fd(d);
> +    int res = 0;
> +
> +    res = pread(fd, buf, len, pos);
> +    if (res < 0) {
> +        fprintf(stderr, "host_pci_config: read failed: %s (fd: %i)\n",
> +                strerror(errno), fd);
> +        return -1;
> +    }
> +    return res;
> +}
> +static int host_pci_config_write(HostPCIDevice *d,
> +                                 int pos, const void *buf, int len)
> +{
> +    int fd = host_pci_config_fd(d);
> +    int res = 0;
> +
> +    res = pwrite(fd, buf, len, pos);
> +    if (res < 0) {
> +        fprintf(stderr, "host_pci_config: write failed: %s\n",
> +                strerror(errno));
> +        return -1;
> +    }
> +    return res;
> +}
> +
> +uint8_t host_pci_read_byte(HostPCIDevice *d, int pos)
> +{
> +  uint8_t buf;
> +  host_pci_config_read(d, pos, &buf, 1);
> +  return buf;
> +}
> +uint16_t host_pci_read_word(HostPCIDevice *d, int pos)
> +{
> +  uint16_t buf;
> +  host_pci_config_read(d, pos, &buf, 2);
> +  return le16_to_cpu(buf);
> +}
> +uint32_t host_pci_read_long(HostPCIDevice *d, int pos)
> +{
> +  uint32_t buf;
> +  host_pci_config_read(d, pos, &buf, 4);
> +  return le32_to_cpu(buf);
> +}
> +int host_pci_read_block(HostPCIDevice *d, int pos, uint8_t *buf, int len)
> +{
> +  return host_pci_config_read(d, pos, buf, len);
> +}
> +
> +int host_pci_write_byte(HostPCIDevice *d, int pos, uint8_t data)
> +{
> +  return host_pci_config_write(d, pos, &data, 1);
> +}
> +int host_pci_write_word(HostPCIDevice *d, int pos, uint16_t data)
> +{
> +  return host_pci_config_write(d, pos, &data, 2);

You adjust endianess on read, but not on write.

> +}
> +int host_pci_write_long(HostPCIDevice *d, int pos, uint32_t data)
> +{
> +  return host_pci_config_write(d, pos, &data, 4);
> +}
> +int host_pci_write_block(HostPCIDevice *d, int pos, uint8_t *buf, int len)
> +{
> +  return host_pci_config_write(d, pos, buf, len);
> +}
> +
> +HostPCIDevice *host_pci_device_get(uint8_t bus, uint8_t dev, uint8_t func)
> +{
> +    HostPCIDevice *d = NULL;
> +
> +    d = g_new0(HostPCIDevice, 1);
> +
> +    d->config_fd = -1;
> +    d->domain = 0;
> +    d->bus = bus;
> +    d->dev = dev;
> +    d->func = func;
> +
> +    if (host_pci_config_fd(d) == -1)
> +        goto error;
> +    if (get_resource(d) == -1)
> +        goto error;
> +
> +    d->vendor_id = get_value(d, "vendor");
> +    d->device_id = get_value(d, "device");
> +    d->is_virtfn = pci_dev_is_virtfn(d);
> +
> +    return d;
> +error:
> +    if (d->config_fd >= 0)
> +        close(d->config_fd);
> +    g_free(d);
> +    return NULL;
> +}
> diff --git a/hw/host-pci-device.h b/hw/host-pci-device.h
> new file mode 100644
> index 0000000..0137507
> --- /dev/null
> +++ b/hw/host-pci-device.h
> @@ -0,0 +1,36 @@
> +#ifndef HW_HOST_PCI_DEVICE
> +#  define HW_HOST_PCI_DEVICE
> +
> +#include "pci.h"
> +
> +typedef struct HostPCIDevice {
> +    uint16_t domain;
> +    uint8_t bus;
> +    uint8_t dev;
> +    uint8_t func;
> +
> +    uint16_t vendor_id;
> +    uint16_t device_id;
> +
> +    pcibus_t base_addr[PCI_NUM_REGIONS - 1];
> +    pcibus_t size[PCI_NUM_REGIONS - 1];
> +    pcibus_t rom_base_addr;
> +    pcibus_t rom_size;

Regions deserve their own type IMHO. In KVM we have

typedef struct {
    int type;           /* Memory or port I/O */
    int valid;
    uint32_t base_addr;
    uint32_t size;    /* size of the region */
    int resource_fd;
} PCIRegion;

Should probably become HostPCIIORegion (vs. virtual PCIIORegion), and
our field types need some cleanups.

> +
> +    bool is_virtfn;
> +
> +    int config_fd;
> +} HostPCIDevice;
> +
> +HostPCIDevice *host_pci_device_get(uint8_t bus, uint8_t dev, uint8_t func);

And what about some host_pci_device_put when we're done with it?

> +
> +uint8_t host_pci_read_byte(HostPCIDevice *d, int pos);
> +uint16_t host_pci_read_word(HostPCIDevice *d, int pos);
> +uint32_t host_pci_read_long(HostPCIDevice *d, int pos);
> +int host_pci_read_block(HostPCIDevice *d, int pos, uint8_t *buf, int len);
> +int host_pci_write_byte(HostPCIDevice *d, int pos, uint8_t data);
> +int host_pci_write_word(HostPCIDevice *d, int pos, uint16_t data);
> +int host_pci_write_long(HostPCIDevice *d, int pos, uint32_t data);
> +int host_pci_write_block(HostPCIDevice *d, int pos, uint8_t *buf, int len);

I think these should be analogous to our pci layer:
host_pci_get/set_byte/word/long/quad.

Looks like it's generally useful for KVM as well.

Jan


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