From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <yuanhan.liu@linux.intel.com>
Received: from mga09.intel.com (mga09.intel.com [134.134.136.24])
 by dpdk.org (Postfix) with ESMTP id 000ECC5E6
 for <dev@dpdk.org>; Fri, 29 Jan 2016 10:12:19 +0100 (CET)
Received: from orsmga001.jf.intel.com ([10.7.209.18])
 by orsmga102.jf.intel.com with ESMTP; 29 Jan 2016 01:12:18 -0800
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.22,363,1449561600"; d="scan'208";a="871553305"
Received: from yliu-dev.sh.intel.com (HELO yliu-dev) ([10.239.66.49])
 by orsmga001.jf.intel.com with ESMTP; 29 Jan 2016 01:12:18 -0800
Date: Fri, 29 Jan 2016 17:13:06 +0800
From: Yuanhan Liu <yuanhan.liu@linux.intel.com>
To: Tetsuya Mukawa <mukawa@igel.co.jp>
Message-ID: <20160129091306.GW4257@yliu-dev.sh.intel.com>
References: <1453108389-21006-2-git-send-email-mukawa@igel.co.jp>
 <1453374478-30996-6-git-send-email-mukawa@igel.co.jp>
 <20160129085723.GV4257@yliu-dev.sh.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20160129085723.GV4257@yliu-dev.sh.intel.com>
User-Agent: Mutt/1.5.23 (2014-03-12)
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [RFC PATCH 5/5] virtio: Extend virtio-net PMD to
 support container environment
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: patches and discussions about DPDK <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Fri, 29 Jan 2016 09:12:20 -0000

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