From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f68.google.com (mail-wm0-f68.google.com [74.125.82.68]) by dpdk.org (Postfix) with ESMTP id F05A81E2B for ; Mon, 20 Aug 2018 18:45:36 +0200 (CEST) Received: by mail-wm0-f68.google.com with SMTP id i134-v6so7523239wmf.0 for ; Mon, 20 Aug 2018 09:45:36 -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=A9MGUOOlelVM+1vh7nH3IFM8UUxLlgFfjq+MeAm8gj8=; b=bvDoyyxH2ch/zJ5+VM91n6AJQSW59+dmlUOxgnGqcJBAnbnq8SON4XpZqkEQ4uMh6Y 0aGCqB0DR5qFAy7hLH+UNxspJSrY+NYGHS7S8c392QWsnf9osAbOSCRRo+b9L9KZZjkV FnzA+DBNf1yTnC1Q6bwJFS+E0mV+VzIt3TklbMi43OM923GtuuXcfo2Dg5XoZH/wHy6j zkejwOfwWKT6zZPIqqXXSMJ25RGudhyVJusq7LUTto1s2/Dz+ccs4X6J1VdWbi0uBLeW ukagPwXnWUpDSxUeMrKes5BPlKkQKfwQhQnm5xe6jxe2sFctqvV09SpA0r8VOrWbYZWU a8jQ== X-Gm-Message-State: AOUpUlE4RFuADL098EUO6mcbWsk3Ylq1b0owlyr9HIzfSzACxQD3/0KH aFciEVHIY+MNW6+k7doEyRQ= X-Google-Smtp-Source: AA+uWPwaO3s/mLZIhii71Q4w+Js5IXGOwTy1VDfso6M9QUIwdjvsYCBV2FUUCZ9yKJ6qo3y6c/z9LA== X-Received: by 2002:a7b:c0d5:: with SMTP id s21-v6mr8100494wmh.106.1534783536681; Mon, 20 Aug 2018 09:45:36 -0700 (PDT) Received: from localhost ([2a01:4b00:f419:6f00:8361:8946:ba2b:d556]) by smtp.gmail.com with ESMTPSA id l24-v6sm10373530wrb.65.2018.08.20.09.45.35 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 20 Aug 2018 09:45:35 -0700 (PDT) Message-ID: <1534783535.5764.93.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: Mon, 20 Aug 2018 17:45:35 +0100 In-Reply-To: <20180820081804.GA29806@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> 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: Mon, 20 Aug 2018 16:45:37 -0000 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 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 :-) I've sent a v3 that removes that individual change. --=20 Kind regards, Luca Boccassi