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
