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

[Paolo Bonzini <pbonzini@redhat.com>]

Il 10/10/2014 14:09, Jiri Slaby ha scritto:
> 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 <jslaby@suse.cz>
> ---
>  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.

>  3 files changed, 342 insertions(+)
>  create mode 100644 hw/pci-host/edu.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 206bf7ea4582..7f4e8591b74b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -567,6 +567,11 @@ F: hw/xtensa/xtensa_lx60.c
>  
>  Devices
>  -------
> +EDU
> +M: Jiri Slaby <jslaby@suse.cz>
> +S: Maintained
> +F: hw/pci-host/edu.c
> +
>  IDE
>  M: Kevin Wolf <kwolf@redhat.com>
>  M: Stefan Hajnoczi <stefanha@redhat.com>
> diff --git a/hw/pci-host/Makefile.objs b/hw/pci-host/Makefile.objs
> index bb65f9c4d2d0..b01f614ed248 100644
> --- a/hw/pci-host/Makefile.objs
> +++ b/hw/pci-host/Makefile.objs
> @@ -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.

>  common-obj-$(CONFIG_PCI_PIIX) += piix.o
>  common-obj-$(CONFIG_PCI_Q35) += q35.o
> diff --git a/hw/pci-host/edu.c b/hw/pci-host/edu.c
> new file mode 100644
> index 000000000000..72e09dff6f5d
> --- /dev/null
> +++ b/hw/pci-host/edu.c
> @@ -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);

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. :)

> +	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)

Please document the semantics in the docs/ directory.

> +{
> +    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;

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.

> +	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)
> 

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