[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Xen-devel] [PATCH V10 3/8] Introduce XenHostPCIDevice
From: |
Konrad Rzeszutek Wilk |
Subject: |
Re: [Qemu-devel] [Xen-devel] [PATCH V10 3/8] Introduce XenHostPCIDevice to access a pci device on the host. |
Date: |
Fri, 30 Mar 2012 10:39:09 -0400 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Fri, Mar 30, 2012 at 03:21:07PM +0100, Anthony PERARD wrote:
> On Wed, Mar 28, 2012 at 19:52, Konrad Rzeszutek Wilk
> <address@hidden> 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
[Qemu-devel] [PATCH V10 8/8] Introduce Xen PCI Passthrough, MSI (3/3), Anthony PERARD, 2012/03/28
[Qemu-devel] [PATCH V10 5/8] Introduce Xen PCI Passthrough, qdevice (1/3), Anthony PERARD, 2012/03/28
[Qemu-devel] [PATCH V10 6/8] Introduce Xen PCI Passthrough, PCI config space helpers (2/3), Anthony PERARD, 2012/03/28