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

[Jiri Slaby <jslaby@suse.cz>]

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