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

[Paolo Bonzini]

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