On Fri, Jan 29, 2016 at 12:31 PM, Yuanhan Liu
<yuanhan.liu at linux.intel.com> wrote:
> On Tue, Jan 19, 2016 at 05:16:11PM +0530, Santosh Shukla wrote:
>> For non-x86 arch, Compiler will throw build error for in/out apis. Including
>> dummy api function so to pass build.
>>
>> Note that: For virtio to work for non-x86 arch - RTE_EAL_VFIO is the only
>> supported method. RTE_EAL_IGB_UIO is not supported for non-x86 arch.
>>
>> So, Virtio support for arch and supported interface by that arch:
>>
>> ARCH IGB_UIO VFIO
>> x86 Y Y
>> ARM64 N/A Y
>> PPC_64 N/A Y (Not tested but likely should work, as
>> vfio is
>> arch independent)
>>
>> Note: Applicable for virtio spec 0.95
>>
>> Signed-off-by: Santosh Shukla <sshukla at mvista.com>
>> ---
>> drivers/net/virtio/virtio_pci.h | 46
>> +++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 46 insertions(+)
>>
>> diff --git a/drivers/net/virtio/virtio_pci.h
>> b/drivers/net/virtio/virtio_pci.h
>> index f550d22..b88f9ec 100644
>> --- a/drivers/net/virtio/virtio_pci.h
>> +++ b/drivers/net/virtio/virtio_pci.h
>> @@ -46,6 +46,7 @@
>> #endif
>>
>> #include <rte_ethdev.h>
>> +#include "virtio_logs.h"
>>
>> struct virtqueue;
>>
>> @@ -320,6 +321,51 @@ outl_p(unsigned int data, unsigned int port)
>> }
>> #endif
>>
>> +#if !defined(RTE_ARCH_X86_64) && !defined(RTE_ARCH_I686) && \
>> + defined(RTE_EXEC_ENV_LINUXAPP)
>> +static inline uint8_t
>> +inb(unsigned long addr __rte_unused)
>> +{
>> + PMD_INIT_LOG(ERR, "inb() not supported for this RTE_ARCH\n");
>> + return 0;
>> +}
>
> The whole port read/write stuff is getting messy here: we not only have
> to care the FreeBSD and Linux difference, but also the x86 and non-x86
> difference. And you just added yet another vfio layer.
>
> First of all, they are not belong here (virtio_pci.h). A new place like
> virtio_io.h sounds much better to me. Therefore, I'd suggest you to
> put all those stuff there, like the one I have just cooked up:
>
>
> #ifndef __VIRTIO_IO_H__
>
> #if defined(RTE_ARCH_X86_64) || defined(RTE_ARCH_I686)
>
> #ifdef __FreeBSD__
> #include <sys/types.h>
> #include <machine/cpufunc.h>
>
> #define outb_p outb
> #define outw_p outw
> #define outl_p outl
> #else
> #include <sys/io.h>
> #endif
>
> #else /* ! X86 */
>
> static inline uint8_t
> inb(unsigned long addr __rte_unused)
> {
> PMD_INIT_LOG(ERR, "inb() not supported for this RTE_ARCH\n");
> return 0;
> }
>
>
> ....
> #endif /* X86 */
>
>
> /* And put the vfio io port read/write here */
>
> #endif /* __VIRTIO_IO_H__ */
>
> Note that you may need squash patch 4 (build fix for sys/io.h ...)
> here. They both resolve one thing: to make it build on non-x86 platforms.
>
> Another minor note is that while you are trying this way, I'd suggest
> you to make a patch to introduce virtio_io.h, and then make another
> patch to fix build errors on non-x86 platforms.
>
Ok.
> Another generic comment about this patchset is that it VERY okay to
> include several components change in one set, but putting them in
> order helps review a lot.
>
> Say, this patch set has dependence on VFIO stuff, therefore, it'd be
> much better __IF__ you can put all VFIO related patches first, and
> then virtio related patches follows, but not in an interleaved way
> you did. If, for somereason, you can't do that, you should at least
> try to minimise the chance of interleave.
>
I agree that, but this patch series dependent on other patches
including virtio 1.0 and then vfio-noiommu, its was difficult for me
to keep topic-wise sanity in patch series.
V6 will take care patch ordering. Thanks
> --yliu
