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

Yuanhan Liu yuanhan.liu at linux.intel.com
Fri Jan 29 10:13:06 CET 2016


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:
> > +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
> 
> > +
> > +	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 */


More information about the dev mailing list