Skip to contentSkip to conversation

qemu educational device

PATCH In review Jiri Slaby 3 participants 15 messages Oct 10, 2014 – Dec 5, 2014

lore
Share & tools
lore

Jiri Slaby

author
#0 lore raw

PATCH 1/1 qemu educational device

Hello guys,

I am using qemu for teaching the Linux kernel at our university. I wrote a simple PCI device that can answer to writes/reads, generate interrupts and perform DMA. As I am dragging it locally over 2 years, would you be interested in including it upstream?

thanks,

Signature · 2 lines
js
suse labs
---
 hw/pci-host/Makefile.objs |    1 
 hw/pci-host/edu.c         |  336 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 337 insertions(+)
hw/pci-host/Makefile.objs +1 -0
@@ -13,5 +13,6 @@ common-obj-$(CONFIG_VERSATILE_PCI) += ve
 
 common-obj-$(CONFIG_PCI_APB) += apb.o
 common-obj-$(CONFIG_FULONG) += bonito.o
+common-obj-$(CONFIG_PCI) += edu.o
 common-obj-$(CONFIG_PCI_PIIX) += piix.o
 common-obj-$(CONFIG_PCI_Q35) += q35.o
hw/pci-host/edu.c new file +336 -0

336 lines not on this page · raw message

#1 lore raw

Il 10/10/2014 13:37, Jiri Slaby ha scritto:

Quoted 8 lines

Hello guys,

I am using qemu for teaching the Linux kernel at our university. I wrote a simple PCI device that can answer to writes/reads, generate interrupts and perform DMA. As I am dragging it locally over 2 years, would you be interested in including it upstream?

thanks,

Just send it our way! :)

Paolo

author
#2 lore raw

PATCH 1/1 pci-host: add educational driver

I am using qemu for teaching the Linux kernel at our university. I wrote a simple PCI device that can answer to writes/reads, generate interrupts and perform DMA. As I am dragging it locally over 2 years, I am sending it to you now.

Signed-off-by Jiri Slaby <[email protected]>
---
 MAINTAINERS               |   5 +
 hw/pci-host/Makefile.objs |   1 +
 hw/pci-host/edu.c         | 336 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 342 insertions(+)
 create mode 100644 hw/pci-host/edu.c
MAINTAINERS +5 -0
index 206bf7ea4582..7f4e8591b74b 100644
@@ -567,6 +567,11 @@ F: hw/xtensa/xtensa_lx60.c
 
 Devices
 -------
+EDU
+M: Jiri Slaby <[email protected]>
+S: Maintained
+F: hw/pci-host/edu.c
+
 IDE
 M: Kevin Wolf <[email protected]>
 M: Stefan Hajnoczi <[email protected]>
hw/pci-host/Makefile.objs +1 -0
index bb65f9c4d2d0..b01f614ed248 100644
@@ -13,5 +13,6 @@ common-obj-$(CONFIG_VERSATILE_PCI) += versatile.o
 
 common-obj-$(CONFIG_PCI_APB) += apb.o
 common-obj-$(CONFIG_FULONG) += bonito.o
+common-obj-$(CONFIG_PCI) += edu.o
 common-obj-$(CONFIG_PCI_PIIX) += piix.o
 common-obj-$(CONFIG_PCI_Q35) += q35.o
hw/pci-host/edu.c new file +336 -0

336 lines not on this page · raw message

Signature · 1 line
2.1.1
#3 lore raw

Re: [Qemu-devel] [PATCH 1/1] pci-host: add educational driver

Hello,

Quoted 6 lines · Jiri Slaby

I am using qemu for teaching the Linux kernel at our university. I wrote a simple PCI device that can answer to writes/reads, generate interrupts and perform DMA. As I am dragging it locally over 2 years, I am sending it to you now.

Signed-off-by Jiri Slaby <[email protected]>

is this supposed to be architecture independent, or is it X86-specific? Also at first glance I see multiple 32bit variables used to hold addresses, is this 32bit-only?

I wonder if this work could be merged / integrated with the Generic PCI host patches that are flying around since some time...

Thanks,

Claudio

Quoted 377 lines
---
 MAINTAINERS               |   5 +
 hw/pci-host/Makefile.objs |   1 +
 hw/pci-host/edu.c         | 336 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 342 insertions(+)
 create mode 100644 hw/pci-host/edu.c
MAINTAINERS +5 -0
index 206bf7ea4582..7f4e8591b74b 100644
@@ -567,6 +567,11 @@ F: hw/xtensa/xtensa_lx60.c
 
 Devices
 -------
+EDU
+M: Jiri Slaby <[email protected]>
+S: Maintained
+F: hw/pci-host/edu.c
+
 IDE
 M: Kevin Wolf <[email protected]>
 M: Stefan Hajnoczi <[email protected]>
hw/pci-host/Makefile.objs +1 -0
index bb65f9c4d2d0..b01f614ed248 100644
@@ -13,5 +13,6 @@ common-obj-$(CONFIG_VERSATILE_PCI) += versatile.o
 
 common-obj-$(CONFIG_PCI_APB) += apb.o
 common-obj-$(CONFIG_FULONG) += bonito.o
+common-obj-$(CONFIG_PCI) += edu.o
 common-obj-$(CONFIG_PCI_PIIX) += piix.o
 common-obj-$(CONFIG_PCI_Q35) += q35.o
hw/pci-host/edu.c new file +336 -0
new file mode 100644
index 000000000000..72e09dff6f5d
@@ -0,0 +1,336 @@
+/*
+ * QEMU education PCI device
+ *
+ * Copyright (c) 2012 Jiri Slaby
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ */
+
+#include "hw/pci/pci.h"
+#include "qemu/timer.h"
+
+#define DMA_START	0x40000
+#define DMA_SIZE	4096
+#define DMA_IRQ		0x00000100
+
+typedef struct {
+    PCIDevice pdev;
+    MemoryRegion mmio;
+
+    QemuThread thread;
+    QemuMutex thr_mutex;
+    QemuCond thr_cond;
+    bool stopping;
+
+    uint32_t addr4;
+    uint32_t fact;
+#define EDU_STATUS_COMPUTING	0x1
+    uint32_t status;
+
+    uint32_t irq_status;
+    QemuMutex irq_mutex;
+
+#define EDU_DMA_RUN		0x1
+#define EDU_DMA_DIR(cmd)	(((cmd) & 0x2) >> 1)
+# define EDU_DMA_FROM_PCI	0
+# define EDU_DMA_TO_PCI		1
+#define EDU_DMA_IRQ		0x4
+    struct dma_state {
+	    uint32_t src;
+	    uint32_t dst;
+	    uint32_t cnt;
+	    uint32_t cmd;
+    } dma;
+    QEMUTimer *dma_timer;
+    QemuMutex dma_mutex;
+    char dma_buf[DMA_SIZE];
+} EduState;
+
+static bool within(uint32_t addr, uint32_t start, uint32_t end)
+{
+	return start <= addr && addr < end;
+}
+
+static void check_range(uint32_t addr, uint32_t size1, uint32_t start,
+		uint32_t size2)
+{
+	uint32_t end1 = addr + size1;
+	uint32_t end2 = start + size2;
+
+	if (within(addr, start, end2) &&
+			end1 > addr && within(end1, start, end2))
+		return;
+
+	hw_error("EDU: DMA range 0x%.8x-0x%.8x out of bounds (0x%.8x-0x%.8x)!",
+			addr, end1 - 1, start, end2 - 1);
+}
+
+static void edu_dma_timer(void *opaque)
+{
+	EduState *edu = opaque;
+	bool raise_irq = false;
+
+	qemu_mutex_lock(&edu->dma_mutex);
+	if (!(edu->dma.cmd & EDU_DMA_RUN))
+		goto end;
+
+	if (EDU_DMA_DIR(edu->dma.cmd) == EDU_DMA_FROM_PCI) {
+		uint32_t dst = edu->dma.dst;
+		check_range(dst, edu->dma.cnt, DMA_START, DMA_SIZE);
+		dst -= DMA_START;
+		pci_dma_read(&edu->pdev, edu->dma.src, edu->dma_buf + dst,
+				edu->dma.cnt);
+	} else {
+		uint32_t src = edu->dma.src;
+		check_range(src, edu->dma.cnt, DMA_START, DMA_SIZE);
+		src -= DMA_START;
+		pci_dma_write(&edu->pdev, edu->dma.dst, edu->dma_buf + src,
+				edu->dma.cnt);
+	}
+
+	edu->dma.cmd &= ~EDU_DMA_RUN;
+	if (edu->dma.cmd & EDU_DMA_IRQ)
+		raise_irq = true;
+end:
+	qemu_mutex_unlock(&edu->dma_mutex);
+
+	if (raise_irq) {
+		qemu_mutex_lock(&edu->irq_mutex);
+		edu->irq_status |= DMA_IRQ;
+		pci_set_irq(&edu->pdev, 1);
+		qemu_mutex_unlock(&edu->irq_mutex);
+	}
+}
+
+static void dma_rw(EduState *edu, bool write, uint64_t *val, uint32_t *dma,
+		bool timer)
+{
+	qemu_mutex_lock(&edu->dma_mutex);
+	if (write && (edu->dma.cmd & EDU_DMA_RUN)) {
+		qemu_mutex_unlock(&edu->dma_mutex);
+		return;
+	}
+	if (write)
+		*dma = *val;
+	else
+		*val = *dma;
+	if (timer)
+		timer_mod(edu->dma_timer,
+				qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 100);
+	qemu_mutex_unlock(&edu->dma_mutex);
+}
+
+static uint64_t edu_mmio_read(void *opaque, hwaddr addr, unsigned size)
+{
+    EduState *edu = opaque;
+    uint64_t val = ~0ULL;
+
+    if (size != 4)
+	return val;
+
+    switch (addr) {
+    case 0x00:
+	val = 0x010000edu;
+	break;
+    case 0x04:
+	val = edu->addr4;
+	break;
+    case 0x08:
+	val = edu->fact;
+	break;
+    case 0x20:
+	val = edu->status;
+	break;
+    case 0x24:
+	qemu_mutex_lock(&edu->irq_mutex);
+	val = edu->irq_status;
+	qemu_mutex_unlock(&edu->irq_mutex);
+	break;
+    case 0x80:
+	dma_rw(edu, false, &val, &edu->dma.src, false);
+	break;
+    case 0x84:
+	dma_rw(edu, false, &val, &edu->dma.dst, false);
+	break;
+    case 0x88:
+	dma_rw(edu, false, &val, &edu->dma.cnt, false);
+	break;
+    case 0x8c:
+	dma_rw(edu, false, &val, &edu->dma.cmd, false);
+	break;
+    }
+#ifdef PCNET_DEBUG_IO
+    printf("edu_mmio_readl addr=0x" TARGET_FMT_plx " val=0x%64lx\n", addr,
+           val);
+#endif
+    return val;
+}
+
+static void edu_mmio_write(void *opaque, hwaddr addr, uint64_t val,
+		unsigned size)
+{
+    EduState *edu = opaque;
+
+    if (size != 4)
+	return;
+
+    switch (addr) {
+    case 0x04:
+	edu->addr4 = ~val;
+	break;
+    case 0x08:
+	if (edu->status & EDU_STATUS_COMPUTING)
+		break;
+	edu->status |= EDU_STATUS_COMPUTING;
+	edu->fact = val;
+	qemu_mutex_lock(&edu->thr_mutex);
+	qemu_cond_signal(&edu->thr_cond);
+	qemu_mutex_unlock(&edu->thr_mutex);
+	break;
+    case 0x60:
+	qemu_mutex_lock(&edu->irq_mutex);
+	edu->irq_status |= val;
+	pci_set_irq(&edu->pdev, 1);
+	qemu_mutex_unlock(&edu->irq_mutex);
+	break;
+    case 0x64:
+	qemu_mutex_lock(&edu->irq_mutex);
+	edu->irq_status &= ~val;
+	if (!edu->irq_status)
+		pci_set_irq(&edu->pdev, 0);
+	qemu_mutex_unlock(&edu->irq_mutex);
+	break;
+    case 0x80:
+	dma_rw(edu, true, &val, &edu->dma.src, false);
+	break;
+    case 0x84:
+	dma_rw(edu, true, &val, &edu->dma.dst, false);
+	break;
+    case 0x88:
+	dma_rw(edu, true, &val, &edu->dma.cnt, false);
+	break;
+    case 0x8c:
+	if (!(val & EDU_DMA_RUN))
+		break;
+	dma_rw(edu, true, &val, &edu->dma.cmd, true);
+	break;
+    }
+}
+
+static const MemoryRegionOps edu_mmio_ops = {
+    .read = edu_mmio_read,
+    .write = edu_mmio_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static void *edu_fact_thread(void *opaque)
+{
+	EduState *edu = opaque;
+
+	while (1) {
+		uint32_t val, ret = 1;
+
+		qemu_mutex_lock(&edu->thr_mutex);
+		qemu_cond_wait(&edu->thr_cond, &edu->thr_mutex);
+		qemu_mutex_unlock(&edu->thr_mutex);
+
+		if (edu->stopping)
+			break;
+
+		val = edu->fact;
+		while (val > 0)
+			ret *= val--;
+
+		edu->fact = ret;
+		smp_wmb();
+		edu->status &= ~EDU_STATUS_COMPUTING;
+	}
+
+	return NULL;
+}
+
+static int pci_edu_init(PCIDevice *pdev)
+{
+    EduState *edu = DO_UPCAST(EduState, pdev, pdev);
+    uint8_t *pci_conf = pdev->config;
+
+    edu->dma_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL, edu_dma_timer, edu);
+
+    qemu_mutex_init(&edu->dma_mutex);
+    qemu_mutex_init(&edu->irq_mutex);
+    qemu_mutex_init(&edu->thr_mutex);
+    qemu_cond_init(&edu->thr_cond);
+    qemu_thread_create(&edu->thread, "edu", edu_fact_thread,
+                       edu, QEMU_THREAD_JOINABLE);
+
+    pci_set_word(pci_conf + PCI_SUBSYSTEM_VENDOR_ID, 0x0);
+    pci_set_word(pci_conf + PCI_SUBSYSTEM_ID, 0x0);
+
+    pci_config_set_interrupt_pin(pci_conf, 1);
+
+    memory_region_init_io(&edu->mmio, OBJECT(edu), &edu_mmio_ops, edu,
+		    "edu-mmio", 1 << 20);
+    pci_register_bar(pdev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &edu->mmio);
+
+    return 0;
+}
+
+static void pci_edu_uninit(PCIDevice *pdev)
+{
+    EduState *edu = DO_UPCAST(EduState, pdev, pdev);
+
+    memory_region_destroy(&edu->mmio);
+
+    edu->stopping = true;
+    qemu_cond_signal(&edu->thr_cond);
+    qemu_thread_join(&edu->thread);
+
+    qemu_cond_destroy(&edu->thr_cond);
+    qemu_mutex_destroy(&edu->thr_mutex);
+    qemu_mutex_destroy(&edu->irq_mutex);
+    qemu_mutex_destroy(&edu->dma_mutex);
+
+    timer_del(edu->dma_timer);
+    timer_free(edu->dma_timer);
+}
+
+static void edu_class_init(ObjectClass *class, void *data)
+{
+    PCIDeviceClass *k = PCI_DEVICE_CLASS(class);
+
+    k->init = pci_edu_init;
+    k->exit = pci_edu_uninit;
+    k->vendor_id = PCI_VENDOR_ID_QEMU;
+    k->device_id = 0x11e8;
+    k->revision = 0x10;
+    k->class_id = PCI_CLASS_OTHERS;
+}
+
+static void pci_edu_register_types(void)
+{
+    static const TypeInfo edu_info = {
+	.name          = "edu",
+	.parent        = TYPE_PCI_DEVICE,
+	.instance_size = sizeof(EduState),
+	.class_init    = edu_class_init,
+    };
+
+    type_register_static(&edu_info);
+}
+type_init(pci_edu_register_types)
author
#4 lore raw

Re: [Qemu-devel] [PATCH 1/1] pci-host: add educational driver

Quoted 11 lines · Claudio Fontana

Hello,

Quoted 6 lines · Jiri Slaby

I am using qemu for teaching the Linux kernel at our university. I wrote a simple PCI device that can answer to writes/reads, generate interrupts and perform DMA. As I am dragging it locally over 2 years, I am sending it to you now.

Signed-off-by Jiri Slaby <[email protected]>

is this supposed to be architecture independent, or is it X86-specific?

Hi,

I did not plan it to be only x86 specific. If you see any problems, I will fix them.

Quoted 2 lines

Also at first glance I see multiple 32bit variables used to hold addresses, is this 32bit-only?

No, the DMA addresses are on purpose 32-bit: to teach the people always set the dma mask properly in the driver. This driver copies COMBO6x devices (liberouter.org) behaviour which I used until the cards got obsoleted (hard to find PCI-X slots nowadays).

I can make this configurable if you wish.

Quoted 1 line

I wonder if this work could be merged / integrated with the Generic PCI host patches that are flying around since some time...

Could you point me to some?

thanks,

Signature · 2 lines
js
suse labs
#5 lore raw

Re: [Qemu-devel] [PATCH 1/1] pci-host: add educational driver

Il 13/10/2014 10:32, Jiri Slaby ha scritto:

Quoted 6 lines

No, the DMA addresses are on purpose 32-bit: to teach the people always set the dma mask properly in the driver. This driver copies COMBO6x devices (liberouter.org) behaviour which I used until the cards got obsoleted (hard to find PCI-X slots nowadays).

I can make this configurable if you wish.

Yeah, that would help (to avoid setting a bad example). You could have extra commands to switch between 32- and 64-bit DMA masks.

Paolo

#6 lore raw

Re: [Qemu-devel] [PATCH 1/1] pci-host: add educational driver

In reply to Jiri Slaby

Il 10/10/2014 14:09, Jiri Slaby ha scritto:

Quoted 10 lines

I am using qemu for teaching the Linux kernel at our university. I wrote a simple PCI device that can answer to writes/reads, generate interrupts and perform DMA. As I am dragging it locally over 2 years, I am sending it to you now.

Signed-off-by Jiri Slaby <[email protected]>
---
 MAINTAINERS               |   5 +
 hw/pci-host/Makefile.objs |   1 +
 hw/pci-host/edu.c         | 336 ++++++++++++++++++++++++++++++++++++++++++++++

hw/pci-host is for PCI host bridges. You can put it in hw/misc.

Quoted 24 lines
 3 files changed, 342 insertions(+)
 create mode 100644 hw/pci-host/edu.c
MAINTAINERS +5 -0
index 206bf7ea4582..7f4e8591b74b 100644
@@ -567,6 +567,11 @@ F: hw/xtensa/xtensa_lx60.c
 
 Devices
 -------
+EDU
+M: Jiri Slaby <[email protected]>
+S: Maintained
+F: hw/pci-host/edu.c
+
 IDE
 M: Kevin Wolf <[email protected]>
 M: Stefan Hajnoczi <[email protected]>
hw/pci-host/Makefile.objs +0 -0
index bb65f9c4d2d0..b01f614ed248 100644

hw/pci-host/Makefile.objs common-obj-$(CONFIG_VERSATILE_PCI) += versatile.o · line 16

Quoted 4 lines
hw/pci-host/Makefile.objs +1 -0
@@ -13,5 +13,6 @@ common-obj-$(CONFIG_VERSATILE_PCI) += versatile.o
 
 common-obj-$(CONFIG_PCI_APB) += apb.o
 common-obj-$(CONFIG_FULONG) += bonito.o
+common-obj-$(CONFIG_PCI) += edu.o

Please add CONFIG_EDU=y to default-configs/pci.mak, and use CONFIG_EDU here.

Quoted 87 lines
hw/pci-host/Makefile.objs +0 -0
 common-obj-$(CONFIG_PCI_PIIX) += piix.o
 common-obj-$(CONFIG_PCI_Q35) += q35.o
hw/pci-host/edu.c new file +79 -0
new file mode 100644
index 000000000000..72e09dff6f5d
@@ -0,0 +1,336 @@
+/*
+ * QEMU education PCI device
+ *
+ * Copyright (c) 2012 Jiri Slaby
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ */
+
+#include "hw/pci/pci.h"
+#include "qemu/timer.h"
+
+#define DMA_START	0x40000
+#define DMA_SIZE	4096
+#define DMA_IRQ		0x00000100
+
+typedef struct {
+    PCIDevice pdev;
+    MemoryRegion mmio;
+
+    QemuThread thread;
+    QemuMutex thr_mutex;
+    QemuCond thr_cond;
+    bool stopping;
+
+    uint32_t addr4;
+    uint32_t fact;
+#define EDU_STATUS_COMPUTING	0x1
+    uint32_t status;
+
+    uint32_t irq_status;
+    QemuMutex irq_mutex;
+
+#define EDU_DMA_RUN		0x1
+#define EDU_DMA_DIR(cmd)	(((cmd) & 0x2) >> 1)
+# define EDU_DMA_FROM_PCI	0
+# define EDU_DMA_TO_PCI		1
+#define EDU_DMA_IRQ		0x4
+    struct dma_state {
+	    uint32_t src;
+	    uint32_t dst;
+	    uint32_t cnt;
+	    uint32_t cmd;
+    } dma;
+    QEMUTimer *dma_timer;
+    QemuMutex dma_mutex;
+    char dma_buf[DMA_SIZE];
+} EduState;
+
+static bool within(uint32_t addr, uint32_t start, uint32_t end)
+{
+	return start <= addr && addr < end;
+}
+
+static void check_range(uint32_t addr, uint32_t size1, uint32_t start,
+		uint32_t size2)
+{
+	uint32_t end1 = addr + size1;
+	uint32_t end2 = start + size2;
+
+	if (within(addr, start, end2) &&
+			end1 > addr && within(end1, start, end2))
+		return;
+

hw/pci-host/edu.c line 89

Quoted 10 lines
hw/pci-host/edu.c new file +10 -0
+	hw_error("EDU: DMA range 0x%.8x-0x%.8x out of bounds (0x%.8x-0x%.8x)!",
+			addr, end1 - 1, start, end2 - 1);
+}
+
+static void edu_dma_timer(void *opaque)
+{
+	EduState *edu = opaque;
+	bool raise_irq = false;
+
+	qemu_mutex_lock(&edu->dma_mutex);

dma_mutex and mutex and irq_mutex are not necessary. All I/O happens under the big QEMU lock (qemu_lock/unlock_iothread). I can certainly imagine that edu.c would be one of the first devices we make thread-safe, but... not yet. :)

Quoted 40 lines
hw/pci-host/edu.c new file +40 -0
+	if (!(edu->dma.cmd & EDU_DMA_RUN))
+		goto end;
+
+	if (EDU_DMA_DIR(edu->dma.cmd) == EDU_DMA_FROM_PCI) {
+		uint32_t dst = edu->dma.dst;
+		check_range(dst, edu->dma.cnt, DMA_START, DMA_SIZE);
+		dst -= DMA_START;
+		pci_dma_read(&edu->pdev, edu->dma.src, edu->dma_buf + dst,
+				edu->dma.cnt);
+	} else {
+		uint32_t src = edu->dma.src;
+		check_range(src, edu->dma.cnt, DMA_START, DMA_SIZE);
+		src -= DMA_START;
+		pci_dma_write(&edu->pdev, edu->dma.dst, edu->dma_buf + src,
+				edu->dma.cnt);
+	}
+
+	edu->dma.cmd &= ~EDU_DMA_RUN;
+	if (edu->dma.cmd & EDU_DMA_IRQ)
+		raise_irq = true;
+end:
+	qemu_mutex_unlock(&edu->dma_mutex);
+
+	if (raise_irq) {
+		qemu_mutex_lock(&edu->irq_mutex);
+		edu->irq_status |= DMA_IRQ;
+		pci_set_irq(&edu->pdev, 1);
+		qemu_mutex_unlock(&edu->irq_mutex);
+	}
+}
+
+static void dma_rw(EduState *edu, bool write, uint64_t *val, uint32_t *dma,
+		bool timer)
+{
+	qemu_mutex_lock(&edu->dma_mutex);
+	if (write && (edu->dma.cmd & EDU_DMA_RUN)) {
+		qemu_mutex_unlock(&edu->dma_mutex);
+		return;
+	}
+	if (write)

hw/pci-host/edu.c line 139

Quoted 10 lines
hw/pci-host/edu.c new file +10 -0
+		*dma = *val;
+	else
+		*val = *dma;
+	if (timer)
+		timer_mod(edu->dma_timer,
+				qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 100);
+	qemu_mutex_unlock(&edu->dma_mutex);
+}
+
+static uint64_t edu_mmio_read(void *opaque, hwaddr addr, unsigned size)

Please document the semantics in the docs/ directory.

Quoted 9 lines
hw/pci-host/edu.c new file +9 -0
+{
+    EduState *edu = opaque;
+    uint64_t val = ~0ULL;
+
+    if (size != 4)
+	return val;
+
+    switch (addr) {
+    case 0x00:

hw/pci-host/edu.c line 158

Quoted 10 lines
hw/pci-host/edu.c new file +10 -0
+	val = 0x010000edu;
+	break;
+    case 0x04:
+	val = edu->addr4;
+	break;
+    case 0x08:
+	val = edu->fact;
+	break;
+    case 0x20:
+	val = edu->status;

I guess you theoretically need an smp_rmb() here. In practice there is plenty of barriers between two device accesses. However, you may still want to use atomic_mb_read/atomic_mb_set to access edu->fact and edu->status, for the sake of education.

Quoted 169 lines
hw/pci-host/edu.c new file +168 -0
+	break;
+    case 0x24:
+	qemu_mutex_lock(&edu->irq_mutex);
+	val = edu->irq_status;
+	qemu_mutex_unlock(&edu->irq_mutex);
+	break;
+    case 0x80:
+	dma_rw(edu, false, &val, &edu->dma.src, false);
+	break;
+    case 0x84:
+	dma_rw(edu, false, &val, &edu->dma.dst, false);
+	break;
+    case 0x88:
+	dma_rw(edu, false, &val, &edu->dma.cnt, false);
+	break;
+    case 0x8c:
+	dma_rw(edu, false, &val, &edu->dma.cmd, false);
+	break;
+    }
+#ifdef PCNET_DEBUG_IO
+    printf("edu_mmio_readl addr=0x" TARGET_FMT_plx " val=0x%64lx\n", addr,
+           val);
+#endif
+    return val;
+}
+
+static void edu_mmio_write(void *opaque, hwaddr addr, uint64_t val,
+		unsigned size)
+{
+    EduState *edu = opaque;
+
+    if (size != 4)
+	return;
+
+    switch (addr) {
+    case 0x04:
+	edu->addr4 = ~val;
+	break;
+    case 0x08:
+	if (edu->status & EDU_STATUS_COMPUTING)
+		break;
+	edu->status |= EDU_STATUS_COMPUTING;
+	edu->fact = val;
+	qemu_mutex_lock(&edu->thr_mutex);
+	qemu_cond_signal(&edu->thr_cond);
+	qemu_mutex_unlock(&edu->thr_mutex);
+	break;
+    case 0x60:
+	qemu_mutex_lock(&edu->irq_mutex);
+	edu->irq_status |= val;
+	pci_set_irq(&edu->pdev, 1);
+	qemu_mutex_unlock(&edu->irq_mutex);
+	break;
+    case 0x64:
+	qemu_mutex_lock(&edu->irq_mutex);
+	edu->irq_status &= ~val;
+	if (!edu->irq_status)
+		pci_set_irq(&edu->pdev, 0);
+	qemu_mutex_unlock(&edu->irq_mutex);
+	break;
+    case 0x80:
+	dma_rw(edu, true, &val, &edu->dma.src, false);
+	break;
+    case 0x84:
+	dma_rw(edu, true, &val, &edu->dma.dst, false);
+	break;
+    case 0x88:
+	dma_rw(edu, true, &val, &edu->dma.cnt, false);
+	break;
+    case 0x8c:
+	if (!(val & EDU_DMA_RUN))
+		break;
+	dma_rw(edu, true, &val, &edu->dma.cmd, true);
+	break;
+    }
+}
+
+static const MemoryRegionOps edu_mmio_ops = {
+    .read = edu_mmio_read,
+    .write = edu_mmio_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static void *edu_fact_thread(void *opaque)
+{
+	EduState *edu = opaque;
+
+	while (1) {
+		uint32_t val, ret = 1;
+
+		qemu_mutex_lock(&edu->thr_mutex);
+		qemu_cond_wait(&edu->thr_cond, &edu->thr_mutex);
+		qemu_mutex_unlock(&edu->thr_mutex);
+
+		if (edu->stopping)
+			break;
+
+		val = edu->fact;
+		while (val > 0)
+			ret *= val--;
+
+		edu->fact = ret;
+		smp_wmb();
+		edu->status &= ~EDU_STATUS_COMPUTING;
+	}
+
+	return NULL;
+}
+
+static int pci_edu_init(PCIDevice *pdev)
+{
+    EduState *edu = DO_UPCAST(EduState, pdev, pdev);
+    uint8_t *pci_conf = pdev->config;
+
+    edu->dma_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL, edu_dma_timer, edu);
+
+    qemu_mutex_init(&edu->dma_mutex);
+    qemu_mutex_init(&edu->irq_mutex);
+    qemu_mutex_init(&edu->thr_mutex);
+    qemu_cond_init(&edu->thr_cond);
+    qemu_thread_create(&edu->thread, "edu", edu_fact_thread,
+                       edu, QEMU_THREAD_JOINABLE);
+
+    pci_set_word(pci_conf + PCI_SUBSYSTEM_VENDOR_ID, 0x0);
+    pci_set_word(pci_conf + PCI_SUBSYSTEM_ID, 0x0);
+
+    pci_config_set_interrupt_pin(pci_conf, 1);
+
+    memory_region_init_io(&edu->mmio, OBJECT(edu), &edu_mmio_ops, edu,
+		    "edu-mmio", 1 << 20);
+    pci_register_bar(pdev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &edu->mmio);
+
+    return 0;
+}
+
+static void pci_edu_uninit(PCIDevice *pdev)
+{
+    EduState *edu = DO_UPCAST(EduState, pdev, pdev);
+
+    memory_region_destroy(&edu->mmio);
+
+    edu->stopping = true;
+    qemu_cond_signal(&edu->thr_cond);
+    qemu_thread_join(&edu->thread);
+
+    qemu_cond_destroy(&edu->thr_cond);
+    qemu_mutex_destroy(&edu->thr_mutex);
+    qemu_mutex_destroy(&edu->irq_mutex);
+    qemu_mutex_destroy(&edu->dma_mutex);
+
+    timer_del(edu->dma_timer);
+    timer_free(edu->dma_timer);
+}
+
+static void edu_class_init(ObjectClass *class, void *data)
+{
+    PCIDeviceClass *k = PCI_DEVICE_CLASS(class);
+
+    k->init = pci_edu_init;
+    k->exit = pci_edu_uninit;
+    k->vendor_id = PCI_VENDOR_ID_QEMU;
+    k->device_id = 0x11e8;
+    k->revision = 0x10;
+    k->class_id = PCI_CLASS_OTHERS;
+}
+
+static void pci_edu_register_types(void)
+{

hw/pci-host/edu.c line 336

Quoted 10 lines
hw/pci-host/edu.c new file +10 -0
+    static const TypeInfo edu_info = {
+	.name          = "edu",
+	.parent        = TYPE_PCI_DEVICE,
+	.instance_size = sizeof(EduState),
+	.class_init    = edu_class_init,
+    };
+
+    type_register_static(&edu_info);
+}
+type_init(pci_edu_register_types)

An extra comment is that it would be nice to have a testcase for this device, as well. In fact, having a good example of a testcase would be the best reason to merge this kind of device. However, let's get edu.c in good shape first.

Paolo

author
#7 lore raw

Re: [Qemu-devel] [PATCH 1/1] pci-host: add educational driver

Quoted 11 lines · Paolo Bonzini

hw/pci-host/edu.c line 89

Quoted 6 lines
hw/pci-host/edu.c new file +6 -0
+static void edu_dma_timer(void *opaque)
+{
+	EduState *edu = opaque;
+	bool raise_irq = false;
+
+	qemu_mutex_lock(&edu->dma_mutex);

dma_mutex and mutex and irq_mutex are not necessary. All I/O happens under the big QEMU lock (qemu_lock/unlock_iothread). I can certainly imagine that edu.c would be one of the first devices we make thread-safe, but... not yet. :)

Hi,

I finally got to it. I want to make sure that I understand this correctly. So even timers are protected by the "BQL"?

thanks,

Signature · 2 lines
js
suse labs
#8 lore raw

Re: [Qemu-devel] [PATCH 1/1] pci-host: add educational driver

Quoted 13 lines · Jiri Slaby
Quoted 11 lines · Paolo Bonzini

hw/pci-host/edu.c line 89

Quoted 6 lines
hw/pci-host/edu.c new file +6 -0
+static void edu_dma_timer(void *opaque)
+{
+	EduState *edu = opaque;
+	bool raise_irq = false;
+
+	qemu_mutex_lock(&edu->dma_mutex);

dma_mutex and mutex and irq_mutex are not necessary. All I/O happens under the big QEMU lock (qemu_lock/unlock_iothread). I can certainly imagine that edu.c would be one of the first devices we make thread-safe, but... not yet. :)

Quoted 4 lines

Hi,

I finally got to it. I want to make sure that I understand this correctly. So even timers are protected by the "BQL"?

Everything, unless you create a separate thread.

Paolo

author
#9 lore raw

PATCH v2 1/1 pci-host: add educational driver

In reply to Jiri Slaby

I am using qemu for teaching the Linux kernel at our university. I wrote a simple PCI device that can answer to writes/reads, generate interrupts and perform DMA. As I am dragging it locally over 2 years, I am sending it to you now.

Signed-off-by Jiri Slaby <[email protected]>
Cc Paolo Bonzini <[email protected]>
---
 MAINTAINERS             |   5 +
 default-configs/pci.mak |   1 +
 docs/specs/edu.txt      | 105 +++++++++++++++
 hw/misc/Makefile.objs   |   1 +
 hw/misc/edu.c           | 351 ++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 463 insertions(+)
 create mode 100644 docs/specs/edu.txt
 create mode 100644 hw/misc/edu.c
MAINTAINERS +5 -0
index bcb69e80d2dd..9b69289aa8e3 100644
@@ -593,6 +593,11 @@ F: hw/net/opencores_eth.c
 
 Devices
 -------
+EDU
+M: Jiri Slaby <[email protected]>
+S: Maintained
+F: hw/pci-host/edu.c
+
 IDE
 M: Kevin Wolf <[email protected]>
 M: Stefan Hajnoczi <[email protected]>
default-configs/pci.mak +1 -0
index 91b1e92da53f..29130aba61d6 100644
@@ -30,3 +30,4 @@ CONFIG_IPACK=y
 CONFIG_WDT_IB6300ESB=y
 CONFIG_PCI_TESTDEV=y
 CONFIG_NVME_PCI=y
+CONFIG_EDU=y
docs/specs/edu.txt new file +105 -0

105 lines not on this page · raw message

hw/misc/Makefile.objs +1 -0
index 979e532fdf85..1fe74c6eb3fa 100644
@@ -41,3 +41,4 @@ obj-$(CONFIG_SLAVIO) += slavio_misc.o
 obj-$(CONFIG_ZYNQ) += zynq_slcr.o
 
 obj-$(CONFIG_PVPANIC) += pvpanic.o
+obj-$(CONFIG_EDU) += edu.o
hw/misc/edu.c new file +351 -0

351 lines not on this page · raw message

Signature · 1 line
2.1.3
author
#10 lore raw

Re: [Qemu-devel] [PATCH v2 1/1] pci-host: add educational driver

MAINTAINERS F: hw/net/opencores_eth.c · line 599

Quoted 13 lines · Jiri Slaby
 hw/misc/edu.c           | 351 ++++++++++++++++++++++++++++++++++++++++++++++++
MAINTAINERS +4 -0
@@ -593,6 +593,11 @@ F: hw/net/opencores_eth.c
 
 Devices
 -------
+EDU
+M: Jiri Slaby <[email protected]>
+S: Maintained
+F: hw/pci-host/edu.c

Which does not really correspond :/

Signature · 2 lines
js
suse labs
#11 lore raw

Re: [Qemu-devel] [PATCH v2 1/1] pci-host: add educational driver

In reply to Jiri Slaby

Hi Jirka,

because this is supposed to be a poster of good QEMU practices, the review is going to be a bit picky. Most comments are trivial to apply.

Review

Quoted 4 lines

+0x20 (RO) : status register, bitwise OR

(quoted diff) +2 -0
+	    0x01 -- computing factorial
+

Perhaps bit 0 can be read-only, while bit 1 is set/clear by the guest and is "raise interrupt at the end of factorial computation"?

Quoted 52 lines

new file mode 100644
index 000000000000..7b3d320eeba5

hw/misc/edu.c new file +47 -0
@@ -0,0 +1,351 @@
+/*
+ * QEMU educational PCI device
+ *
+ * Copyright (c) 2012-2014 Jiri Slaby
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ */
+
+#include "hw/pci/pci.h"
+#include "qemu/timer.h"
+
+#define DMA_START	0x40000
+#define DMA_SIZE	4096
+#define DMA_IRQ		0x00000100
+
+typedef struct {
+    PCIDevice pdev;
+    MemoryRegion mmio;
+
+    QemuThread thread;
+    QemuMutex thr_mutex;
+    QemuCond thr_cond;
+    bool stopping;
+
+    uint32_t addr4;
+    uint32_t fact;
+#define EDU_STATUS_COMPUTING	0x1
+    uint32_t status;
+
+    uint32_t irq_status;
+

hw/misc/edu.c line 57

Quoted 10 lines
hw/misc/edu.c new file +10 -0
+#define EDU_DMA_RUN		0x1
+#define EDU_DMA_DIR(cmd)	(((cmd) & 0x2) >> 1)
+# define EDU_DMA_FROM_PCI	0
+# define EDU_DMA_TO_PCI		1
+#define EDU_DMA_IRQ		0x4
+    struct dma_state {
+	    dma_addr_t src;
+	    dma_addr_t dst;
+	    dma_addr_t cnt;
+	    dma_addr_t cmd;

QEMU has its own scripts/checkpatch.pl, and this patch wouldn't survive. :)

hw/misc/edu.c line 59

Quoted 2 lines
hw/misc/edu.c new file +2 -0
+    } dma;
+    QEMUTimer *dma_timer;

Please use timer_init instead of timer_new, and declare this as "QEMUTimer dma_timer" instead of a pointer. Nobody does this, everybody should do this.

hw/misc/edu.c line 63

Quoted 4 lines
hw/misc/edu.c new file +4 -0
+    char dma_buf[DMA_SIZE];
+} EduState;
+
+static uint64_t dma_mask = (1UL << 28) - 1;

Please move this inside EduState.

hw/misc/edu.c line 67

Quoted 3 lines
hw/misc/edu.c new file +3 -0
+static bool within(uint32_t addr, uint32_t start, uint32_t end)
+{
+	return start <= addr && addr < end;

4-space indent.

hw/misc/edu.c line 78

Quoted 11 lines
hw/misc/edu.c new file +11 -0
+}
+
+static void edu_check_range(uint32_t addr, uint32_t size1, uint32_t start,
+		uint32_t size2)
+{
+	uint32_t end1 = addr + size1;
+	uint32_t end2 = start + size2;
+
+	if (within(addr, start, end2) &&
+			end1 > addr && within(end1, start, end2))
+		return;

More TAB indents, and missing braces around single-statement if-then-elses.

Quoted 6 lines
hw/misc/edu.c new file +6 -0
+
+	hw_error("EDU: DMA range 0x%.8x-0x%.8x out of bounds (0x%.8x-0x%.8x)!",
+			addr, end1 - 1, start, end2 - 1);
+}
+
+static dma_addr_t edu_clamp_addr(dma_addr_t addr)

hw/misc/edu.c line 94

Quoted 10 lines
hw/misc/edu.c new file +10 -0
+{
+	if (addr & ~dma_mask)
+		printf("EDU: clamping DMA 0x%.16lx to 0x%.16lx!\n", addr,
+				addr & dma_mask);
+	return addr & dma_mask;
+}
+
+static void edu_dma_timer(void *opaque)
+{
+	EduState *edu = opaque;

If you use timer_init, you might as well use container_of here.

Quoted 16 lines
hw/misc/edu.c new file +16 -0
+	bool raise_irq = false;
+
+	if (!(edu->dma.cmd & EDU_DMA_RUN))
+		return;
+
+	if (EDU_DMA_DIR(edu->dma.cmd) == EDU_DMA_FROM_PCI) {
+		uint32_t dst = edu->dma.dst;
+		edu_check_range(dst, edu->dma.cnt, DMA_START, DMA_SIZE);
+		dst -= DMA_START;
+		pci_dma_read(&edu->pdev, edu_clamp_addr(edu->dma.src),
+				edu->dma_buf + dst, edu->dma.cnt);
+	} else {
+		uint32_t src = edu->dma.src;
+		edu_check_range(src, edu->dma.cnt, DMA_START, DMA_SIZE);
+		src -= DMA_START;
+		pci_dma_write(&edu->pdev, edu_clamp_addr(edu->dma.dst),

hw/misc/edu.c line 120

Quoted 10 lines
hw/misc/edu.c new file +10 -0
+				edu->dma_buf + src, edu->dma.cnt);
+	}
+
+	edu->dma.cmd &= ~EDU_DMA_RUN;
+	if (edu->dma.cmd & EDU_DMA_IRQ)
+		raise_irq = true;
+
+	if (raise_irq) {
+		edu->irq_status |= DMA_IRQ;
+		pci_set_irq(&edu->pdev, 1);

Please wrap these two lines in a separate function.

Quoted 6 lines
hw/misc/edu.c new file +6 -0
+	}
+}
+
+static void dma_rw(EduState *edu, bool write, dma_addr_t *val, dma_addr_t *dma,
+		bool timer)
+{

hw/misc/edu.c line 136

Quoted 10 lines
hw/misc/edu.c new file +10 -0
+	if (write && (edu->dma.cmd & EDU_DMA_RUN))
+		return;
+
+	if (write)
+		*dma = *val;
+	else
+		*val = *dma;
+	if (timer)
+		timer_mod(edu->dma_timer,
+				qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 100);

Jackpot! Four statements, four missing braces! :D

Quoted 11 lines
hw/misc/edu.c new file +11 -0
+}
+
+static uint64_t edu_mmio_read(void *opaque, hwaddr addr, unsigned size)
+{
+    EduState *edu = opaque;
+    uint64_t val = ~0ULL;
+
+    if (size != 4)
+	return val;
+
+    switch (addr) {

hw/misc/edu.c line 157

Quoted 10 lines
hw/misc/edu.c new file +10 -0
+    case 0x00:
+	val = 0x010000edu;
+	break;
+    case 0x04:
+	val = edu->addr4;
+	break;
+    case 0x08:
+	qemu_mutex_lock(&edu->thr_mutex);
+	val = edu->fact;
+	qemu_mutex_unlock(&edu->thr_mutex);

No need for the mutex.

hw/misc/edu.c line 161

Quoted 4 lines
hw/misc/edu.c new file +4 -0
+	break;
+    case 0x20:
+	smp_rmb(); // against thread
+	val = edu->status;

Better:

    /* Client is supposed to read status first, then fact.  */
    val = edu->status;
    smp_rmb();

The smp_rmb() has to go _after_ reading val = edu->status.

Quoted 30 lines
hw/misc/edu.c new file +30 -0
+	break;
+    case 0x24:
+	val = edu->irq_status;
+	break;
+    case 0x80:
+	dma_rw(edu, false, &val, &edu->dma.src, false);
+	break;
+    case 0x88:
+	dma_rw(edu, false, &val, &edu->dma.dst, false);
+	break;
+    case 0x90:
+	dma_rw(edu, false, &val, &edu->dma.cnt, false);
+	break;
+    case 0x98:
+	dma_rw(edu, false, &val, &edu->dma.cmd, false);
+	break;
+    }
+
+    return val;
+}
+
+static void edu_mmio_write(void *opaque, hwaddr addr, uint64_t val,
+		unsigned size)
+{
+    EduState *edu = opaque;
+
+    if (addr < 0x80 && size != 4)
+	return;
+
+    if (addr >= 0x80 && size != 4 && size != 8)

hw/misc/edu.c line 201

Quoted 10 lines
hw/misc/edu.c new file +10 -0
+	return;
+
+    switch (addr) {
+    case 0x04:
+	edu->addr4 = ~val;
+	break;
+    case 0x08:
+	if (edu->status & EDU_STATUS_COMPUTING)
+		break;
+	edu->status |= EDU_STATUS_COMPUTING;

atomic_or(&edu->status, EDU_STATUS_COMPUTING);

hw/misc/edu.c line 202

Quoted 2 lines
hw/misc/edu.c new file +2 -0
+	qemu_mutex_lock(&edu->thr_mutex);
+	edu->fact = val;

Probably the write should be ignored if edu->status has the computing bit set, otherwise if you write 4 and 5 in rapid succession you will end up with 24! in edu->fact.

hw/misc/edu.c line 206

Quoted 3 lines
hw/misc/edu.c new file +3 -0
+	qemu_cond_signal(&edu->thr_cond);
+	qemu_mutex_unlock(&edu->thr_mutex);
+	break;

If you add the above suggestion to use interrupts, you can do:

    case 0x24:
        if (val & EDU_STATUS_FACT_IRQ)
            atomic_or(&edu->status, EDU_STATUS_FACT_IRQ);
        else
            atomic_and(&edu->status, ~EDU_STATUS_FACT_IRQ);
        break;

to leave bit 0 untouched.

hw/misc/edu.c line 209

Quoted 3 lines
hw/misc/edu.c new file +3 -0
+    case 0x60:
+	edu->irq_status |= val;
+	pci_set_irq(&edu->pdev, 1);

Should not set irq if edu->irq_status is zero.

Quoted 39 lines
hw/misc/edu.c new file +39 -0
+	break;
+    case 0x64:
+	edu->irq_status &= ~val;
+	if (!edu->irq_status)
+		pci_set_irq(&edu->pdev, 0);
+	break;
+    case 0x80:
+	dma_rw(edu, true, &val, &edu->dma.src, false);
+	break;
+    case 0x88:
+	dma_rw(edu, true, &val, &edu->dma.dst, false);
+	break;
+    case 0x90:
+	dma_rw(edu, true, &val, &edu->dma.cnt, false);
+	break;
+    case 0x98:
+	if (!(val & EDU_DMA_RUN))
+		break;
+	dma_rw(edu, true, &val, &edu->dma.cmd, true);
+	break;
+    }
+}
+
+static const MemoryRegionOps edu_mmio_ops = {
+    .read = edu_mmio_read,
+    .write = edu_mmio_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+/*
+ * We purposedly use a thread, so that users are forced to wait for the status
+ * register.
+ */
+static void *edu_fact_thread(void *opaque)
+{
+	EduState *edu = opaque;
+
+	while (1) {
+		uint32_t val, ret = 1;

hw/misc/edu.c line 258

Quoted 10 lines
hw/misc/edu.c new file +10 -0
+
+		qemu_mutex_lock(&edu->thr_mutex);
+		qemu_cond_wait(&edu->thr_cond, &edu->thr_mutex);
+
+		if (edu->stopping) {
+			qemu_mutex_unlock(&edu->thr_mutex);
+			break;
+		}
+
+		val = edu->fact;

Should unlock mutex here...

hw/misc/edu.c line 263

Quoted 4 lines
hw/misc/edu.c new file +4 -0
+		while (val > 0)
+			ret *= val--;
+		edu->fact = ret;
+		qemu_mutex_unlock(&edu->thr_mutex);

... put smp_wmb() here...

hw/misc/edu.c line 264

Quoted 1 line
hw/misc/edu.c new file +1 -0
+		edu->status &= ~EDU_STATUS_COMPUTING;

... and use atomic_and here.

In order to raise an interrupt, for now you can just do pci_set_irq wrapped with qemu_mutex_lock/unlock_iothread(). Later we can change it to use a bottom half (QEMUBH), which is QEMU's way to schedule main thread from a separate thread.

Quoted 9 lines
hw/misc/edu.c new file +9 -0
+	}
+
+	return NULL;
+}
+
+static int pci_edu_init(PCIDevice *pdev)
+{
+    EduState *edu = DO_UPCAST(EduState, pdev, pdev);
+    uint8_t *pci_conf = pdev->config;

hw/misc/edu.c line 283

Quoted 10 lines
hw/misc/edu.c new file +10 -0
+
+    edu->dma_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL, edu_dma_timer, edu);
+
+    qemu_mutex_init(&edu->thr_mutex);
+    qemu_cond_init(&edu->thr_cond);
+    qemu_thread_create(&edu->thread, "edu", edu_fact_thread,
+                       edu, QEMU_THREAD_JOINABLE);
+
+    pci_set_word(pci_conf + PCI_SUBSYSTEM_VENDOR_ID, 0x0);
+    pci_set_word(pci_conf + PCI_SUBSYSTEM_ID, 0x0);

These two lines are not needed.

Quoted 28 lines
hw/misc/edu.c new file +28 -0
+    pci_config_set_interrupt_pin(pci_conf, 1);
+
+    memory_region_init_io(&edu->mmio, OBJECT(edu), &edu_mmio_ops, edu,
+		    "edu-mmio", 1 << 20);
+    pci_register_bar(pdev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &edu->mmio);
+
+    return 0;
+}
+
+static void pci_edu_uninit(PCIDevice *pdev)
+{
+    EduState *edu = DO_UPCAST(EduState, pdev, pdev);
+
+    memory_region_destroy(&edu->mmio);
+
+    qemu_mutex_lock(&edu->thr_mutex);
+    edu->stopping = true;
+    qemu_mutex_unlock(&edu->thr_mutex);
+    qemu_cond_signal(&edu->thr_cond);
+    qemu_thread_join(&edu->thread);
+
+    qemu_cond_destroy(&edu->thr_cond);
+    qemu_mutex_destroy(&edu->thr_mutex);
+
+    timer_del(edu->dma_timer);
+    timer_free(edu->dma_timer);
+}
+

hw/misc/edu.c line 322

Quoted 10 lines
hw/misc/edu.c new file +10 -0
+static void edu_obj_uint64(Object *obj, struct Visitor *v, void *opaque,
+		const char *name, Error **errp)
+{
+    uint64_t *val = opaque;
+
+    visit_type_uint64(v, val, name, errp);
+}
+
+static void edu_instance_init(Object *obj)
+{

    EduState *edu = EDU(obj);

hw/misc/edu.c line 324

Quoted 2 lines
hw/misc/edu.c new file +2 -0
+    object_property_add(obj, "dma_mask", "uint64", edu_obj_uint64,
+		    edu_obj_uint64, NULL, &dma_mask, NULL);

... and use &edu->dma_mask here.

Quoted 18 lines
hw/misc/edu.c new file +17 -0
+}
+
+static void edu_class_init(ObjectClass *class, void *data)
+{
+    PCIDeviceClass *k = PCI_DEVICE_CLASS(class);
+
+    k->init = pci_edu_init;
+    k->exit = pci_edu_uninit;
+    k->vendor_id = PCI_VENDOR_ID_QEMU;
+    k->device_id = 0x11e8;
+    k->revision = 0x10;
+    k->class_id = PCI_CLASS_OTHERS;
+}
+
+static void pci_edu_register_types(void)
+{
+    static const TypeInfo edu_info = {

hw/misc/edu.c line 351

Quoted 10 lines
hw/misc/edu.c new file +10 -0
+	.name          = "edu",
+	.parent        = TYPE_PCI_DEVICE,
+	.instance_size = sizeof(EduState),
+	.instance_init = edu_instance_init,
+	.class_init    = edu_class_init,
+    };
+
+    type_register_static(&edu_info);
+}
+type_init(pci_edu_register_types)

Thanks,

Paolo

author
#12 lore raw

Re: [Qemu-devel] [PATCH v2 1/1] pci-host: add educational driver

Quoted 4 lines · Paolo Bonzini

Hi Jirka,

because this is supposed to be a poster of good QEMU practices, the review is going to be a bit picky. Most comments are trivial to apply.

Hi, OK :).

Quoted 3 lines
Quoted 3 lines
hw/misc/edu.c new file +0 -0
@@ -0,0 +1,351 @@

...

Quoted 5 lines

hw/misc/edu.c line 94

Quoted 3 lines
hw/misc/edu.c new file +3 -0
+static void edu_dma_timer(void *opaque)
+{
+	EduState *edu = opaque;

If you use timer_init, you might as well use container_of here.

But how? I do not have the timer as a param, right?

Quoted 13 lines
Quoted 13 lines
hw/misc/edu.c new file +13 -0
+static uint64_t edu_mmio_read(void *opaque, hwaddr addr, unsigned size)
+{
+    EduState *edu = opaque;
+    uint64_t val = ~0ULL;
+
+    if (size != 4)
+	return val;
+
+    switch (addr) {
+    case 0x00:
+	val = 0x010000edu;
+	break;
+    case 0x04:
Quoted 8 lines

hw/misc/edu.c line 157

Quoted 6 lines
hw/misc/edu.c new file +6 -0
+	val = edu->addr4;
+	break;
+    case 0x08:
+	qemu_mutex_lock(&edu->thr_mutex);
+	val = edu->fact;
+	qemu_mutex_unlock(&edu->thr_mutex);

No need for the mutex.

But threads, as you wrote are not protected by the big lock. So shouldn't this be at least atomic_get()?

Quoted 22 lines
Quoted 9 lines
hw/misc/edu.c new file +9 -0
+static void edu_mmio_write(void *opaque, hwaddr addr, uint64_t val,
+		unsigned size)
+{
+    EduState *edu = opaque;
+
+    if (addr < 0x80 && size != 4)
+	return;
+
+    if (addr >= 0x80 && size != 4 && size != 8)

hw/misc/edu.c line 201

Quoted 10 lines
hw/misc/edu.c new file +10 -0
+	return;
+
+    switch (addr) {
+    case 0x04:
+	edu->addr4 = ~val;
+	break;
+    case 0x08:
+	if (edu->status & EDU_STATUS_COMPUTING)
+		break;
+	edu->status |= EDU_STATUS_COMPUTING;

atomic_or(&edu->status, EDU_STATUS_COMPUTING);

Quoted 6 lines

hw/misc/edu.c

Quoted 2 lines
hw/misc/edu.c new file +2 -0
+	qemu_mutex_lock(&edu->thr_mutex);
+	edu->fact = val;

Probably the write should be ignored if edu->status has the computing bit set, otherwise if you write 4 and 5 in rapid succession you will end up with 24! in edu->fact.

But it is, AFAICS above?

Quoted 6 lines

hw/misc/edu.c line 206

Quoted 3 lines
hw/misc/edu.c new file +3 -0
+	qemu_cond_signal(&edu->thr_cond);
+	qemu_mutex_unlock(&edu->thr_mutex);
+	break;

If you add the above suggestion to use interrupts, you can do:

Quoted 8 lines
    case 0x24:
        if (val & EDU_STATUS_FACT_IRQ)
            atomic_or(&edu->status, EDU_STATUS_FACT_IRQ);
        else
            atomic_and(&edu->status, ~EDU_STATUS_FACT_IRQ);
        break;

to leave bit 0 untouched.

Did you mean case 0x20?

Quoted 5 lines

hw/misc/edu.c line 209

Quoted 3 lines
hw/misc/edu.c new file +3 -0
+    case 0x60:
+	edu->irq_status |= val;
+	pci_set_irq(&edu->pdev, 1);

Should not set irq if edu->irq_status is zero.

I don't understand this. 0x60 is supposed to raise interrupts mostly when edu->irq_status is 0.

thanks,

Signature · 2 lines
js
suse labs
#13 lore raw

Re: [Qemu-devel] [PATCH v2 1/1] pci-host: add educational driver

Quoted 12 lines · Jiri Slaby
Quoted 4 lines · Paolo Bonzini

Hi Jirka,

because this is supposed to be a poster of good QEMU practices, the review is going to be a bit picky. Most comments are trivial to apply.

Hi, OK :).

Quoted 3 lines
Quoted 3 lines
hw/misc/edu.c new file +0 -0
@@ -0,0 +1,351 @@

...

Quoted 7 lines
Quoted 5 lines

hw/misc/edu.c line 94

Quoted 3 lines
hw/misc/edu.c new file +3 -0
+static void edu_dma_timer(void *opaque)
+{
+	EduState *edu = opaque;

If you use timer_init, you might as well use container_of here.

But how? I do not have the timer as a param, right?

If you use timer_init, you can choose whether to pass the Timer or EduState as the opaque. With timer_new, you have to pass the opaque. Any of the two can do.

Quoted 22 lines
Quoted 13 lines
Quoted 13 lines
hw/misc/edu.c new file +13 -0
+static uint64_t edu_mmio_read(void *opaque, hwaddr addr, unsigned size)
+{
+    EduState *edu = opaque;
+    uint64_t val = ~0ULL;
+
+    if (size != 4)
+	return val;
+
+    switch (addr) {
+    case 0x00:
+	val = 0x010000edu;
+	break;
+    case 0x04:
Quoted 8 lines

hw/misc/edu.c line 157

Quoted 6 lines
hw/misc/edu.c new file +6 -0
+	val = edu->addr4;
+	break;
+    case 0x08:
+	qemu_mutex_lock(&edu->thr_mutex);
+	val = edu->fact;
+	qemu_mutex_unlock(&edu->thr_mutex);

No need for the mutex.

Quoted 2 lines

But threads, as you wrote are not protected by the big lock. So shouldn't this be at least atomic_get()?

Yes, you can use atomic_read(), same kernel ACCESS_ONCE.

I was a bit confused because you had barriers, and tried to understand what you meant since you had a smp_rmb() but no matching smp_wmb(). I think both fact and status writes need to be under the mutex. As to reads:

- either you put reads under the mutex

- or you put reads outside the mutex, and then you have to use barriers.

Quoted 22 lines
Quoted 22 lines
Quoted 9 lines
hw/misc/edu.c new file +9 -0
+static void edu_mmio_write(void *opaque, hwaddr addr, uint64_t val,
+		unsigned size)
+{
+    EduState *edu = opaque;
+
+    if (addr < 0x80 && size != 4)
+	return;
+
+    if (addr >= 0x80 && size != 4 && size != 8)

hw/misc/edu.c line 201

Quoted 10 lines
hw/misc/edu.c new file +10 -0
+	return;
+
+    switch (addr) {
+    case 0x04:
+	edu->addr4 = ~val;
+	break;
+    case 0x08:
+	if (edu->status & EDU_STATUS_COMPUTING)
+		break;
+	edu->status |= EDU_STATUS_COMPUTING;

atomic_or(&edu->status, EDU_STATUS_COMPUTING);

Quoted 8 lines
Quoted 6 lines

hw/misc/edu.c

Quoted 2 lines
hw/misc/edu.c new file +2 -0
+	qemu_mutex_lock(&edu->thr_mutex);
+	edu->fact = val;

Probably the write should be ignored if edu->status has the computing bit set, otherwise if you write 4 and 5 in rapid succession you will end up with 24! in edu->fact.

But it is, AFAICS above?

Oops. I was only looking inside the critical section. Does it have to be checked inside the mutex, in fact?

Also, the qemu_cond_wait has to be protected with

    while ((edu->status & EDU_STATUS_COMPUTING) == 0 && !edu->stopping)

to avoid problems with spurious wakeups.

Quoted 13 lines
Quoted 13 lines

hw/misc/edu.c line 206

Quoted 3 lines
hw/misc/edu.c new file +3 -0
+	qemu_cond_signal(&edu->thr_cond);
+	qemu_mutex_unlock(&edu->thr_mutex);
+	break;

If you add the above suggestion to use interrupts, you can do:

    case 0x24:
        if (val & EDU_STATUS_FACT_IRQ)
            atomic_or(&edu->status, EDU_STATUS_FACT_IRQ);
        else
            atomic_and(&edu->status, ~EDU_STATUS_FACT_IRQ);
        break;
Quoted 3 lines
Quoted 1 line

to leave bit 0 untouched.

Did you mean case 0x20?

Yes, sorry.

Quoted 8 lines
Quoted 5 lines

hw/misc/edu.c line 209

Quoted 3 lines
hw/misc/edu.c new file +3 -0
+    case 0x60:
+	edu->irq_status |= val;
+	pci_set_irq(&edu->pdev, 1);

Should not set irq if edu->irq_status is zero.

I don't understand this. 0x60 is supposed to raise interrupts mostly when edu->irq_status is 0.

But if you write zero, i.e. edu->irq_status is zero after the OR, it shouldn't raise a spurious interrupt, should it?

Paolo

author
#14 lore raw

PATCH v3 1/1 pci-host: add educational driver

In reply to Jiri Slaby

I am using qemu for teaching the Linux kernel at our university. I wrote a simple PCI device that can answer to writes/reads, generate interrupts and perform DMA. As I am dragging it locally over 2 years, I am sending it to you now.

Signed-off-by Jiri Slaby <[email protected]>
---
 MAINTAINERS             |   5 +
 default-configs/pci.mak |   1 +
 docs/specs/edu.txt      | 105 +++++++++++++++
 hw/misc/Makefile.objs   |   1 +
 hw/misc/edu.c           | 351 ++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 463 insertions(+)
 create mode 100644 docs/specs/edu.txt
 create mode 100644 hw/misc/edu.c
MAINTAINERS +5 -0
index bcb69e80d2dd..4e3b9f1703bf 100644
@@ -593,6 +593,11 @@ F: hw/net/opencores_eth.c
 
 Devices
 -------
+EDU
+M: Jiri Slaby <[email protected]>
+S: Maintained
+F: hw/misc/edu.c
+
 IDE
 M: Kevin Wolf <[email protected]>
 M: Stefan Hajnoczi <[email protected]>
default-configs/pci.mak +1 -0
index 91b1e92da53f..29130aba61d6 100644
@@ -30,3 +30,4 @@ CONFIG_IPACK=y
 CONFIG_WDT_IB6300ESB=y
 CONFIG_PCI_TESTDEV=y
 CONFIG_NVME_PCI=y
+CONFIG_EDU=y
docs/specs/edu.txt new file +105 -0

105 lines not on this page · raw message

hw/misc/Makefile.objs +1 -0
index 979e532fdf85..1fe74c6eb3fa 100644
@@ -41,3 +41,4 @@ obj-$(CONFIG_SLAVIO) += slavio_misc.o
 obj-$(CONFIG_ZYNQ) += zynq_slcr.o
 
 obj-$(CONFIG_PVPANIC) += pvpanic.o
+obj-$(CONFIG_EDU) += edu.o
hw/misc/edu.c new file +351 -0

351 lines not on this page · raw message

Signature · 1 line
2.1.3