[dpdk-dev,v6,6/8] virtio: add vfio api to rd/wr ioport space
Commit Message
For vfio case - Use pread/pwrite api to access virtio
ioport space.
Signed-off-by: Santosh Shukla <sshukla@mvista.com>
Signed-off-by: Rizwan Ansari <ransari@mvista.com>
Signed-off-by: Rakesh Krishnamurthy <rakeshk@mvista.com>
---
v5-->v6:
- renamed inport_in/out to vfio_in/out
- Renamed file from virtio_vfio_rw.h to virtio_vfio_io.h
drivers/net/virtio/virtio_vfio_io.h | 104 +++++++++++++++++++++++++++++++++++
1 file changed, 104 insertions(+)
create mode 100644 drivers/net/virtio/virtio_vfio_io.h
Comments
On Fri, Jan 29, 2016 at 11:51:55PM +0530, Santosh Shukla wrote:
> For vfio case - Use pread/pwrite api to access virtio
> ioport space.
>
> Signed-off-by: Santosh Shukla <sshukla@mvista.com>
> Signed-off-by: Rizwan Ansari <ransari@mvista.com>
> Signed-off-by: Rakesh Krishnamurthy <rakeshk@mvista.com>
> ---
> v5-->v6:
> - renamed inport_in/out to vfio_in/out
> - Renamed file from virtio_vfio_rw.h to virtio_vfio_io.h
>
> drivers/net/virtio/virtio_vfio_io.h | 104 +++++++++++++++++++++++++++++++++++
> 1 file changed, 104 insertions(+)
> create mode 100644 drivers/net/virtio/virtio_vfio_io.h
>
> diff --git a/drivers/net/virtio/virtio_vfio_io.h b/drivers/net/virtio/virtio_vfio_io.h
> new file mode 100644
> index 0000000..218d4ed
> --- /dev/null
> +++ b/drivers/net/virtio/virtio_vfio_io.h
...
> @@ -0,0 +1,104 @@
> +#ifndef _VIRTIO_VFIO_IO_H_
> +#define _VIRTIO_VFIO_IO_H_
> +
> +#include "virtio_logs.h"
> +#if defined(RTE_EAL_VFIO) && defined(RTE_LIBRTE_EAL_LINUXAPP)
Won't it cause build failure if above "#if ..." is false, as
virtio_read/write_reg_x() reference them unconditionally?
BTW, why above check is needed? We have rte_eal_pci_read/write_bar()
implementation with both VFIO and BSD, don't we?
> +#endif /* _VIRTIO_VFIO_RW_H_ */
^^^^
You forgot to do rename here.
BTW, I didn't follow the noIOMMU discussion; how did it end? Do we still
need that? Is this patch a full story to enable virtio on ARM?
--yliu
On Mon, Feb 1, 2016 at 6:18 PM, Yuanhan Liu <yuanhan.liu@linux.intel.com> wrote:
> On Fri, Jan 29, 2016 at 11:51:55PM +0530, Santosh Shukla wrote:
>> For vfio case - Use pread/pwrite api to access virtio
>> ioport space.
>>
>> Signed-off-by: Santosh Shukla <sshukla@mvista.com>
>> Signed-off-by: Rizwan Ansari <ransari@mvista.com>
>> Signed-off-by: Rakesh Krishnamurthy <rakeshk@mvista.com>
>> ---
>> v5-->v6:
>> - renamed inport_in/out to vfio_in/out
>> - Renamed file from virtio_vfio_rw.h to virtio_vfio_io.h
>>
>> drivers/net/virtio/virtio_vfio_io.h | 104 +++++++++++++++++++++++++++++++++++
>> 1 file changed, 104 insertions(+)
>> create mode 100644 drivers/net/virtio/virtio_vfio_io.h
>>
>> diff --git a/drivers/net/virtio/virtio_vfio_io.h b/drivers/net/virtio/virtio_vfio_io.h
>> new file mode 100644
>> index 0000000..218d4ed
>> --- /dev/null
>> +++ b/drivers/net/virtio/virtio_vfio_io.h
> ...
>> @@ -0,0 +1,104 @@
>> +#ifndef _VIRTIO_VFIO_IO_H_
>> +#define _VIRTIO_VFIO_IO_H_
>> +
>> +#include "virtio_logs.h"
>> +#if defined(RTE_EAL_VFIO) && defined(RTE_LIBRTE_EAL_LINUXAPP)
>
Yes, Now that we have pci_eal_read/write_bar() dummy implementation
for freebsd so we don't need both checks. I overlooked it, sending v7
for this patch now. Thanks.
> Won't it cause build failure if above "#if ..." is false, as
> virtio_read/write_reg_x() reference them unconditionally?
>
> BTW, why above check is needed? We have rte_eal_pci_read/write_bar()
> implementation with both VFIO and BSD, don't we?
>
>
>> +#endif /* _VIRTIO_VFIO_RW_H_ */
> ^^^^
> You forgot to do rename here.
>
My bad (:-
>
> BTW, I didn't follow the noIOMMU discussion; how did it end? Do we still
> need that? Is this patch a full story to enable virtio on ARM?
>
Ok, We agreed that explicit __noiommu suffix not required, atleast for
rte_xx_drv struct{}, as because sooner than later we'll have virtio
working for both flavours ie... iommu/noiommu. My only worry was
parsing for _noiommu and default vfio case, as because noiomu needed
user to preset "enable_noiommu_" param for vfio driver to do mode
switch. But we wont need that parsing as because if param is not set
then binding won't happen, which Thomas rightly pointed out, therefore
I choose to drop resource parsing for virtio-for-vfio case, now virtio
driver to check only drv->kdrv == RTE_KDRV_VFIO so to make sure
interface attached to vfio or not.
But perhaps when we have both flavours working for virtio, then we
should at least prompt a INFO message on console that virtio pmd
driver attached to default vfio or noIOMMU.
So we don't need explicit _noIOMMU.
Yes this patch is to enable non-x86 arch to use virtio pmd driver
(virtio 0.95 spec). After this patch merges-in, I am planning to
- replace sys/io.h entirely
- Add raw_read/raw_writel() api for arm/arm64 {Already proposed
similar implementation in v2} so that they could use virtio 1.0spec
mapped memory, for both UIO/VFIO mode.
On Tue, Feb 02, 2016 at 10:00:36AM +0530, Santosh Shukla wrote:
> >
> > BTW, I didn't follow the noIOMMU discussion; how did it end? Do we still
> > need that? Is this patch a full story to enable virtio on ARM?
> >
> Ok, We agreed that explicit __noiommu suffix not required, atleast for
> rte_xx_drv struct{}, as because sooner than later we'll have virtio
> working for both flavours ie... iommu/noiommu. My only worry was
> parsing for _noiommu and default vfio case, as because noiomu needed
> user to preset "enable_noiommu_" param for vfio driver to do mode
> switch. But we wont need that parsing as because if param is not set
> then binding won't happen, which Thomas rightly pointed out, therefore
> I choose to drop resource parsing for virtio-for-vfio case, now virtio
> driver to check only drv->kdrv == RTE_KDRV_VFIO so to make sure
> interface attached to vfio or not.
>
> But perhaps when we have both flavours working for virtio, then we
> should at least prompt a INFO message on console that virtio pmd
> driver attached to default vfio or noIOMMU.
>
> So we don't need explicit _noIOMMU.
Thanks for the explanation.
>
> Yes this patch is to enable non-x86 arch to use virtio pmd driver
> (virtio 0.95 spec). After this patch merges-in, I am planning to
> - replace sys/io.h entirely
Hmm, be more specific? Replace it with what?
> - Add raw_read/raw_writel() api for arm/arm64 {Already proposed
> similar implementation in v2} so that they could use virtio 1.0spec
> mapped memory, for both UIO/VFIO mode.
PCI memory bar mapping works both with UIO/VFIO on ARM, without
any extra efforts, right? If so, it should just work with my
patch set and yours.
--yliu
On Tue, Feb 2, 2016 at 10:49 AM, Yuanhan Liu
<yuanhan.liu@linux.intel.com> wrote:
> On Tue, Feb 02, 2016 at 10:00:36AM +0530, Santosh Shukla wrote:
>> >
>> > BTW, I didn't follow the noIOMMU discussion; how did it end? Do we still
>> > need that? Is this patch a full story to enable virtio on ARM?
>> >
>> Ok, We agreed that explicit __noiommu suffix not required, atleast for
>> rte_xx_drv struct{}, as because sooner than later we'll have virtio
>> working for both flavours ie... iommu/noiommu. My only worry was
>> parsing for _noiommu and default vfio case, as because noiomu needed
>> user to preset "enable_noiommu_" param for vfio driver to do mode
>> switch. But we wont need that parsing as because if param is not set
>> then binding won't happen, which Thomas rightly pointed out, therefore
>> I choose to drop resource parsing for virtio-for-vfio case, now virtio
>> driver to check only drv->kdrv == RTE_KDRV_VFIO so to make sure
>> interface attached to vfio or not.
>>
>> But perhaps when we have both flavours working for virtio, then we
>> should at least prompt a INFO message on console that virtio pmd
>> driver attached to default vfio or noIOMMU.
>>
>> So we don't need explicit _noIOMMU.
>
> Thanks for the explanation.
>>
>> Yes this patch is to enable non-x86 arch to use virtio pmd driver
>> (virtio 0.95 spec). After this patch merges-in, I am planning to
>> - replace sys/io.h entirely
>
> Hmm, be more specific? Replace it with what?
>
Just to remove ifdef clutter in general from dpdk source and mo
>> - Add raw_read/raw_writel() api for arm/arm64 {Already proposed
>> similar implementation in v2} so that they could use virtio 1.0spec
>> mapped memory, for both UIO/VFIO mode.
>
> PCI memory bar mapping works both with UIO/VFIO on ARM, without
> any extra efforts, right? If so, it should just work with my
> patch set and yours.
>
Mostl likely, That I have not tried. (todo),
> --yliu
new file mode 100644
@@ -0,0 +1,104 @@
+/*
+ * BSD LICENSE
+ *
+ * Copyright(c) 2016 Cavium Networks. All rights reserved.
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ * * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ * * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in
+ * the documentation and/or other materials provided with the
+ * distribution.
+ * * Neither the name of Intel Corporation nor the names of its
+ * contributors may be used to endorse or promote products derived
+ * from this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ *
+ */
+#ifndef _VIRTIO_VFIO_IO_H_
+#define _VIRTIO_VFIO_IO_H_
+
+#include "virtio_logs.h"
+#if defined(RTE_EAL_VFIO) && defined(RTE_LIBRTE_EAL_LINUXAPP)
+
+#include <stdint.h>
+#include <rte_pci.h>
+
+/* vfio rd/rw virtio apis */
+static inline void
+vfio_inb(const struct rte_pci_device *pci_dev, uint8_t reg, uint8_t *val)
+{
+ if (rte_eal_pci_read_bar(pci_dev, val, sizeof(uint8_t), reg, 0) <= 0) {
+ PMD_DRV_LOG(ERR, "Can't read from PCI bar space");
+ return;
+ }
+}
+
+static inline void
+vfio_inw(const struct rte_pci_device *pci_dev, uint16_t reg, uint16_t *val)
+{
+ if (rte_eal_pci_read_bar(pci_dev, val, sizeof(uint16_t), reg, 0) <= 0) {
+ PMD_DRV_LOG(ERR, "Can't read from PCI bar space");
+ return;
+ }
+}
+
+static inline void
+vfio_inl(const struct rte_pci_device *pci_dev, uint32_t reg, uint32_t *val)
+{
+ if (rte_eal_pci_read_bar(pci_dev, val, sizeof(uint32_t), reg, 0) <= 0) {
+ PMD_DRV_LOG(ERR, "Can't read from PCI bar space");
+ return;
+ }
+}
+
+static inline void
+vfio_outb_p(const struct rte_pci_device *pci_dev, uint8_t reg, uint8_t val)
+{
+ if (rte_eal_pci_write_bar(pci_dev, &val, sizeof(uint8_t),
+ reg, 0) <= 0) {
+ PMD_DRV_LOG(ERR, "Can't write to PCI bar space");
+ return;
+ }
+}
+
+
+static inline void
+vfio_outw_p(const struct rte_pci_device *pci_dev, uint16_t reg, uint16_t val)
+{
+ if (rte_eal_pci_write_bar(pci_dev, &val, sizeof(uint16_t),
+ reg, 0) <= 0) {
+ PMD_DRV_LOG(ERR, "Can't write to PCI bar space");
+ return;
+ }
+}
+
+
+static inline void
+vfio_outl_p(const struct rte_pci_device *pci_dev, uint32_t reg, uint32_t val)
+{
+ if (rte_eal_pci_write_bar(pci_dev, &val, sizeof(uint32_t),
+ reg, 0) <= 0) {
+ PMD_DRV_LOG(ERR, "Can't write to PCI bar space");
+ return;
+ }
+}
+
+#endif /* RTE_EAL_VFIO && RTE_XX_EAL_LINUXAPP */
+#endif /* _VIRTIO_VFIO_RW_H_ */