Re: [dpdk-dev] [PATCH v5 10/11] virtio: pci: add dummy func definition for in/outb for non-x86 arch

[Santosh Shukla <sshukla@mvista.com>]

On Fri, Jan 29, 2016 at 12:31 PM, Yuanhan Liu
<yuanhan.liu@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@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