From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42968) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XwrK4-0002J3-3A for qemu-devel@nongnu.org; Fri, 05 Dec 2014 06:45:20 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XwrK3-00021t-3o for qemu-devel@nongnu.org; Fri, 05 Dec 2014 06:45:20 -0500 Received: from mail-wg0-x235.google.com ([2a00:1450:400c:c00::235]:39219) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XwrK2-00020l-TL for qemu-devel@nongnu.org; Fri, 05 Dec 2014 06:45:19 -0500 Received: by mail-wg0-f53.google.com with SMTP id l18so656815wgh.40 for ; Fri, 05 Dec 2014 03:45:18 -0800 (PST) Sender: Jiri Slaby Message-ID: <54819ACB.1080204@suse.cz> Date: Fri, 05 Dec 2014 12:45:15 +0100 From: Jiri Slaby MIME-Version: 1.0 References: <1412942996-29645-1-git-send-email-jslaby@suse.cz> <1417773160-27247-1-git-send-email-jslaby@suse.cz> <54818A66.20000@redhat.com> In-Reply-To: <54818A66.20000@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v2 1/1] pci-host: add educational driver List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini , qemu-devel@nongnu.org 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