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
