Re: [Qemu-devel] [PATCH v9 12/12] docs: Sample driver to demonstrate how to use Mediated device framework.

2016-10-19 Thread Kirti Wankhede 


On 10/18/2016 10:47 PM, Alex Williamson wrote:
> On Tue, 18 Oct 2016 10:54:11 +0800
> Dong Jia Shi  wrote:
> 
>> * Kirti Wankhede  [2016-10-18 02:52:12 +0530]:
>>
>> ...snip...
>>
>>> +static ssize_t mdev_access(struct mdev_device *mdev, char *buf,
>>> +   size_t count, loff_t pos, bool is_write)
>>> +{
>>> +   struct mdev_state *mdev_state;
>>> +   unsigned int index;
>>> +   loff_t offset;
>>> +   int ret = 0;
>>> +
>>> +   if (!mdev || !buf)
>>> +   return -EINVAL;
>>> +
>>> +   mdev_state = mdev_get_drvdata(mdev);
>>> +   if (!mdev_state) {
>>> +   pr_err("%s mdev_state not found\n", __func__);
>>> +   return -EINVAL;
>>> +   }
>>> +
>>> +   mutex_lock(_state->ops_lock);
>>> +
>>> +   index = MTTY_VFIO_PCI_OFFSET_TO_INDEX(pos);
>>> +   offset = pos & MTTY_VFIO_PCI_OFFSET_MASK;
>>> +   switch (index) {
>>> +   case VFIO_PCI_CONFIG_REGION_INDEX:
>>> +
>>> +#if defined(DEBUG)
>>> +   pr_info("%s: PCI config space %s at offset 0x%llx\n",
>>> +__func__, is_write ? "write" : "read", offset);
>>> +#endif
>>> +   if (is_write) {
>>> +   dump_buffer(buf, count);
>>> +   handle_pci_cfg_write(mdev_state, offset, buf, count);
>>> +   } else {
>>> +   memcpy(buf, (mdev_state->vconfig + offset), count);
>>> +   dump_buffer(buf, count);  
>> Dear Kirti:
>>
>> Shouldn't we use copy_from_user instead of memcpy on @buf here? And I'm
>> wondering if dump_buffer could really work since it tries to dereference
>> a *__user* marked pointor.
> 
> I agree, the __user attribute is getting lost here and we're operating
> on user buffers as if they were kernel buffers.  That's a bug.  Thanks,
> 

Oh, yes. Thanks for catching that. While transiting all changes from v7
to v8 I missed to update these. I'll have this corrected.

Kirti.

Re: [Qemu-devel] [PATCH v9 12/12] docs: Sample driver to demonstrate how to use Mediated device framework.

2016-10-18 Thread Alex Williamson 
On Tue, 18 Oct 2016 10:54:11 +0800
Dong Jia Shi  wrote:

> * Kirti Wankhede  [2016-10-18 02:52:12 +0530]:
> 
> ...snip...
> 
> > +static ssize_t mdev_access(struct mdev_device *mdev, char *buf,
> > +   size_t count, loff_t pos, bool is_write)
> > +{
> > +   struct mdev_state *mdev_state;
> > +   unsigned int index;
> > +   loff_t offset;
> > +   int ret = 0;
> > +
> > +   if (!mdev || !buf)
> > +   return -EINVAL;
> > +
> > +   mdev_state = mdev_get_drvdata(mdev);
> > +   if (!mdev_state) {
> > +   pr_err("%s mdev_state not found\n", __func__);
> > +   return -EINVAL;
> > +   }
> > +
> > +   mutex_lock(_state->ops_lock);
> > +
> > +   index = MTTY_VFIO_PCI_OFFSET_TO_INDEX(pos);
> > +   offset = pos & MTTY_VFIO_PCI_OFFSET_MASK;
> > +   switch (index) {
> > +   case VFIO_PCI_CONFIG_REGION_INDEX:
> > +
> > +#if defined(DEBUG)
> > +   pr_info("%s: PCI config space %s at offset 0x%llx\n",
> > +__func__, is_write ? "write" : "read", offset);
> > +#endif
> > +   if (is_write) {
> > +   dump_buffer(buf, count);
> > +   handle_pci_cfg_write(mdev_state, offset, buf, count);
> > +   } else {
> > +   memcpy(buf, (mdev_state->vconfig + offset), count);
> > +   dump_buffer(buf, count);  
> Dear Kirti:
> 
> Shouldn't we use copy_from_user instead of memcpy on @buf here? And I'm
> wondering if dump_buffer could really work since it tries to dereference
> a *__user* marked pointor.

I agree, the __user attribute is getting lost here and we're operating
on user buffers as if they were kernel buffers.  That's a bug.  Thanks,

Alex
 
> Otherwise, this is a good example driver. Thanks!
> 
> > +   }
> > +
> > +   break;
> > +
> > +   case VFIO_PCI_BAR0_REGION_INDEX ... VFIO_PCI_BAR5_REGION_INDEX:
> > +   if (!mdev_state->region_info[index].start)
> > +   mdev_read_base(mdev_state);
> > +
> > +   if (is_write) {
> > +   dump_buffer(buf, count);
> > +
> > +#if defined(DEBUG_REGS)
> > +   pr_info("%s: BAR%d  WR @0x%llx %s val:0x%02x dlab:%d\n",
> > +   __func__, index, offset, wr_reg[offset],
> > +   (u8)*buf, mdev_state->s[index].dlab);
> > +#endif
> > +   handle_bar_write(index, mdev_state, offset, buf, count);
> > +   } else {
> > +   handle_bar_read(index, mdev_state, offset, buf, count);
> > +   dump_buffer(buf, count);
> > +
> > +#if defined(DEBUG_REGS)
> > +   pr_info("%s: BAR%d  RD @0x%llx %s val:0x%02x dlab:%d\n",
> > +   __func__, index, offset, rd_reg[offset],
> > +   (u8)*buf, mdev_state->s[index].dlab);
> > +#endif
> > +   }
> > +   break;
> > +
> > +   default:
> > +   ret = -1;
> > +   goto accessfailed;
> > +   }
> > +
> > +   ret = count;
> > +
> > +
> > +accessfailed:
> > +   mutex_unlock(_state->ops_lock);
> > +
> > +   return ret;
> > +}
> > +  
> ...snip...
> 
> > +ssize_t mtty_read(struct mdev_device *mdev, char __user *buf,
> > +   size_t count, loff_t *ppos)
> > +{
> > +   return mdev_access(mdev, buf, count, *ppos, false);
> > +}
> > +
> > +ssize_t mtty_write(struct mdev_device *mdev, const char __user *buf,
> > +size_t count, loff_t *ppos)
> > +{
> > +   return mdev_access(mdev, (char *)buf, count, *ppos, true);
> > +}
> > +  
> ...snip...
>

Re: [Qemu-devel] [PATCH v9 12/12] docs: Sample driver to demonstrate how to use Mediated device framework.

2016-10-17 Thread Dong Jia Shi 
* Kirti Wankhede  [2016-10-18 02:52:12 +0530]:

...snip...

> +static ssize_t mdev_access(struct mdev_device *mdev, char *buf,
> + size_t count, loff_t pos, bool is_write)
> +{
> + struct mdev_state *mdev_state;
> + unsigned int index;
> + loff_t offset;
> + int ret = 0;
> +
> + if (!mdev || !buf)
> + return -EINVAL;
> +
> + mdev_state = mdev_get_drvdata(mdev);
> + if (!mdev_state) {
> + pr_err("%s mdev_state not found\n", __func__);
> + return -EINVAL;
> + }
> +
> + mutex_lock(_state->ops_lock);
> +
> + index = MTTY_VFIO_PCI_OFFSET_TO_INDEX(pos);
> + offset = pos & MTTY_VFIO_PCI_OFFSET_MASK;
> + switch (index) {
> + case VFIO_PCI_CONFIG_REGION_INDEX:
> +
> +#if defined(DEBUG)
> + pr_info("%s: PCI config space %s at offset 0x%llx\n",
> +  __func__, is_write ? "write" : "read", offset);
> +#endif
> + if (is_write) {
> + dump_buffer(buf, count);
> + handle_pci_cfg_write(mdev_state, offset, buf, count);
> + } else {
> + memcpy(buf, (mdev_state->vconfig + offset), count);
> + dump_buffer(buf, count);
Dear Kirti:

Shouldn't we use copy_from_user instead of memcpy on @buf here? And I'm
wondering if dump_buffer could really work since it tries to dereference
a *__user* marked pointor.

Otherwise, this is a good example driver. Thanks!

> + }
> +
> + break;
> +
> + case VFIO_PCI_BAR0_REGION_INDEX ... VFIO_PCI_BAR5_REGION_INDEX:
> + if (!mdev_state->region_info[index].start)
> + mdev_read_base(mdev_state);
> +
> + if (is_write) {
> + dump_buffer(buf, count);
> +
> +#if defined(DEBUG_REGS)
> + pr_info("%s: BAR%d  WR @0x%llx %s val:0x%02x dlab:%d\n",
> + __func__, index, offset, wr_reg[offset],
> + (u8)*buf, mdev_state->s[index].dlab);
> +#endif
> + handle_bar_write(index, mdev_state, offset, buf, count);
> + } else {
> + handle_bar_read(index, mdev_state, offset, buf, count);
> + dump_buffer(buf, count);
> +
> +#if defined(DEBUG_REGS)
> + pr_info("%s: BAR%d  RD @0x%llx %s val:0x%02x dlab:%d\n",
> + __func__, index, offset, rd_reg[offset],
> + (u8)*buf, mdev_state->s[index].dlab);
> +#endif
> + }
> + break;
> +
> + default:
> + ret = -1;
> + goto accessfailed;
> + }
> +
> + ret = count;
> +
> +
> +accessfailed:
> + mutex_unlock(_state->ops_lock);
> +
> + return ret;
> +}
> +
...snip...

> +ssize_t mtty_read(struct mdev_device *mdev, char __user *buf,
> + size_t count, loff_t *ppos)
> +{
> + return mdev_access(mdev, buf, count, *ppos, false);
> +}
> +
> +ssize_t mtty_write(struct mdev_device *mdev, const char __user *buf,
> +  size_t count, loff_t *ppos)
> +{
> + return mdev_access(mdev, (char *)buf, count, *ppos, true);
> +}
> +
...snip...

-- 
Dong Jia

[Qemu-devel] [PATCH v9 12/12] docs: Sample driver to demonstrate how to use Mediated device framework.

2016-10-17 Thread Kirti Wankhede 
The Sample driver creates mdev device that simulates serial port over PCI
card.

Signed-off-by: Kirti Wankhede 
Signed-off-by: Neo Jia 
Change-Id: I857f8f12f8b275f2498dfe8c628a5cdc7193b1b2
---
 Documentation/vfio-mdev/Makefile |   13 +
 Documentation/vfio-mdev/mtty.c   | 1429 ++
 Documentation/vfio-mdev/vfio-mediated-device.txt |  104 +-
 3 files changed, 1544 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/vfio-mdev/Makefile
 create mode 100644 Documentation/vfio-mdev/mtty.c

diff --git a/Documentation/vfio-mdev/Makefile b/Documentation/vfio-mdev/Makefile
new file mode 100644
index ..a932edbe38eb
--- /dev/null
+++ b/Documentation/vfio-mdev/Makefile
@@ -0,0 +1,13 @@
+#
+# Makefile for mtty.c file
+#
+KERNEL_DIR:=/lib/modules/$(shell uname -r)/build
+
+obj-m:=mtty.o
+
+modules clean modules_install:
+   $(MAKE) -C $(KERNEL_DIR) SUBDIRS=$(PWD) $@
+
+default: modules
+
+module: modules
diff --git a/Documentation/vfio-mdev/mtty.c b/Documentation/vfio-mdev/mtty.c
new file mode 100644
index ..8ac321c4c8f1
--- /dev/null
+++ b/Documentation/vfio-mdev/mtty.c
@@ -0,0 +1,1429 @@
+/*
+ * Mediated virtual PCI serial host device driver
+ *
+ * Copyright (c) 2016, NVIDIA CORPORATION. All rights reserved.
+ * Author: Neo Jia 
+ * Kirti Wankhede 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Sample driver that creates mdev device that simulates serial port over PCI
+ * card.
+ *
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+/*
+ * #defines
+ */
+
+#define VERSION_STRING  "0.1"
+#define DRIVER_AUTHOR   "NVIDIA Corporation"
+
+#define MTTY_CLASS_NAME "mtty"
+
+#define MTTY_NAME   "mtty"
+
+#define MTTY_STRING_LEN16
+
+#define MTTY_CONFIG_SPACE_SIZE  0xff
+#define MTTY_IO_BAR_SIZE0x8
+#define MTTY_MMIO_BAR_SIZE  0x10
+
+#define STORE_LE16(addr, val)   (*(u16 *)addr = val)
+#define STORE_LE32(addr, val)   (*(u32 *)addr = val)
+
+#define MAX_FIFO_SIZE   16
+
+#define CIRCULAR_BUF_INC_IDX(idx)(idx = (idx + 1) & (MAX_FIFO_SIZE - 1))
+
+#define MTTY_VFIO_PCI_OFFSET_SHIFT   40
+
+#define MTTY_VFIO_PCI_OFFSET_TO_INDEX(off)   (off >> 
MTTY_VFIO_PCI_OFFSET_SHIFT)
+#define MTTY_VFIO_PCI_INDEX_TO_OFFSET(index) \
+   ((u64)(index) << MTTY_VFIO_PCI_OFFSET_SHIFT)
+#define MTTY_VFIO_PCI_OFFSET_MASK\
+   (((u64)(1) << MTTY_VFIO_PCI_OFFSET_SHIFT) - 1)
+#define MAX_MTTYS  24
+
+/*
+ * Global Structures
+ */
+
+struct mtty_dev {
+   dev_t   vd_devt;
+   struct class*vd_class;
+   struct cdev vd_cdev;
+   struct idr  vd_idr;
+   struct device   dev;
+} mtty_dev;
+
+struct mdev_region_info {
+   u64 start;
+   u64 phys_start;
+   u32 size;
+   u64 vfio_offset;
+};
+
+#if defined(DEBUG_REGS)
+const char *wr_reg[] = {
+   "TX",
+   "IER",
+   "FCR",
+   "LCR",
+   "MCR",
+   "LSR",
+   "MSR",
+   "SCR"
+};
+
+const char *rd_reg[] = {
+   "RX",
+   "IER",
+   "IIR",
+   "LCR",
+   "MCR",
+   "LSR",
+   "MSR",
+   "SCR"
+};
+#endif
+
+/* loop back buffer */
+struct rxtx {
+   u8 fifo[MAX_FIFO_SIZE];
+   u8 head, tail;
+   u8 count;
+};
+
+struct serial_port {
+   u8 uart_reg[8]; /* 8 registers */
+   struct rxtx rxtx;   /* loop back buffer */
+   bool dlab;
+   bool overrun;
+   u16 divisor;
+   u8 fcr; /* FIFO control register */
+   u8 max_fifo_size;
+   u8 intr_trigger_level;  /* interrupt trigger level */
+};
+
+/* State of each mdev device */
+struct mdev_state {
+   int irq_fd;
+   struct file *intx_file;
+   struct file *msi_file;
+   int irq_index;
+   u8 *vconfig;
+   struct mutex ops_lock;
+   struct mdev_device *mdev;
+   struct mdev_region_info region_info[VFIO_PCI_NUM_REGIONS];
+   u32 bar_mask[VFIO_PCI_NUM_REGIONS];
+   struct list_head next;
+   struct serial_port s[2];
+   struct mutex rxtx_lock;
+   struct vfio_device_info dev_info;
+   int nr_ports;
+};
+
+struct mutex mdev_list_lock;
+struct list_head mdev_devices_list;
+
+static const struct file_operations vd_fops = {
+   .owner  = THIS_MODULE,
+};
+
+/* function prototypes */
+
+static int mtty_trigger_interrupt(uuid_le uuid);
+
+/* Helper functions */
+static struct mdev_state *find_mdev_state_by_uuid(uuid_le uuid)
+{
+   struct mdev_state *mds;
+
+   list_for_each_entry(mds, _devices_list,