Re: [RFC PATCH 5/5] virtio: Extend virtio-net PMD to support container environment

On Fri, Jan 29, 2016 at 04:57:23PM +0800, Yuanhan Liu wrote:

On Thu, Jan 21, 2016 at 08:07:58PM +0900, Tetsuya Mukawa wrote:

quoted

+static int
+virt_read_pci_cfg(struct virtio_hw *hw, void *buf, size_t len, off_t offset)
+{
+	qtest_read_pci_cfg(hw, "virtio-net", buf, len, offset);
+	return 0;
+}
+
+static void *
+virt_get_mapped_addr(struct virtio_hw *hw, uint8_t bar,
+		     uint32_t offset, uint32_t length)
+{
+	uint64_t base;
+	uint64_t size;
+
+	if (qtest_get_bar_size(hw, "virtio-net", bar, &size) < 0) {
+		PMD_INIT_LOG(ERR, "invalid bar: %u", bar);
+		return NULL;
+	}
+
+	if (offset + length < offset) {
+		PMD_INIT_LOG(ERR, "offset(%u) + lenght(%u) overflows",
+			offset, length);
+		return NULL;
+	}
+
+	if (offset + length > size) {
+		PMD_INIT_LOG(ERR,
+			"invalid cap: overflows bar space: %u > %"PRIu64,
+			offset + length, size);
+		return NULL;
+	}
+
+	if (qtest_get_bar_addr(hw, "virtio-net", bar, &base) < 0) {
+		PMD_INIT_LOG(ERR, "invalid bar: %u", bar);
+		return NULL;
+	}

So, I understood the usage now, and the cfg_ops abstraction doesn't look
good yet necessary to me.  For EAL managed pci device, bar length and
addr are stored at memory_resources[], and for your case, it's from the
qtest. And judging that it's compile time decision, I'd like it to be:

    #ifdef /* RTE_LIBRTE_VIRTIO_HOST_MODE */

Oops, sorry, I was wrong. Your code could be co-exist with the
traditional virtio pmd driver, thus we can't do that.

But still, I think dynamic "if ... else ..." should be better:
there are just few places (maybe 4: bar_size, bar length, map
device, read config) need that.


On the other hand, if you really want to do that abstraction,
you should go it with more fine granularity, such as the following
methods I proposed, instead of the big one: get_cfg_addr(). In
that way, we could avoid duplicate code.

	--yliu

    
    static uint32_t
    get_bar_size(...)
    {
    	return qtest_get_bar_size(..);
    }
    
    static uint64-t
    get_bar_addr(...)
    {
    	return qtest_get_bar_addr(..);
    }
    
    ...
    ...
    
    #else
    
    static  uint32_t
    get_bar_size(...)
    {
    	return dev->mem_resource[bar].addr;
    }
    
    ...
    
    }
    #endif


And then you just need do related changes at virtio_read_caps() and
get_cfg_addr(). That'd be much simpler, without introducing duplicate
code and uncessary complex.

What do you think of that?

	--yliu

quoted

+
+	return (void *)(base + offset);
+}
+
+static const struct virtio_pci_cfg_ops virt_cfg_ops = {
+	.map			= virt_map_pci_cfg,
+	.unmap			= virt_unmap_pci_cfg,
+	.get_mapped_addr	= virt_get_mapped_addr,
+	.read			= virt_read_pci_cfg,
+};
+#endif /* RTE_LIBRTE_VIRTIO_HOST_MODE */