virtio-fs: shared file system for virtual machines

From: Stefan Hajnoczi <[email protected]>

fs/fuse/virtio_fs.c +21 -0

+static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
+{
+	struct virtio_fs_memremap_info *mi;
+	struct dev_pagemap *pgmap;
+	struct pci_dev *pci_dev;
+	phys_addr_t phys_addr;
+	size_t len;
+	int ret;
+
+	if (!IS_ENABLED(CONFIG_DAX_DRIVER))
+		return 0;
+
+	/* HACK implement VIRTIO shared memory regions instead of
+	 * directly accessing the PCI BAR from a virtio device driver.
+	 */
+	pci_dev = container_of(vdev->dev.parent, struct pci_dev, dev);
+
+	/* TODO Is this safe - the virtio_pci_* driver doesn't use managed
+	 * device APIs? */
+	ret = pcim_enable_device(pci_dev);
+	if (ret < 0)

Quoted 29 lines · Vivek Goyal

From: Stefan Hajnoczi <[email protected]>

fs/fuse/virtio_fs.c +27 -0

+static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
+{
+	struct virtio_fs_memremap_info *mi;
+	struct dev_pagemap *pgmap;
+	struct pci_dev *pci_dev;
+	phys_addr_t phys_addr;
+	size_t len;
+	int ret;
+
+	if (!IS_ENABLED(CONFIG_DAX_DRIVER))
+		return 0;
+
+	/* HACK implement VIRTIO shared memory regions instead of
+	 * directly accessing the PCI BAR from a virtio device driver.
+	 */
+	pci_dev = container_of(vdev->dev.parent, struct pci_dev, dev);
+
+	/* TODO Is this safe - the virtio_pci_* driver doesn't use managed
+	 * device APIs? */
+	ret = pcim_enable_device(pci_dev);
+	if (ret < 0)
+		return ret;
+
+	/* TODO handle case where device doesn't expose BAR? */
+	ret = pci_request_region(pci_dev, VIRTIO_FS_WINDOW_BAR,
+				 "virtio-fs-window");
+	if (ret < 0) {

fs/fuse/virtio_fs.c static const struct dax_operations virtio_fs_dax_ops = { · line 476

Quoted 4 lines

fs/fuse/virtio_fs.c +4 -0

+		dev_err(&vdev->dev, "%s: failed to request window BAR\n",
+			__func__);
+		return ret;
+	}

Can we please have a generic virtio interface to map the address (the default can then fall back to PCI) instead of mapping a PCI bar? This would make it easier to implement virtio-ccw or virtio-mmio.

From: Stefan Hajnoczi <[email protected]>

Experimental QEMU code introduces an MMIO BAR for mapping portions of files in the virtio-fs device. Map this BAR so that FUSE DAX can access file contents from the host page cache.

The DAX window is accessed by the fs/dax.c infrastructure and must have struct pages (at least on x86). Use devm_memremap_pages() to map the DAX window PCI BAR and allocate struct page.

Quoted 6 lines · Vivek Goyal

From: Stefan Hajnoczi <[email protected]>

Experimental QEMU code introduces an MMIO BAR for mapping portions of files in the virtio-fs device. Map this BAR so that FUSE DAX can access file contents from the host page cache.

FUSE DAX sounds terrifying, can you explain a bit more about what this is?

Quoted 3 lines

The DAX window is accessed by the fs/dax.c infrastructure and must have struct pages (at least on x86). Use devm_memremap_pages() to map the DAX window PCI BAR and allocate struct page.

PCI BAR space is not cache coherent,

what prevents these pages from
being used in paths that would do:

   object = page_address(pfn_to_page(virtio_fs_pfn));

...?

Quoted 9 lines · * Dan Williams ([email protected]) wrote:

Quoted 6 lines · Vivek Goyal

From: Stefan Hajnoczi <[email protected]>

Experimental QEMU code introduces an MMIO BAR for mapping portions of files in the virtio-fs device. Map this BAR so that FUSE DAX can access file contents from the host page cache.

FUSE DAX sounds terrifying, can you explain a bit more about what this is?

We've got a guest running in QEMU, it sees an emulated PCI device; that runs a FUSE protocol over virtio on that PCI device, but also has a trick where via commands sent over the virtio queue associated with that device, (fragments of) host files get mmap'd into the qemu virtual memory that corresponds to the kvm slot exposed to the guest for that bar.

The guest sees those chunks in that BAR, and thus you can read/write to the host file by directly writing into that BAR.

Quoted 5 lines

Quoted 3 lines

The DAX window is accessed by the fs/dax.c infrastructure and must have struct pages (at least on x86). Use devm_memremap_pages() to map the DAX window PCI BAR and allocate struct page.

PCI BAR space is not cache coherent,

Note that no real PCI infrastructure is involved - this is all emulated devices, backed by mmap'd files on the host qemu process.

Quoted 18 lines · Dr. David Alan Gilbert

Quoted 9 lines · * Dan Williams ([email protected]) wrote:

Quoted 6 lines · Vivek Goyal

From: Stefan Hajnoczi <[email protected]>

Experimental QEMU code introduces an MMIO BAR for mapping portions of files in the virtio-fs device. Map this BAR so that FUSE DAX can access file contents from the host page cache.

FUSE DAX sounds terrifying, can you explain a bit more about what this is?

We've got a guest running in QEMU, it sees an emulated PCI device; that runs a FUSE protocol over virtio on that PCI device, but also has a trick where via commands sent over the virtio queue associated with that device, (fragments of) host files get mmap'd into the qemu virtual memory that corresponds to the kvm slot exposed to the guest for that bar.

Quoted 2 lines

The guest sees those chunks in that BAR, and thus you can read/write to the host file by directly writing into that BAR.

Ok so it's all software emulated and there won't be hardware DMA initiated by the guest to that address?

I.e. if the host file gets
truncated / hole-punched the guest would just cause a refault and the filesystem could fill in the block,

or the guest is expected to die if
the fault to the truncated file range results in SIGBUS.

Quoted 9 lines

Quoted 6 lines

Quoted 4 lines

The DAX window is accessed by the fs/dax.c infrastructure and must have struct pages (at least on x86). Use devm_memremap_pages() to map the DAX window PCI BAR and allocate struct page.

PCI BAR space is not cache coherent,

Note that no real PCI infrastructure is involved - this is all emulated devices, backed by mmap'd files on the host qemu process.

Ok, terror level decreased.

Quoted 20 lines · Dan Williams

Quoted 18 lines · Dr. David Alan Gilbert

Quoted 9 lines · * Dan Williams ([email protected]) wrote:

Quoted 6 lines · Vivek Goyal

From: Stefan Hajnoczi <[email protected]>

Experimental QEMU code introduces an MMIO BAR for mapping portions of files in the virtio-fs device. Map this BAR so that FUSE DAX can access file contents from the host page cache.

FUSE DAX sounds terrifying, can you explain a bit more about what this is?

We've got a guest running in QEMU, it sees an emulated PCI device; that runs a FUSE protocol over virtio on that PCI device, but also has a trick where via commands sent over the virtio queue associated with that device, (fragments of) host files get mmap'd into the qemu virtual memory that corresponds to the kvm slot exposed to the guest for that bar.

Quoted 5 lines

Quoted 2 lines

The guest sees those chunks in that BAR, and thus you can read/write to the host file by directly writing into that BAR.

Ok so it's all software emulated and there won't be hardware DMA initiated by the guest to that address?

That's my understanding.

Quoted 3 lines

I.e. if the host file gets
truncated / hole-punched the guest would just cause a refault and the filesystem could fill in the block,

Right

Quoted 2 lines

or the guest is expected to die if
the fault to the truncated file range results in SIGBUS.

Are you referring to the case where a file page is mapped in qemu and another guest/process trucates that page and when qemu tries to access it it will get SIGBUS. Have not tried it, will give it a try. Not sure what happens when QEMU receives SIGBUS.

Having said that, this is not different from the case of one process mapping a file and another process truncating the file and first process getting SIGBUS, right?

Quoted 38 lines · Vivek Goyal

Quoted 20 lines · Dan Williams

Quoted 18 lines · Dr. David Alan Gilbert

Quoted 9 lines · * Dan Williams ([email protected]) wrote:

Quoted 6 lines · Vivek Goyal

From: Stefan Hajnoczi <[email protected]>

Experimental QEMU code introduces an MMIO BAR for mapping portions of files in the virtio-fs device. Map this BAR so that FUSE DAX can access file contents from the host page cache.

FUSE DAX sounds terrifying, can you explain a bit more about what this is?

We've got a guest running in QEMU, it sees an emulated PCI device; that runs a FUSE protocol over virtio on that PCI device, but also has a trick where via commands sent over the virtio queue associated with that device, (fragments of) host files get mmap'd into the qemu virtual memory that corresponds to the kvm slot exposed to the guest for that bar.

Quoted 5 lines

Quoted 2 lines

The guest sees those chunks in that BAR, and thus you can read/write to the host file by directly writing into that BAR.

Ok so it's all software emulated and there won't be hardware DMA initiated by the guest to that address?

That's my understanding.

Quoted 3 lines

I.e. if the host file gets
truncated / hole-punched the guest would just cause a refault and the filesystem could fill in the block,

Right

Quoted 2 lines

or the guest is expected to die if
the fault to the truncated file range results in SIGBUS.

Quoted 8 lines

Are you referring to the case where a file page is mapped in qemu and another guest/process trucates that page and when qemu tries to access it it will get SIGBUS. Have not tried it, will give it a try. Not sure what happens when QEMU receives SIGBUS.

Having said that, this is not different from the case of one process mapping a file and another process truncating the file and first process getting SIGBUS, right?

Ok, tried this and guest process hangs.

Stefan, dgilbert, this reminds me that we have faced this issue during our testing and we decided that this will need some fixing in KVM. I even put this in as part of changelog of patch with subject "fuse: Take inode lock for dax inode truncation"

"Another problem is, if we setup a mapping in fuse_iomap_begin(), and
file gets truncated and dax read/write happens, KVM currently hangs. It tries to fault in a page which does not exist on host (file got truncated). It probably requries fixing in KVM."

Not sure what should happen though when qemu receives SIGBUS in this case.

Thanks
Vivek

Instead of assuming we had the fixed bar for the cache, use the value from the capabilities.

Signed-off-by Dr. David Alan Gilbert <[email protected]>

---
 fs/fuse/virtio_fs.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

fs/fuse/virtio_fs.c +1 -6

index 60d496c16841..55bac1465536 100644
@@ -14,11 +14,6 @@
 #include <uapi/linux/virtio_pci.h>
 #include "fuse_i.h"
 
-enum {
-	/* PCI BAR number of the virtio-fs DAX window */
-	VIRTIO_FS_WINDOW_BAR = 2,
-};
-
 /* List of virtio-fs device instances and a lock for the list */
 static DEFINE_MUTEX(virtio_fs_mutex);
 static LIST_HEAD(virtio_fs_instances);

@@ -518,7 +513,7 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
 	struct dev_pagemap *pgmap;
 	struct pci_dev *pci_dev;
 	phys_addr_t phys_addr;
-	size_t len;
+	size_t bar_len;
 	int ret;
 	u8 have_cache, cache_bar;
 	u64 cache_offset, cache_len;

fs/fuse/virtio_fs.c +16 -9

-	ret = pci_request_region(pci_dev, VIRTIO_FS_WINDOW_BAR,
-				 "virtio-fs-window");
+	ret = pci_request_region(pci_dev, cache_bar, "virtio-fs-window");
 	if (ret < 0) {
 		dev_err(&vdev->dev, "%s: failed to request window BAR\n",
 			__func__);
 		return ret;
 	}
 
-	phys_addr = pci_resource_start(pci_dev, VIRTIO_FS_WINDOW_BAR);
-	len = pci_resource_len(pci_dev, VIRTIO_FS_WINDOW_BAR);
-
 	mi = devm_kzalloc(&pci_dev->dev, sizeof(*mi), GFP_KERNEL);
 	if (!mi)
 		return -ENOMEM;

@@ -586,6 +577,17 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
 	pgmap->ref = &mi->ref;
 	pgmap->type = MEMORY_DEVICE_FS_DAX;
 
+	phys_addr = pci_resource_start(pci_dev, cache_bar);
+	bar_len = pci_resource_len(pci_dev, cache_bar);
+
+	if (cache_offset + cache_len > bar_len) {
+		dev_err(&vdev->dev,
+			"%s: cache bar shorter than cap offset+len\n",
+			__func__);
+		return -EINVAL;
+	}
+	phys_addr += cache_offset;
+
 	/* Ideally we would directly use the PCI BAR resource but
 	 * devm_memremap_pages() wants its own copy in pgmap.  So
 	 * initialize a struct resource from scratch (only the start

@@ -594,7 +596,7 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
 	pgmap->res = (struct resource){
 		.name = "virtio-fs dax window",
 		.start = phys_addr,
-		.end = phys_addr + len,
+		.end = phys_addr + cache_len,
 	};
 
 	fs->window_kaddr = devm_memremap_pages(&pci_dev->dev, pgmap);

@@ -607,10 +609,10 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
 		return ret;
 
 	fs->window_phys_addr = phys_addr;
-	fs->window_len = len;
+	fs->window_len = cache_len;
 
-	dev_dbg(&vdev->dev, "%s: window kaddr 0x%px phys_addr 0x%llx len %zu\n",
-		__func__, fs->window_kaddr, phys_addr, len);
+	dev_dbg(&vdev->dev, "%s: cache kaddr 0x%px phys_addr 0x%llx len %llx\n",
+		__func__, fs->window_kaddr, phys_addr, cache_len);
 
 	fs->dax_dev = alloc_dax(fs, NULL, &virtio_fs_dax_ops);
 	if (!fs->dax_dev)

Quoted 36 lines · Vivek Goyal

Instead of assuming we had the fixed bar for the cache, use the value from the capabilities.

Signed-off-by Dr. David Alan Gilbert <[email protected]>

---
 fs/fuse/virtio_fs.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

fs/fuse/virtio_fs.c +1 -6

index 60d496c16841..55bac1465536 100644
@@ -14,11 +14,6 @@
 #include <uapi/linux/virtio_pci.h>
 #include "fuse_i.h"
 
-enum {
-	/* PCI BAR number of the virtio-fs DAX window */
-	VIRTIO_FS_WINDOW_BAR = 2,
-};
-
 /* List of virtio-fs device instances and a lock for the list */
 static DEFINE_MUTEX(virtio_fs_mutex);
 static LIST_HEAD(virtio_fs_instances);

@@ -518,7 +513,7 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
 	struct dev_pagemap *pgmap;
 	struct pci_dev *pci_dev;
 	phys_addr_t phys_addr;
-	size_t len;
+	size_t bar_len;
 	int ret;
 	u8 have_cache, cache_bar;
 	u64 cache_offset, cache_len;

@@ -551,17 +546,13 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
         }
 

fs/fuse/virtio_fs.c static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs) · line 548

Quoted 1 line

fs/fuse/virtio_fs.c +0 -0

 	/* TODO handle case where device doesn't expose BAR? */

For virtio-pmem we decided to not go via BARs as this would effectively make it only usable for virtio-pci implementers. Instead, we are going to export the applicable physical device region directly (e.g. phys_start, phys_size in virtio config), so it is decoupled from PCI details. Doing the same for virtio-fs would allow e.g. also virtio-ccw to make eventually use of this.

Quoted 49 lines

fs/fuse/virtio_fs.c +14 -7

-	ret = pci_request_region(pci_dev, VIRTIO_FS_WINDOW_BAR,
-				 "virtio-fs-window");
+	ret = pci_request_region(pci_dev, cache_bar, "virtio-fs-window");
 	if (ret < 0) {
 		dev_err(&vdev->dev, "%s: failed to request window BAR\n",
 			__func__);
 		return ret;
 	}
 
-	phys_addr = pci_resource_start(pci_dev, VIRTIO_FS_WINDOW_BAR);
-	len = pci_resource_len(pci_dev, VIRTIO_FS_WINDOW_BAR);
-
 	mi = devm_kzalloc(&pci_dev->dev, sizeof(*mi), GFP_KERNEL);
 	if (!mi)
 		return -ENOMEM;

@@ -586,6 +577,17 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
 	pgmap->ref = &mi->ref;
 	pgmap->type = MEMORY_DEVICE_FS_DAX;
 
+	phys_addr = pci_resource_start(pci_dev, cache_bar);
+	bar_len = pci_resource_len(pci_dev, cache_bar);
+
+	if (cache_offset + cache_len > bar_len) {
+		dev_err(&vdev->dev,
+			"%s: cache bar shorter than cap offset+len\n",
+			__func__);
+		return -EINVAL;
+	}
+	phys_addr += cache_offset;
+
 	/* Ideally we would directly use the PCI BAR resource but
 	 * devm_memremap_pages() wants its own copy in pgmap.  So
 	 * initialize a struct resource from scratch (only the start

@@ -594,7 +596,7 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
 	pgmap->res = (struct resource){
 		.name = "virtio-fs dax window",
 		.start = phys_addr,
-		.end = phys_addr + len,
+		.end = phys_addr + cache_len,
 	};
 
 	fs->window_kaddr = devm_memremap_pages(&pci_dev->dev, pgmap);

@@ -607,10 +609,10 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
 		return ret;
 
 	fs->window_phys_addr = phys_addr;
-	fs->window_len = len;
+	fs->window_len = cache_len;
 

Quoted 8 lines

fs/fuse/virtio_fs.c +2 -2

-	dev_dbg(&vdev->dev, "%s: window kaddr 0x%px phys_addr 0x%llx len %zu\n",
-		__func__, fs->window_kaddr, phys_addr, len);
+	dev_dbg(&vdev->dev, "%s: cache kaddr 0x%px phys_addr 0x%llx len %llx\n",
+		__func__, fs->window_kaddr, phys_addr, cache_len);
 
 	fs->dax_dev = alloc_dax(fs, NULL, &virtio_fs_dax_ops);
 	if (!fs->dax_dev)

Signature · 3 lines

Thanks,

David / dhildenb

Quoted 37 lines · * David Hildenbrand ([email protected]) wrote:

Quoted 36 lines · Vivek Goyal

Instead of assuming we had the fixed bar for the cache, use the value from the capabilities.

Signed-off-by Dr. David Alan Gilbert <[email protected]>

---
 fs/fuse/virtio_fs.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

fs/fuse/virtio_fs.c +1 -6

index 60d496c16841..55bac1465536 100644
@@ -14,11 +14,6 @@
 #include <uapi/linux/virtio_pci.h>
 #include "fuse_i.h"
 
-enum {
-	/* PCI BAR number of the virtio-fs DAX window */
-	VIRTIO_FS_WINDOW_BAR = 2,
-};
-
 /* List of virtio-fs device instances and a lock for the list */
 static DEFINE_MUTEX(virtio_fs_mutex);
 static LIST_HEAD(virtio_fs_instances);

@@ -518,7 +513,7 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
 	struct dev_pagemap *pgmap;
 	struct pci_dev *pci_dev;
 	phys_addr_t phys_addr;
-	size_t len;
+	size_t bar_len;
 	int ret;
 	u8 have_cache, cache_bar;
 	u64 cache_offset, cache_len;

@@ -551,17 +546,13 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
         }
 

Quoted 8 lines

fs/fuse/virtio_fs.c static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs) · line 548

Quoted 1 line

fs/fuse/virtio_fs.c +0 -0

 	/* TODO handle case where device doesn't expose BAR? */

For virtio-pmem we decided to not go via BARs as this would effectively make it only usable for virtio-pci implementers. Instead, we are going to export the applicable physical device region directly (e.g. phys_start, phys_size in virtio config), so it is decoupled from PCI details. Doing the same for virtio-fs would allow e.g. also virtio-ccw to make eventually use of this.

That makes it a very odd looking PCI device; I can see that with virtio-pmem it makes some sense, given that it's job is to expose arbitrary chunks of memory.

Dave

Quoted 47 lines · Dr. David Alan Gilbert

Quoted 37 lines · * David Hildenbrand ([email protected]) wrote:

Quoted 36 lines · Vivek Goyal

Instead of assuming we had the fixed bar for the cache, use the value from the capabilities.

Signed-off-by Dr. David Alan Gilbert <[email protected]>

---
 fs/fuse/virtio_fs.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

fs/fuse/virtio_fs.c +1 -6

index 60d496c16841..55bac1465536 100644
@@ -14,11 +14,6 @@
 #include <uapi/linux/virtio_pci.h>
 #include "fuse_i.h"
 
-enum {
-	/* PCI BAR number of the virtio-fs DAX window */
-	VIRTIO_FS_WINDOW_BAR = 2,
-};
-
 /* List of virtio-fs device instances and a lock for the list */
 static DEFINE_MUTEX(virtio_fs_mutex);
 static LIST_HEAD(virtio_fs_instances);

@@ -518,7 +513,7 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
 	struct dev_pagemap *pgmap;
 	struct pci_dev *pci_dev;
 	phys_addr_t phys_addr;
-	size_t len;
+	size_t bar_len;
 	int ret;
 	u8 have_cache, cache_bar;
 	u64 cache_offset, cache_len;

@@ -551,17 +546,13 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
         }
 

Quoted 8 lines

fs/fuse/virtio_fs.c static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs) · line 548

Quoted 1 line

fs/fuse/virtio_fs.c +0 -0

 	/* TODO handle case where device doesn't expose BAR? */

For virtio-pmem we decided to not go via BARs as this would effectively make it only usable for virtio-pci implementers. Instead, we are going to export the applicable physical device region directly (e.g. phys_start, phys_size in virtio config), so it is decoupled from PCI details. Doing the same for virtio-fs would allow e.g. also virtio-ccw to make eventually use of this.

Quoted 5 lines

That makes it a very odd looking PCI device; I can see that with virtio-pmem it makes some sense, given that it's job is to expose arbitrary chunks of memory.

Dave

Well, the fact that your are

- including <uapi/linux/virtio_pci.h>
- adding pci related code

in/to fs/fuse/virtio_fs.c

tells me that these properties might be better communicated on the virtio layer, not on the PCI layer.

Or do you really want to glue virtio-fs to virtio-pci for all eternity?

Signature · 5 lines

-- 

Thanks,

David / dhildenb

Quoted 59 lines · * David Hildenbrand ([email protected]) wrote:

Quoted 47 lines · Dr. David Alan Gilbert

Quoted 37 lines · * David Hildenbrand ([email protected]) wrote:

Quoted 36 lines · Vivek Goyal

Instead of assuming we had the fixed bar for the cache, use the value from the capabilities.

Signed-off-by Dr. David Alan Gilbert <[email protected]>

---
 fs/fuse/virtio_fs.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

fs/fuse/virtio_fs.c +1 -6

index 60d496c16841..55bac1465536 100644
@@ -14,11 +14,6 @@
 #include <uapi/linux/virtio_pci.h>
 #include "fuse_i.h"
 
-enum {
-	/* PCI BAR number of the virtio-fs DAX window */
-	VIRTIO_FS_WINDOW_BAR = 2,
-};
-
 /* List of virtio-fs device instances and a lock for the list */
 static DEFINE_MUTEX(virtio_fs_mutex);
 static LIST_HEAD(virtio_fs_instances);

@@ -518,7 +513,7 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
 	struct dev_pagemap *pgmap;
 	struct pci_dev *pci_dev;
 	phys_addr_t phys_addr;
-	size_t len;
+	size_t bar_len;
 	int ret;
 	u8 have_cache, cache_bar;
 	u64 cache_offset, cache_len;

@@ -551,17 +546,13 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
         }
 

Quoted 8 lines

fs/fuse/virtio_fs.c static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs) · line 548

Quoted 1 line

fs/fuse/virtio_fs.c +0 -0

 	/* TODO handle case where device doesn't expose BAR? */

For virtio-pmem we decided to not go via BARs as this would effectively make it only usable for virtio-pci implementers. Instead, we are going to export the applicable physical device region directly (e.g. phys_start, phys_size in virtio config), so it is decoupled from PCI details. Doing the same for virtio-fs would allow e.g. also virtio-ccw to make eventually use of this.

Quoted 5 lines

That makes it a very odd looking PCI device; I can see that with virtio-pmem it makes some sense, given that it's job is to expose arbitrary chunks of memory.

Dave

Well, the fact that your are

- including <uapi/linux/virtio_pci.h>
- adding pci related code

Quoted 6 lines

in/to fs/fuse/virtio_fs.c

tells me that these properties might be better communicated on the virtio layer, not on the PCI layer.

Or do you really want to glue virtio-fs to virtio-pci for all eternity?

No, these need cleaning up; and the split within the bar
is probably going to change to be communicated via virtio layer rather than pci capabilities. However, I don't want to make our PCI device look odd, just to make portability to non-PCI devices - so it's right to make the split appropriately, but still to use PCI bars for what they were designed for.

Dave

Quoted 67 lines · Dr. David Alan Gilbert

Quoted 59 lines · * David Hildenbrand ([email protected]) wrote:

Quoted 47 lines · Dr. David Alan Gilbert

Quoted 37 lines · * David Hildenbrand ([email protected]) wrote:

Quoted 36 lines · Vivek Goyal

Instead of assuming we had the fixed bar for the cache, use the value from the capabilities.

Signed-off-by Dr. David Alan Gilbert <[email protected]>

---
 fs/fuse/virtio_fs.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

fs/fuse/virtio_fs.c +1 -6

index 60d496c16841..55bac1465536 100644
@@ -14,11 +14,6 @@
 #include <uapi/linux/virtio_pci.h>
 #include "fuse_i.h"
 
-enum {
-	/* PCI BAR number of the virtio-fs DAX window */
-	VIRTIO_FS_WINDOW_BAR = 2,
-};
-
 /* List of virtio-fs device instances and a lock for the list */
 static DEFINE_MUTEX(virtio_fs_mutex);
 static LIST_HEAD(virtio_fs_instances);

@@ -518,7 +513,7 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
 	struct dev_pagemap *pgmap;
 	struct pci_dev *pci_dev;
 	phys_addr_t phys_addr;
-	size_t len;
+	size_t bar_len;
 	int ret;
 	u8 have_cache, cache_bar;
 	u64 cache_offset, cache_len;

@@ -551,17 +546,13 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
         }
 

Quoted 8 lines

fs/fuse/virtio_fs.c static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs) · line 548

Quoted 1 line

fs/fuse/virtio_fs.c +0 -0

 	/* TODO handle case where device doesn't expose BAR? */

For virtio-pmem we decided to not go via BARs as this would effectively make it only usable for virtio-pci implementers. Instead, we are going to export the applicable physical device region directly (e.g. phys_start, phys_size in virtio config), so it is decoupled from PCI details. Doing the same for virtio-fs would allow e.g. also virtio-ccw to make eventually use of this.

Quoted 5 lines

That makes it a very odd looking PCI device; I can see that with virtio-pmem it makes some sense, given that it's job is to expose arbitrary chunks of memory.

Dave

Well, the fact that your are

- including <uapi/linux/virtio_pci.h>
- adding pci related code

Quoted 6 lines

in/to fs/fuse/virtio_fs.c

tells me that these properties might be better communicated on the virtio layer, not on the PCI layer.

Or do you really want to glue virtio-fs to virtio-pci for all eternity?

Quoted 8 lines

No, these need cleaning up; and the split within the bar
is probably going to change to be communicated via virtio layer rather than pci capabilities. However, I don't want to make our PCI device look odd, just to make portability to non-PCI devices - so it's right to make the split appropriately, but still to use PCI bars for what they were designed for.

Dave

Let's discuss after the cleanup. In general I am not convinced this is the right thing to do. Using virtio-pci for anything else than pure transport smells like bad design to me (well, I am no virtio expert after all ;) ). No matter what PCI bars were designed for. If we can't get the same running with e.g. virtio-ccw or virtio-whatever, it is broken by design (or an addon that is tightly glued to virtio-pci, if that is the general idea).

Signature · 3 lines

Thanks,

David / dhildenb

Quoted 77 lines · * David Hildenbrand ([email protected]) wrote:

Quoted 67 lines · Dr. David Alan Gilbert

Quoted 59 lines · * David Hildenbrand ([email protected]) wrote:

Quoted 47 lines · Dr. David Alan Gilbert

Quoted 37 lines · * David Hildenbrand ([email protected]) wrote:

Quoted 36 lines · Vivek Goyal

Instead of assuming we had the fixed bar for the cache, use the value from the capabilities.

Signed-off-by Dr. David Alan Gilbert <[email protected]>

---
 fs/fuse/virtio_fs.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

fs/fuse/virtio_fs.c +1 -6

index 60d496c16841..55bac1465536 100644
@@ -14,11 +14,6 @@
 #include <uapi/linux/virtio_pci.h>
 #include "fuse_i.h"
 
-enum {
-	/* PCI BAR number of the virtio-fs DAX window */
-	VIRTIO_FS_WINDOW_BAR = 2,
-};
-
 /* List of virtio-fs device instances and a lock for the list */
 static DEFINE_MUTEX(virtio_fs_mutex);
 static LIST_HEAD(virtio_fs_instances);

@@ -518,7 +513,7 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
 	struct dev_pagemap *pgmap;
 	struct pci_dev *pci_dev;
 	phys_addr_t phys_addr;
-	size_t len;
+	size_t bar_len;
 	int ret;
 	u8 have_cache, cache_bar;
 	u64 cache_offset, cache_len;

@@ -551,17 +546,13 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
         }
 

Quoted 8 lines

fs/fuse/virtio_fs.c static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs) · line 548

Quoted 1 line

fs/fuse/virtio_fs.c +0 -0

 	/* TODO handle case where device doesn't expose BAR? */

For virtio-pmem we decided to not go via BARs as this would effectively make it only usable for virtio-pci implementers. Instead, we are going to export the applicable physical device region directly (e.g. phys_start, phys_size in virtio config), so it is decoupled from PCI details. Doing the same for virtio-fs would allow e.g. also virtio-ccw to make eventually use of this.

Quoted 5 lines

That makes it a very odd looking PCI device; I can see that with virtio-pmem it makes some sense, given that it's job is to expose arbitrary chunks of memory.

Dave

Well, the fact that your are

- including <uapi/linux/virtio_pci.h>
- adding pci related code

Quoted 6 lines

in/to fs/fuse/virtio_fs.c

tells me that these properties might be better communicated on the virtio layer, not on the PCI layer.

Or do you really want to glue virtio-fs to virtio-pci for all eternity?

Quoted 8 lines

No, these need cleaning up; and the split within the bar
is probably going to change to be communicated via virtio layer rather than pci capabilities. However, I don't want to make our PCI device look odd, just to make portability to non-PCI devices - so it's right to make the split appropriately, but still to use PCI bars for what they were designed for.

Dave

Quoted 7 lines

Let's discuss after the cleanup. In general I am not convinced this is the right thing to do. Using virtio-pci for anything else than pure transport smells like bad design to me (well, I am no virtio expert after all ;) ). No matter what PCI bars were designed for. If we can't get the same running with e.g. virtio-ccw or virtio-whatever, it is broken by design (or an addon that is tightly glued to virtio-pci, if that is the general idea).

I'm sure we can find alternatives for virtio-*, so I wouldn't expect it to be glued to virtio-pci.

Dave

Quoted 86 lines · Dr. David Alan Gilbert

Quoted 77 lines · * David Hildenbrand ([email protected]) wrote:

Quoted 67 lines · Dr. David Alan Gilbert

Quoted 59 lines · * David Hildenbrand ([email protected]) wrote:

Quoted 47 lines · Dr. David Alan Gilbert

Quoted 37 lines · * David Hildenbrand ([email protected]) wrote:

Quoted 36 lines · Vivek Goyal

Instead of assuming we had the fixed bar for the cache, use the value from the capabilities.

Signed-off-by Dr. David Alan Gilbert <[email protected]>

---
 fs/fuse/virtio_fs.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

fs/fuse/virtio_fs.c +1 -6

index 60d496c16841..55bac1465536 100644
@@ -14,11 +14,6 @@
 #include <uapi/linux/virtio_pci.h>
 #include "fuse_i.h"
 
-enum {
-	/* PCI BAR number of the virtio-fs DAX window */
-	VIRTIO_FS_WINDOW_BAR = 2,
-};
-
 /* List of virtio-fs device instances and a lock for the list */
 static DEFINE_MUTEX(virtio_fs_mutex);
 static LIST_HEAD(virtio_fs_instances);

@@ -518,7 +513,7 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
 	struct dev_pagemap *pgmap;
 	struct pci_dev *pci_dev;
 	phys_addr_t phys_addr;
-	size_t len;
+	size_t bar_len;
 	int ret;
 	u8 have_cache, cache_bar;
 	u64 cache_offset, cache_len;

@@ -551,17 +546,13 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
         }
 

Quoted 8 lines

fs/fuse/virtio_fs.c static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs) · line 548

Quoted 1 line

fs/fuse/virtio_fs.c +0 -0

 	/* TODO handle case where device doesn't expose BAR? */  

For virtio-pmem we decided to not go via BARs as this would effectively make it only usable for virtio-pci implementers. Instead, we are going to export the applicable physical device region directly (e.g. phys_start, phys_size in virtio config), so it is decoupled from PCI details. Doing the same for virtio-fs would allow e.g. also virtio-ccw to make eventually use of this.

Quoted 5 lines

That makes it a very odd looking PCI device; I can see that with virtio-pmem it makes some sense, given that it's job is to expose arbitrary chunks of memory.

Dave

Well, the fact that your are

- including <uapi/linux/virtio_pci.h>
- adding pci related code

Quoted 6 lines

in/to fs/fuse/virtio_fs.c

tells me that these properties might be better communicated on the virtio layer, not on the PCI layer.

Or do you really want to glue virtio-fs to virtio-pci for all eternity?

Quoted 8 lines

No, these need cleaning up; and the split within the bar
is probably going to change to be communicated via virtio layer rather than pci capabilities. However, I don't want to make our PCI device look odd, just to make portability to non-PCI devices - so it's right to make the split appropriately, but still to use PCI bars for what they were designed for.

Dave

Quoted 7 lines

Let's discuss after the cleanup. In general I am not convinced this is the right thing to do. Using virtio-pci for anything else than pure transport smells like bad design to me (well, I am no virtio expert after all ;) ). No matter what PCI bars were designed for. If we can't get the same running with e.g. virtio-ccw or virtio-whatever, it is broken by design (or an addon that is tightly glued to virtio-pci, if that is the general idea).

Quoted 4 lines

I'm sure we can find alternatives for virtio-*, so I wouldn't expect it to be glued to virtio-pci.

Dave

As s390x does not have the concept of memory mapped io (RAM is RAM, nothing else), this is not architectured. vitio-ccw can therefore not define anything similar like that. However, in virtual environments we can do whatever we want on top of the pure transport (e.g. on the virtio layer).

Conny can correct me if I am wrong.

Quoted 92 lines · On Thu, 13 Dec 2018 13:24:31 +0100 David Hildenbrand <[email protected]> wrote:

Quoted 86 lines · Dr. David Alan Gilbert

Quoted 77 lines · * David Hildenbrand ([email protected]) wrote:

Quoted 67 lines · Dr. David Alan Gilbert

Quoted 59 lines · * David Hildenbrand ([email protected]) wrote:

Quoted 47 lines · Dr. David Alan Gilbert

Quoted 37 lines · * David Hildenbrand ([email protected]) wrote:

Quoted 36 lines · Vivek Goyal

Instead of assuming we had the fixed bar for the cache, use the value from the capabilities.

Signed-off-by Dr. David Alan Gilbert <[email protected]>

---
 fs/fuse/virtio_fs.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

fs/fuse/virtio_fs.c +1 -6

index 60d496c16841..55bac1465536 100644
@@ -14,11 +14,6 @@
 #include <uapi/linux/virtio_pci.h>
 #include "fuse_i.h"
 
-enum {
-	/* PCI BAR number of the virtio-fs DAX window */
-	VIRTIO_FS_WINDOW_BAR = 2,
-};
-
 /* List of virtio-fs device instances and a lock for the list */
 static DEFINE_MUTEX(virtio_fs_mutex);
 static LIST_HEAD(virtio_fs_instances);

@@ -518,7 +513,7 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
 	struct dev_pagemap *pgmap;
 	struct pci_dev *pci_dev;
 	phys_addr_t phys_addr;
-	size_t len;
+	size_t bar_len;
 	int ret;
 	u8 have_cache, cache_bar;
 	u64 cache_offset, cache_len;

@@ -551,17 +546,13 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
         }
 

Quoted 8 lines

fs/fuse/virtio_fs.c static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs) · line 548

Quoted 1 line

fs/fuse/virtio_fs.c +0 -0

 	/* TODO handle case where device doesn't expose BAR? */  

For virtio-pmem we decided to not go via BARs as this would effectively make it only usable for virtio-pci implementers. Instead, we are going to export the applicable physical device region directly (e.g. phys_start, phys_size in virtio config), so it is decoupled from PCI details. Doing the same for virtio-fs would allow e.g. also virtio-ccw to make eventually use of this.

Quoted 5 lines

That makes it a very odd looking PCI device; I can see that with virtio-pmem it makes some sense, given that it's job is to expose arbitrary chunks of memory.

Dave

Well, the fact that your are

- including <uapi/linux/virtio_pci.h>
- adding pci related code

Quoted 6 lines

in/to fs/fuse/virtio_fs.c

tells me that these properties might be better communicated on the virtio layer, not on the PCI layer.

Or do you really want to glue virtio-fs to virtio-pci for all eternity?

Quoted 8 lines

No, these need cleaning up; and the split within the bar
is probably going to change to be communicated via virtio layer rather than pci capabilities. However, I don't want to make our PCI device look odd, just to make portability to non-PCI devices - so it's right to make the split appropriately, but still to use PCI bars for what they were designed for.

Dave

Quoted 7 lines

Let's discuss after the cleanup. In general I am not convinced this is the right thing to do. Using virtio-pci for anything else than pure transport smells like bad design to me (well, I am no virtio expert after all ;) ). No matter what PCI bars were designed for. If we can't get the same running with e.g. virtio-ccw or virtio-whatever, it is broken by design (or an addon that is tightly glued to virtio-pci, if that is the general idea).

Quoted 4 lines

I'm sure we can find alternatives for virtio-*, so I wouldn't expect it to be glued to virtio-pci.

Dave

Quoted 7 lines

As s390x does not have the concept of memory mapped io (RAM is RAM, nothing else), this is not architectured. vitio-ccw can therefore not define anything similar like that. However, in virtual environments we can do whatever we want on top of the pure transport (e.g. on the virtio layer).

Conny can correct me if I am wrong.

I don't think you're wrong, but I haven't read the code yet and I'm therefore not aware of the purpose of this BAR.

Generally, if there is a memory location shared between host and guest, we need a way to communicate its location, which will likely differ between transports. For ccw, I could imagine a new channel command dedicated to exchanging configuration information (similar to what exists today to communicate the locations of virtqueues), but I'd rather not go down this path.

Without reading the code/design further, can we use one of the following instead of a BAR:
- a virtqueue;
- something in config space?
That would be implementable by any virtio transport.

Quoted 113 lines · Cornelia Huck

Quoted 92 lines · On Thu, 13 Dec 2018 13:24:31 +0100 David Hildenbrand <[email protected]> wrote:

Quoted 86 lines · Dr. David Alan Gilbert

Quoted 77 lines · * David Hildenbrand ([email protected]) wrote:

Quoted 67 lines · Dr. David Alan Gilbert

Quoted 59 lines · * David Hildenbrand ([email protected]) wrote:

Quoted 47 lines · Dr. David Alan Gilbert

Quoted 37 lines · * David Hildenbrand ([email protected]) wrote:

Quoted 36 lines · Vivek Goyal

Instead of assuming we had the fixed bar for the cache, use the value from the capabilities.

Signed-off-by Dr. David Alan Gilbert <[email protected]>

---
 fs/fuse/virtio_fs.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

fs/fuse/virtio_fs.c +1 -6

index 60d496c16841..55bac1465536 100644
@@ -14,11 +14,6 @@
 #include <uapi/linux/virtio_pci.h>
 #include "fuse_i.h"
 
-enum {
-	/* PCI BAR number of the virtio-fs DAX window */
-	VIRTIO_FS_WINDOW_BAR = 2,
-};
-
 /* List of virtio-fs device instances and a lock for the list */
 static DEFINE_MUTEX(virtio_fs_mutex);
 static LIST_HEAD(virtio_fs_instances);

@@ -518,7 +513,7 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
 	struct dev_pagemap *pgmap;
 	struct pci_dev *pci_dev;
 	phys_addr_t phys_addr;
-	size_t len;
+	size_t bar_len;
 	int ret;
 	u8 have_cache, cache_bar;
 	u64 cache_offset, cache_len;

@@ -551,17 +546,13 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
         }
 

Quoted 8 lines

fs/fuse/virtio_fs.c static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs) · line 548

Quoted 1 line

fs/fuse/virtio_fs.c +0 -0

 	/* TODO handle case where device doesn't expose BAR? */    

For virtio-pmem we decided to not go via BARs as this would effectively make it only usable for virtio-pci implementers. Instead, we are going to export the applicable physical device region directly (e.g. phys_start, phys_size in virtio config), so it is decoupled from PCI details. Doing the same for virtio-fs would allow e.g. also virtio-ccw to make eventually use of this.

Quoted 5 lines

That makes it a very odd looking PCI device; I can see that with virtio-pmem it makes some sense, given that it's job is to expose arbitrary chunks of memory.

Dave

Well, the fact that your are

- including <uapi/linux/virtio_pci.h>
- adding pci related code

Quoted 6 lines

in/to fs/fuse/virtio_fs.c

tells me that these properties might be better communicated on the virtio layer, not on the PCI layer.

Or do you really want to glue virtio-fs to virtio-pci for all eternity?

Quoted 8 lines

No, these need cleaning up; and the split within the bar
is probably going to change to be communicated via virtio layer rather than pci capabilities. However, I don't want to make our PCI device look odd, just to make portability to non-PCI devices - so it's right to make the split appropriately, but still to use PCI bars for what they were designed for.

Dave

Quoted 7 lines

Let's discuss after the cleanup. In general I am not convinced this is the right thing to do. Using virtio-pci for anything else than pure transport smells like bad design to me (well, I am no virtio expert after all ;) ). No matter what PCI bars were designed for. If we can't get the same running with e.g. virtio-ccw or virtio-whatever, it is broken by design (or an addon that is tightly glued to virtio-pci, if that is the general idea).

Quoted 4 lines

I'm sure we can find alternatives for virtio-*, so I wouldn't expect it to be glued to virtio-pci.

Dave

Quoted 7 lines

As s390x does not have the concept of memory mapped io (RAM is RAM, nothing else), this is not architectured. vitio-ccw can therefore not define anything similar like that. However, in virtual environments we can do whatever we want on top of the pure transport (e.g. on the virtio layer).

Conny can correct me if I am wrong.

I don't think you're wrong, but I haven't read the code yet and I'm therefore not aware of the purpose of this BAR.

Generally, if there is a memory location shared between host and guest, we need a way to communicate its location, which will likely differ between transports. For ccw, I could imagine a new channel command dedicated to exchanging configuration information (similar to what exists today to communicate the locations of virtqueues), but I'd rather not go down this path.

Quoted 5 lines

Without reading the code/design further, can we use one of the following instead of a BAR:
- a virtqueue;
- something in config space?
That would be implementable by any virtio transport.

The way I think about this is that we wish to extend the VIRTIO device model with the concept of shared memory. virtio-fs, virtio-gpu, and virtio-vhost-user all have requirements for shared memory.

This seems like a transport-level issue to me. PCI supports memory-mapped I/O and that's the right place to do it. If you try to put it into config space or the virtqueue, you'll end up with something that cannot be realized as a PCI device because it bypasses PCI bus address translation.

If CCW needs a side-channel, that's fine. But that side-channel is a CCW-specific mechanism and probably doesn't apply to all other transports.

Quoted 130 lines · On Fri, 14 Dec 2018 13:44:34 +0000 Stefan Hajnoczi <[email protected]> wrote:

Quoted 113 lines · Cornelia Huck

Quoted 92 lines · On Thu, 13 Dec 2018 13:24:31 +0100 David Hildenbrand <[email protected]> wrote:

Quoted 86 lines · Dr. David Alan Gilbert

Quoted 77 lines · * David Hildenbrand ([email protected]) wrote:

Quoted 67 lines · Dr. David Alan Gilbert

Quoted 59 lines · * David Hildenbrand ([email protected]) wrote:

Quoted 47 lines · Dr. David Alan Gilbert

Quoted 37 lines · * David Hildenbrand ([email protected]) wrote:

Quoted 36 lines · Vivek Goyal

Instead of assuming we had the fixed bar for the cache, use the value from the capabilities.

Signed-off-by Dr. David Alan Gilbert <[email protected]>

---
 fs/fuse/virtio_fs.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

fs/fuse/virtio_fs.c +1 -6

index 60d496c16841..55bac1465536 100644
@@ -14,11 +14,6 @@
 #include <uapi/linux/virtio_pci.h>
 #include "fuse_i.h"
 
-enum {
-	/* PCI BAR number of the virtio-fs DAX window */
-	VIRTIO_FS_WINDOW_BAR = 2,
-};
-
 /* List of virtio-fs device instances and a lock for the list */
 static DEFINE_MUTEX(virtio_fs_mutex);
 static LIST_HEAD(virtio_fs_instances);

@@ -518,7 +513,7 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
 	struct dev_pagemap *pgmap;
 	struct pci_dev *pci_dev;
 	phys_addr_t phys_addr;
-	size_t len;
+	size_t bar_len;
 	int ret;
 	u8 have_cache, cache_bar;
 	u64 cache_offset, cache_len;

@@ -551,17 +546,13 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
         }
 

Quoted 8 lines

fs/fuse/virtio_fs.c static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs) · line 548

Quoted 1 line

fs/fuse/virtio_fs.c +0 -0

 	/* TODO handle case where device doesn't expose BAR? */    

For virtio-pmem we decided to not go via BARs as this would effectively make it only usable for virtio-pci implementers. Instead, we are going to export the applicable physical device region directly (e.g. phys_start, phys_size in virtio config), so it is decoupled from PCI details. Doing the same for virtio-fs would allow e.g. also virtio-ccw to make eventually use of this.

Quoted 5 lines

That makes it a very odd looking PCI device; I can see that with virtio-pmem it makes some sense, given that it's job is to expose arbitrary chunks of memory.

Dave

Well, the fact that your are

- including <uapi/linux/virtio_pci.h>
- adding pci related code

Quoted 6 lines

in/to fs/fuse/virtio_fs.c

tells me that these properties might be better communicated on the virtio layer, not on the PCI layer.

Or do you really want to glue virtio-fs to virtio-pci for all eternity?

Quoted 8 lines

No, these need cleaning up; and the split within the bar
is probably going to change to be communicated via virtio layer rather than pci capabilities. However, I don't want to make our PCI device look odd, just to make portability to non-PCI devices - so it's right to make the split appropriately, but still to use PCI bars for what they were designed for.

Dave

Quoted 7 lines

Let's discuss after the cleanup. In general I am not convinced this is the right thing to do. Using virtio-pci for anything else than pure transport smells like bad design to me (well, I am no virtio expert after all ;) ). No matter what PCI bars were designed for. If we can't get the same running with e.g. virtio-ccw or virtio-whatever, it is broken by design (or an addon that is tightly glued to virtio-pci, if that is the general idea).

Quoted 4 lines

I'm sure we can find alternatives for virtio-*, so I wouldn't expect it to be glued to virtio-pci.

Dave

Quoted 7 lines

As s390x does not have the concept of memory mapped io (RAM is RAM, nothing else), this is not architectured. vitio-ccw can therefore not define anything similar like that. However, in virtual environments we can do whatever we want on top of the pure transport (e.g. on the virtio layer).

Conny can correct me if I am wrong.

I don't think you're wrong, but I haven't read the code yet and I'm therefore not aware of the purpose of this BAR.

Generally, if there is a memory location shared between host and guest, we need a way to communicate its location, which will likely differ between transports. For ccw, I could imagine a new channel command dedicated to exchanging configuration information (similar to what exists today to communicate the locations of virtqueues), but I'd rather not go down this path.

Quoted 5 lines

Without reading the code/design further, can we use one of the following instead of a BAR:
- a virtqueue;
- something in config space?
That would be implementable by any virtio transport.

The way I think about this is that we wish to extend the VIRTIO device model with the concept of shared memory. virtio-fs, virtio-gpu, and virtio-vhost-user all have requirements for shared memory.

This seems like a transport-level issue to me. PCI supports memory-mapped I/O and that's the right place to do it. If you try to put it into config space or the virtqueue, you'll end up with something that cannot be realized as a PCI device because it bypasses PCI bus address translation.

Quoted 3 lines

If CCW needs a side-channel, that's fine. But that side-channel is a CCW-specific mechanism and probably doesn't apply to all other transports.

But virtio-gpu works with ccw right now (I haven't checked what it uses); can virtio-fs use an equivalent method?

If there's a more generic case to be made for extending virtio devices with a way to handle shared memory, a ccw for that would be fine. I just want to avoid adding new ccws for everything as the namespace is not infinite.

Quoted 130 lines · On Fri, 14 Dec 2018 13:44:34 +0000 Stefan Hajnoczi <[email protected]> wrote:

Quoted 113 lines · Cornelia Huck

Quoted 92 lines · On Thu, 13 Dec 2018 13:24:31 +0100 David Hildenbrand <[email protected]> wrote:

Quoted 86 lines · Dr. David Alan Gilbert

Quoted 77 lines · * David Hildenbrand ([email protected]) wrote:

Quoted 67 lines · Dr. David Alan Gilbert

Quoted 59 lines · * David Hildenbrand ([email protected]) wrote:

Quoted 47 lines · Dr. David Alan Gilbert

Quoted 37 lines · * David Hildenbrand ([email protected]) wrote:

Quoted 36 lines · Vivek Goyal

Instead of assuming we had the fixed bar for the cache, use the value from the capabilities.

Signed-off-by Dr. David Alan Gilbert <[email protected]>

---
 fs/fuse/virtio_fs.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

fs/fuse/virtio_fs.c +1 -6

index 60d496c16841..55bac1465536 100644
@@ -14,11 +14,6 @@
 #include <uapi/linux/virtio_pci.h>
 #include "fuse_i.h"
 
-enum {
-	/* PCI BAR number of the virtio-fs DAX window */
-	VIRTIO_FS_WINDOW_BAR = 2,
-};
-
 /* List of virtio-fs device instances and a lock for the list */
 static DEFINE_MUTEX(virtio_fs_mutex);
 static LIST_HEAD(virtio_fs_instances);

@@ -518,7 +513,7 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
 	struct dev_pagemap *pgmap;
 	struct pci_dev *pci_dev;
 	phys_addr_t phys_addr;
-	size_t len;
+	size_t bar_len;
 	int ret;
 	u8 have_cache, cache_bar;
 	u64 cache_offset, cache_len;

@@ -551,17 +546,13 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
         }
 

Quoted 8 lines

fs/fuse/virtio_fs.c static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs) · line 548

Quoted 1 line

fs/fuse/virtio_fs.c +0 -0

 	/* TODO handle case where device doesn't expose BAR? */    

For virtio-pmem we decided to not go via BARs as this would effectively make it only usable for virtio-pci implementers. Instead, we are going to export the applicable physical device region directly (e.g. phys_start, phys_size in virtio config), so it is decoupled from PCI details. Doing the same for virtio-fs would allow e.g. also virtio-ccw to make eventually use of this.

Quoted 5 lines

That makes it a very odd looking PCI device; I can see that with virtio-pmem it makes some sense, given that it's job is to expose arbitrary chunks of memory.

Dave

Well, the fact that your are

- including <uapi/linux/virtio_pci.h>
- adding pci related code

Quoted 6 lines

in/to fs/fuse/virtio_fs.c

tells me that these properties might be better communicated on the virtio layer, not on the PCI layer.

Or do you really want to glue virtio-fs to virtio-pci for all eternity?

Quoted 8 lines

No, these need cleaning up; and the split within the bar
is probably going to change to be communicated via virtio layer rather than pci capabilities. However, I don't want to make our PCI device look odd, just to make portability to non-PCI devices - so it's right to make the split appropriately, but still to use PCI bars for what they were designed for.

Dave

Quoted 7 lines

Let's discuss after the cleanup. In general I am not convinced this is the right thing to do. Using virtio-pci for anything else than pure transport smells like bad design to me (well, I am no virtio expert after all ;) ). No matter what PCI bars were designed for. If we can't get the same running with e.g. virtio-ccw or virtio-whatever, it is broken by design (or an addon that is tightly glued to virtio-pci, if that is the general idea).

Quoted 4 lines

I'm sure we can find alternatives for virtio-*, so I wouldn't expect it to be glued to virtio-pci.

Dave

Quoted 7 lines

As s390x does not have the concept of memory mapped io (RAM is RAM, nothing else), this is not architectured. vitio-ccw can therefore not define anything similar like that. However, in virtual environments we can do whatever we want on top of the pure transport (e.g. on the virtio layer).

Conny can correct me if I am wrong.

I don't think you're wrong, but I haven't read the code yet and I'm therefore not aware of the purpose of this BAR.

Generally, if there is a memory location shared between host and guest, we need a way to communicate its location, which will likely differ between transports. For ccw, I could imagine a new channel command dedicated to exchanging configuration information (similar to what exists today to communicate the locations of virtqueues), but I'd rather not go down this path.

Quoted 5 lines

Without reading the code/design further, can we use one of the following instead of a BAR:
- a virtqueue;
- something in config space?
That would be implementable by any virtio transport.

The way I think about this is that we wish to extend the VIRTIO device model with the concept of shared memory. virtio-fs, virtio-gpu, and virtio-vhost-user all have requirements for shared memory.

This seems like a transport-level issue to me. PCI supports memory-mapped I/O and that's the right place to do it. If you try to put it into config space or the virtqueue, you'll end up with something that cannot be realized as a PCI device because it bypasses PCI bus address translation.

Quoted 3 lines

If CCW needs a side-channel, that's fine. But that side-channel is a CCW-specific mechanism and probably doesn't apply to all other transports.

But virtio-gpu works with ccw right now (I haven't checked what it uses); can virtio-fs use an equivalent method?

If there's a more generic case to be made for extending virtio devices with a way to handle shared memory, a ccw for that would be fine. I just want to avoid adding new ccws for everything as the namespace is not infinite.

Quoted 113 lines · Cornelia Huck

Quoted 92 lines · On Thu, 13 Dec 2018 13:24:31 +0100 David Hildenbrand <[email protected]> wrote:

Quoted 86 lines · Dr. David Alan Gilbert

Quoted 77 lines · * David Hildenbrand ([email protected]) wrote:

Quoted 67 lines · Dr. David Alan Gilbert

Quoted 59 lines · * David Hildenbrand ([email protected]) wrote:

Quoted 47 lines · Dr. David Alan Gilbert

Quoted 37 lines · * David Hildenbrand ([email protected]) wrote:

Quoted 36 lines · Vivek Goyal

Instead of assuming we had the fixed bar for the cache, use the value from the capabilities.

Signed-off-by Dr. David Alan Gilbert <[email protected]>

---
 fs/fuse/virtio_fs.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

fs/fuse/virtio_fs.c +1 -6

index 60d496c16841..55bac1465536 100644
@@ -14,11 +14,6 @@
 #include <uapi/linux/virtio_pci.h>
 #include "fuse_i.h"
 
-enum {
-	/* PCI BAR number of the virtio-fs DAX window */
-	VIRTIO_FS_WINDOW_BAR = 2,
-};
-
 /* List of virtio-fs device instances and a lock for the list */
 static DEFINE_MUTEX(virtio_fs_mutex);
 static LIST_HEAD(virtio_fs_instances);

@@ -518,7 +513,7 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
 	struct dev_pagemap *pgmap;
 	struct pci_dev *pci_dev;
 	phys_addr_t phys_addr;
-	size_t len;
+	size_t bar_len;
 	int ret;
 	u8 have_cache, cache_bar;
 	u64 cache_offset, cache_len;

@@ -551,17 +546,13 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
         }
 

Quoted 8 lines

fs/fuse/virtio_fs.c static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs) · line 548

Quoted 1 line

fs/fuse/virtio_fs.c +0 -0

 	/* TODO handle case where device doesn't expose BAR? */  

For virtio-pmem we decided to not go via BARs as this would effectively make it only usable for virtio-pci implementers. Instead, we are going to export the applicable physical device region directly (e.g. phys_start, phys_size in virtio config), so it is decoupled from PCI details. Doing the same for virtio-fs would allow e.g. also virtio-ccw to make eventually use of this.

Quoted 5 lines

That makes it a very odd looking PCI device; I can see that with virtio-pmem it makes some sense, given that it's job is to expose arbitrary chunks of memory.

Dave

Well, the fact that your are

- including <uapi/linux/virtio_pci.h>
- adding pci related code

Quoted 6 lines

in/to fs/fuse/virtio_fs.c

tells me that these properties might be better communicated on the virtio layer, not on the PCI layer.

Or do you really want to glue virtio-fs to virtio-pci for all eternity?

Quoted 8 lines

No, these need cleaning up; and the split within the bar
is probably going to change to be communicated via virtio layer rather than pci capabilities. However, I don't want to make our PCI device look odd, just to make portability to non-PCI devices - so it's right to make the split appropriately, but still to use PCI bars for what they were designed for.

Dave

Quoted 7 lines

Let's discuss after the cleanup. In general I am not convinced this is the right thing to do. Using virtio-pci for anything else than pure transport smells like bad design to me (well, I am no virtio expert after all ;) ). No matter what PCI bars were designed for. If we can't get the same running with e.g. virtio-ccw or virtio-whatever, it is broken by design (or an addon that is tightly glued to virtio-pci, if that is the general idea).

Quoted 4 lines

I'm sure we can find alternatives for virtio-*, so I wouldn't expect it to be glued to virtio-pci.

Dave

Quoted 7 lines

As s390x does not have the concept of memory mapped io (RAM is RAM, nothing else), this is not architectured. vitio-ccw can therefore not define anything similar like that. However, in virtual environments we can do whatever we want on top of the pure transport (e.g. on the virtio layer).

Conny can correct me if I am wrong.

I don't think you're wrong, but I haven't read the code yet and I'm therefore not aware of the purpose of this BAR.

Generally, if there is a memory location shared between host and guest, we need a way to communicate its location, which will likely differ between transports. For ccw, I could imagine a new channel command dedicated to exchanging configuration information (similar to what exists today to communicate the locations of virtqueues), but I'd rather not go down this path.

Quoted 5 lines

Without reading the code/design further, can we use one of the following instead of a BAR:
- a virtqueue;
- something in config space?
That would be implementable by any virtio transport.

The way I think about this is that we wish to extend the VIRTIO device model with the concept of shared memory. virtio-fs, virtio-gpu, and virtio-vhost-user all have requirements for shared memory.

This seems like a transport-level issue to me. PCI supports memory-mapped I/O and that's the right place to do it. If you try to put it into config space or the virtqueue, you'll end up with something that cannot be realized as a PCI device because it bypasses PCI bus address translation.

If CCW needs a side-channel, that's fine. But that side-channel is a CCW-specific mechanism and probably doesn't apply to all other transports.

Stefan

Quoted 134 lines · Stefan Hajnoczi

Quoted 113 lines · Cornelia Huck

Quoted 92 lines · On Thu, 13 Dec 2018 13:24:31 +0100 David Hildenbrand <[email protected]> wrote:

Quoted 86 lines · Dr. David Alan Gilbert

Quoted 77 lines · * David Hildenbrand ([email protected]) wrote:

Quoted 67 lines · Dr. David Alan Gilbert

Quoted 59 lines · * David Hildenbrand ([email protected]) wrote:

Quoted 47 lines · Dr. David Alan Gilbert

Quoted 37 lines · * David Hildenbrand ([email protected]) wrote:

Quoted 36 lines · Vivek Goyal

Instead of assuming we had the fixed bar for the cache, use the value from the capabilities.

Signed-off-by Dr. David Alan Gilbert <[email protected]>

---
 fs/fuse/virtio_fs.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

fs/fuse/virtio_fs.c +1 -6

index 60d496c16841..55bac1465536 100644
@@ -14,11 +14,6 @@
 #include <uapi/linux/virtio_pci.h>
 #include "fuse_i.h"
 
-enum {
-	/* PCI BAR number of the virtio-fs DAX window */
-	VIRTIO_FS_WINDOW_BAR = 2,
-};
-
 /* List of virtio-fs device instances and a lock for the list */
 static DEFINE_MUTEX(virtio_fs_mutex);
 static LIST_HEAD(virtio_fs_instances);

@@ -518,7 +513,7 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
 	struct dev_pagemap *pgmap;
 	struct pci_dev *pci_dev;
 	phys_addr_t phys_addr;
-	size_t len;
+	size_t bar_len;
 	int ret;
 	u8 have_cache, cache_bar;
 	u64 cache_offset, cache_len;

@@ -551,17 +546,13 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
         }
 

Quoted 8 lines

fs/fuse/virtio_fs.c static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs) · line 548

Quoted 1 line

fs/fuse/virtio_fs.c +0 -0

 	/* TODO handle case where device doesn't expose BAR? */  

For virtio-pmem we decided to not go via BARs as this would effectively make it only usable for virtio-pci implementers. Instead, we are going to export the applicable physical device region directly (e.g. phys_start, phys_size in virtio config), so it is decoupled from PCI details. Doing the same for virtio-fs would allow e.g. also virtio-ccw to make eventually use of this.

Quoted 5 lines

That makes it a very odd looking PCI device; I can see that with virtio-pmem it makes some sense, given that it's job is to expose arbitrary chunks of memory.

Dave

Well, the fact that your are

- including <uapi/linux/virtio_pci.h>
- adding pci related code

Quoted 6 lines

in/to fs/fuse/virtio_fs.c

tells me that these properties might be better communicated on the virtio layer, not on the PCI layer.

Or do you really want to glue virtio-fs to virtio-pci for all eternity?

Quoted 8 lines

No, these need cleaning up; and the split within the bar
is probably going to change to be communicated via virtio layer rather than pci capabilities. However, I don't want to make our PCI device look odd, just to make portability to non-PCI devices - so it's right to make the split appropriately, but still to use PCI bars for what they were designed for.

Dave

Quoted 7 lines

Let's discuss after the cleanup. In general I am not convinced this is the right thing to do. Using virtio-pci for anything else than pure transport smells like bad design to me (well, I am no virtio expert after all ;) ). No matter what PCI bars were designed for. If we can't get the same running with e.g. virtio-ccw or virtio-whatever, it is broken by design (or an addon that is tightly glued to virtio-pci, if that is the general idea).

Quoted 4 lines

I'm sure we can find alternatives for virtio-*, so I wouldn't expect it to be glued to virtio-pci.

Dave

Quoted 7 lines

As s390x does not have the concept of memory mapped io (RAM is RAM, nothing else), this is not architectured. vitio-ccw can therefore not define anything similar like that. However, in virtual environments we can do whatever we want on top of the pure transport (e.g. on the virtio layer).

Conny can correct me if I am wrong.

I don't think you're wrong, but I haven't read the code yet and I'm therefore not aware of the purpose of this BAR.

Generally, if there is a memory location shared between host and guest, we need a way to communicate its location, which will likely differ between transports. For ccw, I could imagine a new channel command dedicated to exchanging configuration information (similar to what exists today to communicate the locations of virtqueues), but I'd rather not go down this path.

Quoted 5 lines

Without reading the code/design further, can we use one of the following instead of a BAR:
- a virtqueue;
- something in config space?
That would be implementable by any virtio transport.

The way I think about this is that we wish to extend the VIRTIO device model with the concept of shared memory. virtio-fs, virtio-gpu, and virtio-vhost-user all have requirements for shared memory.

This seems like a transport-level issue to me. PCI supports memory-mapped I/O and that's the right place to do it. If you try to put it into config space or the virtqueue, you'll end up with something that cannot be realized as a PCI device because it bypasses PCI bus address translation.

If CCW needs a side-channel, that's fine. But that side-channel is a CCW-specific mechanism and probably doesn't apply to all other transports.

Quoted 2 lines

Stefan

I think the problem is more fundamental. There is no iommu. Whatever shared region you want to indicate, you want it to be assigned a memory region in guest physical memory. Like a DIMM/NVDIMM. And this should be different to the concept of a BAR. Or am I missing something?

I am ok with using whatever other channel to transport such information. But I believe this is different to a typical BAR. (I wish I knew more about PCI internals ;) ).

I would also like to know how shared memory works as of now for e.g. virtio-gpu.

Quoted 46 lines · David Hildenbrand

Quoted 44 lines · Stefan Hajnoczi

Quoted 21 lines · Cornelia Huck

Quoted 7 lines · On Thu, 13 Dec 2018 13:24:31 +0100 David Hildenbrand <[email protected]> wrote:

As s390x does not have the concept of memory mapped io (RAM is RAM, nothing else), this is not architectured. vitio-ccw can therefore not define anything similar like that. However, in virtual environments we can do whatever we want on top of the pure transport (e.g. on the virtio layer).

Conny can correct me if I am wrong.

I don't think you're wrong, but I haven't read the code yet and I'm therefore not aware of the purpose of this BAR.

Generally, if there is a memory location shared between host and guest, we need a way to communicate its location, which will likely differ between transports. For ccw, I could imagine a new channel command dedicated to exchanging configuration information (similar to what exists today to communicate the locations of virtqueues), but I'd rather not go down this path.

Quoted 5 lines

Without reading the code/design further, can we use one of the following instead of a BAR:
- a virtqueue;
- something in config space?
That would be implementable by any virtio transport.

The way I think about this is that we wish to extend the VIRTIO device model with the concept of shared memory. virtio-fs, virtio-gpu, and virtio-vhost-user all have requirements for shared memory.

This seems like a transport-level issue to me. PCI supports memory-mapped I/O and that's the right place to do it. If you try to put it into config space or the virtqueue, you'll end up with something that cannot be realized as a PCI device because it bypasses PCI bus address translation.

If CCW needs a side-channel, that's fine. But that side-channel is a CCW-specific mechanism and probably doesn't apply to all other transports.

Stefan

Quoted 4 lines

I think the problem is more fundamental. There is no iommu. Whatever shared region you want to indicate, you want it to be assigned a memory region in guest physical memory. Like a DIMM/NVDIMM. And this should be different to the concept of a BAR. Or am I missing something?

If you implement a physical virtio PCI adapter then there is bus addressing and an IOMMU and VIRTIO has support for that. I'm not sure I understand what you mean by "there is no iommu"?

Quoted 7 lines

I am ok with using whatever other channel to transport such information. But I believe this is different to a typical BAR. (I wish I knew more about PCI internals ;) ).

I would also like to know how shared memory works as of now for e.g. virtio-gpu.

virtio-gpu currently does not use shared memory, it needs it for future features.

Quoted 47 lines · On Mon, 17 Dec 2018 14:56:38 +0000 Stefan Hajnoczi <[email protected]> wrote:

Quoted 46 lines · David Hildenbrand

Quoted 44 lines · Stefan Hajnoczi

Quoted 21 lines · Cornelia Huck

Quoted 7 lines · On Thu, 13 Dec 2018 13:24:31 +0100 David Hildenbrand <[email protected]> wrote:

As s390x does not have the concept of memory mapped io (RAM is RAM, nothing else), this is not architectured. vitio-ccw can therefore not define anything similar like that. However, in virtual environments we can do whatever we want on top of the pure transport (e.g. on the virtio layer).

Conny can correct me if I am wrong.

I don't think you're wrong, but I haven't read the code yet and I'm therefore not aware of the purpose of this BAR.

Generally, if there is a memory location shared between host and guest, we need a way to communicate its location, which will likely differ between transports. For ccw, I could imagine a new channel command dedicated to exchanging configuration information (similar to what exists today to communicate the locations of virtqueues), but I'd rather not go down this path.

Quoted 5 lines

Without reading the code/design further, can we use one of the following instead of a BAR:
- a virtqueue;
- something in config space?
That would be implementable by any virtio transport.

The way I think about this is that we wish to extend the VIRTIO device model with the concept of shared memory. virtio-fs, virtio-gpu, and virtio-vhost-user all have requirements for shared memory.

This seems like a transport-level issue to me. PCI supports memory-mapped I/O and that's the right place to do it. If you try to put it into config space or the virtqueue, you'll end up with something that cannot be realized as a PCI device because it bypasses PCI bus address translation.

If CCW needs a side-channel, that's fine. But that side-channel is a CCW-specific mechanism and probably doesn't apply to all other transports.

Stefan

Quoted 8 lines

Quoted 4 lines

I think the problem is more fundamental. There is no iommu. Whatever shared region you want to indicate, you want it to be assigned a memory region in guest physical memory. Like a DIMM/NVDIMM. And this should be different to the concept of a BAR. Or am I missing something?

If you implement a physical virtio PCI adapter then there is bus addressing and an IOMMU and VIRTIO has support for that. I'm not sure I understand what you mean by "there is no iommu"?

For ccw, there is no iommu; channel-program translation is doing similar things. (I hope that is what David meant :)

Quoted 10 lines

Quoted 7 lines

I am ok with using whatever other channel to transport such information. But I believe this is different to a typical BAR. (I wish I knew more about PCI internals ;) ).

I would also like to know how shared memory works as of now for e.g. virtio-gpu.

virtio-gpu currently does not use shared memory, it needs it for future features.

OK, that all sounds like we need to define a generic, per transport, device agnostic way to specify shared memory.

Where is that memory situated? Is it something in guest memory (like virtqueues)? If it is something provided by the device, things will get tricky for ccw (remember that there's no mmio on s390; pci on s390 uses special instructions for that.)

Quoted 73 lines · Cornelia Huck

Quoted 47 lines · On Mon, 17 Dec 2018 14:56:38 +0000 Stefan Hajnoczi <[email protected]> wrote:

Quoted 46 lines · David Hildenbrand

Quoted 44 lines · Stefan Hajnoczi

Quoted 21 lines · Cornelia Huck

Quoted 7 lines · On Thu, 13 Dec 2018 13:24:31 +0100 David Hildenbrand <[email protected]> wrote:

As s390x does not have the concept of memory mapped io (RAM is RAM, nothing else), this is not architectured. vitio-ccw can therefore not define anything similar like that. However, in virtual environments we can do whatever we want on top of the pure transport (e.g. on the virtio layer).

Conny can correct me if I am wrong.

I don't think you're wrong, but I haven't read the code yet and I'm therefore not aware of the purpose of this BAR.

Generally, if there is a memory location shared between host and guest, we need a way to communicate its location, which will likely differ between transports. For ccw, I could imagine a new channel command dedicated to exchanging configuration information (similar to what exists today to communicate the locations of virtqueues), but I'd rather not go down this path.

Quoted 5 lines

Without reading the code/design further, can we use one of the following instead of a BAR:
- a virtqueue;
- something in config space?
That would be implementable by any virtio transport.

The way I think about this is that we wish to extend the VIRTIO device model with the concept of shared memory. virtio-fs, virtio-gpu, and virtio-vhost-user all have requirements for shared memory.

This seems like a transport-level issue to me. PCI supports memory-mapped I/O and that's the right place to do it. If you try to put it into config space or the virtqueue, you'll end up with something that cannot be realized as a PCI device because it bypasses PCI bus address translation.

If CCW needs a side-channel, that's fine. But that side-channel is a CCW-specific mechanism and probably doesn't apply to all other transports.

Stefan

Quoted 8 lines

Quoted 4 lines

I think the problem is more fundamental. There is no iommu. Whatever shared region you want to indicate, you want it to be assigned a memory region in guest physical memory. Like a DIMM/NVDIMM. And this should be different to the concept of a BAR. Or am I missing something?

If you implement a physical virtio PCI adapter then there is bus addressing and an IOMMU and VIRTIO has support for that. I'm not sure I understand what you mean by "there is no iommu"?

For ccw, there is no iommu; channel-program translation is doing similar things. (I hope that is what David meant :)

Quoted 10 lines

Quoted 7 lines

I am ok with using whatever other channel to transport such information. But I believe this is different to a typical BAR. (I wish I knew more about PCI internals ;) ).

I would also like to know how shared memory works as of now for e.g. virtio-gpu.

virtio-gpu currently does not use shared memory, it needs it for future features.

Quoted 8 lines

OK, that all sounds like we need to define a generic, per transport, device agnostic way to specify shared memory.

Where is that memory situated? Is it something in guest memory (like virtqueues)? If it is something provided by the device, things will get tricky for ccw (remember that there's no mmio on s390; pci on s390 uses special instructions for that.)

I am just very very confused right now. What I am struggling with right now (Stefan, hope you can clarify it for me):

We need some place where this shared memory is located in the guest physical memory. On x86 - if I am not wrong - this BAR is placed into the reserved memory area between 3 and 4 GB.

There is no such thing on
s390x. Because we don't have IO via memory (yet). All we have is one or two KVM memory slots filled with all memory.

So what we will need on s390x is on the QEMU side such a reserved memory region where devices like virtio-fs can reserve a region for shared memory.

So it is something like a dimm/nvdimm except that it is smaller and not visible to the user directly (via memory backends).

Instead of assuming we had the fixed bar for the cache, use the value from the capabilities.

Signed-off-by Dr. David Alan Gilbert <[email protected]>

---
 fs/fuse/virtio_fs.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

fs/fuse/virtio_fs.c +13 -11

index 60d496c16841..55bac1465536 100644
@@ -14,11 +14,6 @@
 #include <uapi/linux/virtio_pci.h>
 #include "fuse_i.h"
 
-enum {
-       /* PCI BAR number of the virtio-fs DAX window */
-       VIRTIO_FS_WINDOW_BAR = 2,
-};
-
 /* List of virtio-fs device instances and a lock for the list */
 static DEFINE_MUTEX(virtio_fs_mutex);
 static LIST_HEAD(virtio_fs_instances);

@@ -518,7 +513,7 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
        struct dev_pagemap *pgmap;
        struct pci_dev *pci_dev;
        phys_addr_t phys_addr;
-       size_t len;
+       size_t bar_len;
        int ret;
        u8 have_cache, cache_bar;
        u64 cache_offset, cache_len;

@@ -551,17 +546,13 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
         }
 
        /* TODO handle case where device doesn't expose BAR? */
-       ret = pci_request_region(pci_dev, VIRTIO_FS_WINDOW_BAR,
-                                "virtio-fs-window");
+       ret = pci_request_region(pci_dev, cache_bar, "virtio-fs-window");
        if (ret < 0) {
                dev_err(&vdev->dev, "%s: failed to request window BAR\n",
                        __func__);
                return ret;
        }
 
-       phys_addr = pci_resource_start(pci_dev, VIRTIO_FS_WINDOW_BAR);
-       len = pci_resource_len(pci_dev, VIRTIO_FS_WINDOW_BAR);
-
        mi = devm_kzalloc(&pci_dev->dev, sizeof(*mi), GFP_KERNEL);
        if (!mi)
                return -ENOMEM;

@@ -586,6 +577,17 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
        pgmap->ref = &mi->ref;
        pgmap->type = MEMORY_DEVICE_FS_DAX;
 
+       phys_addr = pci_resource_start(pci_dev, cache_bar);
+       bar_len = pci_resource_len(pci_dev, cache_bar);
+
+       if (cache_offset + cache_len > bar_len) {
+               dev_err(&vdev->dev,
+                       "%s: cache bar shorter than cap offset+len\n",
+                       __func__);
+               return -EINVAL;
+       }
+       phys_addr += cache_offset;
+
        /* Ideally we would directly use the PCI BAR resource but
         * devm_memremap_pages() wants its own copy in pgmap.  So
         * initialize a struct resource from scratch (only the start

fs/fuse/virtio_fs.c +3 -4

        };
 
        fs->window_kaddr = devm_memremap_pages(&pci_dev->dev, pgmap);

@@ -607,10 +609,10 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
                return ret;
 
        fs->window_phys_addr = phys_addr;
-       fs->window_len = len;
+       fs->window_len = cache_len;
 
-       dev_dbg(&vdev->dev, "%s: window kaddr 0x%px phys_addr 0x%llx len %zu\n",
-               __func__, fs->window_kaddr, phys_addr, len);
+       dev_dbg(&vdev->dev, "%s: cache kaddr 0x%px phys_addr 0x%llx len %llx\n",
+               __func__, fs->window_kaddr, phys_addr, cache_len);
 
        fs->dax_dev = alloc_dax(fs, NULL, &virtio_fs_dax_ops);
        if (!fs->dax_dev)
--

2.13.6

Quoted 72 lines · Vivek Goyal

Instead of assuming we had the fixed bar for the cache, use the value from the capabilities.

Signed-off-by Dr. David Alan Gilbert <[email protected]>

---
 fs/fuse/virtio_fs.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

fs/fuse/virtio_fs.c +13 -11

index 60d496c16841..55bac1465536 100644
@@ -14,11 +14,6 @@
 #include <uapi/linux/virtio_pci.h>
 #include "fuse_i.h"
 
-enum {
-       /* PCI BAR number of the virtio-fs DAX window */
-       VIRTIO_FS_WINDOW_BAR = 2,
-};
-
 /* List of virtio-fs device instances and a lock for the list */
 static DEFINE_MUTEX(virtio_fs_mutex);
 static LIST_HEAD(virtio_fs_instances);

@@ -518,7 +513,7 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
        struct dev_pagemap *pgmap;
        struct pci_dev *pci_dev;
        phys_addr_t phys_addr;
-       size_t len;
+       size_t bar_len;
        int ret;
        u8 have_cache, cache_bar;
        u64 cache_offset, cache_len;

@@ -551,17 +546,13 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
         }
 
        /* TODO handle case where device doesn't expose BAR? */
-       ret = pci_request_region(pci_dev, VIRTIO_FS_WINDOW_BAR,
-                                "virtio-fs-window");
+       ret = pci_request_region(pci_dev, cache_bar, "virtio-fs-window");
        if (ret < 0) {
                dev_err(&vdev->dev, "%s: failed to request window BAR\n",
                        __func__);
                return ret;
        }
 
-       phys_addr = pci_resource_start(pci_dev, VIRTIO_FS_WINDOW_BAR);
-       len = pci_resource_len(pci_dev, VIRTIO_FS_WINDOW_BAR);
-
        mi = devm_kzalloc(&pci_dev->dev, sizeof(*mi), GFP_KERNEL);
        if (!mi)
                return -ENOMEM;

@@ -586,6 +577,17 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
        pgmap->ref = &mi->ref;
        pgmap->type = MEMORY_DEVICE_FS_DAX;
 
+       phys_addr = pci_resource_start(pci_dev, cache_bar);
+       bar_len = pci_resource_len(pci_dev, cache_bar);
+
+       if (cache_offset + cache_len > bar_len) {
+               dev_err(&vdev->dev,
+                       "%s: cache bar shorter than cap offset+len\n",
+                       __func__);
+               return -EINVAL;
+       }
+       phys_addr += cache_offset;
+
        /* Ideally we would directly use the PCI BAR resource but
         * devm_memremap_pages() wants its own copy in pgmap.  So
         * initialize a struct resource from scratch (only the start

fs/fuse/virtio_fs.c static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs) · line 599

Quoted 5 lines

fs/fuse/virtio_fs.c +1 -1

@@ -594,7 +596,7 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
        pgmap->res = (struct resource){
                .name = "virtio-fs dax window",
                .start = phys_addr,
-               .end = phys_addr + len,
+               .end = phys_addr + cache_len,

Just in case you haven't noticed/fixed this problem, it should be