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

[Paolo Bonzini]

Hi Jirka,

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

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