WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

[Xen-devel] Re: [PATCH RFC V1 01/11] Introduce HostPCIDevice to access a

To: Anthony PERARD <anthony.perard@xxxxxxxxxx>
Subject: [Xen-devel] Re: [PATCH RFC V1 01/11] Introduce HostPCIDevice to access a pci device on the host.
From: Jan Kiszka <jan.kiszka@xxxxxx>
Date: Tue, 04 Oct 2011 20:21:25 +0200
Cc: Alex Williamson <alex.williamson@xxxxxxxxxx>, Xen Devel <xen-devel@xxxxxxxxxxxxxxxxxxx>, QEMU-devel <qemu-devel@xxxxxxxxxx>, Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
Delivery-date: Tue, 04 Oct 2011 11:23:30 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <1317739882-4809-2-git-send-email-anthony.perard@xxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <1317739882-4809-1-git-send-email-anthony.perard@xxxxxxxxxx> <1317739882-4809-2-git-send-email-anthony.perard@xxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Mozilla/5.0 (X11; U; Linux i686 (x86_64); de; rv:1.8.1.12) Gecko/20080226 SUSE/2.0.0.12-1.1 Thunderbird/2.0.0.12 Mnenhy/0.7.5.666
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@xxxxxxxxxx>
> ---
>  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: signature.asc
Description: OpenPGP digital signature

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel