From mboxrd@z Thu Jan  1 00:00:00 1970
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Subject: Re: [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
Message-ID: <20120330143909.GC22274@phenom.dumpdata.com>
References: <1332934907-24080-1-git-send-email-anthony.perard@citrix.com>
	<1332934907-24080-4-git-send-email-anthony.perard@citrix.com>
	<20120328185214.GA23805@phenom.dumpdata.com>
	<CAJJyHjKjgpOrt2kYs_uJPvjoEfo-cCvouA8HaZpxfwv=bQp9QQ@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Transfer-Encoding: quoted-printable
Return-path: <qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org>
Content-Disposition: inline
In-Reply-To: <CAJJyHjKjgpOrt2kYs_uJPvjoEfo-cCvouA8HaZpxfwv=bQp9QQ@mail.gmail.com>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
Errors-To: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org
Sender: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org
To: Anthony PERARD <anthony.perard@citrix.com>
Cc: Xen Devel <xen-devel@lists.xensource.com>, "Michael S . Tsirkin" <mst@redhat.com>, QEMU-devel <qemu-devel@nongnu.org>, Stefano Stabellini <stefano.stabellini@eu.citrix.com>
List-Id: xen-devel@lists.xenproject.org

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,
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 const char *name, char *buf, ssize_t size)
> >> +{
> >> + =A0 =A0int rc;
> >> +
> >> + =A0 =A0rc =3D snprintf(buf, size, "/sys/bus/pci/devices/%04x:%02x:=
%02x.%x/%s",
> >
> > The format is actually " %04x:%02x:%02x.%d"
> >
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0d->domain, d->bus, d->dev, d->f=
unc, name);
> >> +
> >> + =A0 =A0if (rc >=3D size || rc < 0) {
> >> + =A0 =A0 =A0 =A0/* The ouput is truncated or an other error is enco=
untered */
> >> + =A0 =A0 =A0 =A0return -1;
> >
> > -ENODEV
> >
> >> + =A0 =A0}
> >> + =A0 =A0return 0;
> >> +}
> >> +
> >> +static int xen_host_pci_get_resource(XenHostPCIDevice *d)
> >> +{
> >> + =A0 =A0int i, rc, fd;
> >> + =A0 =A0char path[PATH_MAX];
> >> + =A0 =A0char buf[512];
> >
> > You should have a #define for the 512.
> >
> >> + =A0 =A0unsigned long long start, end, flags, size;
> >> + =A0 =A0char *endptr, *s;
> >> +
> >> + =A0 =A0if (xen_host_pci_sysfs_path(d, "resource", path, sizeof (pa=
th))) {
> >> + =A0 =A0 =A0 =A0return -1;
> >
> > -ENODEV
>=20
> 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?
>=20
>=20
> >> + =A0 =A0}
> >> + =A0 =A0fd =3D open(path, O_RDONLY);
> >> + =A0 =A0if (fd =3D=3D -1) {
> >> + =A0 =A0 =A0 =A0XEN_HOST_PCI_LOG("Error: Can't open %s: %s\n", path=
, strerror(errno));
> >> + =A0 =A0 =A0 =A0return -errno;
> >> + =A0 =A0}
> >> +
> >> + =A0 =A0do {
> >> + =A0 =A0 =A0 =A0rc =3D read(fd, &buf, sizeof (buf));
> >> + =A0 =A0 =A0 =A0if (rc < 0 && errno !=3D EINTR) {
> >> + =A0 =A0 =A0 =A0 =A0 =A0rc =3D -errno;
> >
> > So you are using the errnor types, so you should use them throughout
> > this function.
> >
> >> + =A0 =A0 =A0 =A0 =A0 =A0goto out;
> >> + =A0 =A0 =A0 =A0}
> >> + =A0 =A0} while (rc < 0);
> >> + =A0 =A0buf[rc] =3D 0;
> >> + =A0 =A0rc =3D 0;
> >> +
> >> + =A0 =A0s =3D buf;
> >> + =A0 =A0for (i =3D 0; i < PCI_NUM_REGIONS; i++) {
> >> + =A0 =A0 =A0 =A0start =3D strtoll(s, &endptr, 16);
> >> + =A0 =A0 =A0 =A0if (*endptr !=3D ' ' || s =3D=3D endptr) {
> >> + =A0 =A0 =A0 =A0 =A0 =A0break;
> >> + =A0 =A0 =A0 =A0}
> >> + =A0 =A0 =A0 =A0s =3D endptr + 1;
> >> + =A0 =A0 =A0 =A0end =3D strtoll(s, &endptr, 16);
> >> + =A0 =A0 =A0 =A0if (*endptr !=3D ' ' || s =3D=3D endptr) {
> >> + =A0 =A0 =A0 =A0 =A0 =A0break;
> >> + =A0 =A0 =A0 =A0}
> >> + =A0 =A0 =A0 =A0s =3D endptr + 1;
> >> + =A0 =A0 =A0 =A0flags =3D strtoll(s, &endptr, 16);
> >> + =A0 =A0 =A0 =A0if (*endptr !=3D '\n' || s =3D=3D endptr) {
> >> + =A0 =A0 =A0 =A0 =A0 =A0break;
> >> + =A0 =A0 =A0 =A0}
> >> + =A0 =A0 =A0 =A0s =3D endptr + 1;
> >> +
> >> + =A0 =A0 =A0 =A0if (start) {
> >> + =A0 =A0 =A0 =A0 =A0 =A0size =3D end - start + 1;
> >> + =A0 =A0 =A0 =A0} else {
> >> + =A0 =A0 =A0 =A0 =A0 =A0size =3D 0;
> >> + =A0 =A0 =A0 =A0}
> >> +
> >> + =A0 =A0 =A0 =A0if (i < PCI_ROM_SLOT) {
> >> + =A0 =A0 =A0 =A0 =A0 =A0d->io_regions[i].base_addr =3D start;
> >> + =A0 =A0 =A0 =A0 =A0 =A0d->io_regions[i].size =3D size;
> >> + =A0 =A0 =A0 =A0 =A0 =A0d->io_regions[i].flags =3D flags;
> >> + =A0 =A0 =A0 =A0} else {
> >> + =A0 =A0 =A0 =A0 =A0 =A0d->rom.base_addr =3D start;
> >> + =A0 =A0 =A0 =A0 =A0 =A0d->rom.size =3D size;
> >> + =A0 =A0 =A0 =A0 =A0 =A0d->rom.flags =3D flags;
> >> + =A0 =A0 =A0 =A0}
> >> + =A0 =A0}
> >> + =A0 =A0if (i !=3D PCI_NUM_REGIONS) {
> >> + =A0 =A0 =A0 =A0rc =3D -1;
> >
> > -ENODEV
> >
> >
> >> + =A0 =A0}
> >> +
> >> +out:
> >> + =A0 =A0close(fd);
> >> + =A0 =A0return rc;
> >> +}
> >> +
> >> +static int xen_host_pci_get_value(XenHostPCIDevice *d, const char *=
name,
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
unsigned int *pvalue, int base)
> >> +{
> >> + =A0 =A0char path[PATH_MAX];
> >> + =A0 =A0char buf[42];
> >
> > You should use a #define
> >
> >> + =A0 =A0int fd, rc;
> >> + =A0 =A0unsigned long value;
> >> + =A0 =A0char *endptr;
> >> +
> >> + =A0 =A0if (xen_host_pci_sysfs_path(d, name, path, sizeof (path))) =
{
> >> + =A0 =A0 =A0 =A0return -1;
> >
> > -ENODEV
> >
> >> + =A0 =A0}
> >> + =A0 =A0fd =3D open(path, O_RDONLY);
> >> + =A0 =A0if (fd =3D=3D -1) {
> >> + =A0 =A0 =A0 =A0XEN_HOST_PCI_LOG("Error: Can't open %s: %s\n", path=
, strerror(errno));
> >> + =A0 =A0 =A0 =A0return -errno;
> >> + =A0 =A0}
> >> + =A0 =A0do {
> >> + =A0 =A0 =A0 =A0rc =3D read(fd, &buf, sizeof (buf) - 1);
> >
> > Here you have sizeof(buf) - 1, but in the function xen_host_pci_get_r=
esource(..)
> > you didn't do that. Any particular reason?
>=20
> Nop, it's just a mistake.
>=20
> >> + =A0 =A0 =A0 =A0if (rc < 0 && errno !=3D EINTR) {
> >> + =A0 =A0 =A0 =A0 =A0 =A0rc =3D -errno;
> >> + =A0 =A0 =A0 =A0 =A0 =A0goto out;
> >> + =A0 =A0 =A0 =A0}
> >> + =A0 =A0} while (rc < 0);
> >> + =A0 =A0buf[rc] =3D 0;
> >> + =A0 =A0value =3D strtol(buf, &endptr, base);
> >> + =A0 =A0if (endptr =3D=3D buf || *endptr !=3D '\n') {
> >> + =A0 =A0 =A0 =A0rc =3D -1;
> >
> > ??? -Exx something I think
> >
> >> + =A0 =A0} else if ((value =3D=3D LONG_MIN || value =3D=3D LONG_MAX)=
 && errno =3D=3D ERANGE) {
> >> + =A0 =A0 =A0 =A0rc =3D -errno;
> >> + =A0 =A0} else {
> >> + =A0 =A0 =A0 =A0rc =3D 0;
> >> + =A0 =A0 =A0 =A0*pvalue =3D value;
> >> + =A0 =A0}
> >> +out:
> >> + =A0 =A0close(fd);
> >> + =A0 =A0return rc;
> >> +}
> >> +
> >> +static inline int xen_host_pci_get_hex_value(XenHostPCIDevice *d,
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 =A0 =A0 const char *name,
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 =A0 =A0 unsigned int *pvalue)
> >> +{
> >> + =A0 =A0return xen_host_pci_get_value(d, name, pvalue, 16);
> >> +}
> >> +
> >> +static inline int xen_host_pci_get_dec_value(XenHostPCIDevice *d,
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 =A0 =A0 const char *name,
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 =A0 =A0 unsigned int *pvalue)
> >> +{
> >> + =A0 =A0return xen_host_pci_get_value(d, name, pvalue, 10);
> >> +}
> >> +
> >> +static bool xen_host_pci_dev_is_virtfn(XenHostPCIDevice *d)
> >> +{
> >> + =A0 =A0char path[PATH_MAX];
> >> + =A0 =A0struct stat buf;
> >> +
> >> + =A0 =A0if (xen_host_pci_sysfs_path(d, "physfn", path, sizeof (path=
))) {
> >> + =A0 =A0 =A0 =A0return false;
> >> + =A0 =A0}
> >> + =A0 =A0return !stat(path, &buf);
> >> +}
> >> +
> >> +static int xen_host_pci_config_open(XenHostPCIDevice *d)
> >> +{
> >> + =A0 =A0char path[PATH_MAX];
> >> +
> >> + =A0 =A0if (xen_host_pci_sysfs_path(d, "config", path, sizeof (path=
))) {
> >> + =A0 =A0 =A0 =A0return -1;
> >
> > -ENODEV
> >
> >> + =A0 =A0}
> >> + =A0 =A0d->config_fd =3D open(path, O_RDWR);
> >> + =A0 =A0if (d->config_fd < 0) {
> >> + =A0 =A0 =A0 =A0return -errno;
> >> + =A0 =A0}
> >> + =A0 =A0return 0;
> >> +}
>=20
> [...]
>=20
> >> +int xen_host_pci_find_ext_cap_offset(XenHostPCIDevice *d, uint32_t =
cap)
> >> +{
> >> + =A0 =A0uint32_t header =3D 0;
> >> + =A0 =A0int max_cap =3D XEN_HOST_PCI_MAX_EXT_CAP;
> >> + =A0 =A0int pos =3D PCI_CONFIG_SPACE_SIZE;
> >> +
> >> + =A0 =A0do {
> >> + =A0 =A0 =A0 =A0if (xen_host_pci_get_long(d, pos, &header)) {
> >> + =A0 =A0 =A0 =A0 =A0 =A0break;
> >> + =A0 =A0 =A0 =A0}
> >> + =A0 =A0 =A0 =A0/*
> >> + =A0 =A0 =A0 =A0 * If we have no capabilities, this is indicated by=
 cap ID,
> >> + =A0 =A0 =A0 =A0 * cap version and next pointer all being 0.
> >> + =A0 =A0 =A0 =A0 */
> >> + =A0 =A0 =A0 =A0if (header =3D=3D 0) {
> >> + =A0 =A0 =A0 =A0 =A0 =A0break;
> >> + =A0 =A0 =A0 =A0}
> >> +
> >> + =A0 =A0 =A0 =A0if (PCI_EXT_CAP_ID(header) =3D=3D cap) {
> >> + =A0 =A0 =A0 =A0 =A0 =A0return pos;
> >> + =A0 =A0 =A0 =A0}
> >> +
> >> + =A0 =A0 =A0 =A0pos =3D PCI_EXT_CAP_NEXT(header);
> >> + =A0 =A0 =A0 =A0if (pos < PCI_CONFIG_SPACE_SIZE) {
> >> + =A0 =A0 =A0 =A0 =A0 =A0break;
> >> + =A0 =A0 =A0 =A0}
> >> +
> >> + =A0 =A0 =A0 =A0max_cap--;
> >> + =A0 =A0} while (max_cap > 0);
> >> +
> >> + =A0 =A0return 0;
> >
> > If we fail in this function (which can happen [https://bugzilla.redha=
t.com/show_bug.cgi?id=3D767742])
> > shouldn't this function return something else besides 0?
>=20
> -1 should be good enough, as it's not an offset.
>=20
> > Besides those comments, the patch looks good to me.
>=20
> Thanks,
>=20
> --=20
> Anthony PERARD