From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f66.google.com (mail-wm0-f66.google.com [74.125.82.66]) by dpdk.org (Postfix) with ESMTP id 6550D1B001 for ; Fri, 24 Aug 2018 19:14:40 +0200 (CEST) Received: by mail-wm0-f66.google.com with SMTP id c14-v6so2235007wmb.4 for ; Fri, 24 Aug 2018 10:14:40 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to :references:content-transfer-encoding:mime-version; bh=BCbpik2q6fdwnLegc1/atX2XqDiDR4D8PgGk8DjImoI=; b=Yu0oIvdGFL3Cxfd35CTcXda/I3dvWzeY42utmtJNDhGeGGiWY5WuAUAElw9BVm5Nz0 yPddkt2HqYZu7C4c/Wud8HX56PPRNvlTO9s1daiFOg6bwpFq7hZ0OoNWxqAwcHW26p3U Q3uJ6IHWzHuhT9RZxD4U52rSYzjggm7VeYbZ4QQ0Ggov/KcGkGyaNJr4+Gl4FHVvahZa Zceucp+scL6oq0y26ev9vWdTR4xQ3xg0PaS6EnEfHwUsxkWHY4JUwJgMLWe2ATiRdIPt /wIM2ehPBDfil0X5vh8bGh/D3wmrMINoMlmhXW8x6eo5lPSh1hTkR+xjMOkj9G8QqSOS euMg== X-Gm-Message-State: APzg51B3v8YSog5PP1mnhI3aC54zBBW7WxNKIio8VR1tXKPZhzQl7PYZ 0AGSqRcszhYfpGnukHeMh8A= X-Google-Smtp-Source: ANB0VdbggmqBS101VoWMQBtQPAQZUUInBXk8Vem8XB0/rxSRJvzcnZqTHiAXmd/f0hKaxNgD3A8GTw== X-Received: by 2002:a1c:8406:: with SMTP id g6-v6mr1868641wmd.18.1535130880098; Fri, 24 Aug 2018 10:14:40 -0700 (PDT) Received: from localhost ([2a01:4b00:f419:6f00:8361:8946:ba2b:d556]) by smtp.gmail.com with ESMTPSA id w15-v6sm6038012wrs.8.2018.08.24.10.14.38 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Fri, 24 Aug 2018 10:14:38 -0700 (PDT) Message-ID: <1535130878.5764.148.camel@debian.org> From: Luca Boccassi To: Tiwei Bie Cc: dev@dpdk.org, maxime.coquelin@redhat.com, zhihong.wang@intel.com, bruce.richardson@intel.com, brian.russell@intl.att.com Date: Fri, 24 Aug 2018 18:14:38 +0100 In-Reply-To: <20180824042355.GA18154@debian> References: <20180814143035.19640-1-bluca@debian.org> <20180816184750.30843-1-bluca@debian.org> <20180816184750.30843-2-bluca@debian.org> <1534445383.5764.56.camel@debian.org> <20180820081804.GA29806@debian> <1534783535.5764.93.camel@debian.org> <20180821024030.GA17967@debian> <1535028745.5764.134.camel@debian.org> <20180824042355.GA18154@debian> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Mailer: Evolution 3.22.6-1+deb9u1 Mime-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH 2/2] virtio: fix PCI config err handling X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 24 Aug 2018 17:14:40 -0000 On Fri, 2018-08-24 at 12:23 +0800, Tiwei Bie wrote: > On Thu, Aug 23, 2018 at 01:52:25PM +0100, Luca Boccassi wrote: > > On Tue, 2018-08-21 at 10:40 +0800, Tiwei Bie wrote: > > > On Mon, Aug 20, 2018 at 05:45:35PM +0100, Luca Boccassi wrote: > > > > On Mon, 2018-08-20 at 16:18 +0800, Tiwei Bie wrote: > > > > > On Thu, Aug 16, 2018 at 07:49:43PM +0100, Luca Boccassi > > > > > wrote: > > > > > > On Thu, 2018-08-16 at 19:47 +0100, Luca Boccassi wrote: > > > > > > > From: Brian Russell > > > > > > >=20 > > > > > > > In virtio_read_caps and vtpci_msix_detect, > > > > > > > rte_pci_read_config > > > > > > > returns > > > > > > > the number of bytes read from PCI config or < 0 on error. > > > > > > > If less than the expected number of bytes are read then > > > > > > > log > > > > > > > the > > > > > > > failure and return rather than carrying on with garbage. > > > > > > >=20 > > > > > > > Fixes: 6ba1f63b5ab0 ("virtio: support specification 1.0") > > > > > > >=20 > > > > > > > Signed-off-by: Brian Russell > > > > > > > Signed-off-by: Luca Boccassi > > > > > > > --- > > > > > > > v2: handle additional rte_pci_read_config incomplete > > > > > > > reads > > > > > > >=20 > > > > > > > =C2=A0drivers/net/virtio/virtio_pci.c | 35 > > > > > > > +++++++++++++++++++++ > > > > > > > ---- > > > > > > > ---- > > > > > > > ---- > > > > > > > =C2=A01 file changed, 22 insertions(+), 13 deletions(-) > > > > > > >=20 > > > > > > > diff --git a/drivers/net/virtio/virtio_pci.c > > > > > > > b/drivers/net/virtio/virtio_pci.c > > > > > > > index 6bd22e54a6..ff6f96f361 100644 > > > > > > > --- a/drivers/net/virtio/virtio_pci.c > > > > > > > +++ b/drivers/net/virtio/virtio_pci.c > > > > > >=20 > > > > > > ... > > > > > > > @@ -610,9 +613,13 @@ virtio_read_caps(struct > > > > > > > rte_pci_device > > > > > > > *dev, > > > > > > > struct virtio_hw *hw) > > > > > > > =C2=A0 hw->common_cfg =3D > > > > > > > get_cfg_addr(dev, > > > > > > > &cap); > > > > > > > =C2=A0 break; > > > > > > > =C2=A0 case VIRTIO_PCI_CAP_NOTIFY_CFG: > > > > > > > - rte_pci_read_config(dev, &hw- > > > > > > > > notify_off_multiplier, > > > > > > >=20 > > > > > > > - 4, pos + > > > > > > > sizeof(cap)); > > > > > > > - hw->notify_base =3D > > > > > > > get_cfg_addr(dev, > > > > > > > &cap); > > > > > > > + /* Avoid half-reads into hw */ > > > > > > > + ret =3D rte_pci_read_config(dev, > > > > > > > &multiplier, > > > > > > > 4, > > > > > > > + pos + > > > > > > > sizeof(cap)); > > > > > > > + if (ret =3D=3D 4) { > > > > > > > + hw- > > > > > > > >notify_off_multiplier =3D > > > > > > > multiplier; > > > > > > > + hw->notify_base =3D > > > > > > > get_cfg_addr(dev, > > > > > > > &cap); > > > > > > > + } > > > > > > > =C2=A0 break; > > > > > > > =C2=A0 case VIRTIO_PCI_CAP_DEVICE_CFG: > > > > > > > =C2=A0 hw->dev_cfg =3D get_cfg_addr(dev, > > > > > > > &cap); > > > > > >=20 > > > > > > Tiwei: not 100% sure what's the best way to handle a > > > > > > failure > > > > > > here, > > > > > > this > > > > > > will avoid writing to *hw in case of errors. Let me know if > > > > > > this is > > > > > > OK. > > > > >=20 > > > > > My concern is about reading the virtio capability directly. > > > > > With this patch, it will give up reading other capabilities > > > > > when failed to read a full virtio PCI capability structure > > > > > (the returned bytes are less than the expected bytes) even > > > > > though when the capability it's reading isn't a virtio vendor > > > > > specific capability. I'm not quite sure whether it will bring > > > > > any bad consequence. How about just reading the first two > > > > > bytes first? Something like this: > > > > >=20 > > > > > https://github.com/freebsd/freebsd/blob/72445a41b3ff/sys/dev/ > > > > > pci/ > > > > > pci. > > > > > c#L1491-L1497 > > > >=20 > > > > I'm not sure, to be honest. That bit didn't give me trouble at > > > > all, > > > > so > > > > at this point I'd much rather leave it alone so that the > > > > maintainers > > > > can take care of it how they see fit, if necessary :-) > > > >=20 > > > > I've sent a v3 that removes that individual change. > > >=20 > > > My concern isn't about the above change (which handled the > > > errors when reading multiplier). In fact, above change looks > > > good to me! :-)=C2=A0=C2=A0I mean the below changes in this patch: > > >=20 > > > =C2=A0 while (pos) { > > > =C2=A0 ret =3D rte_pci_read_config(dev, &cap, > > > sizeof(cap), > > > pos); > > > - if (ret < 0) { > > > - PMD_INIT_LOG(ERR, > > > - "failed to read pci cap at pos: > > > %x", > > > pos); > > > + if (ret !=3D sizeof(cap)) { > > > + PMD_INIT_LOG(DEBUG, > > > + =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0"failed to read pci cap at > > > pos: > > > %x ret %d", > > > + =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0pos, ret); > > > =C2=A0 break; > > > =C2=A0 } > > >=20 > > > With this change, it will give up reading other capabilities > > > when failed to read a full virtio PCI capability structure > > > (the returned bytes are less than the expected bytes) even > > > though when the capability it's reading isn't a virtio vendor > > > specific capability. Maybe it would be better to read the > > > first two bytes first and check whether it's the capability > > > we want to parse (i.e. vendor capability and MSIX capability). > > > Something like this: > > >=20 > > > https://github.com/freebsd/freebsd/blob/72445a41b3ff/sys/dev/pci/ > > > pci. > > > c#L1491-L1497 > > >=20 > > > How do you think? > > >=20 > > > Thanks > >=20 > > Ah I see what you mean now, sorry. Would something like the > > following > > be what you are looking for: > >=20 > > ret =3D rte_pci_read_config(dev, &cap, 2, pos); > > if (ret !=3D 2) { > > PMD_INIT_LOG(DEBUG, > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0"failed to read pci cap at pos: %x > > ret %d", > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0pos, ret); > > break; > > } > > if (cap.cap_vndr !=3D PCI_CAP_ID_MSIX && > > cap.cap_vndr !=3D PCI_CAP_ID_VNDR) { > > goto next; > > } > >=20 > > ret =3D rte_pci_read_config(dev, &cap, sizeof(cap), pos); > > if (ret !=3D sizeof(cap)) { > > PMD_INIT_LOG(DEBUG, > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0"failed to read pci cap at pos: %x > > ret %d", > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0pos, ret); > > break; > > } > >=20 >=20 > Yeah, that's what I want. :-) > Just one nit, we don't need to read it as a > virtio cap when dealing with the MSIX cap. > When dealing with the MSIX cap, we just need > to read the next two bytes: >=20 > https://github.com/DPDK/dpdk/blob/7281cf520f89/drivers/net/virtio/vir > tio_pci.c#L584-L594 > https://github.com/freebsd/freebsd/blob/59e4185a9358/sys/dev/pci/pci. > c#L892 >=20 > Thanks! Ok, done, see v4. --=20 Kind regards, Luca Boccassi