From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 4EA8E20212C8D for ; Tue, 27 Aug 2019 04:55:33 -0700 (PDT) Date: Tue, 27 Aug 2019 07:53:17 -0400 From: Vivek Goyal Subject: Re: [PATCH 04/19] virtio: Implement get_shm_region for PCI transport Message-ID: <20190827115317.GA30873@redhat.com> References: <20190821175720.25901-1-vgoyal@redhat.com> <20190821175720.25901-5-vgoyal@redhat.com> <20190827103457.35927d9d.cohuck@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20190827103457.35927d9d.cohuck@redhat.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: linux-nvdimm-bounces@lists.01.org Sender: "Linux-nvdimm" To: Cornelia Huck Cc: kbuild test robot , kvm@vger.kernel.org, miklos@szeredi.hu, linux-nvdimm@lists.01.org, linux-kernel@vger.kernel.org, dgilbert@redhat.com, virtio-fs@redhat.com, Sebastien Boeuf , stefanha@redhat.com, linux-fsdevel@vger.kernel.org List-ID: On Tue, Aug 27, 2019 at 10:34:57AM +0200, Cornelia Huck wrote: > On Wed, 21 Aug 2019 13:57:05 -0400 > Vivek Goyal wrote: > > > From: Sebastien Boeuf > > > > On PCI the shm regions are found using capability entries; > > find a region by searching for the capability. > > > > Cc: kvm@vger.kernel.org > > Signed-off-by: Sebastien Boeuf > > Signed-off-by: Dr. David Alan Gilbert > > Signed-off-by: kbuild test robot > > An s-o-b by a test robot looks a bit odd. I think one of the fixes came from the robot and that's why I put s-o-b from the robot as well. I will review the whole patch and fix all the intendation issues. Vivek > > > --- > > drivers/virtio/virtio_pci_modern.c | 108 +++++++++++++++++++++++++++++ > > include/uapi/linux/virtio_pci.h | 11 ++- > > 2 files changed, 118 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c > > index 7abcc50838b8..1cdedd93f42a 100644 > > --- a/drivers/virtio/virtio_pci_modern.c > > +++ b/drivers/virtio/virtio_pci_modern.c > > @@ -443,6 +443,112 @@ static void del_vq(struct virtio_pci_vq_info *info) > > vring_del_virtqueue(vq); > > } > > > > +static int virtio_pci_find_shm_cap(struct pci_dev *dev, > > + u8 required_id, > > + u8 *bar, u64 *offset, u64 *len) > > +{ > > + int pos; > > + > > + for (pos = pci_find_capability(dev, PCI_CAP_ID_VNDR); > > Indentation looks a bit off here. > > > + pos > 0; > > + pos = pci_find_next_capability(dev, pos, PCI_CAP_ID_VNDR)) { > > + u8 type, cap_len, id; > > + u32 tmp32; > > Here as well. > > > + u64 res_offset, res_length; > > + > > + pci_read_config_byte(dev, pos + offsetof(struct virtio_pci_cap, > > + cfg_type), > > + &type); > > + if (type != VIRTIO_PCI_CAP_SHARED_MEMORY_CFG) > > And here. > > > + continue; > > + > > + pci_read_config_byte(dev, pos + offsetof(struct virtio_pci_cap, > > + cap_len), > > + &cap_len); > > + if (cap_len != sizeof(struct virtio_pci_cap64)) { > > + printk(KERN_ERR "%s: shm cap with bad size offset: %d size: %d\n", > > + __func__, pos, cap_len); > > Probably better to use dev_warn() instead of printk. > > > + continue; > > + } > > Indentation looks off again (might be a space vs tabs issue; maybe > check the whole patch for indentation problems?) > > > + > > + pci_read_config_byte(dev, pos + offsetof(struct virtio_pci_cap, > > + id), > > + &id); > > + if (id != required_id) > > + continue; > > + > > + /* Type, and ID match, looks good */ > > + pci_read_config_byte(dev, pos + offsetof(struct virtio_pci_cap, > > + bar), > > + bar); > > + > > + /* Read the lower 32bit of length and offset */ > > + pci_read_config_dword(dev, pos + offsetof(struct virtio_pci_cap, offset), > > + &tmp32); > > + res_offset = tmp32; > > + pci_read_config_dword(dev, pos + offsetof(struct virtio_pci_cap, length), > > + &tmp32); > > + res_length = tmp32; > > + > > + /* and now the top half */ > > + pci_read_config_dword(dev, > > + pos + offsetof(struct virtio_pci_cap64, > > + offset_hi), > > + &tmp32); > > + res_offset |= ((u64)tmp32) << 32; > > + pci_read_config_dword(dev, > > + pos + offsetof(struct virtio_pci_cap64, > > + length_hi), > > + &tmp32); > > + res_length |= ((u64)tmp32) << 32; > > + > > + *offset = res_offset; > > + *len = res_length; > > + > > + return pos; > > + } > > + return 0; > > +} > > + > > +static bool vp_get_shm_region(struct virtio_device *vdev, > > + struct virtio_shm_region *region, u8 id) > > +{ > > + struct virtio_pci_device *vp_dev = to_vp_device(vdev); > > This whole function looks like it is indented incorrectly. > > > + struct pci_dev *pci_dev = vp_dev->pci_dev; > > + u8 bar; > > + u64 offset, len; > > + phys_addr_t phys_addr; > > + size_t bar_len; > > + char *bar_name; > > + int ret; > > + > > + if (!virtio_pci_find_shm_cap(pci_dev, id, &bar, &offset, &len)) { > > + return false; > > + } > > You can drop the curly braces. > > > + > > + ret = pci_request_region(pci_dev, bar, "virtio-pci-shm"); > > + if (ret < 0) { > > + dev_err(&pci_dev->dev, "%s: failed to request BAR\n", > > + __func__); > > + return false; > > + } > > + > > + phys_addr = pci_resource_start(pci_dev, bar); > > + bar_len = pci_resource_len(pci_dev, bar); > > + > > + if (offset + len > bar_len) { > > + dev_err(&pci_dev->dev, > > + "%s: bar shorter than cap offset+len\n", > > + __func__); > > + return false; > > + } > > + > > + region->len = len; > > + region->addr = (u64) phys_addr + offset; > > + > > + return true; > > +} > > + > > static const struct virtio_config_ops virtio_pci_config_nodev_ops = { > > .get = NULL, > > .set = NULL, > > Apart from the coding style nits, the logic of the patch looks sane to > me. > > (...) > > As does the rest of the patch. _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm