| Message ID | 20200304165845.3081-5-vgoyal@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtiofs: Add DAX support | expand |
On Wed, Mar 04, 2020 at 11:58:29AM -0500, Vivek Goyal wrote: > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c > index 7abcc50838b8..52f179411015 100644 > --- a/drivers/virtio/virtio_pci_modern.c > +++ b/drivers/virtio/virtio_pci_modern.c > @@ -443,6 +443,111 @@ 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); Please fix the mixed tabs vs space indentation in this patch. > +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); > + struct pci_dev *pci_dev = vp_dev->pci_dev; > + u8 bar; > + u64 offset, len; > + phys_addr_t phys_addr; > + size_t bar_len; > + int ret; > + > + if (!virtio_pci_find_shm_cap(pci_dev, id, &bar, &offset, &len)) { > + return false; > + } > + > + 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; > +} Missing pci_release_region()?
On Wed, Mar 04, 2020 at 11:58:29AM -0500, Vivek Goyal wrote: > From: Sebastien Boeuf <sebastien.boeuf@intel.com> > > On PCI the shm regions are found using capability entries; > find a region by searching for the capability. > > Signed-off-by: Sebastien Boeuf <sebastien.boeuf@intel.com> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > Signed-off-by: kbuild test robot <lkp@intel.com> > --- > drivers/virtio/virtio_pci_modern.c | 107 +++++++++++++++++++++++++++++ > include/uapi/linux/virtio_pci.h | 11 ++- > 2 files changed, 117 insertions(+), 1 deletion(-) > > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c > index 7abcc50838b8..52f179411015 100644 > --- a/drivers/virtio/virtio_pci_modern.c > +++ b/drivers/virtio/virtio_pci_modern.c > @@ -443,6 +443,111 @@ 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); > + pos > 0; > + pos = pci_find_next_capability(dev, pos, PCI_CAP_ID_VNDR)) { > + u8 type, cap_len, id; > + u32 tmp32; > + 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) > + 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); > + continue; > + } > + > + 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); > + struct pci_dev *pci_dev = vp_dev->pci_dev; > + u8 bar; > + u64 offset, len; > + phys_addr_t phys_addr; > + size_t bar_len; > + int ret; > + > + if (!virtio_pci_find_shm_cap(pci_dev, id, &bar, &offset, &len)) { > + return false; > + } > + > + 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; > + } > + Something wrong with indentation here. Also as long as you are validating things, it's worth checking offset + len does not overflow. > + 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, > @@ -457,6 +562,7 @@ static const struct virtio_config_ops virtio_pci_config_nodev_ops = { > .bus_name = vp_bus_name, > .set_vq_affinity = vp_set_vq_affinity, > .get_vq_affinity = vp_get_vq_affinity, > + .get_shm_region = vp_get_shm_region, > }; > > static const struct virtio_config_ops virtio_pci_config_ops = { > @@ -473,6 +579,7 @@ static const struct virtio_config_ops virtio_pci_config_ops = { > .bus_name = vp_bus_name, > .set_vq_affinity = vp_set_vq_affinity, > .get_vq_affinity = vp_get_vq_affinity, > + .get_shm_region = vp_get_shm_region, > }; > > /** > diff --git a/include/uapi/linux/virtio_pci.h b/include/uapi/linux/virtio_pci.h > index 90007a1abcab..fe9f43680a1d 100644 > --- a/include/uapi/linux/virtio_pci.h > +++ b/include/uapi/linux/virtio_pci.h > @@ -113,6 +113,8 @@ > #define VIRTIO_PCI_CAP_DEVICE_CFG 4 > /* PCI configuration access */ > #define VIRTIO_PCI_CAP_PCI_CFG 5 > +/* Additional shared memory capability */ > +#define VIRTIO_PCI_CAP_SHARED_MEMORY_CFG 8 > > /* This is the PCI capability header: */ > struct virtio_pci_cap { > @@ -121,11 +123,18 @@ struct virtio_pci_cap { > __u8 cap_len; /* Generic PCI field: capability length */ > __u8 cfg_type; /* Identifies the structure. */ > __u8 bar; /* Where to find it. */ > - __u8 padding[3]; /* Pad to full dword. */ > + __u8 id; /* Multiple capabilities of the same type */ > + __u8 padding[2]; /* Pad to full dword. */ > __le32 offset; /* Offset within bar. */ > __le32 length; /* Length of the structure, in bytes. */ > }; > > +struct virtio_pci_cap64 { > + struct virtio_pci_cap cap; > + __le32 offset_hi; /* Most sig 32 bits of offset */ > + __le32 length_hi; /* Most sig 32 bits of length */ > +}; > + > struct virtio_pci_notify_cap { > struct virtio_pci_cap cap; > __le32 notify_off_multiplier; /* Multiplier for queue_notify_off. */ > -- > 2.20.1
On Tue, Mar 10, 2020 at 11:04:37AM +0000, Stefan Hajnoczi wrote: > On Wed, Mar 04, 2020 at 11:58:29AM -0500, Vivek Goyal wrote: > > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c > > index 7abcc50838b8..52f179411015 100644 > > --- a/drivers/virtio/virtio_pci_modern.c > > +++ b/drivers/virtio/virtio_pci_modern.c > > @@ -443,6 +443,111 @@ 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); > > Please fix the mixed tabs vs space indentation in this patch. Will do. There are plenty of these in this patch. > > > +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); > > + struct pci_dev *pci_dev = vp_dev->pci_dev; > > + u8 bar; > > + u64 offset, len; > > + phys_addr_t phys_addr; > > + size_t bar_len; > > + int ret; > > + > > + if (!virtio_pci_find_shm_cap(pci_dev, id, &bar, &offset, &len)) { > > + return false; > > + } > > + > > + 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; > > +} > > Missing pci_release_region()? Good catch. We don't have a mechanism to call pci_relese_region() and virtio-mmio device's ->get_shm_region() implementation does not even seem to reserve the resources. So how about we leave this resource reservation to the caller. ->get_shm_region() just returns the addr/len pair of requested resource. Something like this patch. --- drivers/virtio/virtio_pci_modern.c | 8 -------- fs/fuse/virtio_fs.c | 13 ++++++++++--- 2 files changed, 10 insertions(+), 11 deletions(-) Index: redhat-linux/fs/fuse/virtio_fs.c =================================================================== --- redhat-linux.orig/fs/fuse/virtio_fs.c 2020-03-10 09:13:34.624565666 -0400 +++ redhat-linux/fs/fuse/virtio_fs.c 2020-03-10 14:11:10.970284651 -0400 @@ -763,11 +763,18 @@ static int virtio_fs_setup_dax(struct vi if (!have_cache) { dev_notice(&vdev->dev, "%s: No cache capability\n", __func__); return 0; - } else { - dev_notice(&vdev->dev, "Cache len: 0x%llx @ 0x%llx\n", - cache_reg.len, cache_reg.addr); } + if (!devm_request_mem_region(&vdev->dev, cache_reg.addr, cache_reg.len, + dev_name(&vdev->dev))) { + dev_warn(&vdev->dev, "could not reserve region addr=0x%llx" + " len=0x%llx\n", cache_reg.addr, cache_reg.len); + return -EBUSY; + } + + dev_notice(&vdev->dev, "Cache len: 0x%llx @ 0x%llx\n", cache_reg.len, + cache_reg.addr); + pgmap = devm_kzalloc(&vdev->dev, sizeof(*pgmap), GFP_KERNEL); if (!pgmap) return -ENOMEM; Index: redhat-linux/drivers/virtio/virtio_pci_modern.c =================================================================== --- redhat-linux.orig/drivers/virtio/virtio_pci_modern.c 2020-03-10 08:51:36.886565666 -0400 +++ redhat-linux/drivers/virtio/virtio_pci_modern.c 2020-03-10 13:43:15.168753543 -0400 @@ -511,19 +511,11 @@ static bool vp_get_shm_region(struct vir u64 offset, len; phys_addr_t phys_addr; size_t bar_len; - int ret; if (!virtio_pci_find_shm_cap(pci_dev, id, &bar, &offset, &len)) { return false; } - 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);
On Tue, Mar 10, 2020 at 07:12:25AM -0400, Michael S. Tsirkin wrote: [..] > > +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); > > + struct pci_dev *pci_dev = vp_dev->pci_dev; > > + u8 bar; > > + u64 offset, len; > > + phys_addr_t phys_addr; > > + size_t bar_len; > > + int ret; > > + > > + if (!virtio_pci_find_shm_cap(pci_dev, id, &bar, &offset, &len)) { > > + return false; > > + } > > + > > + 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; > > + } > > + > > Something wrong with indentation here. Will fix all indentation related issues in this patch. > Also as long as you are validating things, it's worth checking > offset + len does not overflow. Something like addition of following lines? + if ((offset + len) < offset) { + dev_err(&pci_dev->dev, "%s: cap offset+len overflow detected\n", + __func__); + return false; + } Vivek
On Tue, Mar 10, 2020 at 02:47:20PM -0400, Vivek Goyal wrote: > On Tue, Mar 10, 2020 at 07:12:25AM -0400, Michael S. Tsirkin wrote: > [..] > > > +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); > > > + struct pci_dev *pci_dev = vp_dev->pci_dev; > > > + u8 bar; > > > + u64 offset, len; > > > + phys_addr_t phys_addr; > > > + size_t bar_len; > > > + int ret; > > > + > > > + if (!virtio_pci_find_shm_cap(pci_dev, id, &bar, &offset, &len)) { > > > + return false; > > > + } > > > + > > > + 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; > > > + } > > > + > > > > Something wrong with indentation here. > > Will fix all indentation related issues in this patch. > > > Also as long as you are validating things, it's worth checking > > offset + len does not overflow. > > Something like addition of following lines? > > + if ((offset + len) < offset) { > + dev_err(&pci_dev->dev, "%s: cap offset+len overflow detected\n", > + __func__); > + return false; > + } > > Vivek That should do it.
On Tue, Mar 10, 2020 at 02:19:36PM -0400, Vivek Goyal wrote: > On Tue, Mar 10, 2020 at 11:04:37AM +0000, Stefan Hajnoczi wrote: > > On Wed, Mar 04, 2020 at 11:58:29AM -0500, Vivek Goyal wrote: > > > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c > > > index 7abcc50838b8..52f179411015 100644 > > > --- a/drivers/virtio/virtio_pci_modern.c > > > +++ b/drivers/virtio/virtio_pci_modern.c > > > @@ -443,6 +443,111 @@ 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); > > > > Please fix the mixed tabs vs space indentation in this patch. > > Will do. There are plenty of these in this patch. > > > > > > +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); > > > + struct pci_dev *pci_dev = vp_dev->pci_dev; > > > + u8 bar; > > > + u64 offset, len; > > > + phys_addr_t phys_addr; > > > + size_t bar_len; > > > + int ret; > > > + > > > + if (!virtio_pci_find_shm_cap(pci_dev, id, &bar, &offset, &len)) { > > > + return false; > > > + } > > > + > > > + 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; > > > +} > > > > Missing pci_release_region()? > > Good catch. We don't have a mechanism to call pci_relese_region() and > virtio-mmio device's ->get_shm_region() implementation does not even > seem to reserve the resources. > > So how about we leave this resource reservation to the caller. > ->get_shm_region() just returns the addr/len pair of requested resource. > > Something like this patch. > > --- > drivers/virtio/virtio_pci_modern.c | 8 -------- > fs/fuse/virtio_fs.c | 13 ++++++++++--- > 2 files changed, 10 insertions(+), 11 deletions(-) > > Index: redhat-linux/fs/fuse/virtio_fs.c > =================================================================== > --- redhat-linux.orig/fs/fuse/virtio_fs.c 2020-03-10 09:13:34.624565666 -0400 > +++ redhat-linux/fs/fuse/virtio_fs.c 2020-03-10 14:11:10.970284651 -0400 > @@ -763,11 +763,18 @@ static int virtio_fs_setup_dax(struct vi > if (!have_cache) { > dev_notice(&vdev->dev, "%s: No cache capability\n", __func__); > return 0; > - } else { > - dev_notice(&vdev->dev, "Cache len: 0x%llx @ 0x%llx\n", > - cache_reg.len, cache_reg.addr); > } > > + if (!devm_request_mem_region(&vdev->dev, cache_reg.addr, cache_reg.len, > + dev_name(&vdev->dev))) { > + dev_warn(&vdev->dev, "could not reserve region addr=0x%llx" > + " len=0x%llx\n", cache_reg.addr, cache_reg.len); > + return -EBUSY; > + } > + > + dev_notice(&vdev->dev, "Cache len: 0x%llx @ 0x%llx\n", cache_reg.len, > + cache_reg.addr); > + > pgmap = devm_kzalloc(&vdev->dev, sizeof(*pgmap), GFP_KERNEL); > if (!pgmap) > return -ENOMEM; > Index: redhat-linux/drivers/virtio/virtio_pci_modern.c > =================================================================== > --- redhat-linux.orig/drivers/virtio/virtio_pci_modern.c 2020-03-10 08:51:36.886565666 -0400 > +++ redhat-linux/drivers/virtio/virtio_pci_modern.c 2020-03-10 13:43:15.168753543 -0400 > @@ -511,19 +511,11 @@ static bool vp_get_shm_region(struct vir > u64 offset, len; > phys_addr_t phys_addr; > size_t bar_len; > - int ret; > > if (!virtio_pci_find_shm_cap(pci_dev, id, &bar, &offset, &len)) { > return false; > } > > - 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); Do pci_resource_start()/pci_resource_len() work on a BAR where pci_request_region() hasn't been called yet? (I haven't checked the code, sorry...) Assuming yes, then my next question is whether devm_request_mem_region() works in both the VIRTIO PCI and MMIO cases? If yes, then this looks like a solution, though the need for devm_request_mem_region() should be explained in the vp_get_shm_region() doc comments so that callers remember to make that call. Or maybe it can be included in vp_get_shm_region(). Stefan
On Wed, Mar 11, 2020 at 05:34:05PM +0000, Stefan Hajnoczi wrote: > On Tue, Mar 10, 2020 at 02:19:36PM -0400, Vivek Goyal wrote: > > On Tue, Mar 10, 2020 at 11:04:37AM +0000, Stefan Hajnoczi wrote: > > > On Wed, Mar 04, 2020 at 11:58:29AM -0500, Vivek Goyal wrote: > > > > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c > > > > index 7abcc50838b8..52f179411015 100644 > > > > --- a/drivers/virtio/virtio_pci_modern.c > > > > +++ b/drivers/virtio/virtio_pci_modern.c > > > > @@ -443,6 +443,111 @@ 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); > > > > > > Please fix the mixed tabs vs space indentation in this patch. > > > > Will do. There are plenty of these in this patch. > > > > > > > > > +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); > > > > + struct pci_dev *pci_dev = vp_dev->pci_dev; > > > > + u8 bar; > > > > + u64 offset, len; > > > > + phys_addr_t phys_addr; > > > > + size_t bar_len; > > > > + int ret; > > > > + > > > > + if (!virtio_pci_find_shm_cap(pci_dev, id, &bar, &offset, &len)) { > > > > + return false; > > > > + } > > > > + > > > > + 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; > > > > +} > > > > > > Missing pci_release_region()? > > > > Good catch. We don't have a mechanism to call pci_relese_region() and > > virtio-mmio device's ->get_shm_region() implementation does not even > > seem to reserve the resources. > > > > So how about we leave this resource reservation to the caller. > > ->get_shm_region() just returns the addr/len pair of requested resource. > > > > Something like this patch. > > > > --- > > drivers/virtio/virtio_pci_modern.c | 8 -------- > > fs/fuse/virtio_fs.c | 13 ++++++++++--- > > 2 files changed, 10 insertions(+), 11 deletions(-) > > > > Index: redhat-linux/fs/fuse/virtio_fs.c > > =================================================================== > > --- redhat-linux.orig/fs/fuse/virtio_fs.c 2020-03-10 09:13:34.624565666 -0400 > > +++ redhat-linux/fs/fuse/virtio_fs.c 2020-03-10 14:11:10.970284651 -0400 > > @@ -763,11 +763,18 @@ static int virtio_fs_setup_dax(struct vi > > if (!have_cache) { > > dev_notice(&vdev->dev, "%s: No cache capability\n", __func__); > > return 0; > > - } else { > > - dev_notice(&vdev->dev, "Cache len: 0x%llx @ 0x%llx\n", > > - cache_reg.len, cache_reg.addr); > > } > > > > + if (!devm_request_mem_region(&vdev->dev, cache_reg.addr, cache_reg.len, > > + dev_name(&vdev->dev))) { > > + dev_warn(&vdev->dev, "could not reserve region addr=0x%llx" > > + " len=0x%llx\n", cache_reg.addr, cache_reg.len); > > + return -EBUSY; > > + } > > + > > + dev_notice(&vdev->dev, "Cache len: 0x%llx @ 0x%llx\n", cache_reg.len, > > + cache_reg.addr); > > + > > pgmap = devm_kzalloc(&vdev->dev, sizeof(*pgmap), GFP_KERNEL); > > if (!pgmap) > > return -ENOMEM; > > Index: redhat-linux/drivers/virtio/virtio_pci_modern.c > > =================================================================== > > --- redhat-linux.orig/drivers/virtio/virtio_pci_modern.c 2020-03-10 08:51:36.886565666 -0400 > > +++ redhat-linux/drivers/virtio/virtio_pci_modern.c 2020-03-10 13:43:15.168753543 -0400 > > @@ -511,19 +511,11 @@ static bool vp_get_shm_region(struct vir > > u64 offset, len; > > phys_addr_t phys_addr; > > size_t bar_len; > > - int ret; > > > > if (!virtio_pci_find_shm_cap(pci_dev, id, &bar, &offset, &len)) { > > return false; > > } > > > > - 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); > > Do pci_resource_start()/pci_resource_len() work on a BAR where > pci_request_region() hasn't been called yet? (I haven't checked the > code, sorry...) It should. Infact, pci_request_region() itself is calling pci_resource_start() and pci_resource_len(). > > Assuming yes, then my next question is whether devm_request_mem_region() > works in both the VIRTIO PCI and MMIO cases? It should work on MMIO case as well. This basically works on /proc/iomem resource tree to reserve resources. So as long as MMIO memory range has been registered by driver in /proc/iomem, it will work. > > If yes, then this looks like a solution, though the need for > devm_request_mem_region() should be explained in the vp_get_shm_region() > doc comments so that callers remember to make that call. Or maybe it > can be included in vp_get_shm_region(). How about adding a line in include/linux/virtio_config.h right below the @get_shm_region descrition which says. "This does not reserve the resources and caller is expected to call devm_request_mem_region() or similar to reserve resources." Vivek
diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c index 7abcc50838b8..52f179411015 100644 --- a/drivers/virtio/virtio_pci_modern.c +++ b/drivers/virtio/virtio_pci_modern.c @@ -443,6 +443,111 @@ 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); + pos > 0; + pos = pci_find_next_capability(dev, pos, PCI_CAP_ID_VNDR)) { + u8 type, cap_len, id; + u32 tmp32; + 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) + 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); + continue; + } + + 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); + struct pci_dev *pci_dev = vp_dev->pci_dev; + u8 bar; + u64 offset, len; + phys_addr_t phys_addr; + size_t bar_len; + int ret; + + if (!virtio_pci_find_shm_cap(pci_dev, id, &bar, &offset, &len)) { + return false; + } + + 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, @@ -457,6 +562,7 @@ static const struct virtio_config_ops virtio_pci_config_nodev_ops = { .bus_name = vp_bus_name, .set_vq_affinity = vp_set_vq_affinity, .get_vq_affinity = vp_get_vq_affinity, + .get_shm_region = vp_get_shm_region, }; static const struct virtio_config_ops virtio_pci_config_ops = { @@ -473,6 +579,7 @@ static const struct virtio_config_ops virtio_pci_config_ops = { .bus_name = vp_bus_name, .set_vq_affinity = vp_set_vq_affinity, .get_vq_affinity = vp_get_vq_affinity, + .get_shm_region = vp_get_shm_region, }; /** diff --git a/include/uapi/linux/virtio_pci.h b/include/uapi/linux/virtio_pci.h index 90007a1abcab..fe9f43680a1d 100644 --- a/include/uapi/linux/virtio_pci.h +++ b/include/uapi/linux/virtio_pci.h @@ -113,6 +113,8 @@ #define VIRTIO_PCI_CAP_DEVICE_CFG 4 /* PCI configuration access */ #define VIRTIO_PCI_CAP_PCI_CFG 5 +/* Additional shared memory capability */ +#define VIRTIO_PCI_CAP_SHARED_MEMORY_CFG 8 /* This is the PCI capability header: */ struct virtio_pci_cap { @@ -121,11 +123,18 @@ struct virtio_pci_cap { __u8 cap_len; /* Generic PCI field: capability length */ __u8 cfg_type; /* Identifies the structure. */ __u8 bar; /* Where to find it. */ - __u8 padding[3]; /* Pad to full dword. */ + __u8 id; /* Multiple capabilities of the same type */ + __u8 padding[2]; /* Pad to full dword. */ __le32 offset; /* Offset within bar. */ __le32 length; /* Length of the structure, in bytes. */ }; +struct virtio_pci_cap64 { + struct virtio_pci_cap cap; + __le32 offset_hi; /* Most sig 32 bits of offset */ + __le32 length_hi; /* Most sig 32 bits of length */ +}; + struct virtio_pci_notify_cap { struct virtio_pci_cap cap; __le32 notify_off_multiplier; /* Multiplier for queue_notify_off. */