* Re: [Qemu-devel] [PATCH 1/1] pci-host: add educational driver
2014-10-10 12:09 ` [Qemu-devel] [PATCH 1/1] pci-host: add educational driver Jiri Slaby
@ 2014-10-10 14:54 ` Claudio Fontana
2014-10-13 8:32 ` Jiri Slaby
2014-10-13 13:00 ` Paolo Bonzini
` (2 subsequent siblings)
3 siblings, 1 reply; 15+ messages in thread
From: Claudio Fontana @ 2014-10-10 14:54 UTC (permalink / raw)
To: Jiri Slaby, qemu-devel; +Cc: Peter Maydell, Alvise Rigo
Hello,
On 10.10.2014 14:09, Jiri Slaby wrote:
> 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>
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
> ---
> 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
>
> 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
> 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);
> + 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)
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH 1/1] pci-host: add educational driver
2014-10-10 14:54 ` Claudio Fontana
@ 2014-10-13 8:32 ` Jiri Slaby
2014-10-13 13:02 ` Paolo Bonzini
0 siblings, 1 reply; 15+ messages in thread
From: Jiri Slaby @ 2014-10-13 8:32 UTC (permalink / raw)
To: Claudio Fontana, qemu-devel; +Cc: Peter Maydell, Alvise Rigo
On 10/10/2014, 04:54 PM, Claudio Fontana wrote:
> Hello,
>
> On 10.10.2014 14:09, Jiri Slaby wrote:
>> 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>
>
> 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.
> 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.
> 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,
--
js
suse labs
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] pci-host: add educational driver
2014-10-13 8:32 ` Jiri Slaby
@ 2014-10-13 13:02 ` Paolo Bonzini
0 siblings, 0 replies; 15+ messages in thread
From: Paolo Bonzini @ 2014-10-13 13:02 UTC (permalink / raw)
To: Jiri Slaby, Claudio Fontana, qemu-devel; +Cc: Peter Maydell, Alvise Rigo
Il 13/10/2014 10:32, Jiri Slaby ha scritto:
> 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] pci-host: add educational driver
2014-10-10 12:09 ` [Qemu-devel] [PATCH 1/1] pci-host: add educational driver Jiri Slaby
2014-10-10 14:54 ` Claudio Fontana
@ 2014-10-13 13:00 ` Paolo Bonzini
2014-12-03 15:11 ` Jiri Slaby
2014-12-05 9:52 ` [Qemu-devel] [PATCH v2 " Jiri Slaby
2014-12-05 9:54 ` [Qemu-devel] [PATCH v3 " Jiri Slaby
3 siblings, 1 reply; 15+ messages in thread
From: Paolo Bonzini @ 2014-10-13 13:00 UTC (permalink / raw)
To: Jiri Slaby, qemu-devel
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH 1/1] pci-host: add educational driver
2014-10-13 13:00 ` Paolo Bonzini
@ 2014-12-03 15:11 ` Jiri Slaby
2014-12-03 15:18 ` Paolo Bonzini
0 siblings, 1 reply; 15+ messages in thread
From: Jiri Slaby @ 2014-12-03 15:11 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
On 10/13/2014, 03:00 PM, Paolo Bonzini wrote:
>> +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,
--
js
suse labs
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH 1/1] pci-host: add educational driver
2014-12-03 15:11 ` Jiri Slaby
@ 2014-12-03 15:18 ` Paolo Bonzini
0 siblings, 0 replies; 15+ messages in thread
From: Paolo Bonzini @ 2014-12-03 15:18 UTC (permalink / raw)
To: Jiri Slaby, qemu-devel
On 03/12/2014 16:11, Jiri Slaby wrote:
> On 10/13/2014, 03:00 PM, Paolo Bonzini wrote:
>>> +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"?
Everything, unless you create a separate thread.
Paolo
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH v2 1/1] pci-host: add educational driver
2014-10-10 12:09 ` [Qemu-devel] [PATCH 1/1] pci-host: add educational driver Jiri Slaby
2014-10-10 14:54 ` Claudio Fontana
2014-10-13 13:00 ` Paolo Bonzini
@ 2014-12-05 9:52 ` Jiri Slaby
2014-12-05 9:53 ` Jiri Slaby
2014-12-05 10:35 ` Paolo Bonzini
2014-12-05 9:54 ` [Qemu-devel] [PATCH v3 " Jiri Slaby
3 siblings, 2 replies; 15+ messages in thread
From: Jiri Slaby @ 2014-12-05 9:52 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, 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 <jslaby@suse.cz>
Cc: Paolo Bonzini <pbonzini@redhat.com>
---
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
diff --git a/MAINTAINERS b/MAINTAINERS
index bcb69e80d2dd..9b69289aa8e3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -593,6 +593,11 @@ F: hw/net/opencores_eth.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/default-configs/pci.mak b/default-configs/pci.mak
index 91b1e92da53f..29130aba61d6 100644
--- a/default-configs/pci.mak
+++ b/default-configs/pci.mak
@@ -30,3 +30,4 @@ CONFIG_IPACK=y
CONFIG_WDT_IB6300ESB=y
CONFIG_PCI_TESTDEV=y
CONFIG_NVME_PCI=y
+CONFIG_EDU=y
diff --git a/docs/specs/edu.txt b/docs/specs/edu.txt
new file mode 100644
index 000000000000..94409bb58297
--- /dev/null
+++ b/docs/specs/edu.txt
@@ -0,0 +1,105 @@
+
+EDU device
+==========
+
+This is an educational device for writing (kernel) drivers. Its original
+intention was to support the Linux kernel lectures taught at the Masaryk
+University. Students are given this virtual device and are expected to write a
+driver with I/Os, IRQs, DMAs and such.
+
+The devices behaves very similar to the PCI bridge present in the COMBO6 cards
+developed under the Liberouter wings. Both PCI device ID and PCI space is
+inherited from that device.
+
+Command line switches:
+ -device edu[,dma_mask=mask]
+
+ dma_mask makes the virtual device work with DMA addresses with the given
+ mask. For educational purposes, the device supports only 28 bits (256 MiB)
+ by default. Students shall set dma_mask for the device in the OS driver
+ properly.
+
+PCI specs
+---------
+
+PCI ID: 1234:11e8
+
+PCI Region 0:
+ I/O memory, 1 MB in size. Users are supposed to communicate with the card
+ through this memory.
+
+MMIO area spec
+--------------
+
+Only size == 4 accesses are allowed for addresses < 0x80. size == 4 or
+size == 8 for the rest.
+
+0x00 (RO) : identification (0xRRrr00edu)
+ RR -- major version
+ rr -- minor version
+
+0x04 (RW) : card liveness check
+ It is a simple value inversion (~ C operator).
+
+0x08 (RW) : factorial computation
+ The stored value is taken and factorial of it is put back here.
+ This happens only after factorial bit in the status register (0x20
+ below) is cleared.
+
+0x20 (RO) : status register, bitwise OR
+ 0x01 -- computing factorial
+
+0x24 (RO) : interrupt status register
+ It contains values which raised the interrupt (see interrupt raise
+ register below).
+
+0x60 (WO) : interrupt raise register
+ Raise an interrupt. The value will be put to the interrupt status
+ register (using bitwise OR).
+
+0x64 (WO) : interrupt acknowledge register
+ Clear an interrupt. The value will be cleared from the interrupt
+ status register. This needs to be done from the ISR to stop
+ generating interrupts.
+
+0x80 (RW) : DMA source address
+ Where to perform the DMA from.
+
+0x88 (RW) : DMA destination address
+ Where to perform the DMA to.
+
+0x90 (RW) : DMA transfer count
+ The size of the area to perform the DMA on.
+
+0x98 (RW) : DMA command register, bitwise OR
+ 0x01 -- start transfer
+ 0x02 -- direction (0: from RAM to EDU, 1: from EDU to RAM)
+ 0x04 -- raise interrupt 0x100 after finishing the DMA
+
+IRQ controller
+--------------
+An IRQ is generated when written to the interrupt raise register. The value
+appears in interrupt status register when the interrupt is raised and has to
+be written to the interrupt acknowledge register to lower it.
+
+DMA controller
+--------------
+One has to specify, source, destination, size, and start the transfer. One
+4096 bytes long buffer at offset 0x40000 is available in the EDU device. I.e.
+one can perform DMA to/from this space when programmed properly.
+
+Example of transferring a 100 byte block to and from the buffer using a given
+PCI address 'addr':
+addr -> DMA source address
+0x40000 -> DMA destination address
+100 -> DMA transfer count
+1 -> DMA command register
+while (DMA command register & 1)
+ ;
+
+0x40000 -> DMA source address
+addr+100 -> DMA destination address
+100 -> DMA transfer count
+3 -> DMA command register
+while (DMA command register & 1)
+ ;
diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index 979e532fdf85..1fe74c6eb3fa 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -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
diff --git a/hw/misc/edu.c b/hw/misc/edu.c
new file mode 100644
index 000000000000..7b3d320eeba5
--- /dev/null
+++ b/hw/misc/edu.c
@@ -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;
+
+#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;
+ } dma;
+ QEMUTimer *dma_timer;
+ char dma_buf[DMA_SIZE];
+} EduState;
+
+static uint64_t dma_mask = (1UL << 28) - 1;
+
+static bool within(uint32_t addr, uint32_t start, uint32_t end)
+{
+ return start <= addr && addr < end;
+}
+
+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;
+
+ 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)
+{
+ 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;
+ 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),
+ 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);
+ }
+}
+
+static void dma_rw(EduState *edu, bool write, dma_addr_t *val, dma_addr_t *dma,
+ bool timer)
+{
+ 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);
+}
+
+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:
+ qemu_mutex_lock(&edu->thr_mutex);
+ val = edu->fact;
+ qemu_mutex_unlock(&edu->thr_mutex);
+ break;
+ case 0x20:
+ smp_rmb(); // against thread
+ val = edu->status;
+ 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)
+ return;
+
+ switch (addr) {
+ case 0x04:
+ edu->addr4 = ~val;
+ break;
+ case 0x08:
+ if (edu->status & EDU_STATUS_COMPUTING)
+ break;
+ edu->status |= EDU_STATUS_COMPUTING;
+ qemu_mutex_lock(&edu->thr_mutex);
+ edu->fact = val;
+ qemu_cond_signal(&edu->thr_cond);
+ qemu_mutex_unlock(&edu->thr_mutex);
+ break;
+ case 0x60:
+ edu->irq_status |= val;
+ pci_set_irq(&edu->pdev, 1);
+ 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;
+
+ 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;
+ while (val > 0)
+ ret *= val--;
+
+ edu->fact = ret;
+ qemu_mutex_unlock(&edu->thr_mutex);
+ 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->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);
+
+ 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);
+}
+
+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)
+{
+ object_property_add(obj, "dma_mask", "uint64", edu_obj_uint64,
+ edu_obj_uint64, NULL, &dma_mask, NULL);
+}
+
+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),
+ .instance_init = edu_instance_init,
+ .class_init = edu_class_init,
+ };
+
+ type_register_static(&edu_info);
+}
+type_init(pci_edu_register_types)
--
2.1.3
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH v2 1/1] pci-host: add educational driver
2014-12-05 9:52 ` [Qemu-devel] [PATCH v2 " Jiri Slaby
@ 2014-12-05 9:53 ` Jiri Slaby
2014-12-05 10:35 ` Paolo Bonzini
1 sibling, 0 replies; 15+ messages in thread
From: Jiri Slaby @ 2014-12-05 9:53 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini
On 12/05/2014, 10:52 AM, Jiri Slaby wrote:
> hw/misc/edu.c | 351 ++++++++++++++++++++++++++++++++++++++++++++++++
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -593,6 +593,11 @@ F: hw/net/opencores_eth.c
>
> Devices
> -------
> +EDU
> +M: Jiri Slaby <jslaby@suse.cz>
> +S: Maintained
> +F: hw/pci-host/edu.c
Which does not really correspond :/
--
js
suse labs
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/1] pci-host: add educational driver
2014-12-05 9:52 ` [Qemu-devel] [PATCH v2 " Jiri Slaby
2014-12-05 9:53 ` Jiri Slaby
@ 2014-12-05 10:35 ` Paolo Bonzini
2014-12-05 11:45 ` Jiri Slaby
1 sibling, 1 reply; 15+ messages in thread
From: Paolo Bonzini @ 2014-12-05 10:35 UTC (permalink / raw)
To: Jiri Slaby, qemu-devel
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.
>
> +0x20 (RO) : status register, bitwise OR
> + 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"?
> new file mode 100644
> index 000000000000..7b3d320eeba5
> --- /dev/null
> +++ b/hw/misc/edu.c
> @@ -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;
> +
> +#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. :)
> + } 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.
> + char dma_buf[DMA_SIZE];
> +} EduState;
> +
> +static uint64_t dma_mask = (1UL << 28) - 1;
Please move this inside EduState.
> +static bool within(uint32_t addr, uint32_t start, uint32_t end)
> +{
> + return start <= addr && addr < end;
4-space indent.
> +}
> +
> +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.
> +
> + 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)
> +{
> + 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.
> + 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),
> + 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.
> + }
> +}
> +
> +static void dma_rw(EduState *edu, bool write, dma_addr_t *val, dma_addr_t *dma,
> + bool timer)
> +{
> + 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
> +}
> +
> +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:
> + qemu_mutex_lock(&edu->thr_mutex);
> + val = edu->fact;
> + qemu_mutex_unlock(&edu->thr_mutex);
No need for the mutex.
> + 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.
> + 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)
> + 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);
> + 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.
> + 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.
> + case 0x60:
> + edu->irq_status |= val;
> + pci_set_irq(&edu->pdev, 1);
Should not set irq if edu->irq_status is zero.
> + 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;
> +
> + 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...
> + while (val > 0)
> + ret *= val--;
> + edu->fact = ret;
> + qemu_mutex_unlock(&edu->thr_mutex);
... put smp_wmb() here...
> + 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.
> + }
> +
> + 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->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.
> + 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);
> +}
> +
> +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);
> + object_property_add(obj, "dma_mask", "uint64", edu_obj_uint64,
> + edu_obj_uint64, NULL, &dma_mask, NULL);
... and use &edu->dma_mask here.
> +}
> +
> +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),
> + .instance_init = edu_instance_init,
> + .class_init = edu_class_init,
> + };
> +
> + type_register_static(&edu_info);
> +}
> +type_init(pci_edu_register_types)
>
Thanks,
Paolo
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH v2 1/1] pci-host: add educational driver
2014-12-05 10:35 ` Paolo Bonzini
@ 2014-12-05 11:45 ` Jiri Slaby
2014-12-05 12:07 ` Paolo Bonzini
0 siblings, 1 reply; 15+ messages in thread
From: Jiri Slaby @ 2014-12-05 11:45 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
On 12/05/2014, 11:35 AM, Paolo Bonzini wrote:
> 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 :).
>> --- /dev/null
>> +++ b/hw/misc/edu.c
>> @@ -0,0 +1,351 @@
...
>> +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?
>> +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:
>> + 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()?
>> +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)
>> + 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);
>
>> + 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?
>> + 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.
Did you mean case 0x20?
>> + 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,
--
js
suse labs
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH v2 1/1] pci-host: add educational driver
2014-12-05 11:45 ` Jiri Slaby
@ 2014-12-05 12:07 ` Paolo Bonzini
0 siblings, 0 replies; 15+ messages in thread
From: Paolo Bonzini @ 2014-12-05 12:07 UTC (permalink / raw)
To: Jiri Slaby, qemu-devel
On 05/12/2014 12:45, Jiri Slaby wrote:
> On 12/05/2014, 11:35 AM, Paolo Bonzini wrote:
>> 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 :).
>
>>> --- /dev/null
>>> +++ b/hw/misc/edu.c
>>> @@ -0,0 +1,351 @@
> ...
>>> +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.
>>> +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:
>>> + 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()?
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.
>>> +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)
>>> + 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);
>>
>>> + 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.
>>> + 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.
>
> Did you mean case 0x20?
Yes, sorry.
>>> + 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH v3 1/1] pci-host: add educational driver
2014-10-10 12:09 ` [Qemu-devel] [PATCH 1/1] pci-host: add educational driver Jiri Slaby
` (2 preceding siblings ...)
2014-12-05 9:52 ` [Qemu-devel] [PATCH v2 " Jiri Slaby
@ 2014-12-05 9:54 ` Jiri Slaby
3 siblings, 0 replies; 15+ messages in thread
From: Jiri Slaby @ 2014-12-05 9:54 UTC (permalink / raw)
To: qemu-devel; +Cc: 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 <jslaby@suse.cz>
---
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
diff --git a/MAINTAINERS b/MAINTAINERS
index bcb69e80d2dd..4e3b9f1703bf 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -593,6 +593,11 @@ F: hw/net/opencores_eth.c
Devices
-------
+EDU
+M: Jiri Slaby <jslaby@suse.cz>
+S: Maintained
+F: hw/misc/edu.c
+
IDE
M: Kevin Wolf <kwolf@redhat.com>
M: Stefan Hajnoczi <stefanha@redhat.com>
diff --git a/default-configs/pci.mak b/default-configs/pci.mak
index 91b1e92da53f..29130aba61d6 100644
--- a/default-configs/pci.mak
+++ b/default-configs/pci.mak
@@ -30,3 +30,4 @@ CONFIG_IPACK=y
CONFIG_WDT_IB6300ESB=y
CONFIG_PCI_TESTDEV=y
CONFIG_NVME_PCI=y
+CONFIG_EDU=y
diff --git a/docs/specs/edu.txt b/docs/specs/edu.txt
new file mode 100644
index 000000000000..94409bb58297
--- /dev/null
+++ b/docs/specs/edu.txt
@@ -0,0 +1,105 @@
+
+EDU device
+==========
+
+This is an educational device for writing (kernel) drivers. Its original
+intention was to support the Linux kernel lectures taught at the Masaryk
+University. Students are given this virtual device and are expected to write a
+driver with I/Os, IRQs, DMAs and such.
+
+The devices behaves very similar to the PCI bridge present in the COMBO6 cards
+developed under the Liberouter wings. Both PCI device ID and PCI space is
+inherited from that device.
+
+Command line switches:
+ -device edu[,dma_mask=mask]
+
+ dma_mask makes the virtual device work with DMA addresses with the given
+ mask. For educational purposes, the device supports only 28 bits (256 MiB)
+ by default. Students shall set dma_mask for the device in the OS driver
+ properly.
+
+PCI specs
+---------
+
+PCI ID: 1234:11e8
+
+PCI Region 0:
+ I/O memory, 1 MB in size. Users are supposed to communicate with the card
+ through this memory.
+
+MMIO area spec
+--------------
+
+Only size == 4 accesses are allowed for addresses < 0x80. size == 4 or
+size == 8 for the rest.
+
+0x00 (RO) : identification (0xRRrr00edu)
+ RR -- major version
+ rr -- minor version
+
+0x04 (RW) : card liveness check
+ It is a simple value inversion (~ C operator).
+
+0x08 (RW) : factorial computation
+ The stored value is taken and factorial of it is put back here.
+ This happens only after factorial bit in the status register (0x20
+ below) is cleared.
+
+0x20 (RO) : status register, bitwise OR
+ 0x01 -- computing factorial
+
+0x24 (RO) : interrupt status register
+ It contains values which raised the interrupt (see interrupt raise
+ register below).
+
+0x60 (WO) : interrupt raise register
+ Raise an interrupt. The value will be put to the interrupt status
+ register (using bitwise OR).
+
+0x64 (WO) : interrupt acknowledge register
+ Clear an interrupt. The value will be cleared from the interrupt
+ status register. This needs to be done from the ISR to stop
+ generating interrupts.
+
+0x80 (RW) : DMA source address
+ Where to perform the DMA from.
+
+0x88 (RW) : DMA destination address
+ Where to perform the DMA to.
+
+0x90 (RW) : DMA transfer count
+ The size of the area to perform the DMA on.
+
+0x98 (RW) : DMA command register, bitwise OR
+ 0x01 -- start transfer
+ 0x02 -- direction (0: from RAM to EDU, 1: from EDU to RAM)
+ 0x04 -- raise interrupt 0x100 after finishing the DMA
+
+IRQ controller
+--------------
+An IRQ is generated when written to the interrupt raise register. The value
+appears in interrupt status register when the interrupt is raised and has to
+be written to the interrupt acknowledge register to lower it.
+
+DMA controller
+--------------
+One has to specify, source, destination, size, and start the transfer. One
+4096 bytes long buffer at offset 0x40000 is available in the EDU device. I.e.
+one can perform DMA to/from this space when programmed properly.
+
+Example of transferring a 100 byte block to and from the buffer using a given
+PCI address 'addr':
+addr -> DMA source address
+0x40000 -> DMA destination address
+100 -> DMA transfer count
+1 -> DMA command register
+while (DMA command register & 1)
+ ;
+
+0x40000 -> DMA source address
+addr+100 -> DMA destination address
+100 -> DMA transfer count
+3 -> DMA command register
+while (DMA command register & 1)
+ ;
diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index 979e532fdf85..1fe74c6eb3fa 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -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
diff --git a/hw/misc/edu.c b/hw/misc/edu.c
new file mode 100644
index 000000000000..7b3d320eeba5
--- /dev/null
+++ b/hw/misc/edu.c
@@ -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;
+
+#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;
+ } dma;
+ QEMUTimer *dma_timer;
+ char dma_buf[DMA_SIZE];
+} EduState;
+
+static uint64_t dma_mask = (1UL << 28) - 1;
+
+static bool within(uint32_t addr, uint32_t start, uint32_t end)
+{
+ return start <= addr && addr < end;
+}
+
+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;
+
+ 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)
+{
+ 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;
+ 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),
+ 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);
+ }
+}
+
+static void dma_rw(EduState *edu, bool write, dma_addr_t *val, dma_addr_t *dma,
+ bool timer)
+{
+ 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);
+}
+
+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:
+ qemu_mutex_lock(&edu->thr_mutex);
+ val = edu->fact;
+ qemu_mutex_unlock(&edu->thr_mutex);
+ break;
+ case 0x20:
+ smp_rmb(); // against thread
+ val = edu->status;
+ 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)
+ return;
+
+ switch (addr) {
+ case 0x04:
+ edu->addr4 = ~val;
+ break;
+ case 0x08:
+ if (edu->status & EDU_STATUS_COMPUTING)
+ break;
+ edu->status |= EDU_STATUS_COMPUTING;
+ qemu_mutex_lock(&edu->thr_mutex);
+ edu->fact = val;
+ qemu_cond_signal(&edu->thr_cond);
+ qemu_mutex_unlock(&edu->thr_mutex);
+ break;
+ case 0x60:
+ edu->irq_status |= val;
+ pci_set_irq(&edu->pdev, 1);
+ 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;
+
+ 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;
+ while (val > 0)
+ ret *= val--;
+
+ edu->fact = ret;
+ qemu_mutex_unlock(&edu->thr_mutex);
+ 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->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);
+
+ 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);
+}
+
+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)
+{
+ object_property_add(obj, "dma_mask", "uint64", edu_obj_uint64,
+ edu_obj_uint64, NULL, &dma_mask, NULL);
+}
+
+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),
+ .instance_init = edu_instance_init,
+ .class_init = edu_class_init,
+ };
+
+ type_register_static(&edu_info);
+}
+type_init(pci_edu_register_types)
--
2.1.3
^ permalink raw reply related [flat|nested] 15+ messages in thread