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
